Problem Builder - Review for release on edx.org #119
Conversation
Fix two bugs (OC-839)
Add an extra selector to fix CSS rule ambiguity
previous PR (#47). The new tests check if: - preferred block attrs are used when building block tree for current course - block tree excludes pb-choice blocks - blocks are correctly marked as (in)eligible - new-style keys are handled correctly - student_view passes calls rendering method with correct template args - author_view and studio_view show appropriate messages
Add unit tests for instructor tool
…ames Instructor Tool: Support searching for answers from multiple users
Simplify get_message: Return message instead of template
correctly in the LMS.
in column headers.
Fix: Translate links in table column headers
…sage-fix Making question checkmark display question-level feedback + tests
Assessment targeted review
New version of mentoring block that supports explicit steps
navigating to next step.
…buttons in Problem Builder only if (deprecated) assessment mode is enabled. * Always shows the "on-assessment-review-question" button in Step Builder Questionnaire problems. * Fixes button styles to prevent long button names from overflowing the buttons.
Remove "Message (Assessment Review)" from Problem Builder in Studio
pomegranited
changed the title
Handle unresolvable anonymous ids gracefully.
|
@pomegranited Are all the commits/changed files in the PR truly all for this specific PR, or have some been merged in from elsewhere? If so, can you rebase as to make the review process easier? |
|
@gsong Yes, unfortunately, this truly is the commits for this PR. edX runs off the @antoviaque can you confirm? |
|
@gsong @pomegranited Yes - if you look at the requirements on edx-platform, you'll see that it's using v2.0.4 https://github.com/edx/edx-platform/blob/af841336c7e39d634c238cd8a11c5a3a661aa9e2/requirements/edx/edx-private.txt#L11 - which was based on master, during the last update last year: v2.0.4...master Note that the main requirement for Davidson is to have the Instructor Tool XBlock, which wasn't in v2.0.4; if some of the other changes are too big, we can always look at deactivating some of the new blocks this is adding. We will need to keep the timeline and time budget small, so don't hesitate. |
…their related XBlocks. * Moves the choice-label <input> elements inside of their <label> tags. * Wraps long-answer and slider question text in a <label> tag. * Uses <th> instead of <td> elements inside <thead>, for the instructor tool and plot preview tables. * Links the feedback, and choice-tip text with the choice labels via aria-describedby. * Wraps the choice-result feedback icons inside the label elements, to match CAPA a11y design. * Adds translated, aria-visible labels for the "correct answer/choice" checkmark and "incorrect answer/choice" X. * Adds the 'aria-live="polite"' attribute to the divs whose content changes dynamically. * Removes block-level children from <legend>, placing the h3.question-title outside the <fieldset>. * Fixes tests broken by above commits * Adds tests for new aria-label attributes..
Accessibility updates
| </select> | ||
| {% endif %} | ||
| <button class="mentoring-share-button"> | ||
| <i class="fa fa-share-alt"></i> Share |
There was a problem hiding this comment.
This string isn't being translated.
There was a problem hiding this comment.
Oo.. thank you for catching all of these! Fixed with 8d096b6.
| <div class="share-errors"></div> | ||
| </div> | ||
| <div class="share-action-buttons"> | ||
| <button class="do-share-button">Share</button> |
| </p> | ||
| <p> | ||
| You can change the user you're currently displaying using the drop-down selector above. | ||
| {% endblocktrans %} |
There was a problem hiding this comment.
We generally avoid including HTML in our translation strings. I think it makes sense for these two paragraphs to be different blocktrans blocks.
There was a problem hiding this comment.
Ah, good point, thank you!. Fixed with 8d096b6.
| {% if course_name %}{% trans "Course" %}: {{course_name}}<br>{% endif %} | ||
| {% trans "Date" %}: {% now "DATE_FORMAT" %}<br> | ||
| </div> | ||
| REPORT_GOES_HERE |
There was a problem hiding this comment.
I don't think this should be getting translated, but I'm not sure why this is here? Wouldn't it make sense to put in some more user-friendly placeholder text here?
There was a problem hiding this comment.
This string is replaced by review_blocks.js with the table that's already been rendered to the page, and then delivered as an encoded dataURI so that students can download the report.
I agree it isn't the prettiest mechanism, but the string itself never gets seen.
| </div> | ||
| </div> | ||
|
|
||
| <div class="review-link"><a href="#">Review final grade</a></div> |
There was a problem hiding this comment.
This string hasn't been translated.
| <input type="button" class="input-main" value="Submit" disabled="disabled" /> | ||
| <input type="button" class="input-next" value="Next Step" disabled="disabled" /> | ||
| <input type="button" class="input-review" value="Review grade" disabled="disabled" /> | ||
| <input type="button" class="input-try-again" value="Try again" disabled="disabled" /> |
There was a problem hiding this comment.
Several of these buttons haven't been translated.
| </div> | ||
| <div class="review-link"><a href="#">Review final grade</a></div> |
There was a problem hiding this comment.
This string hasn't been translated.
| <p class="review-tips-intro">{% trans "You might consider reviewing the following items before your next assessment attempt:" %}</p> | ||
| <ul class="review-tips-list"> | ||
| {% for tip in tips %} | ||
| <li>{{tip|safe}}</li> |
There was a problem hiding this comment.
There's also a display of tips above that aren't marking the tips as safe. I would fall on the side of removing the safe filter here as well.
There was a problem hiding this comment.
Because our tips editor is the TinyMCE WYSIWYG editor, all our tips are likely to contain some HTML, so we need to keep this |safe filter on to prevent the HTML from being escaped.
The tips above in mentoring_assessment_templates.html are also implicitly displayed without html escaping, as they're using Javascript templates to display the tip text.
You can see an example of editing a tip in the Studio Sandbox course, and the results in the LMS under:
- Features > Assessment Mode > Step Builder - Assessment Mode (implicit) > Step Builder > Step 1 - shows the
sb-review-per-question-feedback.htmltips. - Features > Assessment Mode > Problem Builder - Assessment Mode (deprecated) > Problem Builder > Step 1 - shows the
mentoring_assessment_templates.htmltips.
| def _build_course_tree(self): | ||
| """ | ||
| Return flat tree of blocks belonging to this block's parent course. | ||
| """ |
There was a problem hiding this comment.
There are some concerns about the performance of this code, because reconstructing the course tree is expensive. It won't hold up the merging of this PR, but it's something to think about if/when this block gains greater adoption.
One thing to consider is using the course blocks API in the future for things like this.
There was a problem hiding this comment.
Definitely something to consider, especially if we could hit the Course Blocks API from the client side, so that authentication was already taken care of.
There was a problem hiding this comment.
I'd like to echo @dianakhuang's concerns on this. In addition to the performance implications, there's a real chance that future API changes could cause this sort of "climb to the root and crawl the whole course tree" behavior to break, because we can't realistically support it in a performant way. The Course Blocks API will be much faster for large courses, and is an API we're supporting for these kinds of broad cross-course content queries.
I understand that the contents of this PR are time-sensitive, and the short term risk of performance issues are low, so I'm not going to block this PR over this issue. However, I would like some plan for followup work to remove the course crawling behavior. That might mean hitting the endpoint on the client side, exposing Course Blocks API as a runtime service to XBlocks, or reworking the feature so that the course crawl is no longer necessary.
Can we please get that work scheduled?
There was a problem hiding this comment.
Absolutely, @ormsbee, thank you for raising this! I'll create a ticket and keep you posted.
There was a problem hiding this comment.
@ormsbee I'm working on this issue now, by hitting the Course Blocks API from the client, and it's looking good. @bradenmacdonald says this is something he's wanted to clean up for a while!
I should have something ready for upstream review early next week.
There was a problem hiding this comment.
Awesome, thank you so much!
BTW, we just had a discussion yesterday where it came up that T&L is planning to add the course blocks API as an XBlock runtime service to help power pluggable navigation efforts:
https://openedx.atlassian.net/wiki/display/TNL/UIBlocks+for+Course+Outline
That being said, I think your approach with hitting Course Blocks API from the client-side is the right one in this case, since it won't adversely affect the XBlock server side rendering for large sequences. I just didn't want you folks to be surprised when the course blocks runtime service pops up.
Take care!
| block_types = data.get('block_types', None) | ||
| usernames = data.get('usernames', None) | ||
| root_block_id = data.get('root_block_id', None) | ||
| match_string = data.get('match_string', None) |
There was a problem hiding this comment.
Per @ormsbee, there's a security concern here in that there's the possibility that a staff user could craft a request that gets back the answers for any student in any course. All the CCX coaches now have staff access so we have to be especially careful about this sort of thing.
There was a problem hiding this comment.
That's a valid concern and good to be aware of. I believe this code is safe, but please let me know if my assessment is incorrect?
The blocks returned are fetched via modulestore.get_items(), using the modulestore identified by the current runtime's course_id. See the call to tasks.export_data(), and the export_data task itself.
|
Thank you for your review, @dianakhuang! I've fixed the missing translations, and have addressed your other concerns inline. Please let me know if my responses are insufficient, or if there's anything else that needs fixing before this can be merged? I've also updated the sandbox to use the latest changeset. |
|
It looks good! Thanks for the changes and the responses. 👍 |
|
Thanks, @dianakhuang! @gsong Do I need any other approvals before merging this? Thank you! |
|
@pomegranited Good to go! |
Bump version to 2.6.0
|
Thank you, @dianakhuang and @gsong ! Merging now. |
Desired Deployment Date: 2016-10-21
Main PR for making the latest version of problem-builder available on edx.org.
Sandbox
LMS: http://pr13641.sandbox.opencraft.hosting/
Studio: http://studio-pr13641.sandbox.opencraft.hosting/
Features
Step Builder XBlock
XBlock that replaces and extends assessment mode functionality.
Instead of having Problem Builder support two different modes ("standard" and "assessment"), we now have a dedicated XBlock for the functionality that each mode provides:
To ensure backward compatibility with existing Problem Builder instances operating in assessment mode, the code that implements this mode was not removed from the Problem Builder XBlock when Step Builder was first implemented. It is still part of the Problem Builder code base (but will be removed in the future). However, to start the process of deprecating assessment mode, Studio functionality for setting the mode on a Problem Builder block has been removed, which effectively means that newly created instances of Problem Builder do not support assessment mode.
Main features and follow-up changes are listed in the pull requests below.
Implementation (main features)
#62
Introduces
MentoringWithExplicitStepsBlock(Step Builder) andMentoringStepBlock(represents a single step in Step Builder), and adds support for editing them in Studio.#63
(Follows up on #62)
Adds step navigation and submission functionality to Step Builder.
#65
(Follows up on #63)
Introduces
ReviewStepblock for Step Builder.A Step Builder instance only allows a single
ReviewStepblock. TheReviewStepblock enables assessment functionality for Step Builder:#66
(Follows up on #65)
Refines support for customizable assessment messages for Review Step blocks.
Takes first step towards deprecating assessment functionality of Problem Builder by removing the ability to change the mode of Problem Builder blocks to assessment mode.
Includes a couple of additional fixes:
"grade"event when user completes an attempt (submits the last step).#74
Introduces customizable label for button that takes learner to the next step in Step Builder.
#76
Adds support for steps that don't contain any questions to Step Builder
#73
Adds support for video blocks and thumbnails to Step Builder
#84
Adds support for displaying step-level feedback to Step Builder. Step-level feedback is displayed in a modal window.
#89
Simplifies Review Step code; splits different parts of review screen into XBlocks that are more flexible and re-orderable; makes the Step Builder block preview/work in Studio as students would see it in the LMS
#93
Adds message explaining how to use review links to the "Score Summary" block that can be added to the "Review Step" of a Step Builder exercise
Implementation (follow-up)
#68 (fixes)
Follows up on #65 and #66.
#69 (fix)
(Follows up on #65)
Fixes appearance of links for jumping to individual steps from Review Step block.
#79 (fix)
Updates requirements to pull in fix from this PR: openedx-unsupported/xblock-utils#29
#81
Make behavior of MCQs consistent with behavior of MRQs regarding feedback (display question-level feedback first; switch to per-choice feedback when clicking feedback icon next to choice)
#85 (fixes)
Fixes a number of issues from client QA for Step Builder
#88 (fix)
Fixes JS error causing Step Builder to freeze/crash if it doesn't contain a Review Step block
#90 (fixes)
Fixes additional issues from client QA (for Step Builder and Problem Builder):
Adds support for moving messages below the "Try Again" button in Review Step blocks.
#91
Implements additional fixes and improvements from client QA for Step Builder
#99 (fix)
Fixes performance issue affecting Step Builder blocks with many steps (makes sure click on "Next" button does not add additional rendering step)
#111 (fix)
Makes sure choice-level feedback for MCQs behaves consistently when returning to MCQ in Step Builder (feedback should always be visible)
#115 (fix)
Makes sure video playback stops when navigating to next step in Step Builder
This new feature was added using the pull requests below.
#77
Introduces new Plot XBlock that can be used to visualize answers to pairs of rating questions in Step Builder
#82
(Follows up on #77)
Introduces new Plot Overlay XBlock which allows course authors to define additional data points for learners to compare their own results to when viewing a Plot.
#86
(Follows up on #77 and #78)
#87 (fix)
Makes sure tooltips for plot points work across different browsers
#91
Implements additional fixes and improvements from client QA for Step Builder
This feature was implemented using the pull requests below.
#78
Introduces new Ranged Value Slider XBlock:
This block is a variation of the Rating question type. It functions as a (horizontal) slider that allows learners to select arbitrary values from a predefined range (0-100).
Endpoints of the range are customizable, i.e., course authors can assign arbitrary labels to the highest and lowest possible values.
Both Problem Builder and Step Builder support this XBlock.
#86
(Follows up on #77 and #78)
These features already existed, but their behavior has been modified and extended in the following pull requests.
#81
Make behavior of MCQs consistent with behavior of MRQs regarding feedback (display question-level feedback first; switch to per-choice feedback when clicking feedback icon next to choice)
#102
Introduces instance-wide option to hide previous answers (and feedback) for MCQs in Problem Builder.
When this option is enabled, students still receive per-question feedback after providing answers to MCQs and submitting the containing block, but that feedback is not visible when they return to the block, and all choices for a given MCQ are unchecked.
#101
Changes behavior of MCQs to show choice-level tips right away when clicking "Submit" in Problem Builder (instead of waiting for the user to click the icon displayed next to a choice)
#104
Introduces an instance-wide option to hide global feedback and results (correct/incorrect) icons for Long Answer blocks in Problem Builder.
When this option is enabled (and there are some attempts left), students still receive global and per-question feedback after providing answers and submitting the containing block, but that feedback is not visible when they return to the block.
#105 (fix)
Restores functionality for showing choice-level feedback on click for MCQs in Problem Builder
#110 (fixes)
Follows up on #104 and #102
#116 (fix)
Follows up on #110
Makes sure question-level feedback for MRQs is hidden on return if Problem Builder instance is configured to hide previous answers.
Assessment mode was enhanced with the following pull requests.
#60
Adds ability to bring question-level feedback message back by clicking checkmark icon (assessment mode)
#61
Allow content authors to set an optional "review message" on each MCQ/MRQ/Rating question.
The review messages are shown in assessment mode when the student is reviewing their grade before being allowed to try again. This means that the review screen now includes a list of topics to review, based on which questions the student got wrong.
If the student got all the questions right or has no attempts remaining, the new review section is not shown.
#64 (fix)
Follows up on #61.
Makes sure "jump_to_id" URLs in (review) messages are rewritten to yield working links
#67 (fix)
Fixes regression where last question of a mentoring block in assessment mode can't be submitted if:
#90 (fixes)
Fixes additional issues from client QA (for Step Builder and Problem Builder):
Adds support for moving messages below the "Try Again" button in Review Step blocks.
#122 (fix)
Fixes issues related to the
Message (Assessment Review)andMessage (Review)components:The existing Self-Assessment Summary Dashboard XBlock was enhanced with the following pull requests.
#71 (fix)
Fixes issue keeping Dashboard XBlock from loading (circular dependency)
#107 (fix)
Fixes report image download for Dashboard XBlock
Changes already in edx-release (via #106)
#109
Follows up on #107
Updates image link in tests
#108
Adds instructions for enabling Dashboard XBlock in Studio
The existing Instructor Tool XBlock was enhanced with the following pull requests.
#47
Follows up on #45.
Instructor Tool:
#50
Follows up on #47. Extends unit test coverage for the Instructor Tool block.
#57
Extends existing functionality of the instructor tool by allowing users to enter multiple usernames to search for answers from multiple users simultaneously.
The existing Table XBlock was enhanced with the following pull requests.
#49
Follow-up to #43, which introduced functionality for
Implements various UI fixes related to functionality introduced in the previous PR.
#59 (fix)
Makes sure links to course units that are part of table column headers are expanded correctly inside the LMS.
Dogwood compatibility was improved with the following pull requests, including a version bump to 2.5.0.
#94
Adds Django 1.8 migrations to Problem Builder.
This PR was opened against the edx-release branch, and the changes it introduces were brought into master via #95 (along with additional migrations for models not present in the edx-release branch).
#95
Adds Django 1.8 migrations to Problem Builder.
Extended version of #94 (adds additional migrations; updates xblock-utils version in requirements.txt)
#98
Version bump to v2.5.0 for master
These fixes were applied to the Problem Builder Mentoring feature.
#48 (fix)
Fixes JS bug when HTML block is followed by an answer block. Bug was related to Mentoring v1 Problem Builder migration.
#51 (fix)
Adds option for specifying that question titles should not be shown, to work around the fact that they were getting added to questions that didn't have them before (because they were created using a version of xblock-mentoring that didn't offer question titles as an option). Bug was related to Mentoring v1 Problem Builder migration.
#52 (fixes)
Implements fixes for two more migration-related issues:
The following pull requests describe other miscellaneous fixes and improvements.
#53 (fix)
Fixes Problem Builder's LMS theme on HGSE instance
#54 (fix)
Handle situations in which
self.runtime.get_blockreturnsNonegracefully (display error message instead of replacing whole subsection with 500 error)#58
Clean up: Have
MentoringBlock.get_messagereturn message itself (not a template of it).#80 (fix)
Makes sure Problem Builder blocks appear on a student's "Progress" page, even if a learner has not yet submitted a grade.
#92
Fixes for flaky tests
#103
Follows up on #102
Ensures default theme is used by Problem Builder and Step Builder if theme has not been customized
#120 (fix)
New version includes fixes from openedx-unsupported/xblock-utils#38 and openedx-unsupported/xblock-utils#39.
Also fixes an issue affecting dependency installation for CircleCI builds for problem-builder. See the commit message of 51277a3 for details.
#121 (fix)
Refactors Problem Builder (MentoringBlock) to subclass
StudioContainerWithNestedXBlocksMixin from xblock-utils, which is now the
standardized way of dealing with nested XBlocks.
Allows children of Problem Builder blocks to be reordered in Studio.
#124 (fix)
In some cases it is possible that the anonymous
student_idof a submission can't be reversed to the original user. In this case, the functionuser_by_anonymous_id()returnsNone, resulting in a crash when building reports.This change fixes the crash by returning
student_idinstead of the actual user name in case it cannot be reconstructed.#125 (fix)
Improves the accessibility of the Problem Builder, Step Builder, and their related XBlocks.
<label>tags.<label>tag.<th>instead of<td>elements inside<thead>, for the instructor tool and plot preview tables.aria-describedby.<label>elements, to match CAPA a11y design.aria-labels for the "correct answer/choice" checkmark and "incorrect answer/choice" X.<fieldset><legend>The following known issues will be fixed as part of subsequent PRs. However, if any of them are showstoppers, we may instead decide to remove the affected feature(s) from this PR.
Low priority issues
These issues have been identified and deemed "low priority", and may be fixed in subsequent PRs.
The full list of merged PRs is given below.
aft/problem-builder/pull/119/commits/313837a) Merge pull request #115 from open-craft/jbzdak/oc-1441/detach-html-elements