diff --git a/app/models/assignment_group.rb b/app/models/assignment_group.rb index ffa0267f84a..4f8839fc43b 100644 --- a/app/models/assignment_group.rb +++ b/app/models/assignment_group.rb @@ -26,14 +26,21 @@ class AssignmentGroup < ActiveRecord::Base has_a_broadcast_policy serialize :integration_data, Hash + has_many :scores, -> { active } has_many :assignments, -> { order('position, due_at, title') }, dependent: :destroy - has_many :active_assignments, -> { where("assignments.workflow_state<>'deleted'").order('assignments.position, assignments.due_at, assignments.title') }, class_name: 'Assignment' - has_many :published_assignments, -> { where(workflow_state: 'published').order('assignments.position, assignments.due_at, assignments.title') }, class_name: 'Assignment' - validates_presence_of :context_id, :context_type, :workflow_state - validates_length_of :rules, :maximum => maximum_text_length, :allow_nil => true, :allow_blank => true - validates_length_of :default_assignment_name, :maximum => maximum_string_length, :allow_nil => true - validates_length_of :name, :maximum => maximum_string_length, :allow_nil => true + has_many :active_assignments, -> { + where("assignments.workflow_state<>'deleted'").order('assignments.position, assignments.due_at, assignments.title') + }, class_name: 'Assignment' + + has_many :published_assignments, -> { + where(workflow_state: 'published').order('assignments.position, assignments.due_at, assignments.title') + }, class_name: 'Assignment' + + validates :context_id, :context_type, :workflow_state, presence: true + validates :rules, length: { maximum: maximum_text_length }, allow_nil: true, allow_blank: true + validates :default_assignment_name, length: { maximum: maximum_string_length }, allow_nil: true + validates :name, length: { maximum: maximum_string_length }, allow_nil: true before_save :set_context_code before_save :generate_default_values diff --git a/app/models/enrollment.rb b/app/models/enrollment.rb index 73cee6c350a..85e2b61e48c 100644 --- a/app/models/enrollment.rb +++ b/app/models/enrollment.rb @@ -1018,34 +1018,36 @@ class Enrollment < ActiveRecord::Base end end - def computed_current_grade(grading_period_id: nil) - cached_score_or_grade(:current, :grade, grading_period_id: grading_period_id) + def computed_current_grade(id_opts=nil) + cached_score_or_grade(:current, :grade, id_opts) end - def computed_final_grade(grading_period_id: nil) - cached_score_or_grade(:final, :grade, grading_period_id: grading_period_id) + def computed_final_grade(id_opts=nil) + cached_score_or_grade(:final, :grade, id_opts) end - def computed_current_score(grading_period_id: nil) - cached_score_or_grade(:current, :score, grading_period_id: grading_period_id) + def computed_current_score(id_opts=nil) + cached_score_or_grade(:current, :score, id_opts) end - def computed_final_score(grading_period_id: nil) - cached_score_or_grade(:final, :score, grading_period_id: grading_period_id) + def computed_final_score(id_opts=nil) + cached_score_or_grade(:final, :score, id_opts) end - def cached_score_or_grade(current_or_final, score_or_grade, grading_period_id: nil) - score = find_score(grading_period_id: grading_period_id) + def cached_score_or_grade(current_or_final, score_or_grade, id_opts=nil) + score = find_score(id_opts) score&.send("#{current_or_final}_#{score_or_grade}") end private :cached_score_or_grade - # when grading period is nil, we are fetching the overall course score - def find_score(grading_period_id: nil) + def find_score(id_opts=nil) + id_opts ||= Score.params_for_course + valid_keys = %i(course_score grading_period grading_period_id assignment_group assignment_group_id) + return nil if id_opts.except(*valid_keys).any? if scores.loaded? - scores.find { |score| score.grading_period_id == grading_period_id } + scores.detect { |score| score.attributes >= id_opts.with_indifferent_access } else - scores.where(grading_period_id: grading_period_id).first + scores.where(id_opts).first end end diff --git a/app/models/score.rb b/app/models/score.rb index bafa5d36a4f..2607a988570 100644 --- a/app/models/score.rb +++ b/app/models/score.rb @@ -20,19 +20,23 @@ class Score < ActiveRecord::Base include Canvas::SoftDeletable belongs_to :enrollment - belongs_to :grading_period + belongs_to :grading_period, optional: true + belongs_to :assignment_group, optional: true has_one :course, through: :enrollment validates :enrollment, presence: true validates :current_score, :final_score, numericality: true, allow_nil: true - validates_uniqueness_of :enrollment_id, scope: :grading_period_id + + validate :scorable_association_check, if: -> { Score.course_score_populated? } + + before_validation :set_course_score, unless: :course_score_changed? set_policy do - given { |user, session| + given do |user, _session| (user&.id == enrollment.user_id && !course.hide_final_grades?) || course.grants_any_right?(user, :manage_grades, :view_all_grades) || enrollment.user.grants_right?(user, :read_as_parent) - } + end can :read end @@ -44,5 +48,44 @@ class Score < ActiveRecord::Base score_to_grade(final_score) end + def scorable + # if you're calling this method, you might want to preload objects to avoid N+1 + grading_period || assignment_group || enrollment.course + end + + def self.course_score_populated? + ENV['GRADEBOOK_COURSE_SCORE_POPULATED'].present? + end + + def self.params_for_course + Score.course_score_populated? ? { course_score: true } : { grading_period_id: nil } + end + delegate :score_to_grade, to: :course + + private + + def set_course_score + gpid = read_attribute(:grading_period_id) + agid = read_attribute(:assignment_group_id) + write_attribute(:course_score, (gpid || agid).nil?) + true + end + + def scorable_association_check + scc = scorable_association_count + if scc == 0 && !course_score + errors.add(:course_score, "should be true when there are no scorable associations") + elsif scc == 1 && course_score + errors.add(:course_score, "should be false when there is a scorable association") + elsif scc > 1 + errors.add(:base, "may not have multiple scorable associations") + else + return true + end + end + + def scorable_association_count + [grading_period_id, assignment_group_id].compact.length + end end diff --git a/db/migrate/20170815211443_add_assignment_group_id_to_scores.rb b/db/migrate/20170815211443_add_assignment_group_id_to_scores.rb new file mode 100644 index 00000000000..679bc87af07 --- /dev/null +++ b/db/migrate/20170815211443_add_assignment_group_id_to_scores.rb @@ -0,0 +1,58 @@ +# +# 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 . +# + +class AddAssignmentGroupIdToScores < ActiveRecord::Migration[5.0] + tag :predeploy + disable_ddl_transaction! + + def change + add_belongs_to :scores, :assignment_group, limit: 8, null: true + add_column :scores, :course_score, :boolean + + change_column_default :scores, :course_score, from: nil, to: false + + add_index :scores, :enrollment_id, + algorithm: :concurrently, + name: :index_enrollment_scores + + add_index :scores, %i(enrollment_id grading_period_id), unique: true, + where: "grading_period_id IS NOT NULL", + algorithm: :concurrently, + name: :index_grading_period_scores + + add_index :scores, %i(enrollment_id assignment_group_id), unique: true, + where: "assignment_group_id IS NOT NULL", + algorithm: :concurrently, + name: :index_assignment_group_scores + + add_index :scores, :enrollment_id, unique: true, + where: "course_score", # course_score is already boolean + algorithm: :concurrently, + name: :index_course_scores + + remove_index :scores, column: :enrollment_id, + name: :index_scores_on_enrollment_id + + remove_index :scores, column: [:enrollment_id, :grading_period_id], + name: :index_scores_on_enrollment_id_and_grading_period_id + + reversible do |direction| + direction.down { DataFixup::DeleteScoresForAssignmentGroups.run } + end + end +end diff --git a/db/migrate/20170816172211_populate_course_score_on_scores.rb b/db/migrate/20170816172211_populate_course_score_on_scores.rb new file mode 100644 index 00000000000..c67ea319a0d --- /dev/null +++ b/db/migrate/20170816172211_populate_course_score_on_scores.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 . +# + +class PopulateCourseScoreOnScores < ActiveRecord::Migration[5.0] + tag :postdeploy + + def up + DataFixup::PopulateScoresCourseScore.send_later_if_production_enqueue_args( + :run, + priority: Delayed::LOW_PRIORITY, + strand: "populate_scores_course_score_#{Shard.current.database_server.id}" + ) + end +end diff --git a/lib/data_fixup/delete_scores_for_assignment_groups.rb b/lib/data_fixup/delete_scores_for_assignment_groups.rb new file mode 100644 index 00000000000..e494c1a835d --- /dev/null +++ b/lib/data_fixup/delete_scores_for_assignment_groups.rb @@ -0,0 +1,23 @@ +# +# 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 . +# + +module DataFixup::DeleteScoresForAssignmentGroups + def self.run + Score.where.not(assignment_group_id: nil).in_batches { |scores| scores.delete_all } + end +end diff --git a/lib/data_fixup/populate_scores_course_score.rb b/lib/data_fixup/populate_scores_course_score.rb new file mode 100644 index 00000000000..b39d781f9c2 --- /dev/null +++ b/lib/data_fixup/populate_scores_course_score.rb @@ -0,0 +1,25 @@ +# +# 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 . +# + +module DataFixup::PopulateScoresCourseScore + def self.run + Score.where(course_score: nil).in_batches do |scores| + scores.update_all("course_score = (grading_period_id IS NULL)") + end + end +end diff --git a/lib/grade_calculator.rb b/lib/grade_calculator.rb index d4eda312f19..6ccde95a5c5 100644 --- a/lib/grade_calculator.rb +++ b/lib/grade_calculator.rb @@ -45,6 +45,8 @@ class GradeCalculator @user_ids = Array(user_ids).map { |id| Shard.relative_id_for(id, Shard.current, @course.shard) } @current_updates = {} @final_updates = {} + @current_groups = {} + @final_groups = {} @ignore_muted = opts[:ignore_muted] @effective_due_dates = opts[:effective_due_dates] end @@ -100,7 +102,7 @@ class GradeCalculator next unless enrollments_by_user[user_id].first group_sums = compute_group_sums_for_user(user_id) scores = compute_scores_for_user(user_id, group_sums) - update_changes_hash_for_user(user_id, scores) + update_changes_hash_for_user(user_id, scores, group_sums) { current: scores[:current], current_groups: group_sums[:current].index_by { |group| group[:id] }, @@ -138,9 +140,11 @@ class GradeCalculator scores end - def update_changes_hash_for_user(user_id, scores) + def update_changes_hash_for_user(user_id, scores, group_sums) @current_updates[user_id] = scores[:current][:grade] @final_updates[user_id] = scores[:final][:grade] + @current_groups[user_id] = group_sums[:current] + @final_groups[user_id] = group_sums[:final] end def calculate_total_from_weighted_grading_periods(user_id) @@ -268,7 +272,24 @@ class GradeCalculator # GradeCalculator sometimes divides by 0 somewhere, # resulting in NaN. Treat that as null here score = nil if score.try(:nan?) - score || 'NULL' + score || 'NULL::float' + end + + def group_rows + enrollments_by_user.keys.map do |user_id| + current = @current_groups[user_id].pluck(:global_id, :grade).to_h + final = @final_groups[user_id].pluck(:global_id, :grade).to_h + @groups.map do |group| + agid = group.global_id + enrollments_by_user[user_id].map do |enrollment| + "(#{enrollment.id}, #{group.id}, #{number_or_null(current[agid])}, #{number_or_null(final[agid])})" + end + end + end.flatten + end + + def updated_at + @updated_at ||= Score.connection.quote(Time.now.utc) end def save_scores @@ -279,71 +300,120 @@ class GradeCalculator return if @grading_period && @grading_period.deleted? @course.touch - updated_at = Score.connection.quote(Time.now.utc) + save_scores_in_transaction + end + + def save_scores_in_transaction Score.transaction do @course.shard.activate do - # Construct upsert statement to update existing Scores or create them if needed. - Score.connection.execute(" - UPDATE #{Score.quoted_table_name} - SET - current_score = CASE enrollment_id - #{@current_updates.map do |user_id, score| - enrollments_by_user[user_id].map do |enrollment| - "WHEN #{enrollment.id} THEN #{number_or_null(score)}" - end.join(' ') - end.join(' ')} - ELSE current_score - END, - final_score = CASE enrollment_id - #{@final_updates.map do |user_id, score| - enrollments_by_user[user_id].map do |enrollment| - "WHEN #{enrollment.id} THEN #{number_or_null(score)}" - end.join(' ') - end.join(' ')} - ELSE final_score - END, - updated_at = #{updated_at}, - -- if workflow_state was previously deleted for some reason, update it to active - workflow_state = COALESCE(NULLIF(workflow_state, 'deleted'), 'active') - WHERE - enrollment_id IN (#{joined_enrollment_ids}) AND - grading_period_id #{@grading_period ? "= #{@grading_period.id}" : 'IS NULL'}; - INSERT INTO #{Score.quoted_table_name} - (enrollment_id, grading_period_id, current_score, final_score, created_at, updated_at) - SELECT - enrollments.id as enrollment_id, - #{@grading_period.try(:id) || 'NULL'} as grading_period_id, - CASE enrollments.id - #{@current_updates.map do |user_id, score| - enrollments_by_user[user_id].map do |enrollment| - "WHEN #{enrollment.id} THEN #{number_or_null(score)}" - end.join(' ') - end.join(' ')} - ELSE NULL - END :: float AS current_score, - CASE enrollments.id - #{@final_updates.map do |user_id, score| - enrollments_by_user[user_id].map do |enrollment| - "WHEN #{enrollment.id} THEN #{number_or_null(score)}" - end.join(' ') - end.join(' ')} - ELSE NULL - END :: float AS final_score, - #{updated_at} as created_at, - #{updated_at} as updated_at - FROM #{Enrollment.quoted_table_name} enrollments - LEFT OUTER JOIN #{Score.quoted_table_name} scores on - scores.enrollment_id = enrollments.id AND - scores.grading_period_id #{@grading_period ? "= #{@grading_period.id}" : 'IS NULL'} - WHERE - enrollments.id IN (#{joined_enrollment_ids}) AND - scores.id IS NULL; - ") + save_course_and_grading_period_scores + rows = group_rows + if @grading_period.nil? && rows.any? && Score.course_score_populated? + save_assignment_group_scores(rows.join(',')) + end end end end + def save_course_and_grading_period_scores + # Construct upsert statement to update existing Scores or create them if needed, + # for course and grading period Scores. + Score.connection.execute(" + UPDATE #{Score.quoted_table_name} + SET + current_score = CASE enrollment_id + #{@current_updates.map do |user_id, score| + enrollments_by_user[user_id].map do |enrollment| + "WHEN #{enrollment.id} THEN #{number_or_null(score)}" + end.join(' ') + end.join(' ')} + ELSE current_score + END, + final_score = CASE enrollment_id + #{@final_updates.map do |user_id, score| + enrollments_by_user[user_id].map do |enrollment| + "WHEN #{enrollment.id} THEN #{number_or_null(score)}" + end.join(' ') + end.join(' ')} + ELSE final_score + END, + updated_at = #{updated_at}, + -- if workflow_state was previously deleted for some reason, update it to active + workflow_state = COALESCE(NULLIF(workflow_state, 'deleted'), 'active') + WHERE + enrollment_id IN (#{joined_enrollment_ids}) AND + assignment_group_id IS NULL AND + grading_period_id #{@grading_period ? "= #{@grading_period.id}" : 'IS NULL'}; + INSERT INTO #{Score.quoted_table_name} + (enrollment_id, grading_period_id, current_score, final_score, course_score, created_at, updated_at) + SELECT + enrollments.id as enrollment_id, + #{@grading_period.try(:id) || 'NULL'} as grading_period_id, + CASE enrollments.id + #{@current_updates.map do |user_id, score| + enrollments_by_user[user_id].map do |enrollment| + "WHEN #{enrollment.id} THEN #{number_or_null(score)}" + end.join(' ') + end.join(' ')} + ELSE NULL + END :: float AS current_score, + CASE enrollments.id + #{@final_updates.map do |user_id, score| + enrollments_by_user[user_id].map do |enrollment| + "WHEN #{enrollment.id} THEN #{number_or_null(score)}" + end.join(' ') + end.join(' ')} + ELSE NULL + END :: float AS final_score, + #{@grading_period ? 'FALSE' : 'TRUE'} AS course_score, + #{updated_at} as created_at, + #{updated_at} as updated_at + FROM #{Enrollment.quoted_table_name} enrollments + LEFT OUTER JOIN #{Score.quoted_table_name} scores on + scores.enrollment_id = enrollments.id AND + scores.grading_period_id #{@grading_period ? "= #{@grading_period.id}" : 'IS NULL'} + WHERE + enrollments.id IN (#{joined_enrollment_ids}) AND + scores.id IS NULL; + ") + end + + def save_assignment_group_scores(group_values) + # Construct upsert statement to update existing Scores or create them if needed, + # for assignment group Scores. + Score.connection.execute(" + UPDATE #{Score.quoted_table_name} scores + SET + current_score = val.current_score, + final_score = val.final_score, + updated_at = #{updated_at}, + workflow_state = COALESCE(NULLIF(workflow_state, 'deleted'), 'active') + FROM (VALUES #{group_values}) val + (enrollment_id, assignment_group_id, current_score, final_score) + WHERE val.enrollment_id = scores.enrollment_id AND + val.assignment_group_id = scores.assignment_group_id; + INSERT INTO #{Score.quoted_table_name} + (enrollment_id, assignment_group_id, current_score, final_score, + course_score, created_at, updated_at) + SELECT + val.enrollment_id AS enrollment_id, + val.assignment_group_id as assignment_group_id, + val.current_score AS current_score, + val.final_score AS final_score, + FALSE AS course_score, + #{updated_at} AS created_at, + #{updated_at} AS updated_at + FROM (VALUES #{group_values}) val + (enrollment_id, assignment_group_id, current_score, final_score) + LEFT OUTER JOIN #{Score.quoted_table_name} sc ON + sc.enrollment_id = val.enrollment_id AND + sc.assignment_group_id = val.assignment_group_id + WHERE + sc.id IS NULL; + ") + end + # returns information about assignments groups in the form: # [ # { @@ -409,11 +479,12 @@ class GradeCalculator } { - :id => group.id, - :score => score, - :possible => possible, - :weight => group.group_weight, - :grade => ((score.to_f / possible * 100).round(2) if possible > 0), + id: group.id, + global_id: group.global_id, + score: score, + possible: possible, + weight: group.group_weight, + grade: ((score.to_f / possible * 100).round(2) if possible > 0), }.tap { |group_grade_info| Rails.logger.info "GRADES: calculated #{group_grade_info.inspect}" } diff --git a/spec/lib/grade_calculator_spec.rb b/spec/lib/grade_calculator_spec.rb index 21a587de982..0803664ac97 100644 --- a/spec/lib/grade_calculator_spec.rb +++ b/spec/lib/grade_calculator_spec.rb @@ -102,6 +102,32 @@ describe GradeCalculator do context "sharding" do specs_require_sharding + let(:seed_assignment_groups_with_scores) do + now = Time.zone.now + groups = [] + assignments = [] + submissions = [] + @shard1.activate do + account = Account.create! + course_with_student(active_all: true, account: account, user: @user) + @course.update_attribute(:group_weighting_scheme, "percent") + groups << + @course.assignment_groups.create!(name: "some group 1", group_weight: 50) << + @course.assignment_groups.create!(name: "some group 2", group_weight: 50) + asgt_opts = { due_at: now, points_possible: 10 } + assignments << + @course.assignments.create!(title: "Some Assignment 1", assignment_group: groups[0], **asgt_opts) << + @course.assignments.create!(title: "Some Assignment 2", assignment_group: groups[1], **asgt_opts) << + @course.assignments.create!(title: "Some Assignment 3", assignment_group: groups[1], **asgt_opts) + submissions << + assignments[0].submissions.find_by!(user: @user) << + assignments[1].submissions.find_by!(user: @user) + end + submissions[0].update_column(:score, 5.0) + submissions[1].update_column(:score, 2.5) + groups + end + it "should delete irrelevant cross-shard scores" do @user = User.create! @@ -181,6 +207,24 @@ describe GradeCalculator do expect(Enrollment.shard(@course.shard).where(user_id: @user.global_id).first.computed_current_score(grading_period_id: @grading_period.id)).to eql(20.0) expect(Enrollment.shard(@course.shard).where(user_id: @user.global_id).first.computed_final_score(grading_period_id: @grading_period.id)).to eql(10.0) end + + it("should update cross-shard scores with assignment groups", if: Score.course_score_populated?) do + @user = User.create! + + allow(GradeCalculator).to receive(:recompute_final_score) {} + groups = seed_assignment_groups_with_scores + + allow(GradeCalculator).to receive(:recompute_final_score).and_call_original + GradeCalculator.recompute_final_score(@user.id, @course.id) + + enrollment = Enrollment.shard(@course.shard).where(user_id: @user.global_id).first + expect(enrollment.computed_current_score).to be(37.5) + expect(enrollment.computed_final_score).to be(31.25) + expect(enrollment.computed_current_score(assignment_group_id: groups[0].id)).to be(50.0) + expect(enrollment.computed_final_score(assignment_group_id: groups[0].id)).to be(50.0) + expect(enrollment.computed_current_score(assignment_group_id: groups[1].id)).to be(25.0) + expect(enrollment.computed_final_score(assignment_group_id: groups[1].id)).to be(12.5) + end end it "should recompute when an assignment's points_possible changes'" do @@ -613,13 +657,13 @@ describe GradeCalculator do it "should convert NaN to NULL" do calc = GradeCalculator.new [@user.id], @course.id score = 0/0.0 - expect(calc.send(:number_or_null, score)).to eql('NULL') + expect(calc.send(:number_or_null, score)).to eql('NULL::float') end it "should convert nil to NULL" do calc = GradeCalculator.new [@user.id], @course.id score = nil - expect(calc.send(:number_or_null, score)).to eql('NULL') + expect(calc.send(:number_or_null, score)).to eql('NULL::float') end end @@ -643,7 +687,7 @@ describe GradeCalculator do end let(:scores) { @student.enrollments.first.scores.index_by(&:grading_period_id) } - let(:overall_course_score) { scores[nil] } + let(:overall_course_score) { @student.enrollments.first.scores.find_by(course_score: true) } let(:submission_for_first_assignment) { Submission.find_by(user: @student, assignment: @first_assignment) } let(:submission_for_second_assignment) { Submission.find_by(user: @student, assignment: @second_assignment) } @@ -938,6 +982,58 @@ describe GradeCalculator do expect(overall_course_score.final_score).to eq(0.0) end end + + it("does not save assignment group scores", unless: Score.course_score_populated?) do + @course.assignment_groups.create!(name: "some group") + GradeCalculator.new(@student.id, @course).compute_and_save_scores + expect(Score.where.not(assignment_group_id: nil).count).to be 0 + end + + context("assignment group scores", if: Score.course_score_populated?) do + before(:each) do + @group1 = @course.assignment_groups.create!(name: "some group 1") + @assignment1 = @course.assignments.create!(name: "assignment 1", points_possible: 20, assignment_group: @group1) + @assignment1.grade_student(@student, grade: 12, grader: @teacher) + @group2 = @course.assignment_groups.create!(name: "some group 2") + @assignment2 = @course.assignments.create!(name: "assignment 2", points_possible: 20, assignment_group: @group2) + @assignment2.grade_student(@student, grade: 18, grader: @teacher) + GradeCalculator.new(@student.id, @course).compute_and_save_scores + end + + let(:student_scores) { @student.enrollments.first.scores } + + it "stores separate assignment group scores for each of a student’s enrollments" do + (1..2).each do |i| + section = @course.course_sections.create!(name: "section #{i}") + @course.enroll_user(@student, 'StudentEnrollment', section: section, + enrollment_state: 'active', allow_multiple_enrollments: true) + end + GradeCalculator.new(@student.id, @course).compute_and_save_scores + scored_enrollment_ids = Score.where(assignment_group_id: @group1.id).map(&:enrollment_id) + expect(scored_enrollment_ids).to contain_exactly *(@student.enrollments.map(&:id)) + end + + it "updates active score rows for assignment groups if they already exist" do + orig_score_id = student_scores.first.id + @assignment1.grade_student(@student, grade: 15, grader: @teacher) + GradeCalculator.new(@student.id, @course).compute_and_save_scores + new_score_id = student_scores.first.id + expect(orig_score_id).to be new_score_id + end + + it "activates previously soft deleted assignment group scores when updating them" do + student_scores.update_all(workflow_state: 'deleted') + GradeCalculator.new(@student.id, @course).compute_and_save_scores + expect(student_scores.map(&:workflow_state).uniq).to contain_exactly('active') + end + + it "inserts score rows for assignment groups unless they already exist" do + expect do + @course.assignment_groups.create!(name: "yet another group") + GradeCalculator.new(@student.id, @course).compute_and_save_scores + end.to change{ Score.count }.by(1) + end + end end it "should return grades in the order they are requested" do @@ -1232,6 +1328,12 @@ describe GradeCalculator do expect(final_grade_info(@user, @course)[:total]).to eq 20 expect(final_grade_info(@user, @course)[:possible]).to eq 40 end + it("saves scores for all assignment group and enrollment combinations", if: Score.course_score_populated?) do + user_ids = @course.enrollments.map(&:user_id).uniq + group_ids = @assignments.map(&:assignment_group_id).uniq + GradeCalculator.new(user_ids, @course.id).compute_and_save_scores + expect(Score.where(assignment_group_id: group_ids).count).to be @course.enrollments.count * group_ids.length + end end end diff --git a/spec/models/assignment_group_spec.rb b/spec/models/assignment_group_spec.rb index ee6ace6dfa7..a169f68630e 100644 --- a/spec/models/assignment_group_spec.rb +++ b/spec/models/assignment_group_spec.rb @@ -42,6 +42,13 @@ describe AssignmentGroup do expect(ag.group_weight).to eq 0 end + it "allows association with scores" do + ag = @course.assignment_groups.create!(@valid_attributes) + enrollment = @course.student_enrollments.first + score = ag.scores.create!(enrollment: enrollment, current_score: 9.0, final_score: 10.0) + expect(score.assignment_group_id).to be ag.id + end + context "visible assignments" do before(:each) do @ag = @course.assignment_groups.create!(@valid_attributes) diff --git a/spec/models/enrollment_spec.rb b/spec/models/enrollment_spec.rb index ee7df5cc48f..22c358c97d0 100644 --- a/spec/models/enrollment_spec.rb +++ b/spec/models/enrollment_spec.rb @@ -187,6 +187,8 @@ describe Enrollment do ) end + let(:a_group) { @course.assignment_groups.create!(name: 'a group') } + describe '#computed_current_score' do it 'uses the value from the associated score object, if one exists' do @enrollment.scores.create!(current_score: 80.3) @@ -256,6 +258,41 @@ describe Enrollment do end end + describe '#find_score' do + before(:each) do + @course.update!(grading_standard_enabled: true) + allow(GradeCalculator).to receive(:recompute_final_score) {} + @enrollment.scores.create!(current_score: 85.3) + @enrollment.scores.create!(grading_period: period, current_score: 99.1) + @enrollment.scores.create!(assignment_group: a_group, current_score: 66.3) + allow(GradeCalculator).to receive(:recompute_final_score).and_call_original + end + + it 'returns the course score' do + expect(@enrollment.find_score.current_score).to be 85.3 + end + + it 'returns grading period scores' do + expect(@enrollment.find_score(grading_period_id: period.id).current_score).to be 99.1 + end + + it 'returns assignment group scores' do + expect(@enrollment.find_score(assignment_group_id: a_group.id).current_score).to be 66.3 + end + + it 'returns no score when given an invalid grading period id' do + expect(@enrollment.find_score(grading_period_id: 99999)).to be nil + end + + it 'returns no score when given an invalid assignment group id' do + expect(@enrollment.find_score(assignment_group_id: 8888888)).to be nil + end + + it 'returns no score when given unrecognized id keys' do + expect(@enrollment.find_score(flavor: 'Anchovied Caramel')).to be nil + end + end + describe '#graded_at' do it 'uses the updated_at from the associated score object, if one exists' do score = @enrollment.scores.create!(current_score: 80.3) diff --git a/spec/models/grading_period_spec.rb b/spec/models/grading_period_spec.rb index d31ba2b0c9b..3f880913490 100644 --- a/spec/models/grading_period_spec.rb +++ b/spec/models/grading_period_spec.rb @@ -19,7 +19,7 @@ require_relative '../spec_helper' describe GradingPeriod do - subject(:grading_period) { grading_period_group.grading_periods.build(params) } + subject(:grading_period) { grading_period_group.grading_periods.create!(params) } let(:group_helper) { Factories::GradingPeriodGroupHelper.new } let(:account) { Account.create! } @@ -890,7 +890,7 @@ describe GradingPeriod do end it 'creates scores for the grading period upon its creation' do - expect{ grading_period.save! }.to change{ Score.count }.from(1).to(2) + expect{ grading_period.save! }.to change{ Score.count }.by(1) end it 'updates grading period scores when the grading period end date is changed' do diff --git a/spec/models/score_spec.rb b/spec/models/score_spec.rb index 928fb19e50a..a6a8ea7a8b2 100644 --- a/spec/models/score_spec.rb +++ b/spec/models/score_spec.rb @@ -19,28 +19,42 @@ require 'spec_helper' describe Score do + before(:once) do + grading_periods + test_course.assignment_groups.create!(name: 'Assignments') + end + let(:test_course) { Course.create! } let(:student) { student_in_course(course: test_course) } - let(:params) { + let(:params) do { course: test_course, current_score: 80.2, final_score: 74.0, updated_at: 1.week.ago } - } + end + + let(:grading_period_score_params) do + params.merge(grading_period_id: GradingPeriod.first.id) + end + let(:assignment_group_score_params) do + params.merge(assignment_group_id: AssignmentGroup.first.id) + end + let(:grading_period_score) { student.scores.create!(grading_period_score_params) } + let(:assignment_group_score) { student.scores.create!(assignment_group_score_params) } + subject_once(:score) { student.scores.create!(params) } it_behaves_like "soft deletion" do - before do - grading_periods - end - - let(:creation_arguments) { [ - params.merge(grading_period: GradingPeriod.first), - params.merge(grading_period: GradingPeriod.last) - ] } subject { student.scores } + + let(:creation_arguments) do + [ + params.merge(grading_period: GradingPeriod.first), + params.merge(grading_period: GradingPeriod.last) + ] + end end describe 'validations' do @@ -51,12 +65,22 @@ describe Score do expect(score).to be_invalid end - it 'is invalid without unique enrollment' do + it 'is invalid without unique enrollment for course' do student.scores.create!(params) - expect { student.scores.create!(params) }.to raise_error(ActiveRecord::RecordInvalid) + expect { student.scores.create!(params) }.to raise_error(ActiveRecord::RecordNotUnique) end - shared_context "score attribute" do + it 'is invalid without unique enrollment for grading period' do + student.scores.create!(grading_period_score_params) + expect { student.scores.create!(grading_period_score_params) }.to raise_error(ActiveRecord::RecordNotUnique) + end + + it('is invalid without unique enrollment for assignment group', if: Score.course_score_populated?) do + student.scores.create!(assignment_group_score_params) + expect { student.scores.create!(assignment_group_score_params) }.to raise_error(ActiveRecord::RecordNotUnique) + end + + shared_examples "score attribute" do it 'is valid as nil' do score.write_attribute(attribute, nil) expect(score).to be_valid @@ -73,8 +97,43 @@ describe Score do end end - include_context('score attribute') { let(:attribute) { :current_score } } - include_context('score attribute') { let(:attribute) { :final_score } } + include_examples('score attribute') { let(:attribute) { :current_score } } + include_examples('score attribute') { let(:attribute) { :final_score } } + + context("scorable associations", if: Score.course_score_populated?) do + before(:once) { grading_periods } + + it 'is valid with course_score true and no scorable associations' do + expect(student.scores.create!(course_score: true, **params)).to be_valid + end + + it 'is valid with course_score false and a grading period association' do + expect(student.scores.create!(course_score: false, **grading_period_score_params)).to be_valid + end + + it 'is valid with course_score false and an assignment group association' do + expect(student.scores.create!(course_score: false, **assignment_group_score_params)).to be_valid + end + + it 'is invalid with course_score false and no scorable associations' do + expect do + score = student.scores.create!(params) + score.update!(course_score: false) + end.to raise_error(ActiveRecord::RecordInvalid) + end + + it 'is invalid with course_score true and a scorable association' do + expect do + student.scores.create!(course_score: true, **grading_period_score_params) + end.to raise_error(ActiveRecord::RecordInvalid) + end + + it 'is invalid with multiple scorable associations' do + expect do + student.scores.create!(grading_period_id: GradingPeriod.first.id, **assignment_group_score_params) + end.to raise_error(ActiveRecord::RecordInvalid) + end + end end describe '#current_grade' do @@ -111,16 +170,54 @@ describe Score do end end + describe('#scorable', if: Score.course_score_populated?) do + it 'returns course for course score' do + expect(score.scorable).to be score.enrollment.course + end + + it 'returns grading period for grading period score' do + expect(grading_period_score.scorable).to be grading_period_score.grading_period + end + + it 'returns assignment group for assignment group score' do + expect(assignment_group_score.scorable).to be assignment_group_score.assignment_group + end + end + + describe('#course_score', if: Score.course_score_populated?) do + it 'sets course_score to true when there are no scorable associations' do + expect(score.course_score).to be true + end + + it 'sets course_score to false for grading period scores' do + expect(grading_period_score.course_score).to be false + end + + it 'sets course_score to false for assignment group scores' do + expect(assignment_group_score.course_score).to be false + end + end + + describe('#params_for_course') do + it('uses course_score', if: Score.course_score_populated?) do + expect(Score.params_for_course).to eq(course_score: true) + end + + it('uses nil grading period id', unless: Score.course_score_populated?) do + expect(Score.params_for_course).to eq(grading_period_id: nil) + end + end + context "permissions" do it "allows the proper people" do - expect(score.grants_right? @enrollment.user, :read).to eq true + expect(score.grants_right?(@enrollment.user, :read)).to eq true teacher_in_course(active_all: true) - expect(score.grants_right? @teacher, :read).to eq true + expect(score.grants_right?(@teacher, :read)).to eq true end it "doesn't work for nobody" do - expect(score.grants_right? nil, :read).to eq false + expect(score.grants_right?(nil, :read)).to eq false end it "doesn't allow random classmates to read" do