Assessment mode: display item feedback #93

e-kolpakov · 2016-08-11T13:22:23Z

Note: ~~This PR is based on #87, only relevant commit is the last one.~~

Description: This PR allows DnDv2 to show item feedback messages in assessment mode.

JIRA: SOL-1980

Discussions: SOL-1980

Dependencies: ~~#87~~

Sandboxes:

Testing instructions:

Configure DnDv2 to have unique "incorrect" item feedback messages. Set to assessment mode.
In LMS, put some items into wrong zones.
Check answer (click "Submit")
Feedback popup should appear showing incorrect feedback messages for every misplaced item.
~~If only one item is misplaced, only feedback message is shown; if multiple items are misplaced,~~ popup contains a note about ~~multiple~~ item(s) being misplaced, and feedback messages are shown in form of bullet list.

Screenshots:

Reviewers:

internal: @itsjeyd
internal: @haikuginger
TNL: @cahrens
TNL: @staubina
~~a11y: @cptvitamin~~ a11y review will be performed later, for all changes in v2.1: SOL-1978
Product: @sstack22

Checklist:

Internal review
Upstream review
Squash commits

openedx-webhooks · 2016-08-11T13:22:28Z

Thanks for the pull request, @e-kolpakov! 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?number=93&repo=edx-solutions%2Fxblock-drag-and-drop-v2

itsjeyd · 2016-08-12T10:14:07Z

drag_and_drop_v2/drag_and_drop_v2.py

@@ -221,7 +221,6 @@ def items_without_answers():
            "target_img_description": self.target_img_description,
            "item_background_color": self.item_background_color or None,
            "item_text_color": self.item_text_color or None,
-            "initial_feedback": self.data['feedback']['start'],


@e-kolpakov I probably missed some of the more recent discussions about "Introductory Feedback" in assessment mode, but don't we at least need to pass this to the client for problems in "standard" mode?

@itsjeyd as far as I can tell "initial_feedback" is not used anywhere in JS.

@e-kolpakov so should "start" feedback be deleted entirely (Python, studio)?

@cahrens no, it is still shown in feedback area - it is passed through get_user_state handler, rather than here.

e-kolpakov · 2016-08-12T13:47:10Z

@itsjeyd Just once no tests failed, but I didn't notice pep8 errors :(

e-kolpakov · 2016-08-12T14:24:25Z

@itsjeyd Sandbox updated

e-kolpakov · 2016-08-12T15:47:12Z

@itsjeyd I might have updated this sandbox with the version intended for other PR (sandbox9) - if you're to test it while I'm away, could you please reinstall DnDv2 on the sandbox? Thank you!

haikuginger · 2016-08-12T20:06:19Z

The sandbox looks to be correctly provisioned at this point.

itsjeyd · 2016-08-15T09:23:48Z

drag_and_drop_v2/public/js/drag_and_drop.js

