Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,6 @@ class CourseGradingSerializer(serializers.Serializer):
course_details = CourseGradingModelSerializer()
show_credit_eligibility = serializers.BooleanField()
is_credit_course = serializers.BooleanField()
default_grade_designations = serializers.ListSerializer(
child=serializers.CharField()
)
3 changes: 2 additions & 1 deletion cms/djangoapps/contentstore/rest_api/v1/views/grading.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ def get(self, request: Request, course_id: str):
"minimum_grade_credit": 0.7
},
"show_credit_eligibility": false,
"is_credit_course": true
"is_credit_course": true,
"default_grade_designations": ["A","B","C","D"]
kaustavb12 marked this conversation as resolved.
Show resolved Hide resolved
}
```
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,22 @@ def test_course_grading_response(self):
"course_details": grading_data.__dict__,
"show_credit_eligibility": False,
"is_credit_course": False,
"default_grade_designations": ['A', 'B', 'C', 'D'],
}

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertDictEqual(expected_response, response.data)

@patch("django.conf.settings.DEFAULT_GRADE_DESIGNATIONS", ['A', 'B'])
def test_default_grade_designations_setting(self):
"""
Check that DEFAULT_GRADE_DESIGNATIONS setting reflects correctly in API.
"""
response = self.client.get(self.url)

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(['A', 'B'], response.data["default_grade_designations"])

@patch.dict("django.conf.settings.FEATURES", {"ENABLE_CREDIT_ELIGIBILITY": True})
def test_credit_eligibility_setting(self):
"""
Expand Down
3 changes: 2 additions & 1 deletion cms/djangoapps/contentstore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,8 @@ def get_course_grading(course_key):
'grading_url': reverse_course_url('grading_handler', course_key),
'is_credit_course': is_credit_course(course_key),
'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_key),
'course_assignment_lists': dict(course_assignment_lists)
'course_assignment_lists': dict(course_assignment_lists),
'default_grade_designations': settings.DEFAULT_GRADE_DESIGNATIONS
}

return grading_context
Expand Down
9 changes: 9 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2439,6 +2439,15 @@
# Rate limit for regrading tasks that a grading policy change can kick off
POLICY_CHANGE_TASK_RATE_LIMIT = '900/h'

# .. setting_name: DEFAULT_GRADE_DESIGNATIONS
# .. setting_default: ['A', 'B', 'C', 'D']
# .. setting_description: The default 'pass' grade cutoff designations to be used. The failure grade
# is always 'F' and should not be included in this list.
# .. setting_warning: The DEFAULT_GRADE_DESIGNATIONS list must have more than one designation,
# or else ['A', 'B', 'C', 'D'] will be used as the default grade designations. Also, only the first
# 11 grade designations are used by the UI, so it's advisable to restrict the list to 11 items.
DEFAULT_GRADE_DESIGNATIONS = ['A', 'B', 'C', 'D']

############## Settings for CourseGraph ############################

# .. setting_name: COURSEGRAPH_JOB_QUEUE
Expand Down
5 changes: 3 additions & 2 deletions cms/static/js/factories/settings_graders.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ define([
], function($, GradingView, CourseGradingPolicyModel) {
'use strict';

return function(courseDetails, gradingUrl, courseAssignmentLists) {
return function(courseDetails, gradingUrl, courseAssignmentLists, gradeDesignations) {
var model, editor;

$('form :input')
Expand All @@ -19,7 +19,8 @@ define([
editor = new GradingView({
el: $('.settings-grading'),
model: model,
courseAssignmentLists: courseAssignmentLists
courseAssignmentLists: courseAssignmentLists,
gradeDesignations: gradeDesignations
});
editor.render();
};
Expand Down
6 changes: 5 additions & 1 deletion cms/static/js/views/settings/grading.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) {
$('#course_grade_cutoff-tpl').text()
);
this.setupCutoffs();
this.setupGradeDesignations(options.gradeDesignations);

this.listenTo(this.model, 'invalid', this.handleValidationError);
this.listenTo(this.model, 'change', this.showNotificationBar);
Expand Down Expand Up @@ -318,7 +319,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) {
Copy link
Contributor

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()

Copy link
Contributor Author

@kaustavb12 kaustavb12 Jun 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keithgg

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.

// TODO shouldn't we disable the button
return;
}
Expand Down Expand Up @@ -399,6 +400,9 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) {
this.descendingCutoffs = _.sortBy(this.descendingCutoffs,
function(gradeEle) { return -gradeEle.cutoff; });
},
setupGradeDesignations: function(gradeDesignations) {
if (Array.isArray(gradeDesignations) && gradeDesignations.length > 1) { this.GRADES = gradeDesignations.slice(0, 11); }
},
revertView: function() {
var self = this;
this.model.fetch({
Expand Down
10 changes: 5 additions & 5 deletions cms/static/sass/views/_settings.scss
Original file line number Diff line number Diff line change
Expand Up @@ -777,23 +777,23 @@
height: 17px;
}

&:nth-child(1) {
&:nth-child(5n+1) {
background: #4fe696;
}

&:nth-child(2) {
&:nth-child(5n+2) {
background: #ffdf7e;
}

&:nth-child(3) {
&:nth-child(5n+3) {
background: #ffb657;
}

&:nth-child(4) {
&:nth-child(5n+4) {
background: #ef54a1;
}

&:nth-child(5),
&:nth-child(5n+5),
&.bar-fail {
background: #fb336c;
}
Expand Down
1 change: 1 addition & 0 deletions cms/templates/settings_graders.html
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
),
"${grading_url | n, js_escaped_string}",
${course_assignment_lists | n, dump_js_escaped_json},
${default_grade_designations | n, dump_js_escaped_json},
);
});
</%block>
Expand Down
Loading