ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

make individual override dates preferred 

When multiple overrides apply to a student on an assignment,
always pick the "individual" override due date and availability date,
if one exists. Previous behavior was to always pick the most lenient
override due date and availability date.

closes EVAL-2067
flag=prioritize_individual_overrides

[fsc-max-nodes=20]
[fsc-timeout=45]

Test Plan:
1. Enable the "Prioritize Individual Overrides" Site Admin feature flag.
2. Create an assignment with:
   a) An individual override for one student due tomorrow and with
      availability dates from yesterday to tomorrow, and
   b) A section override for the section the student is in with a due
      date 2 days from now, and with availability dates from a week ago
      to a week from now.
3. Log in as the student. Verify the assignment shows as being due
   tomorrow (not 2 days from now), and the availability dates show as
   from yesterday to tomorrow (not a week ago to a week from now).

Change-Id: I9571b9eee63ec06eccf2287415eea0fa9f129466
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/285841
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Kai Bjorkman <kbjorkman@instructure.com>
Product-Review: Jody Sailor
Reviewed-by: Eduardo Escobar <eduardo.escobar@instructure.com>
Reviewed-by: Kai Bjorkman <kbjorkman@instructure.com>
This commit is contained in:
Spencer Olson 2022-02-23 11:38:54 -06:00
parent 4410f1cb67
commit e3737b3f91
10 changed files with 287 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
app/models/assignment_override.rb
View File

@ -234,6 +234,10 @@ class AssignmentOverride < ActiveRecord::Base
end
protected :default_values
def adhoc?
set_type == "ADHOC"
end
def mastery_paths?
set_type == SET_TYPE_NOOP && set_id == NOOP_MASTERY_PATHS
end
@ -327,6 +331,8 @@ class AssignmentOverride < ActiveRecord::Base
end
def availability_expired?
return false if adhoc? && Account.site_admin.feature_enabled?(:prioritize_individual_overrides)
lock_at_overridden &&
lock_at.present? &&
lock_at <= Time.zone.now

15
config/feature_flags/apogee_release_flags.yml
View File

@ -162,3 +162,18 @@ peer_reviews_for_a2:
display_name: Peer Review Support in Assignment Enhancements
description: Students will be able to provide feedback on another student's assignment submission
using the enhanced assignment view
prioritize_individual_overrides:
state: hidden
applies_to: SiteAdmin
display_name: Prioritize Individual Overrides
description: If multiple due date overrides apply to a student on a given assignment
and one of those overrides is an individual override, always use the date from the
individual override. Previous behavior was to always choose the date from the override
that granted the student the most amount of time to submit.
environments:
ci:
state: allowed_on # enable for automated testing builds and local testing
development:
state: allowed_on # enable for local development
test:
state: allowed_on # enable for the deployed 'test' environment

6
lib/assignment_override_applicator.rb
View File

@ -370,6 +370,8 @@ module AssignmentOverrideApplicator
applicable_overrides = overrides.select(&:due_at_overridden)
if applicable_overrides.empty?
assignment_or_quiz
elsif Account.site_admin.feature_enabled?(:prioritize_individual_overrides) && (adhoc_override = applicable_overrides.detect(&:adhoc?))
adhoc_override
elsif (override = applicable_overrides.detect { |o| o.due_at.nil? })
override
else
@ -397,6 +399,8 @@ module AssignmentOverrideApplicator
if applicable_overrides.empty?
assignment_or_quiz.unlock_at
elsif Account.site_admin.feature_enabled?(:prioritize_individual_overrides) && (adhoc_override = applicable_overrides.detect(&:adhoc?))
adhoc_override.unlock_at
elsif applicable_overrides.any? { |o| o.unlock_at.nil? }
nil
else
@ -408,6 +412,8 @@ module AssignmentOverrideApplicator
applicable_overrides = overrides.select(&:lock_at_overridden)
if applicable_overrides.empty?
assignment_or_quiz.lock_at
elsif Account.site_admin.feature_enabled?(:prioritize_individual_overrides) && (adhoc_override = applicable_overrides.detect(&:adhoc?))
adhoc_override.lock_at
elsif applicable_overrides.detect { |o| o.lock_at.nil? }
nil
else

16
lib/effective_due_dates.rb
View File

@ -132,6 +132,12 @@ class EffectiveDueDates
private
def prioritize_individual_overrides?
return @prioritize_individual_overrides if defined?(@prioritize_individual_overrides)
@prioritize_individual_overrides = Account.site_admin.feature_enabled?(:prioritize_individual_overrides)
end
def any_student_in_closed_grading_period?(assignment_due_dates)
return false unless assignment_due_dates
@ -226,7 +232,7 @@ class EffectiveDueDates
o.due_at,
o.set_type AS override_type,
o.due_at_overridden,
1 AS priority
#{prioritize_individual_overrides? ? 1 : 2} AS priority
FROM
overrides o
INNER JOIN #{AssignmentOverrideStudent.quoted_table_name} os ON os.assignment_override_id = o.id AND
@ -246,7 +252,7 @@ class EffectiveDueDates
o.due_at,
o.set_type AS override_type,
o.due_at_overridden,
1 AS priority
2 AS priority
FROM
overrides o
INNER JOIN #{Group.quoted_table_name} g ON g.id = o.set_id
@ -268,7 +274,7 @@ class EffectiveDueDates
o.due_at,
o.set_type AS override_type,
o.due_at_overridden,
1 AS priority
2 AS priority
FROM
overrides o
INNER JOIN #{CourseSection.quoted_table_name} s ON s.id = o.set_id
@ -292,7 +298,7 @@ class EffectiveDueDates
a.due_at,
'Everyone Else'::varchar AS override_type,
FALSE AS due_at_overridden,
2 AS priority
3 AS priority
FROM
models a
INNER JOIN #{Enrollment.quoted_table_name} e ON e.course_id = a.context_id
@ -319,7 +325,7 @@ class EffectiveDueDates
SELECT DISTINCT ON (student_id, assignment_id)
*
FROM override_all_students
ORDER BY student_id ASC, assignment_id ASC, priority ASC, due_at_overridden DESC, due_at DESC NULLS FIRST
ORDER BY student_id ASC, assignment_id ASC, due_at_overridden DESC, priority ASC, due_at DESC NULLS FIRST
),
/* now find all grading periods, including both

55
spec/apis/v1/calendar_events_api_spec.rb
View File

@ -2280,18 +2280,49 @@ describe CalendarEventsApiController, type: :request do
expect(json.first["end_at"]).to eq "2012-01-14T12:00:00Z"
end
it "returns later override with user and section overrides" do
override = assignment_override_model(assignment: @default_assignment,
due_at: DateTime.parse("2012-01-12 12:00:00"))
override.assignment_override_students.create!(user: @user)
assignment_override_model(assignment: @default_assignment, set: @section2,
due_at: DateTime.parse("2012-01-14 12:00:00"))
json = api_call(:get, "/api/v1/calendar_events?type=assignment&start_date=2012-01-07&end_date=2012-01-16&per_page=25&context_codes[]=course_#{@course.id}", {
controller: "calendar_events_api", action: "index", format: "json", type: "assignment",
context_codes: ["course_#{@course.id}"], start_date: "2012-01-07", end_date: "2012-01-16", per_page: "25"
})
expect(json.size).to eq 1
expect(json.first["end_at"]).to eq "2012-01-14T12:00:00Z"
context "with user and section overrides" do
before do
override = assignment_override_model(
assignment: @default_assignment,
due_at: DateTime.parse("2012-01-12 12:00:00")
)
override.assignment_override_students.create!(user: @user)
assignment_override_model(
assignment: @default_assignment,
set: @section2,
due_at: DateTime.parse("2012-01-14 12:00:00")
)
end
let(:json) do
api_call(
:get,
"/api/v1/calendar_events?type=assignment&start_date=2012-01-07&end_date=2012-01-16&per_page=25&context_codes[]=course_#{@course.id}",
controller: "calendar_events_api",
action: "index",
format: "json",
type: "assignment",
context_codes: ["course_#{@course.id}"],
start_date: "2012-01-07",
end_date: "2012-01-16",
per_page: "25"
)
end
it "returns later override with prioritize_individual_overrides disabled" do
Account.site_admin.disable_feature!(:prioritize_individual_overrides)
aggregate_failures do
expect(json.size).to eq 1
expect(json.first["end_at"]).to eq "2012-01-14T12:00:00Z"
end
end
it "returns user override with prioritize_individual_overrides enabled" do
aggregate_failures do
expect(json.size).to eq 1
expect(json.first["end_at"]).to eq "2012-01-12T12:00:00Z"
end
end
end
end
end

71
spec/lib/assignment_override_applicator_spec.rb
View File

@ -990,6 +990,28 @@ describe AssignmentOverrideApplicator do
@override = assignment_override_model(assignment: @assignment)
end
context "adhoc override prioritization" do
before do
@adhoc_override = @override
@section_override = assignment_override_model(assignment: @assignment, set: @course.default_section)
@adhoc_override.override_due_at(6.days.from_now)
@section_override.override_due_at(7.days.from_now)
end
let(:due_at) do
AssignmentOverrideApplicator.overridden_due_at(@assignment, [@adhoc_override, @section_override])
end
it "always uses the adhoc due_at, if one exists, when prioritize_individual_overrides is enabled" do
expect(due_at).to eq @adhoc_override.due_at
end
it "prefers most lenient override when prioritize_individual_overrides is disabled" do
Account.site_admin.disable_feature!(:prioritize_individual_overrides)
expect(due_at).to eq @section_override.due_at
end
end
it "uses overrides that override due_at" do
@override.override_due_at(7.days.from_now)
due_at = AssignmentOverrideApplicator.overridden_due_at(@assignment, [@override])
@ -1003,14 +1025,6 @@ describe AssignmentOverrideApplicator do
expect(due_at).to eq @override2.due_at
end
it "prefers most lenient override" do
@override.override_due_at(6.days.from_now)
@override2 = assignment_override_model(assignment: @assignment)
@override2.override_due_at(7.days.from_now)
due_at = AssignmentOverrideApplicator.overridden_due_at(@assignment, [@override, @override2])
expect(due_at).to eq @override2.due_at
end
it "considers no due date as most lenient" do
@override.override_due_at(nil)
@override2 = assignment_override_model(assignment: @assignment)
@ -1110,11 +1124,42 @@ describe AssignmentOverrideApplicator do
expect(unlock_at).to eq @override.unlock_at
end
it "does not include unlock_at for previous overrides that have already been locked" do
@override.override_unlock_at(10.days.ago)
@override.override_lock_at(5.days.ago)
unlock_at = AssignmentOverrideApplicator.overridden_unlock_at(@assignment, [@override])
expect(unlock_at).to eq @assignment.unlock_at
context "with prioritize_individual_overrides enabled" do
it "includes unlock_at for previous adhoc overrides that have already been locked" do
@override.override_unlock_at(10.days.ago)
@override.override_lock_at(5.days.ago)
unlock_at = AssignmentOverrideApplicator.overridden_unlock_at(@assignment, [@override])
expect(unlock_at).to eq @override.unlock_at
end
it "does not include unlock_at for previous non-adhoc overrides that have already been locked" do
@override.set_type = "CourseSection"
@override.override_unlock_at(10.days.ago)
@override.override_lock_at(5.days.ago)
unlock_at = AssignmentOverrideApplicator.overridden_unlock_at(@assignment, [@override])
expect(unlock_at).to eq @assignment.unlock_at
end
end
context "with prioritize_individual_overrides disabled" do
before do
Account.site_admin.disable_feature!(:prioritize_individual_overrides)
end
it "does not include unlock_at for previous adhoc overrides that have already been locked" do
@override.override_unlock_at(10.days.ago)
@override.override_lock_at(5.days.ago)
unlock_at = AssignmentOverrideApplicator.overridden_unlock_at(@assignment, [@override])
expect(unlock_at).to eq @assignment.unlock_at
end
it "does not include unlock_at for previous non-adhoc overrides that have already been locked" do
@override.set_type = "CourseSection"
@override.override_unlock_at(10.days.ago)
@override.override_lock_at(5.days.ago)
unlock_at = AssignmentOverrideApplicator.overridden_unlock_at(@assignment, [@override])
expect(unlock_at).to eq @assignment.unlock_at
end
end
end

75
spec/lib/effective_due_dates_spec.rb
View File

@ -938,10 +938,62 @@ describe Course do
end
context "when multiple override types apply" do
it "picks the due date that gives the student the most amount of time to submit" do
it "picks the individual override due date, if one exists" do
# adhoc
override = @assignment2.assignment_overrides.create!(due_at: 3.days.from_now(@now), due_at_overridden: true)
override.assignment_override_students.create!(user: @student1)
individual_override = @assignment2.assignment_overrides.create!(due_at: 3.days.from_now(@now), due_at_overridden: true)
individual_override.assignment_override_students.create!(user: @student1)
# group
group_with_user(user: @student1, active_all: true)
@assignment2.assignment_overrides.create!(
due_at: 6.days.from_now(@now),
due_at_overridden: true,
set: @group
)
# everyone else
@assignment2.due_at = 4.days.from_now(@now)
@assignment2.save!
edd = EffectiveDueDates.for_course(@test_course, @assignment2)
result = edd.to_hash
expected = {
@assignment2.id => {
@student1.id => {
due_at: individual_override.due_at,
grading_period_id: nil,
in_closed_grading_period: false,
override_id: individual_override.id,
override_source: "ADHOC"
},
@student2.id => {
due_at: 4.days.from_now(@now),
grading_period_id: nil,
in_closed_grading_period: false,
override_id: nil,
override_source: "Everyone Else"
},
@student3.id => {
due_at: 4.days.from_now(@now),
grading_period_id: nil,
in_closed_grading_period: false,
override_id: nil,
override_source: "Everyone Else"
}
}
}
expect(result).to eq expected
end
it "picks the due date that gives the student the most time to submit, if no individual override exists" do
# section
section = CourseSection.create!(name: "My Awesome Section", course: @test_course)
student_in_section(section, user: @student1)
@assignment2.assignment_overrides.create!(
due_at: 3.days.from_now(@now),
due_at_overridden: true,
set: section
)
# group
group_with_user(user: @student1, active_all: true)
@ -960,7 +1012,7 @@ describe Course do
expected = {
@assignment2.id => {
@student1.id => {
due_at: 6.days.from_now(@now),
due_at: group_override.due_at,
grading_period_id: nil,
in_closed_grading_period: false,
override_id: group_override.id,
@ -986,9 +1038,14 @@ describe Course do
end
it "treats null due dates as the most permissive due date for a student" do
# adhoc
override = @assignment2.assignment_overrides.create!(due_at: nil, due_at_overridden: true)
override.assignment_override_students.create!(user: @student1)
# section
section = CourseSection.create!(name: "My Awesome Section", course: @test_course)
student_in_section(section, user: @student1)
section_override = @assignment2.assignment_overrides.create!(
due_at: nil,
due_at_overridden: true,
set: section
)
# group
group_with_user(user: @student1, active_all: true)
@ -1006,8 +1063,8 @@ describe Course do
due_at: nil,
grading_period_id: nil,
in_closed_grading_period: false,
override_id: override.id,
override_source: "ADHOC"
override_id: section_override.id,
override_source: "CourseSection"
},
@student2.id => {
due_at: 4.days.from_now(@now),

2
spec/lib/gradebook_exporter_spec.rb
View File

@ -81,6 +81,7 @@ describe GradebookExporter do
end
it "returns assignments ordered by assignment group position when feature is disabled" do
expect(Account.site_admin).to receive(:feature_enabled?).and_call_original
expect(Account.site_admin).to receive(:feature_enabled?).with(:gradebook_csv_export_order_matches_gradebook_grid).and_return(false)
actual_assignment_headers = headers[4, 3]
@ -125,6 +126,7 @@ describe GradebookExporter do
end
it "returns assignments ordered by assignment group position when feature is disabled" do
expect(Account.site_admin).to receive(:feature_enabled?).and_call_original
expect(Account.site_admin).to receive(:feature_enabled?).with(:gradebook_csv_export_order_matches_gradebook_grid).and_return(false)
actual_assignment_headers = headers[4, 3]

48
spec/models/assignment_override_spec.rb
View File

@ -110,6 +110,26 @@ describe AssignmentOverride do
expect(@override2.set).to eq [@student]
end
describe "#adhoc?" do
let(:override) { AssignmentOverride.new }
it "returns true if the override is an ad hoc override" do
override.set_type = "ADHOC"
expect(override).to be_adhoc
end
it "returns false if the override is not an ad hoc override" do
aggregate_failures do
override.set_type = "CourseSection"
expect(override).not_to be_adhoc
override.set_type = "Group"
expect(override).not_to be_adhoc
override.set_type = "Noop"
expect(override).not_to be_adhoc
end
end
end
context "#mastery_paths?" do
let(:override) do
described_class.new({
@ -598,7 +618,33 @@ describe AssignmentOverride do
override.lock_at = 10.minutes.ago
end
it { is_expected.to be(true) }
context "with prioritize_individual_overrides enabled" do
it "returns false if the override is ad hoc" do
override.set_type = "ADHOC"
expect(subject).to be(false)
end
it "returns true if the override is not ad hoc" do
override.set_type = "CourseSection"
expect(subject).to be(true)
end
end
context "with prioritize_individual_overrides disabled" do
before do
Account.site_admin.disable_feature!(:prioritize_individual_overrides)
end
it "returns true if the override is ad hoc" do
override.set_type = "ADHOC"
expect(subject).to be(true)
end
it "returns true if the override is not ad hoc" do
override.set_type = "CourseSection"
expect(subject).to be(true)
end
end
end
end
end

42
spec/models/submission_spec.rb
View File

@ -457,7 +457,7 @@ describe Submission do
}.from(20.minutes.ago(@now)).to(15.minutes.ago(@now))
end
it "does not change if an override is added for the student and the due date is earlier" \
it "changes if an individual override is added for the student, even when the due date is earlier" \
" than an existing override that applies to the student for the assignment" do
section = @course.course_sections.create!(name: "My Awesome Section")
student_in_section(section, user: @student)
@ -472,9 +472,33 @@ describe Submission do
due_at_overridden: true
)
expect { override.assignment_override_students.create!(user: @student) }.not_to change {
expect { override.assignment_override_students.create!(user: @student) }.to change {
submission.reload.cached_due_date
}
}.from(10.minutes.ago(@now)).to(15.minutes.ago(@now))
end
it "does not change if a non-individual-override is added for the student and the due date" \
" is earlier than an existing override that applies to the student for the assignment" do
category = @course.group_categories.create!(name: "New Group Category")
group = @course.groups.create!(group_category: category)
group.add_user(@student, "active")
assignment = @course.assignments.create!(group_category: category)
section = @course.course_sections.create!(name: "My Awesome Section")
student_in_section(section, user: @student)
assignment.assignment_overrides.create!(
due_at: 10.minutes.ago(@now),
due_at_overridden: true,
set: section
)
expect do
assignment.assignment_overrides.create!(
due_at: 15.minutes.ago(@now),
due_at_overridden: true,
set: group
)
end.not_to change { submission.reload.cached_due_date }
end
it "changes if an override is removed for the student" do
@ -609,7 +633,7 @@ describe Submission do
end
end
it "uses the most lenient due date if there are multiple overrides" do
it "uses the individual override date, otherwise most lenient, if there are multiple overrides" do
category = @course.group_categories.create!(name: "New Group Category")
group = @course.groups.create!(group_category: category)
assignment = @course.assignments.create!(group_category: category, due_at: 20.minutes.ago(@now))
@ -637,13 +661,13 @@ describe Submission do
override_student = student_override.assignment_override_students.create!(user: @student)
submission = assignment.submissions.find_by!(user: @student)
expect { @student.enrollments.find_by(course_section: section).destroy }.to change {
submission.reload.cached_due_date
}.from(6.minutes.ago(@now)).to(14.minutes.ago(@now))
expect { override_student.destroy }.to change {
submission.reload.cached_due_date
}.from(14.minutes.ago(@now)).to(21.minutes.ago(@now))
}.from(14.minutes.ago(@now)).to(6.minutes.ago(@now))
expect { @student.enrollments.find_by(course_section: section).destroy }.to change {
submission.reload.cached_due_date
}.from(6.minutes.ago(@now)).to(21.minutes.ago(@now))
end
it "uses override due dates instead of assignment due dates, even if the assignment due date is more lenient" do