Eliminate assignment_context_modules_without_overrides n+1
refs LX-1871
flag = selective_release_backend
[skip-crystalball]
Test plan:
- Open the assignments index page on a course with a lot of
assignments
- Search request logs for:
`SELECT "assignment_overrides".* FROM "public"."assignment_overrides"
WHERE "assignment_overrides"."workflow_state" = 'active'
AND "assignment_overrides"."context_module_id" =`
- Expect there to be significantly fewer of these than without this
change
- Open the assignments index API and expect the visible_to_everyone
value to still be correct (particularly for assignments in multiple
modules where some modules have overrides)
Change-Id: I3791315786e94ac064b1f45475bb22b2ce44e3b5
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/352497
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Sarah Gerard <sarah.gerard@instructure.com>
QA-Review: Sarah Gerard <sarah.gerard@instructure.com>
Product-Review: Jackson Howe <jackson.howe@instructure.com>
This commit is contained in:
Jackson Howe
parent
3fb40be4ff
commit
13694bfa85
|
|
@ -25,7 +25,8 @@ module DatesOverridable
|
|||
:has_too_many_overrides,
|
||||
:preloaded_override_students,
|
||||
:has_course_overrides,
|
||||
:preloaded_module_ids
|
||||
:preloaded_module_ids,
|
||||
:preloaded_module_overrides
|
||||
attr_writer :without_overrides
|
||||
|
||||
include DifferentiableAssignment
|
||||
|
|
@ -83,24 +84,20 @@ module DatesOverridable
|
|||
|
||||
def all_assignment_overrides
|
||||
if Account.site_admin.feature_enabled? :selective_release_backend
|
||||
assignment_overrides.or(context_module_overrides)
|
||||
assignment_overrides.or(AssignmentOverride.active.where(context_module_id: module_ids))
|
||||
else
|
||||
assignment_overrides.where.not(set_type: "Course")
|
||||
end
|
||||
end
|
||||
|
||||
def context_module_overrides
|
||||
AssignmentOverride.active.where(context_module_id: module_ids)
|
||||
end
|
||||
|
||||
def visible_to_everyone
|
||||
if Account.site_admin.feature_enabled? :selective_release_backend
|
||||
if is_a?(DiscussionTopic)
|
||||
# need to check if is_section_specific for ungraded discussions
|
||||
# this column will eventually be deprecated and then this can be removed
|
||||
course_overrides? || ((!only_visible_to_overrides && !is_section_specific) && (module_ids.empty? || (module_ids.any? && assignment_context_modules_without_overrides.any?)))
|
||||
course_overrides? || ((!only_visible_to_overrides && !is_section_specific) && (module_ids.empty? || (module_ids.any? && modules_without_overrides?)))
|
||||
else
|
||||
course_overrides? || (!only_visible_to_overrides && (module_ids.empty? || (module_ids.any? && assignment_context_modules_without_overrides.any?)))
|
||||
course_overrides? || (!only_visible_to_overrides && (module_ids.empty? || (module_ids.any? && modules_without_overrides?)))
|
||||
end
|
||||
else
|
||||
!only_visible_to_overrides
|
||||
|
|
@ -120,9 +117,10 @@ module DatesOverridable
|
|||
end
|
||||
end
|
||||
|
||||
def assignment_context_modules_without_overrides
|
||||
context_modules_with_overrides = context_module_overrides.select(:context_module_id)
|
||||
assignment_context_modules.where.not(id: context_modules_with_overrides)
|
||||
def modules_without_overrides?
|
||||
module_ids_with_overrides = context_module_overrides.map(&:context_module_id)
|
||||
module_ids_without_overrides = module_ids.reject { |module_id| module_ids_with_overrides.include?(module_id) }
|
||||
module_ids_without_overrides.any?
|
||||
end
|
||||
|
||||
def self.preload_override_data_for_objects(learning_objects)
|
||||
|
|
@ -131,6 +129,7 @@ module DatesOverridable
|
|||
|
||||
preload_has_course_overrides(learning_objects)
|
||||
preload_module_ids(learning_objects)
|
||||
preload_module_overrides(learning_objects)
|
||||
end
|
||||
|
||||
def self.preload_has_course_overrides(learning_objects)
|
||||
|
|
@ -182,6 +181,14 @@ module DatesOverridable
|
|||
end
|
||||
end
|
||||
|
||||
def self.preload_module_overrides(learning_objects)
|
||||
all_module_ids = learning_objects.map(&:module_ids).flatten.uniq
|
||||
all_module_overrides = AssignmentOverride.active.where(context_module_id: all_module_ids)
|
||||
learning_objects.each do |lo|
|
||||
lo.preloaded_module_overrides = all_module_overrides.select { |ao| lo.module_ids.include?(ao.context_module_id) }
|
||||
end
|
||||
end
|
||||
|
||||
def self.overrides_for_objects(learning_objects)
|
||||
AssignmentOverride.where(assignment_id: learning_objects.select { |lo| lo.is_a?(Assignment) }.map(&:id))
|
||||
.or(AssignmentOverride.where(quiz_id: learning_objects.select { |lo| lo.is_a?(Quizzes::Quiz) }.map(&:id)))
|
||||
|
|
@ -201,6 +208,12 @@ module DatesOverridable
|
|||
@preloaded_module_ids
|
||||
end
|
||||
|
||||
def context_module_overrides
|
||||
return AssignmentOverride.active.where(context_module_id: module_ids) if @preloaded_module_overrides.nil?
|
||||
|
||||
@preloaded_module_overrides
|
||||
end
|
||||
|
||||
def multiple_due_dates?
|
||||
if overridden
|
||||
!!multiple_due_dates_apply_to?(overridden_for_user)
|
||||
|
|
|
|||
|
|
@ -692,12 +692,13 @@ describe "preload_override_data_for_objects" do
|
|||
@discussion2 = discussion_topic_model(context: @course)
|
||||
@page1 = wiki_page_model(course: @course)
|
||||
@page2 = wiki_page_model(course: @course)
|
||||
@all_objects = [@assignment1, @assignment2, @quiz1, @quiz2, @discussion1, @discussion2, @page1, @page2]
|
||||
end
|
||||
|
||||
let(:all_objects) { [@assignment1, @assignment2, @quiz1, @quiz2, @discussion1, @discussion2, @page1, @page2] }
|
||||
|
||||
describe "preload_has_course_overrides" do
|
||||
it "sets has_course_overrides to nil by default" do
|
||||
@all_objects.each do |lo|
|
||||
all_objects.each do |lo|
|
||||
expect(lo.has_course_overrides).to be_nil
|
||||
end
|
||||
end
|
||||
|
|
@ -706,22 +707,22 @@ describe "preload_override_data_for_objects" do
|
|||
[@assignment1, @quiz2, @discussion1, @page2].each do |lo|
|
||||
lo.assignment_overrides.create!(set: @course)
|
||||
end
|
||||
DatesOverridable.preload_has_course_overrides(@all_objects)
|
||||
DatesOverridable.preload_has_course_overrides(all_objects)
|
||||
expected_values = [true, false, false, true, true, false, false, true]
|
||||
expect(@all_objects.map(&:has_course_overrides)).to eq expected_values
|
||||
expect(@all_objects.map(&:course_overrides?)).to eq expected_values
|
||||
expect(all_objects.map(&:has_course_overrides)).to eq expected_values
|
||||
expect(all_objects.map(&:course_overrides?)).to eq expected_values
|
||||
end
|
||||
|
||||
it "ignores deleted overrides" do
|
||||
@assignment1.assignment_overrides.create!(set: @course, workflow_state: "deleted")
|
||||
DatesOverridable.preload_has_course_overrides(@all_objects)
|
||||
DatesOverridable.preload_has_course_overrides(all_objects)
|
||||
expect(@assignment1.has_course_overrides).to be false
|
||||
expect(@assignment1.course_overrides?).to be false
|
||||
end
|
||||
|
||||
it "ignores non-course overrides" do
|
||||
@assignment1.assignment_overrides.create!(set: @course.default_section)
|
||||
DatesOverridable.preload_has_course_overrides(@all_objects)
|
||||
DatesOverridable.preload_has_course_overrides(all_objects)
|
||||
expect(@assignment1.has_course_overrides).to be false
|
||||
expect(@assignment1.course_overrides?).to be false
|
||||
end
|
||||
|
|
@ -735,7 +736,7 @@ describe "preload_override_data_for_objects" do
|
|||
|
||||
describe "preload_module_ids" do
|
||||
it "sets preloaded_module_ids to nil by default" do
|
||||
@all_objects.each do |lo|
|
||||
all_objects.each do |lo|
|
||||
expect(lo.preloaded_module_ids).to be_nil
|
||||
end
|
||||
end
|
||||
|
|
@ -746,10 +747,10 @@ describe "preload_override_data_for_objects" do
|
|||
@quiz1.context_module_tags.create!(context_module: @module1, context: @course, tag_type: "context_module")
|
||||
@discussion2.context_module_tags.create!(context_module: @module2, context: @course, tag_type: "context_module")
|
||||
|
||||
DatesOverridable.preload_module_ids(@all_objects)
|
||||
DatesOverridable.preload_module_ids(all_objects)
|
||||
expected_values = [[@module1.id, @module2.id], [], [@module1.id], [], [], [@module2.id], [], []]
|
||||
expect(@all_objects.map(&:preloaded_module_ids)).to eq expected_values
|
||||
expect(@all_objects.map(&:module_ids)).to eq expected_values
|
||||
expect(all_objects.map(&:preloaded_module_ids)).to eq expected_values
|
||||
expect(all_objects.map(&:module_ids)).to eq expected_values
|
||||
end
|
||||
|
||||
it "works for assignments that are part of a quiz" do
|
||||
|
|
@ -781,7 +782,7 @@ describe "preload_override_data_for_objects" do
|
|||
it "ignores deleted ContentTags" do
|
||||
@assignment1.context_module_tags.create!(context_module: @module1, context: @course, tag_type: "context_module", workflow_state: "deleted")
|
||||
@assignment1.context_module_tags.create!(context_module: @module2, context: @course, tag_type: "context_module")
|
||||
DatesOverridable.preload_module_ids(@all_objects)
|
||||
DatesOverridable.preload_module_ids(all_objects)
|
||||
expect(@assignment1.preloaded_module_ids).to eq [@module2.id]
|
||||
expect(@assignment1.module_ids).to eq [@module2.id]
|
||||
end
|
||||
|
|
@ -792,4 +793,43 @@ describe "preload_override_data_for_objects" do
|
|||
expect(@assignment1.module_ids).to eq [@module1.id]
|
||||
end
|
||||
end
|
||||
|
||||
describe "preload_module_overrides" do
|
||||
before :once do
|
||||
@assignment1.context_module_tags.create!(context_module: @module1, context: @course, tag_type: "context_module")
|
||||
@assignment1.context_module_tags.create!(context_module: @module2, context: @course, tag_type: "context_module")
|
||||
@quiz1.context_module_tags.create!(context_module: @module1, context: @course, tag_type: "context_module")
|
||||
@discussion2.context_module_tags.create!(context_module: @module2, context: @course, tag_type: "context_module")
|
||||
@ao1 = @module1.assignment_overrides.create!
|
||||
@ao2 = @module1.assignment_overrides.create!
|
||||
@ao3 = @module2.assignment_overrides.create!
|
||||
end
|
||||
|
||||
it "sets preloaded_module_overrides to nil by default" do
|
||||
all_objects.each do |lo|
|
||||
expect(lo.preloaded_module_overrides).to be_nil
|
||||
end
|
||||
end
|
||||
|
||||
it "sets the preloaded_module_overrides attribute correctly" do
|
||||
DatesOverridable.preload_module_ids(all_objects)
|
||||
DatesOverridable.preload_module_overrides(all_objects)
|
||||
expected_values = [[@ao1, @ao2, @ao3], [], [@ao1, @ao2], [], [], [@ao3], [], []]
|
||||
expect(all_objects.map(&:preloaded_module_overrides)).to eq expected_values
|
||||
expect(all_objects.map(&:context_module_overrides)).to eq expected_values
|
||||
end
|
||||
|
||||
it "ignores deleted overrides" do
|
||||
@ao1.destroy
|
||||
DatesOverridable.preload_module_ids(all_objects)
|
||||
DatesOverridable.preload_module_overrides(all_objects)
|
||||
expect(@assignment1.preloaded_module_overrides).to eq [@ao2, @ao3]
|
||||
expect(@assignment1.context_module_overrides).to eq [@ao2, @ao3]
|
||||
end
|
||||
|
||||
it "falls back to one-off calculation if not preloaded" do
|
||||
expect(@assignment1.preloaded_module_overrides).to be_nil
|
||||
expect(@assignment1.context_module_overrides).to eq [@ao1, @ao2, @ao3]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
Loading…
Reference in New Issue