From e2e1fbc00e8f98c95f8c03810571282a9f9f12ee Mon Sep 17 00:00:00 2001 From: Rob Orton Date: Thu, 19 Nov 2020 12:50:49 -0700 Subject: [PATCH] add course_ids for appointments appointments are actually calendar events that can and do belong to many courses. If a user has a notification preference in any of the courses, we will respect that preference. test plan - it should set course_ids and respect notification preferences fixes VICE-983 flag=none Change-Id: I4ccaaab541eb92599f4e1ca1fb63f1930556a8d5 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/253226 Tested-by: Service Cloud Jenkins Reviewed-by: Caleb Guanzon QA-Review: Caleb Guanzon Product-Review: Caleb Guanzon --- .../types/communication_channel_type.rb | 2 +- app/models/appointment_group.rb | 8 +++- app/models/calendar_event.rb | 2 + app/models/notification_policy_override.rb | 20 +++++++--- lib/notification_message_creator.rb | 40 ++++++++++--------- spec/models/appointment_group_spec.rb | 31 +++++++++++++- spec/models/calendar_event_spec.rb | 16 +++++++- .../notification_policy_override_spec.rb | 2 +- 8 files changed, 92 insertions(+), 29 deletions(-) diff --git a/app/graphql/types/communication_channel_type.rb b/app/graphql/types/communication_channel_type.rb index 5f8ab1c077d..abba85de4fc 100644 --- a/app/graphql/types/communication_channel_type.rb +++ b/app/graphql/types/communication_channel_type.rb @@ -53,7 +53,7 @@ module Types end def notification_policy_overrides(account_id: nil, course_id: nil, context_type: nil) overrides_for = ->(context) do - NotificationPolicyOverride.find_all_for(current_user, context, channel: object) + NotificationPolicyOverride.find_all_for(current_user, [context], channel: object) end case context_type diff --git a/app/models/appointment_group.rb b/app/models/appointment_group.rb index e17c90c1a0f..d44457843cb 100644 --- a/app/models/appointment_group.rb +++ b/app/models/appointment_group.rb @@ -263,8 +263,12 @@ class AppointmentGroup < ActiveRecord::Base def broadcast_data data = {} - data[:root_account_id] = context.root_account_id if context.root_account_id - data[:course_id] = context.id if context.is_a?(Course) + ids = appointment_group_contexts.where(context_type: 'Course'). + joins("INNER JOIN #{Course.quoted_table_name} ON courses.id = context_id").pluck(:context_id, :root_account_id) + ids += appointment_group_contexts.where(context_type: 'CourseSection'). + joins("INNER JOIN #{CourseSection.quoted_table_name} ON course_sections.id = context_id").pluck(:course_id, :root_account_id) + data[:root_account_id] = ids.map(&:last).first + data[:course_ids] = ids.map(&:first).sort data end diff --git a/app/models/calendar_event.rb b/app/models/calendar_event.rb index cc21a20f984..29e07e17d58 100644 --- a/app/models/calendar_event.rb +++ b/app/models/calendar_event.rb @@ -406,6 +406,8 @@ class CalendarEvent < ActiveRecord::Base has_a_broadcast_policy def course_broadcast_data + return appointment_group.broadcast_data if appointment_group + if context.respond_to?(:broadcast_data) context.broadcast_data else diff --git a/app/models/notification_policy_override.rb b/app/models/notification_policy_override.rb index 02b555bb913..07b0298820d 100644 --- a/app/models/notification_policy_override.rb +++ b/app/models/notification_policy_override.rb @@ -69,16 +69,26 @@ class NotificationPolicyOverride < ActiveRecord::Base end def self.enabled_for(user, context, channel: nil) - !(find_all_for(user, context, channel: channel).find { |npo| npo.notification_id.nil? && npo.workflow_state == 'disabled' }) + enabled_for_all_contexts(user, [context], channel: channel) end - def self.find_all_for(user, context, channel: nil) + def self.enabled_for_all_contexts(user, contexts, channel: nil) + !(find_all_for(user, contexts, channel: channel).find { |npo| npo.notification_id.nil? && npo.workflow_state == 'disabled' }) + end + + def self.find_all_for(user, contexts, channel: nil) + raise ArgumentError, "can only pass one type of context" if contexts.map(&:class).map(&:name).uniq.length > 1 + if channel&.notification_policy_overrides&.loaded? - channel.notification_policy_overrides.select { |npo| npo.context_id == context.id && npo.context_type == context.class.name } + npos = [] + contexts.each do |context| + npos += channel.notification_policy_overrides.select { |npo| npo.context_id == context.id && npo.context_type == context.class.name } + end + npos elsif channel - channel.notification_policy_overrides.where(context_id: context.id, context_type: context.class.name) + channel.notification_policy_overrides.where(context_id: contexts.map(&:id), context_type: contexts.first.class.name) else - user.notification_policy_overrides.where(context_id: context.id, context_type: context.class.name) + user.notification_policy_overrides.where(context_id: contexts.map(&:id), context_type: contexts.first.class.name) end end diff --git a/lib/notification_message_creator.rb b/lib/notification_message_creator.rb index 9a54384b621..a50429451f8 100644 --- a/lib/notification_message_creator.rb +++ b/lib/notification_message_creator.rb @@ -29,6 +29,7 @@ class NotificationMessageCreator include LocaleSelection attr_accessor :notification, :asset, :to_user_channels, :message_data + attr_reader :courses, :account # Options can include: # :to_list - A list of Users, User IDs, and CommunicationChannels to send to @@ -39,11 +40,12 @@ class NotificationMessageCreator @to_user_channels = user_channels(options[:to_list]) @user_counts = recent_messages_for_users(@to_user_channels.keys) @message_data = options.delete(:data) - course_id = @message_data&.dig(:course_id) + course_ids = @message_data&.dig(:course_ids) + course_ids ||= [@message_data&.dig(:course_id)] root_account_id = @message_data&.dig(:root_account_id) - if course_id && root_account_id + if course_ids.any? && root_account_id @account = Account.new(id: root_account_id) - @course = Course.new(id: course_id) + @courses = course_ids.map { |id| Course.new(id: id, root_account_id: @account&.id) } end end @@ -76,7 +78,8 @@ class NotificationMessageCreator # dashboard message in addition to itself. channels.each do |channel| channel.set_root_account_ids(persist_changes: true, log: true) - next unless notifications_enabled_for_context?(user, @course) + next unless notifications_enabled_for_courses?(user) + if immediate_policy?(user, channel) immediate_messages << build_immediate_message_for(user, channel) delayed_messages << build_fallback_for(user, channel) @@ -103,12 +106,12 @@ class NotificationMessageCreator # root_account_id on the message, or look up policy overrides in the future. # A user can disable notifications for a course with a notification policy # override. - def notifications_enabled_for_context?(user, context) + def notifications_enabled_for_courses?(user) # if the message is not summarizable?, it is in a context that notifications # cannot be disabled, so return true before checking. return true unless @notification.summarizable? - return NotificationPolicyOverride.enabled_for(user, context) if context + return NotificationPolicyOverride.enabled_for_all_contexts(user, courses) if courses&.any? true end @@ -214,11 +217,12 @@ class NotificationMessageCreator end def effective_policy_for(user, channel) - # a user can override the notification preference for a context, the context - # needs to be provided in the notification from broadcast_policy, the lowest - # level override is the one that should be respected. - policy = override_policy_for(channel, @message_data&.dig(:course_id), 'Course') - policy ||= override_policy_for(channel, @message_data&.dig(:root_account_id), 'Account') + # a user can override the notification preference for a course or a + # root_account, the course_id needs to be provided in the notification from + # broadcast_policy in message_data, the lowest level override is the one + # that should be respected. + policy = override_policy_for(channel, 'Course') + policy ||= override_policy_for(channel, 'Account') if !policy && should_use_default_policy?(user, channel) policy ||= channel.notification_policies.new(notification_id: @notification.id, frequency: @notification.default_frequency(user)) @@ -238,15 +242,15 @@ class NotificationMessageCreator user.email_channel == channel end - def override_policy_for(channel, context_id, context_type) + def override_policy_for(channel, context_type) # NotificationPolicyOverrides are already loaded and this find block is on # an array and can only have one for a given context and channel. - if context_id - channel.notification_policy_overrides.find do |np| - np.notification_id == @notification.id && - np.context_id == context_id && - np.context_type == context_type - end + ops = channel.notification_policy_overrides.select { |np| np.notification_id == @notification.id && np.context_type == context_type } + case context_type + when 'Course' + ops.find { |np| courses.map(&:id).include?(np.context_id) } if courses&.any? + when 'Account' + ops.find { |np| np.context_id == account.id } if account end end diff --git a/spec/models/appointment_group_spec.rb b/spec/models/appointment_group_spec.rb index 59be304fb95..3df9fc365a7 100644 --- a/spec/models/appointment_group_spec.rb +++ b/spec/models/appointment_group_spec.rb @@ -84,7 +84,36 @@ describe AppointmentGroup do group.contexts = [@course] group.save! - expect(group.broadcast_data).to eql({root_account_id: @course.root_account_id, course_id: @course.id}) + expect(group.broadcast_data).to eql({root_account_id: @course.root_account_id, course_ids: [@course.id]}) + end + + it 'should include all course_ids' do + course_with_student(:active_all => true) + course2 = @course.root_account.courses.create!(name: 'course2', workflow_state: 'available') + group = AppointmentGroup.new(:title => "test") + group.contexts = [@course, course2] + group.save! + + expect(group.broadcast_data).to eql({root_account_id: @course.root_account_id, course_ids: [@course.id, course2.id]}) + end + + it 'should include course_id if the context is a section' do + course_with_student(:active_all => true) + group = AppointmentGroup.new(:title => "test") + group.contexts = [@course.default_section] + group.save! + + expect(group.broadcast_data).to eql({root_account_id: @course.root_account_id, course_ids: [@course.id]}) + end + + it 'should include mixed contexts course_ids' do + course_with_student(:active_all => true) + course2 = @course.root_account.courses.create!(name: 'course2', workflow_state: 'available') + group = AppointmentGroup.new(:title => "test") + group.contexts = [@course.default_section, course2] + group.save! + + expect(group.broadcast_data).to eql({root_account_id: @course.root_account_id, course_ids: [@course.id, course2.id]}) end end diff --git a/spec/models/calendar_event_spec.rb b/spec/models/calendar_event_spec.rb index e5d4de6dc19..88944b9a4e3 100644 --- a/spec/models/calendar_event_spec.rb +++ b/spec/models/calendar_event_spec.rb @@ -552,8 +552,22 @@ describe CalendarEvent do Message.where(notification_id: BroadcastPolicy.notification_finder.by_name(notification_name), user_id: @expected_users).pluck(:user_id).sort end - it "should notify all participants except the person reserving", priority: "1", test_id: 193149 do + it 'should include course_ids from appointment_groups' do reservation = @appointment2.reserve_for(@group, @student1) + expect(reservation.course_broadcast_data).to eql({root_account_id: @course.root_account_id, course_ids: [@course.id]}) + end + + it 'should include multiple course_ids' do + course2 = @course.root_account.courses.create!(name: 'course2', workflow_state: 'available') + course2.enroll_teacher(@teacher).accept! + ag = AppointmentGroup.create!(title: "test", contexts: [@course, course2]) + appointment = ag.appointments.create!(start_at: '2012-01-01 12:00:00', end_at: '2012-01-01 13:00:00') + reservation = appointment.reserve_for(@student1, @student1) + expect(reservation.course_broadcast_data).to eql({root_account_id: @course.root_account_id, course_ids: [@course.id, course2.id]}) + end + + it "should notify all participants except the person reserving", priority: "1", test_id: 193149 do + @appointment2.reserve_for(@group, @student1) expect(message_recipients_for('Appointment Reserved For User')).to eq @expected_users - [@student1.id, @teacher.id] end diff --git a/spec/models/notification_policy_override_spec.rb b/spec/models/notification_policy_override_spec.rb index 704a284928f..2d40a34de4c 100644 --- a/spec/models/notification_policy_override_spec.rb +++ b/spec/models/notification_policy_override_spec.rb @@ -54,7 +54,7 @@ describe 'Notification Policy Override' do it 'queries a users policy overrides correctly when another shard is active' do NotificationPolicyOverride.create_or_update_for(@channel, 'Due Date', 'immediately', @course) @shard2.activate do - npos = NotificationPolicyOverride.find_all_for(@teacher, @course, channel: @channel) + npos = NotificationPolicyOverride.find_all_for(@teacher, [@course], channel: @channel) expect(npos.count).to eq 1 expect(npos.first.frequency).to eq 'immediately' expect(npos.first.communication_channel_id).to eq @channel.id