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

Ensure old problems work on new version. #98

Merged
merged 2 commits into from
Aug 30, 2016

Conversation

mtyaka
Copy link
Contributor

@mtyaka mtyaka commented Aug 26, 2016

Description

Recent versions of the drag and drop v2 xblock require the 'correct' field to be present in items stored in user state, but the 'correct' field did not exist before assessment mode has been implemented and problems created and completed before the introduction of assessment mode lack the 'correct' field.

Loading these stored items did not work on recent versions of DnDv2 xblock because recent versions are assuming the 'correct' field is always present.

This patch adds the 'correct' field to item state at load time. In standard mode, only items that are placed in correct zone are stored in item state, so we can assume that we can set the 'correct' field to True for any item stored in the state that is missing the 'correct' field.

Dependencies

None

JIRA

Sandbox URLs

I did not prepare a sandbox because verifying the changes involves (re)installing several versions of the DnDv2 xblock which cannot easily be done on a sandbox.
I can spin up a sandbox if you think it would still be useful, though.

Testing Instructions

In order to test this, you need to create and complete (or partially complete) a DnDv2 problem using an older version of DnDv2 xblock. I used v2.0.6, but any pre-assessment mode version should work.

Assuming you have xblock-drag-and-drop-v2 installed in editable mode (pip install -e .) in your devstack, follow these steps:

  1. Check out version v2.0.6 of DnD xblock in your devstack.
  2. Restart the Studio and add a DnD problem to a unit. Default settings are fine.
  3. Go to the LMS and complete (or partially complete) the problem.
  4. Check out edx-solutions/master version of DnDv2 xblock.
  5. Restart the LMS and try to load the DnD problem. The problem will fail to load. You will see a KeyError in the logs.
  6. Checkout out open-craft/mtyaka/legacy-item-state branch.
  7. Restart the LMS.
  8. The problem should now load and work correctly.

Reviewers

Recent versions of the drag and drop v2 xblock require the 'correct'
field to be present in items stored in user state, but the 'correct'
field did not exist before assessment mode has been implemented and so
problems created and completed before the introduction of assessment
mode lack the 'correct' field.

This patch adds the 'correct' field to item state at load time.
In standard mode, only items that are placed in correct zone are stored
in item state, so we can assume that any item stored in the state that
does not have the 'correct' field is 'correct'.
@mtyaka mtyaka mentioned this pull request Aug 26, 2016
7 tasks
if self.mode == self.ASSESSMENT_MODE:
# In assessment mode, if item is placed correctly and than the page is refreshed, "correct"
# will spill to the frontend, making item "disabled", thus allowing students to obtain answer by trial
# and error + refreshing the page. In order to avoid that, we remove "correct" from an item here
Copy link
Contributor

Choose a reason for hiding this comment

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

@mtyaka @mtyaka Typo: "than" should be "then"

And I know you just copied an existing comment, but I'm not sure what

making item "disabled", thus allowing students to obtain answer by trial and error ...

means (esp. the first part)? It would be great if you could clarify :)

@itsjeyd
Copy link
Contributor

itsjeyd commented Aug 26, 2016

@mtyaka This is a really nice patch! I only had a couple small comments about ... some of the comments :) 👍 once they are addressed.

  • I tested this: When following test instructions, legacy problem loads correctly in my local dev env
  • I read through the code
  • Includes documentation Doesn't introduce or alter functionality, so no need to add or update documentation

@mtyaka
Copy link
Contributor Author

mtyaka commented Aug 27, 2016

Thanks @itsjeyd!

@staubina this is now ready for your review.

@staubina
Copy link
Contributor

@cahrens FYI
@mtyaka I will take a look at this as soon as I can

@cahrens
Copy link

cahrens commented Aug 29, 2016

👍

1 similar comment
@staubina
Copy link
Contributor

👍

@mtyaka
Copy link
Contributor Author

mtyaka commented Aug 30, 2016

@sstack22 This PR is ready for merge. Does it look good to you - can we go ahead and merge?

@sstack22
Copy link

@mtyaka - looks good on my end 👍

@mtyaka
Copy link
Contributor Author

mtyaka commented Aug 30, 2016

Thanks!

@mtyaka mtyaka merged commit 6b21193 into openedx:master Aug 30, 2016
@mtyaka mtyaka deleted the mtyaka/legacy-item-state branch August 30, 2016 13:01
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