-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat: default grade designations configurable from settings #32406
feat: default grade designations configurable from settings #32406
Conversation
Thanks for the pull request, @kaustavb12! 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. |
c3428c8
to
59e3957
Compare
@@ -314,7 +315,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { | |||
addNewGrade: function(e) { | |||
e.preventDefault(); | |||
var gradeLength = this.descendingCutoffs.length; // cutoffs doesn't include fail/f so this is only the passing grades | |||
if (gradeLength > 3) { | |||
if (gradeLength > this.GRADES.length - 1) { |
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.
@kaustavb12 could be a dumb question, but is there a possibility of this.GRADES
being undefined if this line is invoked?
Eg. if options.gradeDesignations == 0 then these lines will trigger an error:
this.setupGradeDesignations([])
this.addNewGrade()
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.
Great question.
We already have a default value of this.GRADES
, therefore I feel the chances of this.GRADES
being undefined is slim.
The place which is most likely to break with an erroneous value of options.gradeDesignations
is the length check in the setupGradeDesignations
method. Although, for most scenarios, such as passing an intereger, strings, empty arrays etc., this seems to be handled elegently by the browser and nothing breaks. Only explicitly passing None/null
seems to break this.
To mitigate that, I have now added a check to verify that gradeDesignations
is infact a valid array. This seems to handle those edge cases I mentioned above.
a399053
to
376514b
Compare
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.
@kaustavb12 LGTM 👍
- I read through the code
- I confirmed that I could replicate the test instructions
376514b
to
7a7af8c
Compare
Hi @jmakowski1123 - checking on this one to see if it should go through Product review? |
Hi @jmakowski1123! Any update on this? |
Product Feature Ticket is ready for product review: https://github.com/orgs/openedx/projects/4/views/20 |
1574512
to
a57f5fb
Compare
I believe this feature now has product approval and I have made the changes requested there. Are we good to move this PR to review now ? |
Thanks for flagging, @kaustavb12! I've sent this over to @openedx/2u-arch-bom to take a look at. |
@mphilbrick211 Just checking to see, could this ticket be CC reviewed ? |
Sorry, looks like we didn't end up routing this properly. @openedx/2u-tnl can you take a look? |
@mphilbrick211 I can help to review this PR 😇 |
Thank you so much, @farhaanbukhsh! |
@farhaanbukhsh, @kaustavb12, if I'm not mistaken, we have around two weeks before the Redwood release cut. It'd be really awesome if we could get this PR reviewed and merged before the cut. |
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.
@kaustavb12 apart from the clarification everything looks good, Kudos on the work.
cb99549
to
5911cbf
Compare
Thanks a lot for the review. Review comment addressed. I have also rebased and squashed the commits. |
jenkins run all |
32e10d0
to
851dafa
Compare
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 tested this on devstack and it works perfectly
- ✅ I read through the code
- ❌ I checked for accessibility issues
- ❌ Includes documentation
- ❌ I made sure any change in configuration variables is reflected in the corresponding client's
configuration-secure
repository.
@kaustavb12 can you rebase the PR, thanks :) |
851dafa
to
218a010
Compare
@farhaanbukhsh Rebased and Squashed |
@kaustavb12 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Currently a maximum of 5 grade cutoffs are configurable from the Grading Settings in Studio.
However, some universities might require more that 5 cutoffs depending on their grading policy.
Also, when a new grade cutoff is added, a default grade desgination is assigned to this cufoff. These default grade designations are hard coded in the javascript code. This means, if the maximum number of grade cutoff are to be increased, more default grade designations also need to be defined.
This PR address this issue.
In this PR, a new setting
DEFAULT_GRADE_DESIGNATIONS
is introduced which can be used to define the pass grade designations. This has a default value of['A', 'B', 'C', 'D']
, which is the current default.The maximum number of grade cutoffs is now dependent on the length of the grade designations list, instead of being hard-coded to 5.
As before, the failure grade designation is always set to
F
which cannot be changed and is not to be included in theDEFAULT_GRADE_DESIGNATIONS
list.Ex. If there is a need to configure 7 grade cutoffs instead of the default 5, the following setting can be used:
The
DEFAULT_GRADE_DESIGNATIONS
list must have more than 1 desginations, otherwise the default grade designations falls back to['A', 'B', 'C', 'D']
.Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Default Scenario
Settings
-->Grading
+
button repeatedly to add new grade cutoffs'A', 'B', 'C', 'D' and 'F'
Modify Grade Designations
cms\envs\devstack.py
:DEFAULT_GRADE_DESIGNATIONS = ['A+', 'A', 'B+', 'B', 'C', 'D']
Settings
-->Grading
+
button repeatedly to add new grade cutoffs'A+', 'A', 'B+', 'B', 'C', 'D' and 'F'
Grade Designations Fallback
cms\envs\devstack.py
:DEFAULT_GRADE_DESIGNATIONS = ['A+']
Settings
-->Grading
+
button repeatedly to add new grade cutoffs'A', 'B', 'C', 'D' and 'F'
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
OpenCraft internal ticket : BB-7378