-
Notifications
You must be signed in to change notification settings - Fork 71
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
Assessment mode: display item feedback #93
Conversation
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 |
4eda851
to
ccc1f85
Compare
@@ -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'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsjeyd as far as I can tell "initial_feedback" is not used anywhere in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e-kolpakov so should "start" feedback be deleted entirely (Python, studio)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cahrens no, it is still shown in feedback area - it is passed through get_user_state
handler, rather than here.
8929cda
to
bf36721
Compare
@itsjeyd Just once no tests failed, but I didn't notice pep8 errors :( |
@itsjeyd Sandbox updated |
@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! |
The sandbox looks to be correctly provisioned at this point. |
if (msgs.length > 1) { | ||
popup_content = h("div.popup-content", {}, [ | ||
if (ctx.mode == DragAndDropBlock.ASSESSMENT_MODE) { | ||
var content_items = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e-kolpakov message_items
would be a little more to the point I think.
Thanks for making the behavior consistent!
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:
... which is not true anymore as of this PR :) 👍 once remaining comments have been addressed and build is green.
|
d37c21e
to
1ad0a6b
Compare
@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. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e-kolpakov I think this got added accidentally?
@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? |
@@ -1115,6 +1115,7 @@ function DragAndDropBlock(runtime, element, configuration) { | |||
|
|||
if (attemptsRemain()) { | |||
data.misplaced_items.forEach(function(misplaced_item_id) { | |||
state.last_action_correct = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do some additional debugging to see if with #96 in place, data.correct
is giving us what we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
bcb06d8
to
feef9ae
Compare
rebased from bcb06d8 |
0d61569
to
648cdd5
Compare
@e-kolpakov Please let us know when this PR is ready for TNL re-review. |
@cahrens Sure - I believe @itsjeyd @haikuginger should review and approve it first. |
@@ -1021,7 +1097,7 @@ function DragAndDropBlock(runtime, element, configuration) { | |||
focusFirstDraggable(); | |||
}); | |||
}; | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e-kolpakov Nit: Remove trailing whitespace.
@e-kolpakov I reviewed your recent updates to the code and verified that the fix for decoy item feedback works:
Per-item feedback for "normal"/non-decoy items is working as expected as well. 👍
|
648cdd5
to
dc1e3a6
Compare
Just to clarify, is this now ready for TNL re-review? |
@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. |
👍
|
👍 |
@cahrens are we good to merge this PR, or would you like to take another pass? |
I'll take a quick look at it later today. |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this TODO be done in a future story/PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
👍 |
@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) |
dc1e3a6
to
b95d2b0
Compare
Squashed commits + rebased; previous revision is dc1e3a6 |
00adb1d
to
223510c
Compare
* "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
223510c
to
5f7d26c
Compare
@e-kolpakov - I've re-reviewed, looks go to go 👍 |
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:
#87Sandboxes:
Testing instructions:
If only one item is misplaced, only feedback message is shown; if multiple items are misplaced,popup contains a note aboutmultipleitem(s) being misplaced, and feedback messages are shown in form of bullet list.Screenshots:
Reviewers:
a11y: @cptvitamina11y review will be performed later, for all changes in v2.1: SOL-1978Checklist: