diff --git a/Changelog.md b/Changelog.md index a5560171cb..868d7fc951 100644 --- a/Changelog.md +++ b/Changelog.md @@ -6,6 +6,7 @@ ### ✨ New features and improvements - Added new loading spinner icon for tables (#7602) +- Added functionality to apply bonuses and penalties as a percentage of the student's earned marks to ExtraMark model (#7702) ### 🐛 Bug fixes - Fix name column search in graders table (#7693) diff --git a/app/models/assignment.rb b/app/models/assignment.rb index ba0303d5d6..6f0ecbe1f8 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -777,8 +777,9 @@ def summary_csv(role) headers[1] << '' result_ids = groupings.pluck('results.id').uniq.compact - total_marks_hash = Result.get_total_marks(result_ids) - 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 = Result.get_total_marks(result_ids, subtotals: subtotals, extra_marks: extra_marks_hash) CSV.generate do |csv| csv << headers[0] csv << headers[1] diff --git a/app/models/extra_mark.rb b/app/models/extra_mark.rb index b81b75dbf0..3c1d22cb83 100644 --- a/app/models/extra_mark.rb +++ b/app/models/extra_mark.rb @@ -7,12 +7,14 @@ class ExtraMark < ApplicationRecord # When you want to avoid allocating strings... PERCENTAGE = 'percentage'.freeze POINTS = 'points'.freeze + PERCENTAGE_OF_MARK = 'percentage_of_mark'.freeze scope :percentage, -> { where(unit: ExtraMark::PERCENTAGE) } scope :points, -> { where(unit: ExtraMark::POINTS) } + scope :percentage_of_mark, -> { where(unit: ExtraMark::PERCENTAGE_OF_MARK) } validates :unit, presence: true - validates :unit, format: { with: /\Apercentage|points\z/ } + validates :unit, format: { with: /\Apercentage|points|percentage_of_mark\z/ } scope :positive, -> { where('extra_mark > 0') } scope :negative, -> { where('extra_mark < 0') } diff --git a/app/models/result.rb b/app/models/result.rb index d994e893a7..662055b012 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -73,9 +73,9 @@ def get_total_mark end # Return a hash mapping each id in +result_ids+ to the total mark for the result with that id. - def self.get_total_marks(result_ids, user_visibility: :ta_visible) - subtotals = Result.get_subtotals(result_ids, user_visibility: user_visibility) - extra_marks = Result.get_total_extra_marks(result_ids, user_visibility: user_visibility) + def self.get_total_marks(result_ids, user_visibility: :ta_visible, subtotals: nil, extra_marks: nil) + subtotals ||= Result.get_subtotals(result_ids, user_visibility: user_visibility) + extra_marks ||= Result.get_total_extra_marks(result_ids, user_visibility: user_visibility, subtotals: subtotals) subtotals.map { |r_id, subtotal| [r_id, [0, (subtotal || 0) + (extra_marks[r_id] || 0)].max] }.to_h end @@ -110,19 +110,20 @@ def self.get_subtotals(result_ids, user_visibility: :ta_visible, criterion_ids: # # +user_visibility+ is passed to the Assignment.max_mark method to determine the # max_mark value only if the +max_mark+ argument is nil. - def self.get_total_extra_marks(result_ids, max_mark: nil, user_visibility: :ta_visible) + def self.get_total_extra_marks(result_ids, max_mark: nil, user_visibility: :ta_visible, subtotals: nil) 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) extra_marks_hash = Hash.new { |h, k| h[k] = nil } max_mark_hash = {} result_data.each do |id, extra_mark, unit, assessment_id| if extra_marks_hash[id].nil? extra_marks_hash[id] = 0 end - if unit == 'points' + if unit == ExtraMark::POINTS extra_marks_hash[id] += extra_mark.round(2) - elsif unit == 'percentage' + elsif unit == ExtraMark::PERCENTAGE if max_mark assignment_max_mark = max_mark else @@ -130,6 +131,9 @@ def self.get_total_extra_marks(result_ids, max_mark: nil, user_visibility: :ta_v assignment_max_mark = max_mark_hash[assessment_id] end extra_marks_hash[id] += (extra_mark * assignment_max_mark / 100).round(2) + elsif unit == ExtraMark::PERCENTAGE_OF_MARK + marks_earned = subtotals[id] || 0 + extra_marks_hash[id] += (extra_mark * marks_earned / 100).round(2) end end extra_marks_hash diff --git a/spec/factories/extra_marks.rb b/spec/factories/extra_marks.rb index b215af1788..0019bdcf87 100644 --- a/spec/factories/extra_marks.rb +++ b/spec/factories/extra_marks.rb @@ -1,14 +1,23 @@ FactoryBot.define do - factory :extra_mark do + factory :extra_mark, class: 'ExtraMark' do association :result description { Faker::Lorem.sentence } unit { 'percentage' } extra_mark { -10.0 } - end - factory :extra_mark_points, class: 'ExtraMark' do - association :result - description { Faker::Lorem.sentence } - unit { 'points' } - extra_mark { 1 } + + factory :extra_mark_percentage do + unit { 'percentage' } + extra_mark { -10.0 } + end + + factory :extra_mark_points do + unit { 'points' } + extra_mark { 1 } + end + + factory :extra_mark_percentage_of_mark do + unit { 'percentage_of_mark' } + extra_mark { 5.0 } + end end end diff --git a/spec/models/result_spec.rb b/spec/models/result_spec.rb index fe5d70668e..de7f34a45f 100644 --- a/spec/models/result_spec.rb +++ b/spec/models/result_spec.rb @@ -172,6 +172,58 @@ expect(Result.get_total_extra_marks(ids)).to eq(expected) end end + + context 'with percentage_of_mark extra marks' do + it 'should calculate extra marks based on percentage of earned score' do + ids = Result.ids + result = Result.find(ids.first) + subtotal = result.get_subtotal + extra_mark = 5.0 + + create(:extra_mark_percentage_of_mark, result: result, extra_mark: extra_mark) + expected = Hash.new { |h, k| h[k] = nil } + expected[ids.first] = (extra_mark * subtotal / 100).round(2) + expect(Result.get_total_extra_marks(ids)).to eq(expected) + end + + it 'should handle zero subtotal when calculating percentage of score' do + ids = Result.ids + result = Result.find(ids.first) + result.marks.update_all(mark: 0) + extra_mark = 10.0 + + create(:extra_mark_percentage_of_mark, result: result, extra_mark: extra_mark) + expected = Hash.new { |h, k| h[k] = nil } + expected[ids.first] = 0.0 + expect(Result.get_total_extra_marks(ids)).to eq(expected) + end + + it 'should calculate extra marks based on percentage of earned score with negative deduction' do + ids = Result.ids + result = Result.find(ids.first) + subtotal = result.get_subtotal + extra_mark = -5.0 + + create(:extra_mark_percentage_of_mark, result: result, extra_mark: extra_mark) + expected = Hash.new { |h, k| h[k] = nil } + expected[ids.first] = (extra_mark * subtotal / 100).round(2) + expect(Result.get_total_extra_marks(ids)).to eq(expected) + end + + it 'should round to 2 decimal places' do + ids = Result.ids + result = Result.find(ids.first) + subtotal = result.get_subtotal + extra_mark = 7.0 + + create(:extra_mark_percentage_of_mark, result: result, extra_mark: extra_mark) + actual = Result.get_total_extra_marks(ids)[ids.first] + expect(actual).to eq(actual.round(2)) + + expected = (extra_mark * subtotal / 100).round(2) + expect(actual).to eq(expected) + end + end end describe '#get_total_mark' do