populate communication_channels root_account_ids

communication_channels only live on users shard and they only work on
users shard. There have been bugs that were incorrectly placing them on
a different shard, but they never worked.

users invited into a course get a user object created as pre-registered,
but they do not get root_account_ids set until after they have clicking
the email. the email can also cause a user merge that would put the
communication_channel on the merged user but again the root_account_ids
would not be present until after that process so before_create was not
enough to populate communication_channels.

this commit also makes it so the channels are just populated on a user
merge event, since they could change, this is also appropriate.

after a datafix up is run ids should be populated but log a stat when we
send a message to ensure it.

fixes VICE-895
flag=none

Change-Id: I4721a563988f8fa399fca9d23b73e17c2a6371e7
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/249227
Reviewed-by: Matthew Lemon <mlemon@instructure.com>
QA-Review: Matthew Lemon <mlemon@instructure.com>
Product-Review: Matthew Lemon <mlemon@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>

app/models/communication_channel.rb

@@ -32,7 +32,7 @@ class CommunicationChannel < ActiveRecord::Base
   has_many :delayed_messages, :dependent => :destroy
   has_many :messages
 
-  before_create :set_root_account_ids
+  before_save :set_root_account_ids
   before_save :assert_path_type, :set_confirmation_code
   before_save :consider_building_pseudonym
   validates_presence_of :path, :path_type, :user, :workflow_state
@@ -451,23 +451,13 @@ class CommunicationChannel < ActiveRecord::Base
     end
   end
 
-  def set_root_account_ids(persist_changes: false)
+  def set_root_account_ids(persist_changes: false, log: false)
-    self.root_account_ids = user.root_account_ids&.map do |root_account_id|
+    # communication_channels always are on the same shard as the user object and
-      local_account_id, account_shard = Shard.local_id_for(root_account_id)
+    # can be used for any root_account, so just set root_account_ids from user.
-
+    self.root_account_ids = user.root_account_ids
-      # If the root_account_id from the user was a local ID, assume shard of user
+    if root_account_ids_changed? && log
-      account_shard ||= user.shard
+      InstStatsd::Statsd.increment("communication_channel.root_account_ids_set", short_stat: 'communication_channel.root_account_ids_set')
-
-      # If the root account's shard is the same as the communication channel's shard,
-      # we want to use a local ID to reference it. Otherwise use a global ID that
-      # points to the root account on a foreign shard
-      if account_shard == self.shard
-        local_account_id
-      else
-        Shard.global_id_for(local_account_id, account_shard)
-      end
     end
-
     save! if persist_changes && root_account_ids_changed?
   end
 

app/models/split_users.rb

@@ -240,9 +240,11 @@ class SplitUsers
     end
 
     # move moved communication channels back
-    max_position = restored_user.communication_channels.last.try(:position) || 0
+    max_position = restored_user.communication_channels.last&.position&.+(1) || 0
     scope = source_user.communication_channels.where(id: cc_records.where(previous_user_id: restored_user).pluck(:context_id))
-    scope.update_all(["user_id=?, position=position+?", restored_user.id, max_position]) unless scope.empty?
+    # passing the array to update_all so we can get postgres to add the position for us.
+    scope.update_all(["user_id=?, position=position+?, root_account_ids='{?}'",
+                      restored_user.id, max_position, restored_user.root_account_ids]) unless scope.empty?
 
     cc_records.where.not(previous_workflow_state: 'non existent').each do |cr|
       CommunicationChannel.where(id: cr.context_id).update_all(workflow_state: cr.previous_workflow_state)

app/models/user.rb

@@ -430,45 +430,32 @@ class User < ActiveRecord::Base
   ensure
     @skip_updating_account_associations = false
   end
+
   def self.skip_updating_account_associations?
     !!@skip_updating_account_associations
   end
 
   # Update the root_account_ids column on the user
