remove ProvisionalGrade#position

index provisional grades by scorer instead, which simplifies
creation of multiple provisional grades in speedgrader

test plan: specs pass

refs CNVS-22010

Change-Id: I5ee3dd65174549cd251ad6e3c137d89780024043
Reviewed-on: https://gerrit.instructure.com/61279
Tested-by: Jenkins
Reviewed-by: James Williams  <jamesw@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
QA-Review: Jeremy Stanley <jeremy@instructure.com>

app/models/moderated_grading/provisional_grade.rb

@@ -6,13 +6,11 @@ class ModeratedGrading::ProvisionalGrade < ActiveRecord::Base
   belongs_to :submission
   belongs_to :scorer, class_name: 'User'
 
-  validates :position, uniqueness: { scope: :submission_id }
-  validates :position, presence: true
   validates :scorer, presence: true
   validates :submission, presence: true
 
   def valid?(*)
-    set_graded_at if grade_changed?
+    set_graded_at if grade_changed? || score_changed?
     super
   end
 

app/models/submission.rb

@@ -904,20 +904,20 @@ class Submission < ActiveRecord::Base
     false
   end
 
-  def provisional_grade(position)
-    self.provisional_grades.where(position: position).first!
+  def provisional_grade(scorer)
+    self.provisional_grades.where(scorer_id: scorer).first!
   end
 
-  def find_or_create_provisional_grade!(scorer:, position:, score: nil)
+  def find_or_create_provisional_grade!(scorer:, score: nil, grade: nil)
     ModeratedGrading::ProvisionalGrade.unique_constraint_retry do
-      pg = self.provisional_grades.where(position: position).first
+      pg = self.provisional_grades.where(scorer_id: scorer).first
       unless pg
-        attrs = {position: position}
-        attrs.merge!({score: score}) unless score.nil?
-        pg = self.provisional_grades.build(attrs)
+        pg = self.provisional_grades.build
         pg.scorer_id = scorer.id
-        pg.save!
       end
+      pg.grade = grade if grade
+      pg.score = score if score
+      pg.save! if pg.changed?
       pg
     end
   end
@@ -934,8 +934,8 @@ class Submission < ActiveRecord::Base
         opts[:comment] = t('attached_files_comment', "See attached files.")
       end
     end
-    if (pg_pos = opts.delete(:provisional_grade_position))
-      pg = find_or_create_provisional_grade!(scorer: opts[:author], position: pg_pos)
+    if opts.delete(:provisional)
+      pg = find_or_create_provisional_grade!(scorer: opts[:author])
       opts[:provisional_grade_id] = pg.id
     end
     self.save! if self.new_record?

config/initializers/dropped_columns.rb

@@ -113,7 +113,8 @@ class ActiveRecord::Base
     'stream_item_instances' => %w(context_code).freeze,
     'submissions' => %w(changed_since_publish late).freeze,
     'wiki_pages' => %w(hide_from_students).freeze,
-    'lti_resource_placements' => %w(resource_handler_id).freeze
+    'lti_resource_placements' => %w(resource_handler_id).freeze,
+    'moderated_grading_provisional_grades' => %w(position).freeze
   }.freeze
 
   def self.columns_with_remove_dropped_columns

db/migrate/20150819165426_add_unique_index_on_provisional_grade_scorer.rb

@@ -0,0 +1,20 @@
+class AddUniqueIndexOnProvisionalGradeScorer < ActiveRecord::Migration
+  tag :predeploy
+
+  def up
+    # remove constraints on position, which will be dropped in a postdeploy migration
+    change_column_null :moderated_grading_provisional_grades, :position, true
+    remove_index :moderated_grading_provisional_grades, :name => :idx_mg_provisional_grades_unique_submission_position
+
+    # keep only the newest provisional grade for each scorer/submission pair, then add the unique constraint
+    ModeratedGrading::ProvisionalGrade.where("id NOT IN (SELECT * FROM (SELECT MAX(id) FROM moderated_grading_provisional_grades GROUP BY submission_id, scorer_id) x)").delete_all
+    add_index :moderated_grading_provisional_grades,
+              [:submission_id, :scorer_id],
+              unique: true,
+              name: :idx_mg_provisional_grades_unique_submission_scorer
+  end
+
+  def down
+    remove_index :moderated_grading_provisional_grades, :name => :idx_mg_provisional_grades_unique_submission_scorer
+  end
+end

db/migrate/20150819165427_remove_provisional_grade_position.rb

@@ -0,0 +1,16 @@
+class RemoveProvisionalGradePosition < ActiveRecord::Migration
+  tag :postdeploy
+
+  def up
+    # the unique index was dropped in a predeploy migration
+    remove_column :moderated_grading_provisional_grades, :position
+  end
+
+  def down
+    add_column :moderated_grading_provisional_grades, :position, :integer, :limit => 8
+    add_index :moderated_grading_provisional_grades,
+              [:submission_id, :position],
+              unique: true,
+              name: :idx_mg_provisional_grades_unique_submission_position
+  end
+end

