fix floating point error in Quiz points_possible arithmetc

closes CNVS-33804

Test Plan
before checking out the change, create a quiz with enough fractional
point questions to create rounding error. Here's one: 29 questions at
3.3 points each. Or 20 at 4.02 points each.
The quiz show page should have a points value that isn't precisely right.
As a student, create a submission to the quiz.
Check out this change. Run migrations.
Ensure the quiz no longer has imprecise points_possible.
Ensure the submission does not need regrading.
Ensure editing or creating quizzes with fractional points no longer
creates rounding error. Ensure submissions no longer need regrading
after editing quiz fields unrelated to score.

Change-Id: I6d761a6ceb8dae47b72e9454912c14aa94b6e50b
Reviewed-on: https://gerrit.instructure.com/111956
Reviewed-by: Ryan Taylor <rtaylor@instructure.com>
Tested-by: Jenkins
QA-Review: Michael Hargiss <mhargiss@instructure.com>
Product-Review: Kevin Dougherty <jdougherty@instructure.com>

app/models/quizzes/quiz.rb

@@ -219,18 +219,23 @@ class Quizzes::Quiz < ActiveRecord::Base
     end
   end
 
-  def current_points_possible
-    entries = self.root_entries
-    possible = 0
+  def self.count_points_possible(entries)
+    util = Quizzes::QuizQuestion::AnswerSerializers::Util
+    possible = BigDecimal('0.0')
     entries.each do |e|
-      if e[:question_points]
-        possible += (e[:question_points].to_f * e[:pick_count])
+      if e[:question_points] # QuizGroup
+        possible += (util.to_decimal(e[:question_points].to_s) * util.to_decimal(e[:pick_count].to_s))
       else
-        possible += e[:points_possible].to_f unless e[:unsupported]
+        possible += util.to_decimal(e[:points_possible].to_s) unless e[:unsupported]
       end
     end
-    possible = self.assignment.points_possible if entries.empty? && self.assignment
-    possible
+    possible.to_f
+  end
+
+  def current_points_possible
+    entries = self.root_entries
+    return self.assignment.points_possible if entries.empty? && self.assignment
+    self.class.count_points_possible(entries)
   end
 
   def set_unpublished_question_count
@@ -691,23 +696,17 @@ class Quizzes::Quiz < ActiveRecord::Base
   # be held in Quizzes::Quiz.quiz_data
   def generate_quiz_data(opts={})
     entries = self.root_entries(true)
-    possible = 0
     t = Time.now
     entries.each do |e|
-      if e[:question_points] #QuizGroup
-        possible += (e[:question_points].to_f * e[:pick_count])
-      else
-        possible += e[:points_possible].to_f
-      end
       e[:published_at] = t
     end
-    possible = 0 if possible < 0
     data = entries
     if opts[:persist] != false
       self.quiz_data = data
 
       if !self.survey?
-        self.points_possible = possible
+        possible = self.class.count_points_possible(data)
+        self.points_possible = [possible, 0].max
       end
       self.allowed_attempts ||= 1
       check_if_submissions_need_review
@@ -844,7 +843,9 @@ class Quizzes::Quiz < ActiveRecord::Base
     old_version = self.versions.get(version_number).model
 
     needs_review = false
-    needs_review = true if old_version.points_possible != self.points_possible
+    # Allow for floating point rounding error comparing to versions created before BigDecimal was used
+    needs_review = true if [old_version.points_possible, self.points_possible].select(&:present?).count == 1 ||
+      ((old_version.points_possible || 0) - (self.points_possible || 0)).abs > 0.0001
     needs_review = true if (old_version.quiz_data || []).length != (self.quiz_data || []).length
     if !needs_review
       new_data = self.quiz_data

db/migrate/20170516154345_fix_points_possible_sums_in_quizzes.rb

@@ -0,0 +1,29 @@
+#
+# Copyright (C) 2017 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+
+class FixPointsPossibleSumsInQuizzes < ActiveRecord::Migration[4.2]
+  tag :postdeploy
+
+  def up
+    DataFixup::FixPointsPossibleSumsInQuizzes.send_later_if_production_enqueue_args(
+      :run, {
+        priority: Delayed::LOW_PRIORITY,
+        strand: "fix_points_possible_sums:#{Shard.current.database_server.id}"
+      }
+    )
+  end
+end

lib/data_fixup/fix_points_possible_sums_in_quizzes.rb

