Add percentage_of_mark type to ExtraMark model

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)

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]

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') }

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,26 +110,30 @@ 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
           max_mark_hash[assessment_id] ||= Assignment.find(assessment_id)&.max_mark(user_visibility)
           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

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

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