spec/models/assignment_spec.rb

@@ -2614,7 +2614,7 @@ describe Assignment do
     it "should exclude provisional comments" do
       setup_assignment_with_homework
       @submission = @assignment.submissions.first
-      @comment = @submission.add_comment(:comment => 'comment', :provisional_grade_position => 1)
+      @comment = @submission.add_comment(:comment => 'comment', :provisional => true)
       json = @assignment.speed_grader_json(@user)
       expect(json[:submissions].first[:submission_comments]).to be_empty
     end
@@ -3436,7 +3436,7 @@ describe Assignment do
       assignment_model(course: @course, moderated_grading: true)
       expect(@assignment).to be_moderated_grading
       submission = @assignment.submit_homework @student, body: "blah"
-      pg = submission.find_or_create_provisional_grade! scorer: @teacher, position: 1, score: 0
+      pg = submission.find_or_create_provisional_grade! scorer: @teacher, score: 0
       @assignment.moderated_grading = false
       expect(@assignment.save).to eq false
       expect(@assignment.errors[:moderated_grading]).to be_present

spec/models/moderated_grading/provisional_grade_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe ModeratedGrading::ProvisionalGrade do
   subject(:provisional_grade) do
-    submission.provisional_grades.new(grade: 'A', score: 100.0, position: 1, scorer: user).tap do |grade|
+    submission.provisional_grades.new(grade: 'A', score: 100.0, scorer: user).tap do |grade|
       grade.scorer = user
     end
   end
@@ -13,18 +13,33 @@ describe ModeratedGrading::ProvisionalGrade do
   let(:now) { Time.zone.now }
 
   it { is_expected.to be_valid }
-  it { is_expected.to validate_presence_of(:position) }
   it { is_expected.to validate_presence_of(:scorer) }
   it { is_expected.to validate_presence_of(:submission) }
-  it { is_expected.to validate_uniqueness_of(:position).scoped_to(:submission_id) }
+
+  describe 'unique constraint' do
+    it "disallows multiple provisional grades from the same user" do
+      pg1 = submission.provisional_grades.build(score: 75)
+      pg1.scorer = user
+      pg1.save!
+      pg2 = submission.provisional_grades.build(score: 80)
+      pg2.scorer = user
+      expect { pg2.save! }.to raise_error(ActiveRecord::RecordNotUnique)
+    end
+  end
 
   describe '#graded_at when a grade changes' do
     it { expect(provisional_grade.graded_at).to be_nil }
-    it 'updates the graded_at timestamp' do
+    it 'updates the graded_at timestamp when changing grade' do
       Timecop.freeze(now) do
         provisional_grade.update_attributes(grade: 'B')
         expect(provisional_grade.graded_at).to eql now
       end
     end
+    it 'updates the graded_at timestamp when changing score' do
+      Timecop.freeze(now) do
+        provisional_grade.update_attributes(score: 80)
+        expect(provisional_grade.graded_at).to eql now
+      end
+    end
   end
 end

spec/models/submission_comment_spec.rb

@@ -58,7 +58,7 @@ describe SubmissionComment do
     end
 
     it "should not dispatch notification on create for provisional comments" do
-      @comment = @submission.add_comment(:author => @teacher, :comment => "huttah!", :provisional_grade_position => 1)
+      @comment = @submission.add_comment(:author => @teacher, :comment => "huttah!", :provisional => true)
       expect(@comment.messages_sent).to be_empty
     end
 
@@ -130,7 +130,7 @@ This text has a http://www.google.com link in it...
   it "should not create a stream item for a provisional comment" do
     prepare_test_submission
     expect {
-      @submission.add_comment(:author => @teacher, :comment => "some comment", :provisional_grade_position => 1)
+      @submission.add_comment(:author => @teacher, :comment => "some comment", :provisional => true)
     }.to change(StreamItem, :count).by(0)
   end
 
@@ -217,10 +217,10 @@ This text has a http://www.google.com link in it...
     end
 
     it "should create reply in the same provisional grade" do
-      comment = @submission.add_comment(:user => @teacher, :comment => "blah", :provisional_grade_position => 1)
+      comment = @submission.add_comment(:user => @teacher, :comment => "blah", :provisional => true)
       reply = comment.reply_from(:user => @teacher, :text => "oops I meant blah")
+      expect(reply.provisional_grade).to eq(comment.provisional_grade)
       expect(reply.provisional_grade.scorer).to eq @teacher
-      expect(reply.provisional_grade.position).to eq 1
     end
   end