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

Don't delete misplaced items on final attempt #106

Merged
merged 2 commits into from
Oct 18, 2016

Conversation

itsjeyd
Copy link
Contributor

@itsjeyd itsjeyd commented Sep 28, 2016

Description

In assessment mode, don't delete misplaced items from the user's state when handling the final attempt.

This is a server-side change to match what Javascript already did. In other words, upon submitting the final attempt, misplaced items were already left on the board. However, since the corresponding state was deleted server-side, upon refreshing the page the items were returned to the bank.

Author

@arbrandes

OpenCraft review via open-craft#13.

Dependencies

None

JIRA

None

Sandbox URLs

Latest commit

Testing instructions

  1. Go to the pre-deployed DnDv2 instance on the LMS, and log in as staff@example.com.
  2. Click on Staff Debug Info, and Delete Student State for the staff@example.com user.
  3. Close the debug info window, and drag a single item into its correct zone (for instance, "Goes to the Top" to the top zone).
  4. Click Submit twice, so that you only have one attempt left.
  5. Drag an item into an incorrect zone (for instance, drag "I don't belong anywhere" to the bottom zone).
  6. Click Submit.
  7. Close the incorrect answer popup.
  8. Refresh the page.
  9. Notice that even though you have no attempts left, the wrongly placed item is still there.

Screenshots

Wrongly placed item is still there after refreshing the page.

screenshot

Reviewers

@openedx-webhooks
Copy link

Thanks for the pull request, @itsjeyd! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx-solutions%2Fxblock-drag-and-drop-v2&number=106

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 28, 2016

@arbrandes 👍

  • I tested this: Works as advertised in PR description
  • I read through the code
  • I checked for accessibility issues This is a bug fix that doesn't introduce new workflows (or alter existing ones).
  • Includes documentation This is a bug fix that doesn't require documentation updates.

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 28, 2016

@cahrens @staubina This is ready for you to have a look. Sandbox links are in the PR description. Thanks!

@cahrens
Copy link

cahrens commented Sep 28, 2016

👍 Note that you have test failures though

@staubina
Copy link
Contributor

👍 Please correct failures before merging.

@arbrandes
Copy link
Contributor

Trying to reproduce the failure - I'll report back shortly.

On Sep 29, 2016 12:39, "Albert (AJ) St. Aubin" notifications@github.com
wrote:

👍 Please correct failures before merging.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#106 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuWO4RQUTwquY7qWQMJMa4KmwFKTfJoks5qu9uhgaJpZM4KIkIq
.

@arbrandes
Copy link
Contributor

@cahrens, @staubina, I ran the full integration test suite locally, and could not reproduce any of the errors. Looks like some Travis flakyness, as usual.

In any case, I just squashed the last fixup, and pushed again. The code in place is exactly the same, but let's see if Travis behaves a little better this time around.

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 30, 2016

@cahrens @staubina Thanks for reviewing!

Looks like we got a green build. Thanks @arbrandes for investigating.

@sstack22 Can I get you to have a quick look at this as well? Thanks!

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 30, 2016

@arbrandes One more thing: To release this, we'll need to open a version bump PR against edx-platform. Could you update the version number in setup.py and .travis.yml to 2.0.11, and add an appropriate entry to Changelog.md please? See 45b0e1a for an example. A separate commit is fine. Thanks!

@arbrandes
Copy link
Contributor

@itsjeyd, done on 83f81c7!

@bdero
Copy link

bdero commented Oct 4, 2016

@arbrandes It looks like this is no longer a green build, can you investigate this?

@sstack22 Let me know when you've reviewed this. I was asked to merge it. Thanks!

@arbrandes
Copy link
Contributor

@bdero, hey, long time no see! :) I cannot reproduce these failures locally. It seems, once more, these are due to Travis being flaky. I'm just going to try issuing a rebuild, if I can - if not, I'll just push a rebase.

In assessment mode, don't delete misplaced items from the user's state
when handling the final attempt.

This is a server-side change to match what Javascript already does: upon
submitting the final attempt, misplaced items are left on the board.
However, since the corresponding state was deleted server-side, upon
refreshing the page items were returned to the bank.
@bdero
Copy link

bdero commented Oct 7, 2016

Thanks @arbrandes!

@sstack22 The tests are passing here again so this is all ready for your review. :)

@antoviaque
Copy link
Contributor

@sstack22 To be able to plan for this, when would you be able to review?

@sstack22
Copy link

@antoviaque - thanks for the ping :) I'll review now.

@sstack22
Copy link

👍 !

@antoviaque
Copy link
Contributor

@sstack22 Great, thank you : )

@itsjeyd itsjeyd merged commit 24c6ced into openedx:master Oct 18, 2016
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.

8 participants