Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/vik/studio oe #160

Merged
merged 16 commits into from
Jul 19, 2013
Merged

Fix/vik/studio oe #160

merged 16 commits into from
Jul 19, 2013

Conversation

VikParuchuri
Copy link
Contributor

Fix several issues with open ended assessment.

@ghost ghost assigned shnayder Jun 13, 2013
@VikParuchuri
Copy link
Contributor Author

@shnayder Any time to look at this? Fixes some crucial issues. I will get the tests fixed up tomorrow or the next day, so may make sense to look at it then.

@shnayder
Copy link

I like the problems solved... can you ping me and Diana when it's ready to
go?

On Wed, Jun 19, 2013 at 12:24 PM, Vik Paruchuri [email protected]:

@shnayder https://github.com/shnayder Any time to look at this? Fixes
some crucial issues. I will get the tests fixed up tomorrow or the next
day, so may make sense to look at it then.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/160#issuecomment-19695315
.

@VikParuchuri
Copy link
Contributor Author

@shnayder
Copy link

Build still failing.

@VikParuchuri
Copy link
Contributor Author

Yup. Sadly, jenkins is no longer working with my VPN. Will look at it
when I come in this morning.

On Fri, Jun 28, 2013 at 12:21 AM, Victor Shnayder
[email protected]:

Build still failing.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/160#issuecomment-20169727
.

@VikParuchuri
Copy link
Contributor Author

Jenkins is being strange, but tests are passing locally. @shnayder should be good to review.

@shnayder
Copy link

@cpennington -- can you look at the LMS / xmodule side of this? Diana doesn't want to be the only one who knows this code...

@cahrens can you look at the studio side or delegate as makes sense?

Thanks!

@VikParuchuri
Copy link
Contributor Author

@shnayder Did this get looked at?

@cahrens
Copy link

cahrens commented Jul 11, 2013

I think we should hold off on merging this until after https://github.com/edx/edx-platform/pull/270.

@dmitchell would be a good person to review, as he understands how this PR overlaps with his own.

@shnayder
Copy link

@cpennington would it make sense to rebase this on top of Don's branch, or are you still changing the history around there?

@VikParuchuri
Copy link
Contributor Author

This and #270 have some, but not much, overlap. The area of overlap is in the "default" view when the studio editor is opened, and I think the defaults set in this PR are more desirable than the ones set in #270, as these feature a more realistic question and comprehensive rubric.

@cpennington
Copy link
Contributor

I am changing history around for #270. I hope to have #270 merged today
(google permitting). I haven't had a chance to review this PR, but i'd be
happy to have better defaults for open ended modules.

-Cale

On Thu, Jul 11, 2013 at 11:19 AM, Vik Paruchuri [email protected]:

This and #270 https://github.com/edx/edx-platform/issues/270 have some,
but not much, overlap. The area of overlap is in the "default" view when
the studio editor is opened, and I think the defaults set in this PR are
more desirable than the ones set in #270https://github.com/edx/edx-platform/issues/270,
as these feature a more realistic question and comprehensive rubric.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/160#issuecomment-20819045
.

ready_to_reset = Boolean(
help="If the problem is ready to be reset or not.", default=False,
help="If the problem is ready to be reset or not.",
default=False,
scope=Scope.user_state
)
attempts = Integer(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be too late to make these consistent w/ capa_module, but in that, attempts is the user's # of attempts (what you've called student_attempts) and max_attempts is this field. I might further ask if this should inherit from CapaFields, but it has a bunch of garbage which this probably doesn't want (perhaps a common superclass of fields). CapaXXX certainly has a distinct and overlapping set of logic around is graded, is resettable, and duplicates attempts, max_attempts, due, grace_period,

@VikParuchuri
Copy link
Contributor Author

@shnayder @dmitchell Made some changes to rename variables and fix some logic around handling the due date in peer grading. Good to go?

@@ -304,6 +422,11 @@ class CombinedOpenEndedDescriptor(CombinedOpenEndedFields, RawDescriptor):
js_module_name = "OpenEndedMarkdownEditingDescriptor"
css = {'scss': [resource_string(__name__, 'css/editor/edit.scss'), resource_string(__name__, 'css/combinedopenended/edit.scss')]}

metadata_translations = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to be added to the parent's metadata_translations so as to not lose those? (i.e.,

super(..).metadata_translations.update({...})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that it would be safe to overwrite, as 'slug' and 'name' were never used, but you are right, it makes sense in case another field is added later.

@dmitchell
Copy link
Contributor

Looks ok other than the overwrite of metadata_translations (which I'm not sure I'm correct about)

VikParuchuri added a commit that referenced this pull request Jul 19, 2013
@VikParuchuri VikParuchuri merged commit a2522c4 into master Jul 19, 2013
chrisndodge pushed a commit that referenced this pull request Aug 7, 2014
cdodge/update-opaque-key: update user social stats to use the legacy cou...
jbau pushed a commit that referenced this pull request Jan 21, 2015
Fixes 3 issues found after deploying to stage:
prabhanshu pushed a commit to prabhanshu/edx-platform that referenced this pull request Oct 13, 2018
identical to PR openedx#113 

* Adds support for objuscating last two octets of ip address in tracking logs

* Cleaning up change

* Fixing a couple edge case events

* live oauth support (openedx#115)

* Added settings import to fix tests

* Fixes tests

* Remove vscode folder

* Fixing quality errors

* simpler obfuscation method

* cleanup2 simpler pr template (openedx#130)

* simpler pr template

* more accurate phrasing

* more specific phrasing
CrewS pushed a commit to CrewS/edx-platform-1 that referenced this pull request Dec 28, 2018
课程详情添加视频,fix bugs
edx-secure pushed a commit that referenced this pull request Sep 6, 2019
rediris pushed a commit to gymnasium/edx-platform that referenced this pull request Feb 25, 2021
…re/badges-fix-badgr-integration

Fixes for Badgr.io badges integration
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants