account for assignment override student in user merge
It turns out when we query for effective due dates, which happens in due_date_cacher, we fetch all students affected by adhoc overrides. This happens with a join on AssignmentOverrideStudent, so without accounting for such objects, we have logic that just deletes the submission if it's not "viewable" to the target user. fixes FOO-2773 flag = none Test plan: • Create an assignment and assign it to the one student with a due date (Assignment due date override for individual user) excluding the "everyone" default section scope • Create a second user and merge the first student into the newly created user • Check the submission API for the merged student and assignment and notice that the "cached_due_date" displays the assigned due date used when creating the assignment and the workflow state is is as it was before "unsubmitted" Change-Id: Ic9701326a9dac0f8a44dbddfad59738032be230a Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/290068 Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> Reviewed-by: Ben Rinaca <brinaca@instructure.com> Migration-Review: Ben Rinaca <brinaca@instructure.com> QA-Review: August Thornton <august@instructure.com> Product-Review: August Thornton <august@instructure.com>
This commit is contained in:
parent
6637a68fee
commit
324a3476aa
|
@ -22,5 +22,6 @@ class UserMergeDataRecord < ActiveRecord::Base
|
||||||
belongs_to :merge_data, class_name: "UserMergeData", inverse_of: :records, foreign_key: "user_merge_data_id"
|
belongs_to :merge_data, class_name: "UserMergeData", inverse_of: :records, foreign_key: "user_merge_data_id"
|
||||||
belongs_to :context, polymorphic: [:account_user, :enrollment, :pseudonym, :user_observer, :user_observation_link,
|
belongs_to :context, polymorphic: [:account_user, :enrollment, :pseudonym, :user_observer, :user_observation_link,
|
||||||
:attachment, :communication_channel, :user_service,
|
:attachment, :communication_channel, :user_service,
|
||||||
:submission, { quiz_submission: "Quizzes::QuizSubmission" }]
|
:submission, { quiz_submission: "Quizzes::QuizSubmission" },
|
||||||
|
:assignment_override_student]
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,38 @@
|
||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
#
|
||||||
|
# Copyright (C) 2022 - 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 ModifyAssignmentOverrideStudentUserForeignKeyConstraint < ActiveRecord::Migration[6.0]
|
||||||
|
tag :predeploy
|
||||||
|
|
||||||
|
def up
|
||||||
|
alter_constraint(
|
||||||
|
:assignment_override_students,
|
||||||
|
find_foreign_key(:assignment_override_students, :users),
|
||||||
|
deferrable: true
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
def down
|
||||||
|
alter_constraint(
|
||||||
|
:assignment_override_students,
|
||||||
|
find_foreign_key(:assignment_override_students, :users),
|
||||||
|
deferrable: false
|
||||||
|
)
|
||||||
|
end
|
||||||
|
end
|
|
@ -609,7 +609,7 @@ class UserMerge
|
||||||
scope = model.where(user_id: from_user)
|
scope = model.where(user_id: from_user)
|
||||||
case model.name
|
case model.name
|
||||||
when "Submission"
|
when "Submission"
|
||||||
# we prefer submissions that have grades then submissions that have
|
# we prefer submissions that have grades than submissions that have
|
||||||
# a submission... that sort of makes sense.
|
# a submission... that sort of makes sense.
|
||||||
# we swap empty objects in cases of collision so that we don't
|
# we swap empty objects in cases of collision so that we don't
|
||||||
# end up causing a unique index violation for a given assignment for
|
# end up causing a unique index violation for a given assignment for
|
||||||
|
@ -631,6 +631,7 @@ class UserMerge
|
||||||
merge_data.build_more_data(to_move, data: data) unless to_move.empty?
|
merge_data.build_more_data(to_move, data: data) unless to_move.empty?
|
||||||
merge_data.build_more_data(move_back, data: data) unless move_back.empty?
|
merge_data.build_more_data(move_back, data: data) unless move_back.empty?
|
||||||
swap_submission(model, move_back, table, to_move, to_move_ids, "fk_rails_8d85741475")
|
swap_submission(model, move_back, table, to_move, to_move_ids, "fk_rails_8d85741475")
|
||||||
|
swap_assignment_override_student(model, move_back, to_move)
|
||||||
when "Quizzes::QuizSubmission"
|
when "Quizzes::QuizSubmission"
|
||||||
subscope = already_scope.to_a
|
subscope = already_scope.to_a
|
||||||
to_move = model.where(user_id: from_user).joins(:submission).where(submissions: { user_id: target_user }).to_a
|
to_move = model.where(user_id: from_user).joins(:submission).where(submissions: { user_id: target_user }).to_a
|
||||||
|
@ -641,6 +642,7 @@ class UserMerge
|
||||||
merge_data.build_more_data(to_move, data: data)
|
merge_data.build_more_data(to_move, data: data)
|
||||||
merge_data.build_more_data(move_back, data: data)
|
merge_data.build_more_data(move_back, data: data)
|
||||||
swap_submission(model, move_back, table, to_move, to_move, "fk_rails_04850db4b4")
|
swap_submission(model, move_back, table, to_move, to_move, "fk_rails_04850db4b4")
|
||||||
|
swap_assignment_override_student(model, move_back, to_move)
|
||||||
end
|
end
|
||||||
rescue => e
|
rescue => e
|
||||||
Rails.logger.error "migrating #{table} column user_id failed: #{e}"
|
Rails.logger.error "migrating #{table} column user_id failed: #{e}"
|
||||||
|
@ -667,6 +669,32 @@ class UserMerge
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def swap_assignment_override_student(model, move_back, to_move)
|
||||||
|
return if to_move.empty?
|
||||||
|
|
||||||
|
to_move_ids = model.where(id: to_move).select(:assignment_id)
|
||||||
|
move_back_ids = model.where(id: move_back).select(:assignment_id)
|
||||||
|
to_move = AssignmentOverrideStudent.where(user_id: from_user, assignment_id: to_move_ids)
|
||||||
|
move_back = AssignmentOverrideStudent.where(user_id: from_user, assignment_id: move_back_ids)
|
||||||
|
# bail early if we don't have any student overrides to move
|
||||||
|
return unless to_move.exists?
|
||||||
|
|
||||||
|
merge_data.build_more_data(to_move, data: data)
|
||||||
|
merge_data.build_more_data(move_back, data: data)
|
||||||
|
|
||||||
|
AssignmentOverrideStudent.transaction do
|
||||||
|
connection = AssignmentOverrideStudent.connection
|
||||||
|
fkey = connection.find_foreign_key(:assignment_override_students, :users)
|
||||||
|
# DEFERRED constraints are not checked until transaction commit (at the end of the statement)
|
||||||
|
# We defer due to the unique constraint on (assignment_id, user_id) so we can
|
||||||
|
# swap user id's without facing a violation
|
||||||
|
connection.execute("SET CONSTRAINTS #{connection.quote_table_name(fkey)} DEFERRED")
|
||||||
|
move_back.update_all(user_id: -from_user.id)
|
||||||
|
to_move.update_all(user_id: target_user.id)
|
||||||
|
move_back.update_all(user_id: from_user.id)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
def update_versions(scope, table, column)
|
def update_versions(scope, table, column)
|
||||||
scope.find_ids_in_batches do |ids|
|
scope.find_ids_in_batches do |ids|
|
||||||
versionable_type = table.to_s.classify
|
versionable_type = table.to_s.classify
|
||||||
|
|
|
@ -137,6 +137,108 @@ describe UserMerge do
|
||||||
expect(at.user_id).to eq user1.id
|
expect(at.user_id).to eq user1.id
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it "recalculates cached_due_date on unsubmitted placeholder submissions for the new user" do
|
||||||
|
due_date_timestamp = DateTime.now.iso8601
|
||||||
|
course1.enroll_user(user2, "StudentEnrollment", enrollment_state: "active")
|
||||||
|
assignment = course1.assignments.create!(
|
||||||
|
title: "some assignment",
|
||||||
|
grading_type: "points",
|
||||||
|
submission_types: "online_text_entry",
|
||||||
|
due_at: due_date_timestamp
|
||||||
|
)
|
||||||
|
expect(Submission.where(user_id: user2.id, assignment_id: assignment.id).take.cached_due_date)
|
||||||
|
.to eq due_date_timestamp
|
||||||
|
|
||||||
|
UserMerge.from(user2).into(user1)
|
||||||
|
|
||||||
|
submission = Submission.where(user_id: user1.id, assignment_id: assignment.id).take
|
||||||
|
expect(submission.cached_due_date).to eq due_date_timestamp
|
||||||
|
expect(submission.workflow_state).to eq "unsubmitted"
|
||||||
|
end
|
||||||
|
|
||||||
|
it "recalculates cached_due_date on submissions for assignments with overrides" do
|
||||||
|
due_date_timestamp = DateTime.now.iso8601
|
||||||
|
course1.enroll_user(user2, "StudentEnrollment", enrollment_state: "active")
|
||||||
|
assignment = course1.assignments.create!(
|
||||||
|
title: "Assignment with student due date override",
|
||||||
|
grading_type: "points",
|
||||||
|
submission_types: "online_text_entry"
|
||||||
|
)
|
||||||
|
override = assignment.assignment_overrides.create!(
|
||||||
|
due_at: due_date_timestamp,
|
||||||
|
due_at_overridden: true,
|
||||||
|
all_day: true,
|
||||||
|
unlock_at_overridden: true,
|
||||||
|
lock_at_overridden: true
|
||||||
|
)
|
||||||
|
override.assignment_override_students.create!(user: user2)
|
||||||
|
assignment.update(due_at: nil, only_visible_to_overrides: true)
|
||||||
|
expect(Submission.where(user_id: user2.id, assignment_id: assignment.id).take.cached_due_date)
|
||||||
|
.to eq due_date_timestamp
|
||||||
|
|
||||||
|
UserMerge.from(user2).into(user1)
|
||||||
|
|
||||||
|
submission = Submission.where(user_id: user1.id, assignment_id: assignment.id).take
|
||||||
|
expect(submission.cached_due_date).to eq due_date_timestamp
|
||||||
|
expect(submission.workflow_state).to eq "unsubmitted"
|
||||||
|
end
|
||||||
|
|
||||||
|
it "deletes from user's assignment override student when both users have them" do
|
||||||
|
due_date_timestamp = DateTime.now.iso8601
|
||||||
|
course1.enroll_user(user1, "StudentEnrollment", enrollment_state: "active")
|
||||||
|
course1.enroll_user(user2, "StudentEnrollment", enrollment_state: "active")
|
||||||
|
a1 = assignment_model(course: course1)
|
||||||
|
s1 = a1.find_or_create_submission(user1)
|
||||||
|
s1.submission_type = "online_quiz"
|
||||||
|
s1.save!
|
||||||
|
s2 = a1.find_or_create_submission(user2)
|
||||||
|
s2.submission_type = "online_quiz"
|
||||||
|
s2.save!
|
||||||
|
override = a1.assignment_overrides.create!(
|
||||||
|
due_at: due_date_timestamp,
|
||||||
|
due_at_overridden: true,
|
||||||
|
all_day: true,
|
||||||
|
unlock_at_overridden: true,
|
||||||
|
lock_at_overridden: true
|
||||||
|
)
|
||||||
|
o1 = override.assignment_override_students.create!(user: user1)
|
||||||
|
o2 = override.assignment_override_students.create!(user: user2)
|
||||||
|
a1.update(due_at: nil, only_visible_to_overrides: true)
|
||||||
|
|
||||||
|
UserMerge.from(user1).into(user2)
|
||||||
|
|
||||||
|
expect(o1.reload.workflow_state).to eq "deleted"
|
||||||
|
expect(o1.reload.user).to eq user1
|
||||||
|
expect(o2.reload.workflow_state).to eq "active"
|
||||||
|
expect(o2.reload.user).to eq user2
|
||||||
|
end
|
||||||
|
|
||||||
|
it "moves and swap assignment override student to target user" do
|
||||||
|
due_date_timestamp = DateTime.now.iso8601
|
||||||
|
course1.enroll_user(user2, "StudentEnrollment", enrollment_state: "active")
|
||||||
|
assignment = course1.assignments.create!(
|
||||||
|
title: "Assignment with student due date override",
|
||||||
|
grading_type: "points",
|
||||||
|
submission_types: "online_text_entry"
|
||||||
|
)
|
||||||
|
override = assignment.assignment_overrides.create!(
|
||||||
|
due_at: due_date_timestamp,
|
||||||
|
due_at_overridden: true,
|
||||||
|
all_day: true,
|
||||||
|
unlock_at_overridden: true,
|
||||||
|
lock_at_overridden: true
|
||||||
|
)
|
||||||
|
o1 = override.assignment_override_students.create!(user: user2)
|
||||||
|
assignment.update(due_at: nil, only_visible_to_overrides: true)
|
||||||
|
expect(AssignmentOverrideStudent.count).to eq 1
|
||||||
|
|
||||||
|
UserMerge.from(user2).into(user1)
|
||||||
|
|
||||||
|
expect(AssignmentOverrideStudent.count).to eq 1
|
||||||
|
expect(o1.reload.workflow_state).to eq "active"
|
||||||
|
expect(o1.reload.user).to eq user1
|
||||||
|
end
|
||||||
|
|
||||||
it "moves submissions to the new user (but only if they don't already exist)" do
|
it "moves submissions to the new user (but only if they don't already exist)" do
|
||||||
a1 = assignment_model
|
a1 = assignment_model
|
||||||
s1 = a1.find_or_create_submission(user1)
|
s1 = a1.find_or_create_submission(user1)
|
||||||
|
@ -161,48 +263,6 @@ describe UserMerge do
|
||||||
expect(user1.submissions.map(&:id)).to be_include(s3.id)
|
expect(user1.submissions.map(&:id)).to be_include(s3.id)
|
||||||
end
|
end
|
||||||
|
|
||||||
it "recalculates cached_due_date on unsubmitted placeholder submissions for the new user" do
|
|
||||||
due_date_timestamp = DateTime.now.iso8601
|
|
||||||
course1.enroll_user(user2, "StudentEnrollment", enrollment_state: "active")
|
|
||||||
assignment = course1.assignments.create!(
|
|
||||||
title: "some assignment",
|
|
||||||
grading_type: "points",
|
|
||||||
submission_types: "online_text_entry",
|
|
||||||
due_at: due_date_timestamp
|
|
||||||
)
|
|
||||||
expect(Submission.where(user_id: user2.id, assignment_id: assignment.id).take.cached_due_date)
|
|
||||||
.to eq due_date_timestamp
|
|
||||||
UserMerge.from(user2).into(user1)
|
|
||||||
run_jobs
|
|
||||||
expect(Submission.where(user_id: user1.id, assignment_id: assignment.id).take.cached_due_date)
|
|
||||||
.to eq due_date_timestamp
|
|
||||||
end
|
|
||||||
|
|
||||||
it "recalculates cached_due_date on submissions for assignments with overrides" do
|
|
||||||
due_date_timestamp = DateTime.now.iso8601
|
|
||||||
course1.enroll_user(user2, "StudentEnrollment", enrollment_state: "active")
|
|
||||||
assignment = course1.assignments.create!(
|
|
||||||
title: "Assignment with student due date override",
|
|
||||||
grading_type: "points",
|
|
||||||
submission_types: "online_text_entry"
|
|
||||||
)
|
|
||||||
override = assignment.assignment_overrides.create!(
|
|
||||||
due_at: due_date_timestamp,
|
|
||||||
due_at_overridden: true,
|
|
||||||
all_day: true,
|
|
||||||
unlock_at_overridden: true,
|
|
||||||
lock_at_overridden: true
|
|
||||||
)
|
|
||||||
override.assignment_override_students.create!(user: user2)
|
|
||||||
assignment.update(due_at: nil, only_visible_to_overrides: true)
|
|
||||||
expect(Submission.where(user_id: user2.id, assignment_id: assignment.id).take.cached_due_date)
|
|
||||||
.to eq due_date_timestamp
|
|
||||||
UserMerge.from(user2).into(user1)
|
|
||||||
run_jobs
|
|
||||||
expect(Submission.where(user_id: user1.id, assignment_id: assignment.id).take.cached_due_date)
|
|
||||||
.to eq due_date_timestamp
|
|
||||||
end
|
|
||||||
|
|
||||||
it "does not move or delete submission when both users have submissions" do
|
it "does not move or delete submission when both users have submissions" do
|
||||||
a1 = assignment_model
|
a1 = assignment_model
|
||||||
s1 = a1.find_or_create_submission(user1)
|
s1 = a1.find_or_create_submission(user1)
|
||||||
|
|
Loading…
Reference in New Issue