-  # and all associated CommunicationChannels
+  # and all the users CommunicationChannels
   def update_root_account_ids
     # See User#associated_shards in MRA for an explanation of
     # shard association levels
     shards = associated_shards(:strong) + associated_shards(:weak)
 
     refreshed_root_account_ids = Set.new
-    communication_channels_to_update = Set.new
 
     Shard.with_each_shard(shards) do
-      UserAccountAssociation.joins(:account).
+      UserAccountAssociation.for_root_accounts.for_user_id(self.id).each do |uaa|
-        where(user: self, accounts: { parent_account_id: nil }).
+        refreshed_root_account_ids << Shard.relative_id_for(uaa.account_id, Shard.current, self.shard)
-        preload(:account).
+      end
-        find_each do |association|
-          refreshed_root_account_ids << association.global_account_id
-        end
-
-      # Users can potentially have communication_channels on
-      # any of their associated shards. Collect those communication
-      # channels here for later root_account_ids updating.
-      communication_channels_to_update.merge(
-        CommunicationChannel.where(user: self)
-      )
     end
 
     # Update the user
-    relative_ids = refreshed_root_account_ids.map do |id|
+    self.root_account_ids = refreshed_root_account_ids.to_a.sort
-      Shard.relative_id_for(id, self.shard, self.shard)
+    if root_account_ids_changed?
-    end
+      save!
-    self.update!(root_account_ids: relative_ids)
+      # Update each communication channel associated with the user
-
+      self.communication_channels.update_all(root_account_ids: self.root_account_ids)
-    # Update each communication channel associated with the user
-    communication_channels_to_update.each do |c|
-      c.set_root_account_ids(persist_changes: true)
     end
   end
 

app/models/user_account_association.rb

@@ -28,23 +28,23 @@ class UserAccountAssociation < ActiveRecord::Base
 
   resolves_root_account through: :account
 
+  scope :for_root_accounts, -> { where('root_account_id = account_id') }
+  scope :for_user_id, lambda { |user_id| where('user_id =?', user_id) }
+
   def for_root_account?
-    # TODO: Once root_account_id backfill is complete on
+    account_id == root_account_id
-    # user_account_associations, we can change this to
-    # just `account_id == root_account_id`
-    account_id == root_account_id || account.root_account?
   end
 
   private
 
   def update_user_root_account_ids
+    return unless for_root_account?
+
     # In some Canvas environments we may not want to populate
-    # root_account_ids due to the hight number of root account associations
+    # root_account_ids due to the high number of root account associations
     # per user. This Setting allows us to control if root_account_ids syncing
     # occurs.
     return unless Setting.get('sync_root_account_ids_on_user_records', 'true') == 'true'
-
-    return unless for_root_account?
     user.update_root_account_ids_later
   end
 

lib/notification_message_creator.rb

@@ -75,6 +75,7 @@ class NotificationMessageCreator
         # otherwise it will create a delayed_message. Any message can create a
         # 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)
           if immediate_policy?(user, channel)
             immediate_messages << build_immediate_message_for(user, channel)

lib/user_merge.rb

@@ -282,7 +282,7 @@ class UserMerge
   end
 
   def handle_communication_channels
-    max_position = target_user.communication_channels.last.try(:position) || 0
+    max_position = target_user.communication_channels.last&.position&.+(1) || 0
     to_retire_ids = []
     known_ccs = target_user.communication_channels.pluck(:id)
     from_user.communication_channels.each do |cc|
@@ -327,7 +327,7 @@ class UserMerge
       scope = scope.where.not(id: to_retire_ids) unless to_retire_ids.empty?
       unless scope.empty?
         merge_data.build_more_data(scope, data: data)
-        scope.update_all(["user_id=?, position=position+?", target_user, max_position])
+        scope.update_all(["user_id=?, position=position+?, root_account_ids='{?}'", target_user, max_position, target_user.root_account_ids])
       end
     end
     merge_data.bulk_insert_merge_data(data) unless data.empty?

