-
Notifications
You must be signed in to change notification settings - Fork 60
Hide MRQ question-level feedback on return. #116
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
Conversation
62d826e to
484e428
Compare
| # has a question level feedback. This feedback should also be suppressed. | ||
| ("feedback_persistence_mcq_no_tips.xml", '.feedback') | ||
| ("feedback_persistence_mcq_no_tips.xml", '.feedback'), | ||
| ("feedback_persistence_mrq_no_tips.xml", '.feedback'), |
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.
@mtyaka I think it would be worth extending this test to also cover cases where an MCQ/MRQ has:
- no question-level feedback and no choice-level feedback (no message displayed when clicking checkmark for individual choice after submitting answer)
- both question-level feedback and choice-level (general message displayed after submitting, choice-level message displayed after clicking corresponding checkmark)
Aside from making the code even more robust against regressions, a test that covers all combinations could also serve to document the expected behavior for us.
|
@mtyaka Works for me! Tested on both LMS and Apros (thanks for the course export). Aside from the comment I left about the integration tests, maybe we should add a comment to the code that explains that |
484e428 to
a02dfe5
Compare
|
@itsjeyd Thanks for the good suggestions! I implemented the changes, please take another look. |
| @@ -0,0 +1,11 @@ | |||
| <vertical_demo> | |||
| <problem-builder url_name="mcq_general_feedback" enforce_dependency="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.
@mtyaka Nit: Can we do without enforce_dependency="false" in the templates that are used by test_feedback_persistence_tips? It seems to be redundant since enforce_dependency has a default value of 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.
Thanks @itsjeyd, I removed enforce_dependency. It was redundant indeed.
|
@mtyaka Looking great, thanks for the updates! I had one more nit (sorry for not noticing sooner), but other than that this is 👍 |
When the pb_mcq_hide_previous_answer is set to True, question-level feedback should not be displayed when user returns to the page.
a02dfe5 to
ea3924a
Compare
When the pb_mcq_hide_previous_answer is set to True, question-level feedback should not be displayed when user returns to the page.
cf. MCKIN-3999
Test Instructions
pb_mcq_hide_previous_answerin yourXBLOCK_SETTINGS.