Add percentage_of_score type to ExtraMark model #7702
Conversation
Pull Request Test Coverage Report for Build 18566832127Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Changelog.md
Outdated
| - Added new loading spinner icon for tables (#7602) | ||
| - Update message and page displaying cannot create new course via external LTI tool (#7669) | ||
| - Provide file viewer the option to render Microsoft files (#7676) | ||
| - Added percentage_of_score type to ExtraMark model (#7702) |
There was a problem hiding this comment.
I would reword this to be more user-facing. Think about how you would explain this change to an instructor.
Also, please do a merge from master; we've just released 2.8.2, so it's important to make sure your entry still appears in the "unreleased" section.
app/models/result.rb
Outdated
| result_data = Result.joins(:extra_marks, submission: [grouping: :assignment]) | ||
| .where(id: result_ids) | ||
| .pluck(:id, :extra_mark, :unit, 'assessments.id') | ||
| subtotals = Result.get_subtotals(result_ids, user_visibility: user_visibility) |
There was a problem hiding this comment.
Okay, so overall this is the right calculation, but this has the potential to make this method run a fair amount more slowly. I suggest adding subtotals as an optional argument (default nil), so that if get_subtotals was previously computed for result_ids, then it can be passed in and not recomputed here.
After making the change to this function, you should then review every place it's called and see how to make use of the get_subtotals method. This will require some deep investigation of other existing code, but it'll be worth it if we can avoid doing unnecessary work.
spec/factories/extra_marks.rb
Outdated
| unit { 'points' } | ||
| extra_mark { 1 } | ||
| end | ||
| factory :extra_mark_percentage_of_score, class: 'ExtraMark' do |
There was a problem hiding this comment.
This is okay, but please refactor all three factories to use a common parent factory and then has three child factories, one for each type. (You'll need to research how to create parent-child relationships with factories using FactoryBot.)
| extra_marks_hash = Result.get_total_extra_marks(result_ids, max_mark: max_mark) | ||
| subtotals = Result.get_subtotals(result_ids) | ||
| extra_marks_hash = Result.get_total_extra_marks(result_ids, max_mark: max_mark, subtotals: subtotals) | ||
| total_marks_hash = subtotals.merge(extra_marks_hash) do |_result_id, subtotal, extra| |
There was a problem hiding this comment.
Overall we should try to avoid duplicating logic where possible. In a similar vein as what I suggested earlier, please modify the Result.get_total_marks method to optionally take in both subtotals and extra_marks hashes (default nil), and reuse those to compute the result.
This will allow you to keep the total_marks_hash = Result.get_total_marks(result_ids) call, just with extra arguments.
| assignment_max_mark = max_mark_hash[assessment_id] | ||
| end | ||
| extra_marks_hash[id] += (extra_mark * assignment_max_mark / 100).round(2) | ||
| elsif unit == 'percentage_of_score' |
There was a problem hiding this comment.
Yikes, it's not great that we were using the hardcoded strings here. Please replace this (and the other two) with the constants from ExtraMark.
| extra_mark { 1 } | ||
| end | ||
|
|
||
| factory :extra_mark_percentage_of_score do |
There was a problem hiding this comment.
Just noting here to rename this as well, and the test labels/variables/etc.
3 participants
Proposed Changes
(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)
Screenshots of your changes (if applicable)
Associated documentation repository pull request (if applicable)
Type of Change
(Write an
Xor 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:
Questions and Comments
(Include any questions or comments you have regarding your changes.)