spec/lib/data_fixup/populate_root_account_ids_on_communication_channels_spec.rb

@@ -33,55 +33,6 @@ describe DataFixup::PopulateRootAccountIdsOnCommunicationChannels do
         }.to change { cc.reload.root_account_ids }.from(nil).to(ra_ids)
       end
     end
-
-    context 'when on a different shard as the main user' do
-      specs_require_sharding
-
-      it 'copies the root_account_ids from the shadow user, correctly globalizing ids' do
-        user0 = user_model
-        user1 = @shard1.activate { user_model }
-        account0 = account_model
-        account1 = @shard1.activate { account_model }
-        account2 = @shard2.activate { account_model }
-        course0 = course_model(account: account0)
-        course1 = @shard1.activate { course_model(account: account1) }
-        course2 = @shard2.activate { course_model(account: account2) }
-
-        # user on shard 0 belongs to shard0 account and shard2 account
-        course0.enroll_user(user0, 'StudentEnrollment', enrollment_state: 'active')
-        course2.enroll_user(user0, 'StudentEnrollment', enrollment_state: 'active')
-        # user on shard 1 belongs to shard0 account, shard1 account and shard2 account
-        course0.enroll_user(user1, 'StudentEnrollment', enrollment_state: 'active')
-        course1.enroll_user(user1, 'StudentEnrollment', enrollment_state: 'active')
-        course2.enroll_user(user1, 'StudentEnrollment', enrollment_state: 'active')
-
-        @shard2.activate do
-          cc0 = CommunicationChannel.create!(user: user0, path: 'canvas@instructure.com')
-          cc1 = CommunicationChannel.create!(user: user1, path: 'canvas@instructure.com')
-
-          [Shard.default, @shard1, @shard2].each do |shard|
-            shard.activate do
-              # CommunicationChannel backfill won't run until the users table
-              # all has root_account_ids.  The backfill doesn't do that itself
-              # because it won't copy over shadow users immediately in this
-              # spec. So just update the root_account_ids to something bogus.
-              # Fine for our purposes since we don't use it directly in
-              # cross-shard CommunicationChannel fills
-              User.update_all(root_account_ids: [Account.first.id])
-            end
-          end
-
-          expect {
-            DataFixup::PopulateRootAccountIdOnModels.run
-          }.to change { [cc0, cc1].map{|cc| cc.reload.root_account_ids&.sort} }.from(
-            [nil, nil]
-          ).to([
-            [account0.id, account2.id].sort,
-            [account0.id, account1.id, account2.id].sort,
-          ])
-        end
-      end
-    end
   end
 
 end

spec/lib/user_merge_spec.rb

@@ -833,6 +833,16 @@ describe UserMerge do
       expect(@user2.associated_shards).to eq [@shard1, Shard.default]
     end
 
+    it 'should handle root_account_ids on ccs' do
+      user1 = user_with_pseudonym(username: 'user1@example.com', active_all: 1)
+      other_account = Account.create(name: 'anuroot')
+      UserAccountAssociation.create!(account: other_account, user: user1)
+      user1.update_root_account_ids
+      user2 = user_with_pseudonym(username: 'user2@example.com', active_all: 1, account: other_account)
+      UserMerge.from(user2).into(user1)
+      expect(@cc.reload.root_account_ids).to eq user1.root_account_ids
+    end
+
     it "should associate the user with all shards" do
       user1 = user_with_pseudonym(:username => 'user1@example.com', :active_all => 1)
       p1 = @pseudonym

spec/models/communication_channel_spec.rb

@@ -490,51 +490,23 @@ describe CommunicationChannel do
 
         before { user.update_columns(root_account_ids: root_account_ids) }
 
-        context 'when the user belongs to a foreign shard' do
+        let(:user) { User.create! }
-          let(:user) { @shard2.activate { User.create! } }
 