@@ -0,0 +1,37 @@
+#
+# Copyright (C) 2017 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+
+module DataFixup
+  module FixPointsPossibleSumsInQuizzes
+    def self.run
+      Quizzes::Quiz.find_ids_in_ranges(:batch_size => 10000) do |min_id, max_id|
+        affected_quizzes.where(id: min_id..max_id).find_each do |quiz|
+          begin
+            possible = Quizzes::Quiz.count_points_possible(quiz.root_entries(true))
+            quiz.update!(points_possible: possible)
+          rescue => e
+            Rails.logger.error("Error occured trying to repair Quiz #{quiz.global_id} #{e}")
+          end
+        end
+      end
+    end
+
+    def self.affected_quizzes
+      Quizzes::Quiz.where('CHAR_LENGTH(CAST(points_possible AS text)) > 8')
+    end
+  end
+end

spec/migrations/fix_points_possible_sums_in_quizzes_spec.rb

@@ -0,0 +1,101 @@
+#
+# Copyright (C) 2017 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+require_relative '../spec_helper.rb'
+
+describe 'FixPointsPossibleSumsInQuizzes' do
+  question_count = 18
+  question_points = 0.544
+  question_sum = 9.792
+
+  before :once do
+    course_factory(active_all: true)
+    q = @course.quizzes.create!(title: 'floating quiz')
+    question_data = { points_possible: question_points, question_type: 'multiple_choice_question' }
+    question_count.times do |i|
+      q.quiz_questions.build(question_data: question_data.merge(name: "root #{i}"))
+    end
+    q.save!
+    @quiz = q
+  end
+
+  before :each do
+    # creates rounding error just like the real thing!
+    points = Array.new(question_count){ question_points }.sum
+    allow(Quizzes::Quiz).to receive(:count_points_possible).and_return(points)
+  end
+
+  context 'for unpublished quiz' do
+    before :each do
+      @quiz.generate_quiz_data
+      @quiz.save!
+      @quiz.reload
+      allow(Quizzes::Quiz).to receive(:count_points_possible).and_call_original
+    end
+
+    it "should repair quiz points_possible from questions" do
+      expect(@quiz.points_possible).to be > question_sum
+      DataFixup::FixPointsPossibleSumsInQuizzes.run
+      expect(@quiz.reload.points_possible).to eq question_sum
+    end
+
+    it "should not require regrade for repaired quizzes" do
+      broken_version = @quiz.versions.current.number
+      DataFixup::FixPointsPossibleSumsInQuizzes.run
+      @quiz.reload
+      expect(@quiz.changed_significantly_since?(broken_version)).to eq false
+      expect(@quiz.versions.current.number).to eq(broken_version + 1)
+    end
+
+    it "should only select quizzes that appear to have rounding error" do
+      healthy_quiz = @course.quizzes.create!(title: 'decimal quiz')
+      healthy_quiz.build_assignment(force: true)
+      healthy_quiz.assignment.points_possible = 10.5
+      healthy_quiz.assignment.save!
+
+      nil_quiz = @course.quizzes.create!(title: 'no points quiz', quiz_type: 'survey')
+
+      affected = DataFixup::FixPointsPossibleSumsInQuizzes.affected_quizzes
+      expect(affected).to include @quiz
+      expect(affected).not_to include healthy_quiz
+      expect(affected).not_to include nil_quiz
+    end
+  end
+
+  context 'for published quiz' do
+    before :each do
+      @quiz.publish!
+      @quiz.reload
+      allow(Quizzes::Quiz).to receive(:count_points_possible).and_call_original
+    end
+
+    it "should repair quiz points_possible from questions" do
+      expect(@quiz.points_possible).to be > question_sum
+      DataFixup::FixPointsPossibleSumsInQuizzes.run
+      expect(@quiz.reload.points_possible).to eq question_sum
+    end
+
+    it "should not require regrade for repaired quizzes" do
+      broken_version = @quiz.versions.current.number
+      DataFixup::FixPointsPossibleSumsInQuizzes.run
+      @quiz.reload
+      expect(@quiz.changed_significantly_since?(broken_version)).to eq false
+      expect(@quiz.versions.current.number).to eq(broken_version + 1)
+    end
+  end
+end

spec/models/quizzes/quiz_spec.rb

@@ -660,6 +660,21 @@ describe Quizzes::Quiz do
     expect(data[2][:answers]).not_to be_nil
   end
 
+  it "should not lose precision when calculating points_possible from entries" do
+    # These values create enough error to test comparison but more arithmetic is
+    # necessary to persist error after roundtrip to storage.
+    expect(Array.new(3){ 3.3 }.sum).to be < 9.9
+
+    q = @course.quizzes.create!(title: "floaty quiz")
+    question_data = { points_possible: 3.3, question_type: 'multiple_choice_question' }
+    3.times do |i|
+      q.quiz_questions.create!(question_data: question_data.merge(name: "root #{i}"))
+    end
+
+    possible = Quizzes::Quiz.count_points_possible(q.root_entries(true))
+    expect(possible).to eq 9.9
+  end
+
   describe "#generate_submission" do
     it "should generate a valid submission for a given user" do
       u = User.create!(:name => "some user")