-
Notifications
You must be signed in to change notification settings - Fork 241
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
Scale rubric criterion levels on max_mark update #7311
base: master
Are you sure you want to change the base?
Scale rubric criterion levels on max_mark update #7311
Conversation
ad5c103
to
83c2862
Compare
Pull Request Test Coverage Report for Build 12602422561Details
💛 - Coveralls |
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.
@pranavrao145 thanks for doing this. I realized now why the code was written the way it was originally, as we didn't want to accidentally override the instructor's inputs on the form (for example, if the instructor changes the criterion max mark and the level weights at the same time).
This is a bit more work, but can you please modify the back-end algorithm so that
- for each existing level, if the
levels_attributes
has a mark that is different from the current level mark, use the newlevels_attributes
mark directly (no scaling); but if thelevels_attributes
mark is the same as the current level mark, scale that. - for each new level, use the instructor-provided mark in
levels_attributes
(no scaling)
I think the easiest way to do this is to keep the original order of criterion/level updating, but prior to the level updating mutate the levels_attributes
hash to compute the new level marks according to my above description.
83c2862
to
5bd8e93
Compare
907828f
to
fc8bf48
Compare
fc8bf48
to
7921401
Compare
self.update(levels_attributes: levels_attributes) | ||
end | ||
|
||
def level_with_mark_closest_to(mark) | ||
self.levels.min_by { |m| (m.mark - mark).abs } | ||
end | ||
|
||
def scale_marks_if_max_mark_changed |
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.
Decided to do this manually, from my obeservations this wasn't getting triggered except on update anyways.
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.
You shouldn't delete this; there may be other ways of updating RubricCriterion
other than the controller method you're working on.
Instead, you can skip this validation in the controller method when calling update
on the criterion.
before_save :round_max_mark | ||
|
||
validates :levels, presence: true | ||
|
||
DEFAULT_MAX_MARK = 4 | ||
# Checks whether the passed in param's level_attributes have unique name and marks. | ||
# Skips the uniqueness validation if true. | ||
def update_levels(levels_attributes) | ||
def update_levels(levels_attributes, scale = 1.0) |
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.
Previously we were looking at changes to max_mark
using self.changes
(which I figured was a callback thing, not sure if that's available/how that works here) - to avoid some weird logic with the same I just made it a parameter the caller can very easily calculate.
6445098
to
8783ba9
Compare
8783ba9
to
8c4d2d3
Compare
should_skip = false | ||
break | ||
# Check if there's a dup in marks or names | ||
unless should_skip |
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.
This branch will never evaluate since should_skip
is initialized to true
self.update(levels_attributes: levels_attributes) | ||
end | ||
|
||
def level_with_mark_closest_to(mark) | ||
self.levels.min_by { |m| (m.mark - mark).abs } | ||
end | ||
|
||
def scale_marks_if_max_mark_changed |
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.
You shouldn't delete this; there may be other ways of updating RubricCriterion
other than the controller method you're working on.
Instead, you can skip this validation in the controller method when calling update
on the criterion.
Proposed Changes
Fixes #7296 by changing the order in which changes are processed when
max_mark
is updated on a rubric criterion.Type of Change
(Write an
X
or a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]
into a[x]
in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request: