From 195284c4c7ec39d374a8c9b0e1699673a46bc130 Mon Sep 17 00:00:00 2001 From: Jackson Howe Date: Wed, 12 Jun 2024 10:44:22 -0600 Subject: [PATCH] Don't enumerate page ids in WikiPage#visible_to_user The WikiPageVisibilityService needs both a user id and either course ids or page ids to run performantly. Previously, we were providing the ids of the pages currently selected by the scope, but the way this scope is used, the scope was targeting almost all pages on the shard. Thus, we'd be enumerating quite a few page ids on larger shards. Fortunately, this scope is only used in 1 place (the planner), so instead of providing user_id+wiki_page_ids to the service, we'll have the planner pass the course_ids it knows the user is interested in and the scope will pass those to the visibility service. closes LX-1824 flag = selective_release_backend Test plan: - Create some pages with todo dates in the next week - Assign some of those pages to a student - Visit the planner as a student and expect to see only the pages they've been assigned Change-Id: I500ab52959dbc28764d84026d8732f2706ad8888 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/349987 Reviewed-by: Sarah Gerard QA-Review: Sarah Gerard Product-Review: Jackson Howe Tested-by: Service Cloud Jenkins --- app/models/user_learning_object_scopes.rb | 3 +- app/models/wiki_page.rb | 18 +++++------ .../wiki_page_visibility_service.rb | 9 ++++++ .../user_learning_object_scopes_spec.rb | 10 ++++++ spec/models/wiki_page_spec.rb | 31 +++++++++++++++---- 5 files changed, 53 insertions(+), 18 deletions(-) diff --git a/app/models/user_learning_object_scopes.rb b/app/models/user_learning_object_scopes.rb index 907dcb7b146..d1c45804717 100644 --- a/app/models/user_learning_object_scopes.rb +++ b/app/models/user_learning_object_scopes.rb @@ -453,8 +453,7 @@ module UserLearningObjectScopes objects_needing("WikiPage", "viewing", :student, params, 120.minutes, **opts) do |wiki_pages_context, shard_course_ids, shard_group_ids| wiki_pages_context .available_to_planner - .visible_to_user(self) - .for_courses_and_groups(shard_course_ids, shard_group_ids) + .visible_to_user_in_courses_and_groups(self, shard_course_ids, shard_group_ids) .todo_date_between(due_after, due_before) end end diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index d460737fdcb..2ea073e8f91 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -103,23 +103,21 @@ class WikiPage < ActiveRecord::Base where.not(Ignore.where(asset_type: "WikiPage", user_id: user, purpose:).where("asset_id=wiki_pages.id").arel.exists) } scope :todo_date_between, ->(starting, ending) { where(todo_date: starting...ending) } - scope :for_courses_and_groups, lambda { |course_ids, group_ids| + + scope :visible_to_user_in_courses_and_groups, lambda { |user_id, course_ids, group_ids| wiki_ids = [] wiki_ids += Course.where(id: course_ids).pluck(:wiki_id) if course_ids.any? wiki_ids += Group.where(id: group_ids).pluck(:wiki_id) if group_ids.any? - where(wiki_id: wiki_ids) - } - - scope :visible_to_user, lambda { |user_id| + context_pages = where(wiki_id: wiki_ids) if Account.site_admin.feature_enabled?(:selective_release_backend) - scope_assignments = where.not(assignment_id: nil).pluck(:assignment_id) - visible_wiki_pages = WikiPageVisibility::WikiPageVisibilityService.wiki_pages_visible_to_student_by_pages(user_id:, wiki_page_ids: ids).map(&:wiki_page_id) + scope_assignments = context_pages.where.not(assignment_id: nil).pluck(:assignment_id) + visible_wiki_pages = WikiPageVisibility::WikiPageVisibilityService.wiki_pages_visible_to_student_in_courses(user_id:, course_ids:).map(&:wiki_page_id) visible_assignments = AssignmentVisibility::AssignmentVisibilityService.assignments_visible_to_student_by_assignment(user_id:, assignment_ids: scope_assignments).map(&:assignment_id) - where("wiki_pages.assignment_id IS NULL AND (wiki_pages.id IN (?) OR wiki_pages.context_type = 'Group')", visible_wiki_pages) - .or(where("wiki_pages.assignment_id IS NOT NULL AND wiki_pages.assignment_id IN (?)", visible_assignments)) + context_pages.where("wiki_pages.assignment_id IS NULL AND (wiki_pages.id IN (?) OR wiki_pages.context_type = 'Group')", visible_wiki_pages) + .or(context_pages.where("wiki_pages.assignment_id IS NOT NULL AND wiki_pages.assignment_id IN (?)", visible_assignments)) else - where("wiki_pages.assignment_id IS NULL OR EXISTS (SELECT 1 FROM #{AssignmentStudentVisibility.quoted_table_name} asv WHERE wiki_pages.assignment_id = asv.assignment_id AND asv.user_id = ?)", user_id) + context_pages.where("wiki_pages.assignment_id IS NULL OR EXISTS (SELECT 1 FROM #{AssignmentStudentVisibility.quoted_table_name} asv WHERE wiki_pages.assignment_id = asv.assignment_id AND asv.user_id = ?)", user_id) end } diff --git a/app/services/wiki_page_visibility/wiki_page_visibility_service.rb b/app/services/wiki_page_visibility/wiki_page_visibility_service.rb index 25cffe0cb31..df307789d2a 100644 --- a/app/services/wiki_page_visibility/wiki_page_visibility_service.rb +++ b/app/services/wiki_page_visibility/wiki_page_visibility_service.rb @@ -38,6 +38,15 @@ module WikiPageVisibility wiki_pages_visible_to_students(course_id_params: course_ids, user_id_params: user_ids) end + def wiki_pages_visible_to_student_in_courses(user_id:, course_ids:) + raise ArgumentError, "course_ids cannot be nil" if course_ids.nil? + raise ArgumentError, "course_ids must be an array" unless course_ids.is_a?(Array) + raise ArgumentError, "user_id cannot be nil" if user_id.nil? + raise ArgumentError, "user_id must not be an array" if user_id.is_a?(Array) + + wiki_pages_visible_to_students(course_id_params: course_ids, user_id_params: user_id) + end + def wiki_page_visible_to_student(wiki_page_id:, user_id:) raise ArgumentError, "wiki_page_id cannot be nil" if wiki_page_id.nil? raise ArgumentError, "wiki_page_id must not be an array" if wiki_page_id.is_a?(Array) diff --git a/spec/models/user_learning_object_scopes_spec.rb b/spec/models/user_learning_object_scopes_spec.rb index 5f167c3c350..54dee761f8e 100644 --- a/spec/models/user_learning_object_scopes_spec.rb +++ b/spec/models/user_learning_object_scopes_spec.rb @@ -1219,6 +1219,16 @@ describe UserLearningObjectScopes do expect(student2.wiki_pages_needing_viewing(**opts)).to eq [@course_page] end + it "does not show wiki pages that are not visible to the user" do + Account.site_admin.enable_feature! :selective_release_backend + @course_page.update!(todo_date: 1.day.from_now, only_visible_to_overrides: true) + section2 = add_section("Section 2") + student2 = student_in_section(section2) + @course_page.assignment_overrides.create!(set: section2) + expect(@student.wiki_pages_needing_viewing(**opts)).to eq [] + expect(student2.wiki_pages_needing_viewing(**opts)).to eq [@course_page] + end + context "include_concluded" do before :once do @u = User.create! diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index a4fa1406f48..50b6e6d7cca 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -1251,7 +1251,7 @@ describe WikiPage do end end - describe "visible_ids_by_user and visible_to_user" do + describe "visible_ids_by_user and visible_to_user_in_courses_and_groups" do before :once do @course1 = course_factory(active_all: true) @page1 = @course1.wiki_pages.create!(title: "page1") @@ -1264,7 +1264,7 @@ describe WikiPage do def assert_visible(user, pages) visible_ids_by_user = WikiPage.visible_ids_by_user({ user_id: [user.id], course_id: [@course1.id] }) - visible_to_user = WikiPage.visible_to_user(user.id).pluck(:id) + visible_to_user = WikiPage.visible_to_user_in_courses_and_groups(user.id, [@course1.id], []).pluck(:id) expect(visible_ids_by_user[user.id]).to contain_exactly(*pages.map(&:id)) expect(visible_to_user).to contain_exactly(*pages.map(&:id)) end @@ -1295,10 +1295,19 @@ describe WikiPage do expect(visible_ids_by_user[@student1.id]).to contain_exactly(@page1.id) end - it "includes group pages" do - group = group_model(context: @course1) - group_page = group.wiki_pages.create!(title: "group page") - expect(WikiPage.visible_to_user(@student1.id)).to include(group_page) + context "group pages" do + before :once do + @group = group_model(context: @course1) + @group_page = @group.wiki_pages.create!(title: "group page") + end + + it "includes group pages" do + expect(WikiPage.visible_to_user_in_courses_and_groups(@student1.id, [@course1.id], [@group.id])).to include(@group_page) + end + + it "does not include group pages not in group_ids" do + expect(WikiPage.visible_to_user_in_courses_and_groups(@student1.id, [@course1.id], [])).not_to include(@group_page) + end end context "with selective_release_backend disabled" do @@ -1346,6 +1355,16 @@ describe WikiPage do assert_visible(@student1, []) assert_visible(@student2, []) end + + it "does not include pages from another course" do + course2 = course_factory(active_all: true) + course2.wiki_pages.create!(title: "page3") + student_in_course(course: course2, user: @student1, active_all: true) + page4 = course2.wiki_pages.create!(title: "page2") + assignment = course2.assignments.create!(title: "assignment") + page4.update!(assignment_id: assignment.id) + assert_visible(@student1, [@page1]) + end end end end