GradeCalculator#compute_scores returns an array of hashes

refs CNVS-26685

While investigating CNVS-26685, I discovered that
GradeCalculator#compute_scores returns an array of arrays
with arrays in them with values. This nested structure hid
the implicit meaning of the data, which maps to the current,
final, current group, and final group for the computed scores.

This more human-readable data structure improves the ability of
developers to reason about the returned values.

Test plan:

 - Run regression tests on the user, course, and enrollment APIs.

Change-Id: I58c8572f94863e47be5d67e6d50fe795d42f1856
Reviewed-on: https://gerrit.instructure.com/74093
Reviewed-by: Cameron Matheson <cameron@instructure.com>
Tested-by: Jenkins
QA-Review: KC Naegle <knaegle@instructure.com>
Product-Review: Keith T. Garner <kgarner@instructure.com>

Author: Strand McCutchen

app/controllers/users_controller.rb

@@ -207,7 +207,7 @@ def grades_for_student
       }
     }
     calculator = grade_calculator([enrollment.user_id], course, grading_periods)
-    totals = calculator.compute_scores.first.first.first
+    totals = calculator.compute_scores.first[:current]
     totals[:hide_final_grades] = course.hide_final_grades?
     render json: totals
   end
@@ -1961,9 +1961,9 @@ def grades_from_grade_calculator(user_ids, course, grading_periods)
     calculator = grade_calculator(user_ids, course, grading_periods)
     grades = {}
     calculator.compute_scores.each_with_index do |score, index|
-     computed_score = score.first.first[:grade]
-     user_id = user_ids[index]
-     grades[user_id] = computed_score
+      computed_score = score[:current][:grade]
+      user_id = user_ids[index]
+      grades[user_id] = computed_score
     end
     grades
   end

lib/api/v1/course_json.rb

