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 <svc.cloudjenkins@instructure.com>
Reviewed-by: Caleb Guanzon <cguanzon@instructure.com>
QA-Review: Caleb Guanzon <cguanzon@instructure.com>
Product-Review: Caleb Guanzon <cguanzon@instructure.com>

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

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
 

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

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
 

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
 

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
 

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
 

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