[TNL-10995] fix: make max attempts setting fallback to default #404
Conversation
The max attempts setting for a problem xblock should be set to null for course default max attempt setting to take effect. This makes sure that xblock setting is updated if course default is updated.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
==========================================
+ Coverage 90.47% 90.49% +0.02%
==========================================
Files 228 228
Lines 4115 4115
Branches 822 822
==========================================
+ Hits 3723 3724 +1
+ Misses 371 370 -1
Partials 21 21
☔ View full report in Codecov by Sentry. |
|
|
||
| test('Test score settings missing with null default', () => { | ||
| const settings = parseSettings(singleSelectWithHints.metadata, { max_attempts: null }); | ||
| expect(settings.scoring).toStrictEqual({ attempts: { number: '', unlimited: true } }); |
There was a problem hiding this comment.
@navinkarkera While the number seems to be either null or a real number everywhere, it is an empty string here and SettingParser.js Line 27. Is this intentional? Does null and empty string have different meanings? Having 2 different ways to express "nothing" is confusing.
If it is intentional, it is better to consolidate them into a single value. And if, for some reason, they are not avoidable and have a special meaning, I think we should extract them into constants like COURSE_DEFAULT_VALUE = null, UNLIMITED_VALUE = '', so their meanings are communicated.
There was a problem hiding this comment.
@tecoholic Nice catch! Initially, I thought without setting the state to '', I cannot display blank in attempts field as setting to null showed up as Null. Your comment made me retrace the state and set the display value without actually changing the state.
I have updated it now to use only null in the state except for few tests which is making sure that we are converting '' to null in the parser.
navinkarkera
force-pushed
the
navin/fix-max-attempts-setting
branch
from
3e05881 to
6889cd1
Compare
There was a problem hiding this comment.
@navinkarkera 👍 LGTM
- I tested this: Verified that the problems matching the default attempts set in the course settings get the value updated when the course's "maximum attempts" are updated.
- I read through the code
- I checked for accessibility issues
- Includes documentation
|
@cablaa77 @jristau1984: Hi, the fix for (TNL-10995) is ready for your review. |
|
@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
navinkarkera
changed the title
The max attempts setting for a problem xblock should be set to null for course default max attempt setting to take effect. This makes sure that xblock setting is updated if course default is updated.
Private-ref: BB-7940Test instructions:
new_core_editors.use_new_text_editorandnew_core_editors.use_new_problem_editorwaffle flag.