diff --git a/Gemfile.d/app.rb b/Gemfile.d/app.rb index 76c52122a27..0301b57d787 100644 --- a/Gemfile.d/app.rb +++ b/Gemfile.d/app.rb @@ -1,4 +1,3 @@ - if CANVAS_RAILS3 gem 'rails', '3.2.22' gem 'rack', '1.4.5' diff --git a/Gemfile.d/test.rb b/Gemfile.d/test.rb index 5188bac9c9a..ae4e06da6ac 100644 --- a/Gemfile.d/test.rb +++ b/Gemfile.d/test.rb @@ -17,6 +17,7 @@ group :test do gem 'rspec-rails', '3.2.3' gem 'rspec-legacy_formatters', '1.0.0' gem 'rspec-collection_matchers', '1.1.2' + gem 'shoulda-matchers', '2.8.0' gem 'rubocop', require: false gem 'rubocop-rspec', require: false diff --git a/app/models/grading_period.rb b/app/models/grading_period.rb index b7962dd16a9..72951062631 100644 --- a/app/models/grading_period.rb +++ b/app/models/grading_period.rb @@ -70,7 +70,6 @@ class GradingPeriod < ActiveRecord::Base .last == self end - def overlapping? overlaps.active.exists? end diff --git a/app/models/moderated_grading.rb b/app/models/moderated_grading.rb new file mode 100644 index 00000000000..64bd55ca36c --- /dev/null +++ b/app/models/moderated_grading.rb @@ -0,0 +1,5 @@ +module ModeratedGrading + def self.table_name_prefix + 'moderated_grading_' + end +end diff --git a/app/models/moderated_grading/provisional_grade.rb b/app/models/moderated_grading/provisional_grade.rb new file mode 100644 index 00000000000..cc2e630e4b2 --- /dev/null +++ b/app/models/moderated_grading/provisional_grade.rb @@ -0,0 +1,23 @@ +class ModeratedGrading::ProvisionalGrade < ActiveRecord::Base + include Canvas::GradeValidations + + attr_accessible :grade, :score, :position + + 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? + super + end + + private + def set_graded_at + self.graded_at = Time.zone.now + end +end diff --git a/app/models/submission.rb b/app/models/submission.rb index 0a0cec9a16b..631c37d5fc0 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -19,11 +19,16 @@ require 'atom' class Submission < ActiveRecord::Base + include Canvas::GradeValidations + include CustomValidations include SendToStream + include Workflow + attr_protected :submitted_at attr_readonly :assignment_id attr_accessor :visible_to_user, :skip_grade_calc + belongs_to :attachment # this refers to the screenshot of the submission if it is a url submission belongs_to :assignment belongs_to :user @@ -31,15 +36,16 @@ class Submission < ActiveRecord::Base belongs_to :group belongs_to :media_object belongs_to :student, :class_name => 'User', :foreign_key => :user_id + belongs_to :quiz_submission, :class_name => 'Quizzes::QuizSubmission' has_many :submission_comments, :order => 'created_at', :dependent => :destroy has_many :visible_submission_comments, :class_name => 'SubmissionComment', :order => 'created_at, id', :conditions => { :hidden => false } has_many :hidden_submission_comments, :class_name => 'SubmissionComment', :order => 'created_at, id', :conditions => { :hidden => true } has_many :assessment_requests, :as => :asset has_many :assigned_assessments, :class_name => 'AssessmentRequest', :as => :assessor_asset - belongs_to :quiz_submission, :class_name => 'Quizzes::QuizSubmission' - has_one :rubric_assessment, :as => :artifact, :conditions => {:assessment_type => "grading"} has_many :rubric_assessments, :as => :artifact has_many :attachment_associations, :as => :context + has_many :provisional_grades, class_name: 'ModeratedGrading::ProvisionalGrade' + has_one :rubric_assessment, :as => :artifact, :conditions => {:assessment_type => "grading"} # we no longer link submission comments and conversations, but we haven't fixed up existing # linked conversations so this relation might be useful @@ -49,23 +55,29 @@ class Submission < ActiveRecord::Base has_many :content_participations, :as => :content EXPORTABLE_ATTRIBUTES = [ - :id, :body, :url, :attachment_id, :grade, :score, :submitted_at, :assignment_id, :user_id, :submission_type, :workflow_state, :created_at, :updated_at, :group_id, - :attachment_ids, :processed, :process_attempts, :grade_matches_current_submission, :published_score, :published_grade, :graded_at, :student_entered_score, :grader_id, - :media_comment_id, :media_comment_type, :quiz_submission_id, :submission_comments_count, :has_rubric_assessment, :attempt, :context_code, :media_object_id, - :turnitin_data, :has_admin_comment, :cached_due_date - ] + :id, :body, :url, :attachment_id, :grade, :score, :submitted_at, + :assignment_id, :user_id, :submission_type, :workflow_state, + :created_at, :updated_at, :group_id, :attachment_ids, :processed, + :process_attempts, :grade_matches_current_submission, :published_score, + :published_grade, :graded_at, :student_entered_score, :grader_id, + :media_comment_id, :media_comment_type, :quiz_submission_id, + :submission_comments_count, :has_rubric_assessment, :attempt, + :context_code, :media_object_id, :turnitin_data, :has_admin_comment, + :cached_due_date + ].freeze EXPORTABLE_ASSOCIATIONS = [ - :attachment, :assignment, :user, :grader, :group, :media_object, :student, :submission_comments, :assessment_requests, :assigned_assessments, :quiz_submission, - :rubric_assessment, :rubric_assessments, :attachments, :content_participations - ] + :attachment, :assignment, :user, :grader, :group, :media_object, + :student, :submission_comments, :assessment_requests, + :assigned_assessments, :quiz_submission, :rubric_assessment, + :rubric_assessments, :attachments, :content_participations + ].freeze serialize :turnitin_data, Hash + validates_presence_of :assignment_id, :user_id validates_length_of :body, :maximum => maximum_long_text_length, :allow_nil => true, :allow_blank => true validates_length_of :published_grade, :maximum => maximum_string_length, :allow_nil => true, :allow_blank => true - validates_length_of :grade, :maximum => maximum_string_length, :allow_nil => true, :allow_blank => true - include CustomValidations validates_as_url :url scope :with_comments, -> { includes(:submission_comments) } @@ -93,6 +105,16 @@ class Submission < ActiveRecord::Base where("submissions.assignment_id IN (SELECT assignments.id FROM assignments WHERE assignments.context_id = ? AND assignments.context_type = 'Course')", course) } + workflow do + state :submitted do + event :grade_it, :transitions_to => :graded + end + state :unsubmitted + state :pending_review + state :graded + end + + # see #needs_grading? def self.needs_grading_conditions conditions = <<-SQL @@ -790,17 +812,6 @@ class Submission < ActiveRecord::Base connection.after_transaction_commit { Auditors::GradeChange.record(self) } end - include Workflow - - workflow do - state :submitted do - event :grade_it, :transitions_to => :graded - end - state :unsubmitted - state :pending_review - state :graded - end - scope :with_assignment, -> { joins(:assignment).where("assignments.workflow_state <> 'deleted'")} scope :graded, -> { where("submissions.grade IS NOT NULL") } diff --git a/db/migrate/20150730170646_create_moderated_grading_provisional_grades.rb b/db/migrate/20150730170646_create_moderated_grading_provisional_grades.rb new file mode 100644 index 00000000000..4e3b7e257cc --- /dev/null +++ b/db/migrate/20150730170646_create_moderated_grading_provisional_grades.rb @@ -0,0 +1,30 @@ +class CreateModeratedGradingProvisionalGrades < ActiveRecord::Migration + tag :predeploy + + def up + create_table :moderated_grading_provisional_grades do |t| + t.string :grade + t.float :score + t.timestamp :graded_at + t.integer :position, null: false, limit: 8 + t.references :scorer, null: false, limit: 8 + t.references :submission, null: false, limit: 8 + + t.timestamps + end + + add_index :moderated_grading_provisional_grades, :submission_id + add_index :moderated_grading_provisional_grades, + [:submission_id, :position], + unique: true, + name: :idx_mg_provisional_grades_unique_submission_position + add_foreign_key :moderated_grading_provisional_grades, :submissions + add_foreign_key :moderated_grading_provisional_grades, + :users, + column: :scorer_id + end + + def down + drop_table :moderated_grading_provisional_grades + end +end diff --git a/lib/canvas.rb b/lib/canvas.rb index e031054102b..f0b376ffd8f 100644 --- a/lib/canvas.rb +++ b/lib/canvas.rb @@ -8,7 +8,10 @@ module Canvas def self.active_record_foreign_key_check(name, type, options) if name.to_s =~ /_id\z/ && type.to_s == 'integer' && options[:limit].to_i < 8 - raise ArgumentError, "All foreign keys need to be 8-byte integers. #{name} looks like a foreign key to me, please add this option: `:limit => 8`" + raise ArgumentError, <<-EOS + All foreign keys need to be at least 8-byte integers. #{name} + looks like a foreign key, please add this option: `:limit => 8` + EOS end end @@ -51,12 +54,21 @@ module Canvas # sanity check the file unless configs.is_a?(Hash) - raise "Invalid config/cache_store.yml: Root is not a hash. See comments in config/cache_store.yml.example" + raise <<-EOS + Invalid config/cache_store.yml: Root is not a hash. See comments in + config/cache_store.yml.example + EOS end non_hashes = configs.keys.select { |k| !configs[k].is_a?(Hash) } non_hashes.reject! { |k| configs[k].is_a?(String) && configs[configs[k]].is_a?(Hash) } - raise "Invalid config/cache_store.yml: Some keys are not hashes: #{non_hashes.join(', ')}. See comments in config/cache_store.yml.example" unless non_hashes.empty? + unless non_hashes.empty? + raise <<-EOS + Invalid config/cache_store.yml: Some keys are not hashes: + #{non_hashes.join(', ')}. See comments in + config/cache_store.yml.example + EOS + end configs.each do |env, config| if config.is_a?(String) diff --git a/lib/canvas/grade_validations.rb b/lib/canvas/grade_validations.rb new file mode 100644 index 00000000000..2ed0d79a4b7 --- /dev/null +++ b/lib/canvas/grade_validations.rb @@ -0,0 +1,9 @@ +require 'active_support/concern' + +module Canvas::GradeValidations + extend ActiveSupport::Concern + + included do + validates_length_of :grade, :maximum => maximum_string_length, :allow_nil => true, :allow_blank => true + end +end diff --git a/spec/models/moderated_grading/provisional_grade_spec.rb b/spec/models/moderated_grading/provisional_grade_spec.rb new file mode 100644 index 00000000000..6ff7f5f7e58 --- /dev/null +++ b/spec/models/moderated_grading/provisional_grade_spec.rb @@ -0,0 +1,30 @@ +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| + grade.scorer = user + end + end + let(:submission) { assignment.submissions.create!(user: user) } + let(:assignment) { course.assignments.create! } + let(:course) { Course.create! } + let(:user) { User.create! } + 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 '#graded_at when a grade changes' do + it { expect(provisional_grade.graded_at).to be_nil } + it 'updates the graded_at timestamp' do + Timecop.freeze(now) do + provisional_grade.update_attributes(grade: 'B') + expect(provisional_grade.graded_at).to eql now + end + end + end +end diff --git a/spec/models/submission_spec.rb b/spec/models/submission_spec.rb index 033e5cf03ba..6ba4020626b 100644 --- a/spec/models/submission_spec.rb +++ b/spec/models/submission_spec.rb @@ -815,19 +815,19 @@ describe Submission do describe "graded?" do it "is false before graded" do - s, _ = @assignment.find_or_create_submission(@user) - expect(s.graded?).to eql false + submission, _ = @assignment.find_or_create_submission(@user) + expect(submission).to_not be_graded end it "is true for graded assignments" do - s, _ = @assignment.grade_student(@user, grade: 1) - expect(s.graded?).to eql true + submission, _ = @assignment.grade_student(@user, grade: 1) + expect(submission).to be_graded end it "is also true for excused assignments" do - s, _ = @assignment.find_or_create_submission(@user) - s.excused = true - expect(s.graded?).to eql true + submission, _ = @assignment.find_or_create_submission(@user) + submission.excused = true + expect(submission).to be_graded end end @@ -835,25 +835,22 @@ describe Submission do let(:submission) { Submission.new } it "returns false when its not autograded" do - assignment = stub(:muted? => false) - @submission = Submission.new - expect(@submission.autograded?).to eq false + submission = Submission.new + expect(submission).to_not be_autograded - @submission.grader_id = Shard.global_id_for(@user.id) - expect(@submission.autograded?).to eq false + submission.grader_id = Shard.global_id_for(@user.id) + expect(submission).to_not be_autograded end it "returns true when its autograded" do - assignment = stub(:muted? => false) - @submission = Submission.new - @submission.grader_id = -1 - expect(@submission.autograded?).to eq true + submission = Submission.new + submission.grader_id = -1 + expect(submission).to be_autograded end end describe "past_due" do before :once do - u1 = @user submission_spec_model @submission1 = @submission