-          before(:each) do
+        context 'is associated with root accounts on a foreign shard' do
-            shadow_user = User.create
+          let(:globalized_ids) { [Shard.global_id_for(1, @shard2), Shard.global_id_for(2, @shard2)] }
-            shadow_user.update_attribute(:id, user.global_id)
+          let(:root_account_ids) { globalized_ids }
-          end
 
-          context 'and is associated with root accounts on the foreign shard' do
+          it 'keeps the root account IDs global' do
-            let(:root_account_ids) { [1, 2] }
+            expect(subject).to match_array globalized_ids
-
-            it 'globalizes the foreign root_account_ids' do
-              expect(subject).to match_array root_account_ids.map { |id| Shard.global_id_for(id, user.shard) }
-            end
-          end
-
-          context 'and is associated with root accounts on the local shard' do
-            let(:localized_ids) { [1, 2] }
-            let(:root_account_ids) { localized_ids.map { |id| Shard.global_id_for(id, Shard.current) } }
-
-            it 'localizes the local root_account_ids' do
-              expect(subject).to match_array localized_ids
-            end
           end
         end
 
-        context 'when the user belongs to the local shard' do
+        context 'is associated with root accounts on the local shard' do
-          let(:user) { User.create! }
+          let(:localized_ids) { [1, 2] }
+          let(:root_account_ids) { localized_ids }
 
-          context 'and is associated with root accounts on a foreign shard' do
+          it 'keeps the root account IDs local' do
-            let(:globalized_ids) { [Shard.global_id_for(1, @shard2), Shard.global_id_for(2, @shard2)] }
+            expect(subject).to match_array localized_ids
-            let(:root_account_ids) { globalized_ids }
-
-            it 'keeps the root account IDs global' do
-              expect(subject).to match_array globalized_ids
-            end
-          end
-
-          context 'and is associated with root accounts on the local shard' do
-            let(:localized_ids) { [1, 2] }
-            let(:root_account_ids) { localized_ids }
-
-            it 'keeps the root account IDs local' do
-              expect(subject).to match_array localized_ids
-            end
           end
         end
       end

spec/models/user_spec.rb

@@ -433,14 +433,14 @@ describe User do
       context 'and communication channels for the user exist' do
         let(:communication_channel) { user.communication_channels.create!(path: 'test@test.com') }
 
-        before { communication_channel.update_attribute(:root_account_ids, []) }
+        before { communication_channel.update(root_account_ids: nil) }
 
         it 'updates root_account_ids on associated communication channels' do
           expect {
             user.update_root_account_ids
           }.to change {
             user.communication_channels.first.root_account_ids
-          }.from([]).to([root_account.id])
+          }.from(nil).to([root_account.id])
         end
       end
     end
@@ -468,38 +468,6 @@ describe User do
           [root_account.id, shard_two_root_account.global_id].sort
         )
       end
-
-      context 'and communication channels exist on each shard' do
-        let(:shard_one_channel) do
-          CommunicationChannel.create!(user: user, path: 'test@test.com')
-        end
-
-        let(:shard_two_channel) do
-          @shard2.activate { CommunicationChannel.create!(user: user, path: 'test@test.com') }
-        end
-
-        before do
-          shard_one_channel.update_attribute(:root_account_ids, [])
-          shard_two_channel.update_attribute(:root_account_ids, [])
-          user.update_root_account_ids
-        end
-
-        it 'populates root_account_ids on the local communication channel' do
-          expect(shard_one_channel.reload.root_account_ids).to match_array [
-            root_account.id,
-            shard_two_root_account.id
-          ]
-        end
-
-        it 'populates root_account_ids on the foreign communication channel' do
-          @shard2.activate {
-            expect(shard_two_channel.reload.root_account_ids).to match_array [
-              root_account.id,
-              shard_two_root_account.id
-            ]
-          }
-        end
-      end
     end
   end