Keep items in dropped position in assessment mode. #82

mtyaka · 2016-07-20T11:48:54Z

Description

This PR implements the following use case:

As a learner, I want to place all items belonging to a drag-and-drop problem in assessment mode on the board first, and then check (submit) my answer by clicking a button.

Items dropped onto wrong zones in standard mode are immediately removed from the board and sent back to the bank.

In assessment mode, items should stick in the dropped position until the entire problem is explicitly submitted by clicking a button.

Note that the submit button and associated xhr handler will be implemented in a future PR.

UX

The workflow for placing items on the board should not change in assessment mode. However, items that were placed on an incorrect zone should stay on the board in assessment mode (at least until the learner checks their answer) instead of returning to the item bank.

Also, a drag-and-drop problem in assessment mode should not show per-item "Success Feedback" or "Error Feedback" to the learner as they place items on the board.

JIRA tickets: SOL-1942

Discussions: Discovery document for DnDv2 assessment mode (relevant section: 2.2.1)

Dependencies: None

Screenshots:

Items stick in dropped position even when dropped onto a wrong zone:

Sandbox URLs (ff7ddfd):

Studio
LMS

Testing instructions:

Add a DnDv2 block to a unit in Studio.
Click "EDIT" and change "Problem mode" to "assessment".
Drag items from the bank into drop zones. Place some items in wrong zones.
Observe that the items "stick" in the placed position (are not moved back to the bank). Observe that per-item feedbacks are not displayed.
Change "Problem mode" back to "standard".
Make sure everything still works correctly in standard mode.

Author notes and concerns:

When you reload the page after dragging some items on the board, the state of the board is not correct. Correctly placed items are left in their positioned (but faded out) while incorrectly placed items are sent back to the bank.
This behavior will be sorted out in future tickets.

Reviewers

@itsjeyd
TNL: @cahrens and/or @staubina
a11y: @cptvitamin and/or @clrux
Product: @sstack22

itsjeyd · 2016-07-20T14:39:22Z

drag_and_drop_v2/public/js/drag_and_drop.js

