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

Consider only relevant discussions when evaluating CMPs 

Context module progressions should only consider items that a user
is able to view when evaluating a module's completion status. Ungraded
discussions can be posted to only certain sections within a course, so
only discussions in section(s) where the student is enrolled should be
considered. This is normally the case, however, when evaluating all of
the progressions within a course, we cache the item visibilities to
prevent recalculating the same thing repeatedly (see
ContextModule#evaluate_all_progressions). This codepath is not
frequently followed, but is, for example, when a module itelf is
edited. In the caching process, we get all of the sections (and
associated discussions) visible to a list of users. This was an issue
becuase the list of users might contain users with different section
visibilities. This patchset considers section visibilities for each
user in the list individually, ensuring that module progressions
evaluate appropriately for all users.

closes LS-3245
flag = none

Test plan:
 - Create a course with 2 sections
 - Create a module in the course
 - Create 2 discussions in the module - both ungraded; assign one to
   the first section, and one to the second section
 - Create a page in the module
 - Edit module requirements - set "contribute to the page" on both
   discussions and "view" on the page
 - Enroll a student in one of the sections and publish the course
 - As the student, view the page and reply to the visible discussion
 - Expect the module to be marked as completed
 - As the teacher, create a new (empty) module, move it to the top of
   the page, then delete it
 - As the student, view the modules page; expect the module to still
   be marked as completed

Change-Id: I4d9a57dae0cf521a46953f4c5217df7358b08835
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/296104
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Luis Oliveira <luis.oliveira@instructure.com>
Reviewed-by: Luis Oliveira <luis.oliveira@instructure.com>
Product-Review: Jackson Howe <jackson.howe@instructure.com>
This commit is contained in:
Jackson Howe 2022-07-13 14:33:57 -06:00
parent 0eb99d975f
commit f3aa9a9f78
2 changed files with 71 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
lib/submittable.rb
View File

@ -42,8 +42,9 @@ module Submittable
module ClassMethods
def visible_ids_by_user(opts)
# pluck id, assignment_id, and user_id from items joined with the SQL view
# Items with an assignment: pluck id, assignment_id, and user_id from items joined with the SQL view
plucked_visibilities = pluck_visibilities(opts).group_by { |_, _, user_id| user_id }
# Assignment-less items are *normally* visible to all -- the exception is
# section-specific discussions, so here get the ones visible to everyone in the
# course, and below get the ones that are visible to the right section.
@ -55,18 +56,33 @@ module Submittable
# Now get the section-specific discussions that are in the proper sections.
ids_visible_to_sections = if opts[:item_type] == :discussion
user_sections = Enrollment.active.where(
course_id: opts[:course_id], user_id: opts[:user_id]
).pluck(:course_section_id)
DiscussionTopicSectionVisibility.active.where(course_section_id: user_sections).pluck(:discussion_topic_id).uniq
# build hash of user_ids to array of section ids
sections_per_user = {}
Enrollment.active.where(course_id: opts[:course_id], user_id: opts[:user_id])
.pluck(:user_id, :course_section_id)
.each { |user_id, section_id| (sections_per_user[user_id] ||= Set.new) << section_id }
# build hash of section_ids to array of visible topic ids
all_section_ids = sections_per_user.values.reduce([]) { |all_ids, section_ids| all_ids.concat(section_ids.to_a) }
topic_ids_per_section = {}
DiscussionTopicSectionVisibility.active.where(course_section_id: all_section_ids)
.pluck(:course_section_id, :discussion_topic_id)
.each { |section_id, topic_id| (topic_ids_per_section[section_id] ||= Set.new) << topic_id }
topic_ids_per_section.each { |section_id, topic_ids| topic_ids_per_section[section_id] = topic_ids.to_a }
# finally, build hash of user_ids to array of visible topic ids
topic_ids_per_user = {}
opts[:user_id].each { |user_id| topic_ids_per_user[user_id] = sections_per_user[user_id]&.map { |section_id| topic_ids_per_section[section_id] }&.flatten&.uniq&.compact }
topic_ids_per_user
else
[]
end
# build map of user_ids to array of item ids {1 => [2,3,4], 2 => [2,4]}
opts[:user_id].index_with do |student_id|
ids_from_pluck = (plucked_visibilities[student_id] || []).map { |id, _, _| id }
ids_from_pluck.concat(ids_visible_to_all).concat(ids_visible_to_sections)
assignment_item_ids = (plucked_visibilities[student_id] || []).map { |id, _, _| id }
section_specific_ids = ids_visible_to_sections[student_id] || []
assignment_item_ids.concat(ids_visible_to_all).concat(section_specific_ids)
end
end

48
spec/lib/submittable_spec.rb
View File

@ -162,6 +162,28 @@ describe "section specific topic" do
expect(vis_hash[student.id].first).to eq(section_specific_topic1.id)
end
it "filters section specific topics properly for multiple users" do
course = course_factory(active_all: true)
section1 = course.course_sections.create!(name: "section 1")
section2 = course.course_sections.create!(name: "section 2")
topic1 = course.discussion_topics.create!(title: "topic 1 (for section 1)")
topic2 = course.discussion_topics.create!(title: "topic 2 (for section 2)")
topic3 = course.discussion_topics.create!(title: "topic 3 (for all sections)")
topic4 = course.discussion_topics.create!(title: "topic 4 (for section 2)")
add_section_to_topic(topic1, section1)
add_section_to_topic(topic2, section2)
add_section_to_topic(topic4, section2)
student = user_factory(active_all: true)
teacher = user_factory(active_all: true)
course.enroll_student(student, section: section2)
course.enroll_teacher(teacher, section: section1)
course.reload
vis_hash = DiscussionTopic.visible_ids_by_user(course_id: course.id, user_id: [student.id, teacher.id], item_type: :discussion)
expect(vis_hash[student.id]).to contain_exactly(topic2.id, topic3.id, topic4.id)
expect(vis_hash[teacher.id]).to contain_exactly(topic1.id, topic3.id)
end
it "properly filters section specific topics for deleted section visibilities" do
course = course_factory(active_course: true)
section1 = course.course_sections.create!(name: "section for student")
@ -174,4 +196,30 @@ describe "section specific topic" do
vis_hash = DiscussionTopic.visible_ids_by_user(course_id: course.id, user_id: [student.id], item_type: :discussion)
expect(vis_hash[student.id].length).to eq(0)
end
it "handles sections that don't have any discussion topics" do
course = course_factory(active_all: true)
section1 = course.course_sections.create!(name: "section 1")
section2 = course.course_sections.create!(name: "section 2")
topic1 = course.discussion_topics.create!(title: "topic 1 (for section 1)")
add_section_to_topic(topic1, section1)
student = user_factory(active_all: true)
course.enroll_student(student, section: section2)
course.reload
vis_hash = DiscussionTopic.visible_ids_by_user(course_id: course.id, user_id: [student.id], item_type: :discussion)
expect(vis_hash[student.id].length).to be(0)
end
it "handles user not enrolled in any sections" do
course = course_factory(active_all: true)
section1 = course.course_sections.create!(name: "section 1")
topic1 = course.discussion_topics.create!(title: "topic 1 (for section 1)")
add_section_to_topic(topic1, section1)
student = user_factory(active_all: true)
course.reload
vis_hash = DiscussionTopic.visible_ids_by_user(course_id: course.id, user_id: [student.id], item_type: :discussion)
expect(vis_hash[student.id].length).to be(0)
end
end