@@ -183,8 +183,8 @@ def nil_grading_period_scores(student_enrollments, mgp_enabled, totals_for_all_g
 
     def mgp_scores_from_calculator(grade_calculator)
       grade_calculator.compute_scores.map do |scores|
-        current_score = scores.first.first[:grade]
-        final_score = scores.second.first[:grade]
+        current_score = scores[:current][:grade]
+        final_score = scores[:final][:grade]
         {
           current_period_computed_current_score: current_score,
           current_period_computed_final_score: final_score,

lib/api/v1/user.rb

@@ -214,31 +214,7 @@ def enrollment_json(enrollment, user, session, includes = [], opts = {})
         json[:sis_import_id] = enrollment.sis_batch_id
       end
       if enrollment.student?
-        json[:grades] = {
-          :html_url => course_student_grades_url(enrollment.course_id, enrollment.user_id),
-        }
-
-        if has_grade_permissions?(user, enrollment)
-          if opts[:grading_period]
-            student_id = user.id
-            if enrollment.is_a? StudentEnrollment
-              student_id = enrollment.student.id
-            end
-
-            course = enrollment.course
-            gc = GradeCalculator.new(student_id, course,
-                                     grading_period: opts[:grading_period])
-            ((current, _), (final, _)) = gc.compute_scores.first
-            json[:grades][:current_score] = current[:grade]
-            json[:grades][:current_grade] = course.score_to_grade(current[:grade])
-            json[:grades][:final_score] = final[:grade]
-            json[:grades][:final_grade] = course.score_to_grade(final[:grade])
-          else
-            %w{current_score final_score current_grade final_grade}.each do |method|
-              json[:grades][method.to_sym] = enrollment.send("computed_#{method}")
-            end
-          end
-        end
+        json[:grades] = grades_hash(enrollment, user, opts[:grading_period])
       end
       if @domain_root_account.grants_any_right?(@current_user, :read_sis, :manage_sis)
         json[:sis_source_id] = enrollment.sis_source_id
@@ -267,8 +243,39 @@ def enrollment_json(enrollment, user, session, includes = [], opts = {})
     end
   end
 
-  protected
-  def has_grade_permissions?(user, enrollment)
+  private
+  def grades_hash(enrollment, user, grading_period)
+    grades = {
+      html_url: course_student_grades_url(enrollment.course_id, enrollment.user_id)
+    }
+
+    if grade_permissions?(user, enrollment)
+      if grading_period
+        student_id = enrollment.is_a?(StudentEnrollment) ? enrollment.student.id : user.id
+        calculator = GradeCalculator.new(
+          student_id,
+          enrollment.course,
+          grading_period: grading_period
+        )
+
+        computed        = calculator.compute_scores.first
+        current, final  = computed[:current], computed[:final]
+
+        grades[:current_score] = current[:grade]
+        grades[:current_grade] = enrollment.course.score_to_grade(current[:grade])
+        grades[:final_score]   = final[:grade]
+        grades[:final_grade]   = enrollment.course.score_to_grade(final[:grade])
+      else
+        grades[:current_score] = enrollment.computed_current_score
+        grades[:current_grade] = enrollment.computed_current_grade
+        grades[:final_score]   = enrollment.computed_final_score
+        grades[:final_grade]   = enrollment.computed_final_grade
+      end
+    end
+    grades
+  end
+
+  def grade_permissions?(user, enrollment)
     course = enrollment.course
 
     (user.id == enrollment.user_id && !course.hide_final_grades?) ||

lib/grade_calculator.rb

@@ -57,19 +57,27 @@ def compute_scores
         select("submissions.id, user_id, assignment_id, score, excused")
     submissions_by_user = @submissions.group_by(&:user_id)
 
-    scores = []
+    result = []
     @user_ids.each_slice(100) do |batched_ids|
       load_assignment_visibilities_for_users(batched_ids)
       batched_ids.each do |user_id|
         user_submissions = submissions_by_user[user_id] || []
         user_submissions.select!{|s| assignment_ids_visible_to_user(user_id).include?(s.assignment_id)}
         current, current_groups = calculate_current_score(user_id, user_submissions)
         final, final_groups = calculate_final_score(user_id, user_submissions)
-        scores << [[current, current_groups], [final, final_groups]]
+
+        scores = {
+          current: current,
+          current_groups: current_groups,
+          final: final,
+          final_groups: final_groups
+        }
+
+        result << scores
       end
       clear_assignment_visibilities_cache
     end
-    scores
+    result
   end
 
   def compute_and_save_scores

lib/gradebook_exporter.rb

@@ -156,21 +156,10 @@ def to_csv
           row << student_sections
           row.concat(student_submissions)
 
-
           if should_show_totals
-            (current_info, current_group_info),
-              (final_info, final_group_info) = grades.shift
-            groups.each do |g|
-              row << current_group_info[g.id][:score] << final_group_info[g.id][:score] if include_points
-              row << current_group_info[g.id][:grade] << final_group_info[g.id][:grade]
-            end
-            row << current_info[:total] << final_info[:total] if include_points
-            row << current_info[:grade] << final_info[:grade]
-            if @course.grading_standard_enabled?
-              row << @course.score_to_grade(current_info[:grade])
-              row << @course.score_to_grade(final_info[:grade])
-            end
+            row += show_totals(grades.shift, groups, include_points)
           end
+
           csv << row
         end
       end
@@ -189,6 +178,34 @@ def enrollments_for_csv(scope, options={})
     enrollments.partition { |e| e.type != "StudentViewEnrollment" }.flatten
   end
 
+  def show_totals(grade, groups, include_points)
+    result = []
+
+    groups.each do |group|
+      if include_points
+        result << grade[:current_groups][group.id][:score]
+        result << grade[:final_groups][group.id][:score]
+      end
+
+      result << grade[:current_groups][group.id][:grade]
+      result << grade[:final_groups][group.id][:grade]
+    end
+
+    if include_points
+      result << grade[:current][:total]
+      result << grade[:final][:total]
+    end
+
+    result << grade[:current][:grade]
+    result << grade[:final][:grade]
+
+    if @course.grading_standard_enabled?
+      result << @course.score_to_grade(grade[:current][:grade])
+      result << @course.score_to_grade(grade[:final][:grade])
+    end
+    result
+  end
+
   def show_totals?
     return true if !@course.feature_enabled?(:multiple_grading_periods)
     return true if @options[:grading_period_id].try(:to_i) != 0

spec/lib/grade_calculator_spec.rb

@@ -133,17 +133,18 @@ def two_groups_two_assignments(g1_weight, a1_possible, g2_weight, a2_possible)
           calc = GradeCalculator.new [@user.id],
                                      @course.id,
                                      :ignore_muted => false
-          (current, _), (final, _) = calc.compute_scores.first
-          expect(current[:grade]).to eq 50
-          expect(final[:grade]).to eq 25
+          scores = calc.compute_scores.first
+          expect(scores[:current][:grade]).to eq 50
+          expect(scores[:final][:grade]).to eq 25
         end
 
         it "should be impossible to save grades that considered muted assignments" do
           @course.update_attribute(:group_weighting_scheme, "percent")
           calc = GradeCalculator.new [@user.id],
                                      @course.id,
                                      :ignore_muted => false
-          expect { calc.save_scores }.to raise_error
+           # save_scores is a private method
+          expect { calc.send(:save_scores) }.to raise_error("Can't save scores when ignore_muted is false")
         end
       end
     end
@@ -153,10 +154,14 @@ def two_groups_two_assignments(g1_weight, a1_possible, g2_weight, a2_possible)
       @assignment.grade_student @user, grade: 5
       @assignment2.grade_student @user, grade: 10
       calc = GradeCalculator.new [@user.id], @course.id
-      (_, current_group_info), (_, final_group_info) = calc.compute_scores.first
-      expect(current_group_info).to eq final_group_info
-      expect(current_group_info[@group.id][:grade]).to eq 50
-      expect(current_group_info[@group2.id][:grade]).to eq 100
+
+      computed_scores = calc.compute_scores.first
+      current_groups  = computed_scores[:current_groups]
+      final_groups    = computed_scores[:final_groups]
+
+      expect(current_groups).to eq final_groups
+      expect(current_groups[@group.id][:grade]).to eq 50
+      expect(current_groups[@group2.id][:grade]).to eq 100
     end
 
     it "should compute a weighted grade when specified" do
@@ -389,16 +394,19 @@ def nil_graded_assignment
 
     calc = GradeCalculator.new([@student2.id, @student1.id], @course)
     grades = calc.compute_scores
-    expect(grades.shift.map { |g,_| g[:grade] }).to eq [100, 100]
-    expect(grades.shift.map { |g,_| g[:grade] }).to eq [50, 50]
+
+    expect(grades.first[:current][:grade]).to eq 100
+    expect(grades.first[:final][:grade]).to eq 100
+    expect(grades.last[:current][:grade]).to eq 50
+    expect(grades.last[:final][:grade]).to eq 50
   end
 
   it "returns point information for unweighted courses" do
     course_with_student
     a = @course.assignments.create! :points_possible => 50
     a.grade_student @student, :grade => 25
     calc = GradeCalculator.new([@student.id], @course)
-    ((grade_info, _), _) = calc.compute_scores.first
+    grade_info = calc.compute_scores.first[:current]
     expect(grade_info).to eq({:grade => 50, :total => 25, :possible => 50})
   end
 
@@ -600,11 +608,11 @@ def check_grades(current, final)
 
       it "can compute grades for a grading period" do
         gc = GradeCalculator.new([@student.id], @course, grading_period: @gp1)
-        (current, _), _ = gc.compute_scores.first
+        current = gc.compute_scores.first[:current]
         expect(current[:grade]).to eql 25.0
 
         gc = GradeCalculator.new([@student.id], @course, grading_period: @gp2)
-        (current, _), _ = gc.compute_scores.first
+        current = gc.compute_scores.first[:current]
         expect(current[:grade]).to eql 75.0
       end
     end
@@ -633,10 +641,8 @@ def set_up_course_for_differentiated_assignments
           create_section_override_for_assignment(@overridden_middle, course_section: @section)
       end
 
-      def get_user_points_and_course_total(user,course)
-        calc = GradeCalculator.new [user.id], course.id
-        final_grade_info = calc.compute_scores.first.last.first
-        [final_grade_info[:total], final_grade_info[:possible]]
+      def final_grade_info(user, course)
+        GradeCalculator.new([user.id], course.id).compute_scores.first[:final]
       end
 
       context "DA" do
@@ -645,22 +651,26 @@ def get_user_points_and_course_total(user,course)
         end
         it "should calculate scores based on visible assignments only" do
           # 5 + 15 + 10 + 20 + 10
-          expect(get_user_points_and_course_total(@user,@course)).to eq [60,100]
+          expect(final_grade_info(@user, @course)[:total]).to eq 60
+          expect(final_grade_info(@user, @course)[:possible]).to eq 100
         end
         it "should drop the lowest visible when that rule is in place" do
           @group.update_attribute(:rules, 'drop_lowest:1')
           # 5 + 15 + 10 + 20 + 10 - 5
-          expect(get_user_points_and_course_total(@user,@course)).to eq [55,80]
+          expect(final_grade_info(@user, @course)[:total]).to eq 55
+          expect(final_grade_info(@user, @course)[:possible]).to eq 80
         end
         it "should drop the highest visible when that rule is in place" do
           @group.update_attribute(:rules, 'drop_highest:1')
           # 5 + 15 + 10 + 20 + 10 - 20
-          expect(get_user_points_and_course_total(@user,@course)).to eq [40,80]
+          expect(final_grade_info(@user, @course)[:total]).to eq 40
+          expect(final_grade_info(@user, @course)[:possible]).to eq 80
         end
         it "shouldnt count an invisible assingment with never drop on" do
           @group.update_attribute(:rules, "drop_lowest:2\nnever_drop:#{@overridden_lowest.id}")
           # 5 + 15 + 10 + 20 + 10 - 10 - 10
-          expect(get_user_points_and_course_total(@user,@course)).to eq [40,60]
+          expect(final_grade_info(@user, @course)[:total]).to eq 40
+          expect(final_grade_info(@user, @course)[:possible]).to eq 60
         end
       end
     end