@@ -1,4 +1,4 @@
-function DragNDropTemplates(url_name) {
+function DragNDropTemplates(configuration) {


@mtyaka Since you're touching this line, could you take the opportunity to make the spelling consistent with the remaining code (i.e., change DragNDropTemplates to DragAndDropTemplates)? That would be great :) There's only two occurrences of this spelling in the entire repo, and both of them are in this file, so it should be a quick change.

@itsjeyd Done, that inconsistency was weird.

mtyaka · 2016-07-21T09:19:39Z

@itsjeyd Thanks for the review. I addressed your comments, please take a look.

itsjeyd · 2016-07-21T11:33:59Z

tests/integration/test_interaction.py

+        # While the XHR is in progress, a spinner icon is shown inside the item.
+        # When the spinner disappears, we can assume that the XHR request has finished.
+        wait.until(lambda e: 'fa-spinner' not in e.get_attribute('innerHTML'),
+                   u"Spinner should not be in {}".format(elem.get_attribute('innerHTML')))


@mtyaka This should probably be reformatted according to edX's style guide for breaking long lines:

wait.until( lambda e: 'fa-spinner' not in e.get_attribute('innerHTML'), u"Spinner should not be in {}".format(elem.get_attribute('innerHTML')) )

Sorry for not noticing sooner :/

@itsjeyd No problem, I should have noticed it when implementing the function :) Fixed.

itsjeyd · 2016-07-21T11:34:26Z

@mtyaka Thanks for the updates! The code is looking great aside from one small formatting issue.

sstack22 · 2016-07-21T21:33:15Z

@mtyaka - 👍 . Outside the scope of PR, I noticed the Note on the edit page: "Note: do not edit the problem if students have already completed it. Delete the problem and create a new one." I was wondering what the original intent of this note was? In general, especially for graded assignments, we don't recommend deleting a problem, as it could impact scores.

mtyaka · 2016-07-22T04:37:22Z

@itsjeyd Thanks, formatting issue addressed.

@sstack22 Thanks for the review! I am not entirely sure, but I'm guessing the original intent of that note was to let staff know that they shouldn't modify problems that students have already started (or completed) working on. I don't think the second part about deleting the problem instead makes much sense, we should probably remove it.

itsjeyd · 2016-07-22T07:02:50Z

@mtyaka 👍

I followed the test instructions on the sandbox and everything worked as advertised.
I read through the code.
~~Includes documentation~~ We will update the README (and our demo course) once assessment mode functionality is mostly complete.

cahrens · 2016-07-22T13:22:06Z

@mtyaka Please let me know when this is ready for TNL review.

mtyaka · 2016-07-22T13:59:40Z

@cahrens This is ready for TNL review.

staubina · 2016-07-22T18:33:24Z

👍
Like the last PR, I tested the visual changes and they all match expectations. The code changes look clear enough also.

Is there a story in Jira for the Refresh issue mentioned in the Notes?

cahrens · 2016-07-22T18:35:16Z

"I don't belong anywhere" can't be dragged (in Standard mode) when it is the only thing left. Up until then, it can.

cahrens · 2016-07-22T18:40:50Z

drag_and_drop_v2/public/js/drag_and_drop.js

+            var description_content;
+            if (configuration.mode === DragAndDropBlock.ASSESSMENT_MODE) {
+              // In assessment mode placed items will "stick" even when not in correct zone.
+              description_content = gettext('Placed in: ') + zone_title;


Should use named placeholders instead of concatenating together strings. Otherwise it cannot be properly translated into all languages.

@cahrens Thanks for pointing that out. I switched to interpolation in this line and in old code a couple of lines below.

cahrens · 2016-07-22T18:50:38Z

I am seeing non-unique IDs when feedback is shown (this is sr feedback).

mtyaka · 2016-07-23T03:34:12Z

@staubina Thanks! The refresh issue will be handled as part of https://openedx.atlassian.net/browse/SOL-1944

mtyaka · 2016-07-23T03:42:12Z

@cahrens Thanks for the review!

"I don't belong anywhere" can't be dragged (in Standard mode) when it is the only thing left. Up until then, it can.

That's the expected behavior in Standard mode. Once you drag all items (except decoy items) to correct zones, the problem is finished and you cannot move any item (including decoy items) unless you hit "Reset problem".

I am seeing non-unique IDs when feedback is shown (this is sr feedback).

Nice catch. That bug would manifest itself when multiple DnDv2 blocks were present in the same unit. This was an old bug that existed prior to the changes in this PR. The most recent commit in this PR fixes it. I also updated the sandbox with the latest fixes.

Please have another look.

cahrens · 2016-07-25T13:26:02Z

👍

mtyaka · 2016-07-25T13:51:31Z

@cptvitamin Would you be able to take a quick look at the changes in this PR?

bradenmacdonald · 2016-07-29T20:14:33Z

@mtyaka Can you please rebase and merge? Since the a11y team likely can't review this before our deadlines, I created SOL-1978 for a final / follow-up a11y review of the assessment mode functionality.

mtyaka · 2016-07-30T14:28:51Z

Squashed from ff7ddfd

mtyaka · 2016-07-30T14:39:16Z

Rebased 5455b50 onto master

Items dropped onto wrong zones in standard mode are immediately removed from the board and sent back to the bank. In assessment mode, items should stick in the dropped position until the entire problem is explicitly submitted by clicking a button. This commit only implements the "stick" part, but does not implement the submit button and associated handler. This commit also fixes an existing bug where the template function would gett reused between blocks. The template function (which is slightly different for each block, depending on block's configuration) was getting assigned to the global DragAndDropBlock function. Each block would overwrite the previous value, which caused bugs.

mtyaka force-pushed the mtyaka/assessment-item-placement branch 2 times, most recently from c8eac81 to b1ff73d Compare July 20, 2016 12:40

itsjeyd reviewed Jul 20, 2016
View reviewed changes

mtyaka force-pushed the mtyaka/assessment-item-placement branch 2 times, most recently from 1d7aacf to 7718cdc Compare July 21, 2016 07:55

itsjeyd reviewed Jul 21, 2016
View reviewed changes

cahrens reviewed Jul 22, 2016
View reviewed changes

mtyaka mentioned this pull request Jul 27, 2016

Move items between zones in assessment mode #84

Merged

4 tasks

e-kolpakov mentioned this pull request Jul 27, 2016

[SOL-1944] Submit Answer UI #86

Merged

4 tasks

mtyaka force-pushed the mtyaka/assessment-item-placement branch from ff7ddfd to 5455b50 Compare July 30, 2016 14:28

mtyaka force-pushed the mtyaka/assessment-item-placement branch from 5455b50 to c1b2152 Compare July 30, 2016 14:39

mtyaka force-pushed the mtyaka/assessment-item-placement branch from c1b2152 to 97c5c4d Compare July 31, 2016 08:49

mtyaka merged commit 30173c4 into openedx:master Jul 31, 2016

mtyaka deleted the mtyaka/assessment-item-placement branch July 31, 2016 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep items in dropped position in assessment mode. #82

Keep items in dropped position in assessment mode. #82

mtyaka commented Jul 20, 2016 •

edited by itsjeyd

Loading

itsjeyd Jul 20, 2016

mtyaka Jul 21, 2016

mtyaka commented Jul 21, 2016

itsjeyd Jul 21, 2016

mtyaka Jul 22, 2016

itsjeyd commented Jul 21, 2016

sstack22 commented Jul 21, 2016

mtyaka commented Jul 22, 2016

itsjeyd commented Jul 22, 2016

cahrens commented Jul 22, 2016

mtyaka commented Jul 22, 2016

staubina commented Jul 22, 2016

cahrens commented Jul 22, 2016

cahrens Jul 22, 2016

mtyaka Jul 23, 2016

cahrens commented Jul 22, 2016

mtyaka commented Jul 23, 2016

mtyaka commented Jul 23, 2016

cahrens commented Jul 25, 2016

mtyaka commented Jul 25, 2016

bradenmacdonald commented Jul 29, 2016

mtyaka commented Jul 30, 2016

mtyaka commented Jul 30, 2016

		@@ -1,4 +1,4 @@
		function DragNDropTemplates(url_name) {
		function DragNDropTemplates(configuration) {

Keep items in dropped position in assessment mode. #82

Keep items in dropped position in assessment mode. #82

Conversation

mtyaka commented Jul 20, 2016 • edited by itsjeyd Loading

itsjeyd Jul 20, 2016

Choose a reason for hiding this comment

mtyaka Jul 21, 2016

Choose a reason for hiding this comment

mtyaka commented Jul 21, 2016

itsjeyd Jul 21, 2016

Choose a reason for hiding this comment

mtyaka Jul 22, 2016

Choose a reason for hiding this comment

itsjeyd commented Jul 21, 2016

sstack22 commented Jul 21, 2016

mtyaka commented Jul 22, 2016

itsjeyd commented Jul 22, 2016

cahrens commented Jul 22, 2016

mtyaka commented Jul 22, 2016

staubina commented Jul 22, 2016

cahrens commented Jul 22, 2016

cahrens Jul 22, 2016

Choose a reason for hiding this comment

mtyaka Jul 23, 2016

Choose a reason for hiding this comment

cahrens commented Jul 22, 2016

mtyaka commented Jul 23, 2016

mtyaka commented Jul 23, 2016

cahrens commented Jul 25, 2016

mtyaka commented Jul 25, 2016

bradenmacdonald commented Jul 29, 2016

mtyaka commented Jul 30, 2016

mtyaka commented Jul 30, 2016

mtyaka commented Jul 20, 2016 •

edited by itsjeyd

Loading