diff --git a/app/models/moderated_grading/provisional_grade.rb b/app/models/moderated_grading/provisional_grade.rb index cc2e630e4b2..6a72824f8a0 100644 --- a/app/models/moderated_grading/provisional_grade.rb +++ b/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 diff --git a/app/models/submission.rb b/app/models/submission.rb index d12095e22a5..afce3780dbe 100644 --- a/app/models/submission.rb +++ b/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? diff --git a/config/initializers/dropped_columns.rb b/config/initializers/dropped_columns.rb index b275a2f1ceb..08f13514a51 100644 --- a/config/initializers/dropped_columns.rb +++ b/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 diff --git a/db/migrate/20150819165426_add_unique_index_on_provisional_grade_scorer.rb b/db/migrate/20150819165426_add_unique_index_on_provisional_grade_scorer.rb new file mode 100644 index 00000000000..a2f062ef94f --- /dev/null +++ b/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 diff --git a/db/migrate/20150819165427_remove_provisional_grade_position.rb b/db/migrate/20150819165427_remove_provisional_grade_position.rb new file mode 100644 index 00000000000..221916414ef --- /dev/null +++ b/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 diff --git a/spec/models/assignment_spec.rb b/spec/models/assignment_spec.rb index a408b319369..7e7f8ea4546 100644 --- a/spec/models/assignment_spec.rb +++ b/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 diff --git a/spec/models/moderated_grading/provisional_grade_spec.rb b/spec/models/moderated_grading/provisional_grade_spec.rb index 6ff7f5f7e58..c8dfef8190c 100644 --- a/spec/models/moderated_grading/provisional_grade_spec.rb +++ b/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 diff --git a/spec/models/submission_comment_spec.rb b/spec/models/submission_comment_spec.rb index 133a780c4e2..2a7b953f486 100644 --- a/spec/models/submission_comment_spec.rb +++ b/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