-        if (msgs.length > 1) {
-            popup_content = h("div.popup-content", {}, [
+        if (ctx.mode == DragAndDropBlock.ASSESSMENT_MODE) {
+            var content_items = [


@e-kolpakov message_items would be a little more to the point I think.

Thanks for making the behavior consistent!

itsjeyd · 2016-08-15T09:25:48Z

Thanks @haikuginger!

@e-kolpakov I tested the feature on the sandbox, and it's working well. My only comment is that it would be nice to adjust the padding of the feedback popup so that long messages don't get a chance to run all the way to the right border:

It would look better if the padding on the right (approximately) matched the padding on the left (space between left edge of popup and bullet).

Other than that, the documentation will need to be updated. It currently says:

In assessment mode, the success and error feedback texts are not used.

... which is not true anymore as of this PR :)

👍 once remaining comments have been addressed and build is green.

I tested this: Feature works as advertised in PR description
I read through the code
Includes documentation

e-kolpakov · 2016-08-15T12:15:14Z

@itsjeyd I've addressed your notes and am updating sandbox right now - if you'd like another look at this this is the right time; I'll ping upstream reviewers in about half an hour.

itsjeyd · 2016-08-15T13:24:41Z

README.md

@@ -141,7 +141,7 @@ standard mode, the feedback text is displayed in a popup after the learner drops
 the item on a zone - the success feedback is shown if the item is dropped on a
 correct zone, while the error feedback is shown when dropping the item on an
 incorrect drop zone.  In assessment mode, the success feedback texts
-are not used, while error feedback texts are shown when learner submits a solution.
+are not used, while error feedback texts are shown when learner submits a solution.3


@e-kolpakov I think this got added accidentally?

itsjeyd · 2016-08-15T13:33:23Z

@e-kolpakov The popup looks good! But now I'm seeing this when submitting an answer on the sandbox (instead of proper feedback):

Probably a regression caused by the rebase?

itsjeyd · 2016-08-26T12:23:41Z

drag_and_drop_v2/public/js/drag_and_drop.js

@@ -1115,6 +1115,7 @@ function DragAndDropBlock(runtime, element, configuration) {

            if (attemptsRemain()) {
                data.misplaced_items.forEach(function(misplaced_item_id) {
+                    state.last_action_correct = false


@haikuginger Hm, I see why this would work, but it feels kind of hacky. It would be good if data.correct were the only "source of truth" regarding the correctness of a solution (in other words, after setting

state.last_action_correct = data.correct;

in L1114 above, ideally it shouldn't be necessary to modify the value of state.last_action_correct again). I haven't looked at #96 in detail, but it seems like after that PR, data.correct should not be truthy if a solution placed one or more decoy items on the board. Is that not the case?

CC @mtyaka, since he reviewed #96.

I'll do some additional debugging to see if with #96 in place, data.correct is giving us what we need.

Okay, looks like #96 did not adjust the logic that sets the correct value on the response. It's not strictly in scope for this task, but I can add it if we want - @itsjeyd, @mtyaka, thoughts?

@haikuginger I think it would be good to take care of it in this context, if it doesn't blow up the scope too much. You might want to confirm with @bradenmacdonald.

We need to get these PRs merged yesterday... so it's your call as to how this affects the scope. If it's a simple, low-risk cleanup, please get it done now. Otherwise, we can postpone it.

Honestly, looking at it, it's pretty much a mess. I'd rather get this delivered and then have some sort of follow-up to clean things up once we actually have a stable codebase.

@haikuginger @itsjeyd @bradenmacdonald I've removed this line and replaced it with a proper fix - in two words, the reason behind this was that self.item_state was manipulated before value for correct was calculated.

So, since this is not the first time I see this problem (expected invocation order violation) in DnDv2 codebase, I've added two TODOs to refactor it to the methods that would benefit from such refactoring most. It doesn't look like a good time to try to address them now, so I've just added TODOs.

@bradenmacdonald let me know if you think we should create tickets for those TODOs.

e-kolpakov · 2016-08-29T12:14:03Z

rebased from bcb06d8

cahrens · 2016-08-29T14:22:34Z

@e-kolpakov Please let us know when this PR is ready for TNL re-review.

e-kolpakov · 2016-08-29T15:19:41Z

@cahrens Sure - I believe @itsjeyd @haikuginger should review and approve it first.

itsjeyd · 2016-08-30T12:10:28Z

drag_and_drop_v2/public/js/drag_and_drop.js

@@ -1021,7 +1097,7 @@ function DragAndDropBlock(runtime, element, configuration) {
            focusFirstDraggable();
        });
    };
-
+    


@e-kolpakov Nit: Remove trailing whitespace.

itsjeyd · 2016-08-30T12:12:34Z

@e-kolpakov I reviewed your recent updates to the code and verified that the fix for decoy item feedback works:

If per-item error feedback is available for a given decoy item, the popup that comes up when placing the item on the board and clicking submit has the expected color (red) and contains the default "Some of your answers were not correct" message, as well as per-item feedback).
If per-item error feedback is not available for a given decoy item, no popup is displayed.

Per-item feedback for "normal"/non-decoy items is working as expected as well.

👍

I tested this: Feature works as advertised in description
I read through the code
Includes documentation

cahrens · 2016-08-30T14:26:13Z

Just to clarify, is this now ready for TNL re-review?

e-kolpakov · 2016-08-30T14:43:46Z

@cahrens Yes, I believe it is now ready.

UPD: ... but to be extra-safe I would recommend waiting for @haikuginger to +1 this PR too.

haikuginger · 2016-08-30T21:39:46Z

👍

I tested this: Quick runthrough of our last few detected bugs shows that they are now working as expected.
I read through the code
Includes documentation

staubina · 2016-08-31T00:41:24Z

👍

e-kolpakov · 2016-08-31T13:36:36Z

@cahrens are we good to merge this PR, or would you like to take another pass?

cahrens · 2016-08-31T14:59:03Z

I'll take a quick look at it later today.

cahrens · 2016-08-31T18:51:17Z

drag_and_drop_v2/drag_and_drop_v2.py

@@ -350,17 +349,29 @@ def do_attempt(self, data, suffix=''):
        self._validate_do_attempt()

        self.attempts += 1
-        self._mark_complete_and_publish_grade()  # must happen before _get_feedback
+        # pylint: disable=fixme
+        # TODO: Refactor this method to "freeze" item_state and pass it to methods that need access to it.


Will this TODO be done in a future story/PR?

@cahrens I've asked @bradenmacdonald about this and haven't got an answer yet. My current understanding is that it's not a good time to further increase the scope of v2.1 with optional refactroings, so it won't be a part of v2.1.

cahrens · 2016-08-31T18:58:32Z

👍

e-kolpakov · 2016-09-01T07:14:53Z

@sstack22 There were quite a lot of changes to this PR since your +1 - do you want to take another look? That'll be the last review step before merging, so I would appreciate if you could allocate some time for this today. Also, I'm going to cut v2.0.9 today to include it into next week release and it would be great to have this PR included.

(I hope it does not sound too pushy - I don't mean to put any pressure on you)

e-kolpakov · 2016-09-01T07:42:58Z

Squashed commits + rebased; previous revision is dc1e3a6

* "Negative" item feedback is displayed for misplaced items when an attempt is submitted. * Avoid showing item-related general feedback (correctly placed-misplaced-not placed) after problem reset. * Disabled submit button while drop_item is in progress. * Added notes and todos about implicit invocation order some methods expect. * Item feedback popup only close when close button is hit or another item is dragged * Translation fixes

sstack22 · 2016-09-01T18:57:23Z

@e-kolpakov - I've re-reviewed, looks go to go 👍

e-kolpakov changed the title ~~Ekolpakov/assessment display item feedback~~ Assessment mode: display item feedback Aug 11, 2016

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch 4 times, most recently from 4eda851 to ccc1f85 Compare August 12, 2016 07:31

itsjeyd reviewed Aug 12, 2016
View reviewed changes

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch 2 times, most recently from 8929cda to bf36721 Compare August 12, 2016 13:13

itsjeyd reviewed Aug 15, 2016
View reviewed changes

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch from d37c21e to 1ad0a6b Compare August 15, 2016 12:11

itsjeyd reviewed Aug 15, 2016
View reviewed changes

itsjeyd reviewed Aug 26, 2016
View reviewed changes

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch from bcb06d8 to feef9ae Compare August 29, 2016 12:13

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch 2 times, most recently from 0d61569 to 648cdd5 Compare August 29, 2016 13:37

itsjeyd reviewed Aug 30, 2016
View reviewed changes

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch from 648cdd5 to dc1e3a6 Compare August 30, 2016 12:53

cahrens reviewed Aug 31, 2016
View reviewed changes

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch from dc1e3a6 to b95d2b0 Compare September 1, 2016 07:42

itsjeyd mentioned this pull request Sep 1, 2016

[SOL-1998] Implement Show Answer button open-craft/xblock-drag-and-drop-v2#10

Merged

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch 2 times, most recently from 00adb1d to 223510c Compare September 1, 2016 13:30

e-kolpakov force-pushed the ekolpakov/assessment-display-item-feedback branch from 223510c to 5f7d26c Compare September 1, 2016 14:35

e-kolpakov mentioned this pull request Sep 1, 2016

Version bump: v2.0.9 #100

Merged

3 tasks

e-kolpakov merged commit f435c5b into master Sep 2, 2016

e-kolpakov deleted the ekolpakov/assessment-display-item-feedback branch November 6, 2017 11:09

Assessment mode: display item feedback #93

Assessment mode: display item feedback #93

Conversation

e-kolpakov commented Aug 11, 2016 • edited Loading

openedx-webhooks commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-kolpakov commented Aug 12, 2016

e-kolpakov commented Aug 12, 2016

e-kolpakov commented Aug 12, 2016

haikuginger commented Aug 12, 2016

Choose a reason for hiding this comment

itsjeyd commented Aug 15, 2016 • edited Loading

e-kolpakov commented Aug 15, 2016

Choose a reason for hiding this comment

itsjeyd commented Aug 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-kolpakov commented Aug 29, 2016

cahrens commented Aug 29, 2016

e-kolpakov commented Aug 29, 2016

Choose a reason for hiding this comment

itsjeyd commented Aug 30, 2016

cahrens commented Aug 30, 2016

e-kolpakov commented Aug 30, 2016 • edited Loading

haikuginger commented Aug 30, 2016

staubina commented Aug 31, 2016

e-kolpakov commented Aug 31, 2016

cahrens commented Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Aug 31, 2016

e-kolpakov commented Sep 1, 2016

e-kolpakov commented Sep 1, 2016 • edited Loading

sstack22 commented Sep 1, 2016

e-kolpakov commented Aug 11, 2016 •

edited

Loading

itsjeyd commented Aug 15, 2016 •

edited

Loading

e-kolpakov commented Aug 30, 2016 •

edited

Loading

e-kolpakov commented Sep 1, 2016 •

edited

Loading