preload parent and root account associations when loading account chain

since we have the objects anyway

Change-Id: I41fd0b8e6a4ccbe5178bcf3f64a3768eaa47341f
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/269228
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

app/models/account.rb

@@ -630,6 +630,14 @@ class Account < ActiveRecord::Base
 
   def root_account
     return self if root_account?
+
+    super
+  end
+
+  def root_account=(value)
+    return if value == self && root_account?
+    raise ArgumentError, "cannot change the root account of a root account" if root_account? && persisted?
+
     super
   end
 
@@ -999,10 +1007,17 @@ class Account < ActiveRecord::Base
   end
 
   def account_chain(include_site_admin: false)
-    @account_chain ||= Account.account_chain(self).freeze
+    @account_chain ||= Account.account_chain(self).tap do |chain|
+      # preload the root account and parent accounts that we also found here
+      ra = chain.find(&:root_account?)
+      chain.each { |a| a.root_account = ra if a.root_account_id == ra.id }
+      chain.each_with_index { |a, idx| a.parent_account = chain[idx + 1] if a.parent_account_id == chain[idx + 1]&.id }
+    end.freeze
+
     if include_site_admin
       return @account_chain_with_site_admin ||= Account.add_site_admin_to_chain!(@account_chain.dup).freeze
     end
+
     @account_chain
   end
 

lib/brand_config_helpers.rb

@@ -42,7 +42,6 @@ module BrandConfigHelpers
     chain = self.account_chain(include_site_admin: true).dup
     chain.shift unless include_self
     chain.select! { |a| a.shard == self.shard }
-    ActiveRecord::Associations::Preloader.new.preload(chain, :root_account)
     chain
   end
 end

spec/apis/v1/assignments_api_spec.rb

@@ -2077,12 +2077,10 @@ describe AssignmentsApiController, type: :request do
     context 'LTI 2.x' do
       include_context 'lti2_spec_helper'
 
-      let(:root_account) { Account.create!(name: 'root account') }
+      let(:root_account) { account.root_account }
       let(:course) { Course.create!(name: 'test course', account: account) }
       let(:teacher) { teacher_in_course(course: course) }
 
-      before { account.update(root_account: root_account) }
-
       it "checks for tool installation in entire account chain" do
         user_session teacher
         allow_any_instance_of(AssignmentConfigurationToolLookup).to receive(:create_subscription).and_return true

spec/apis/v1/sis_api_spec.rb

@@ -35,9 +35,7 @@ describe SisApiController, type: :request do
   describe '#sis_assignments' do
     context 'for an account' do
       before :once do
-        account_model
+        account_model(root_account: Account.default)
-        @account.root_account = Account.default
-        @account.save
         account_admin_user(account: @account, active_all: true)
       end
 

spec/controllers/lti/message_controller_spec.rb

@@ -333,9 +333,7 @@ module Lti
       end
 
       context 'search account chain' do
-        let(:root_account) {Account.create!}
+        let(:root_account) { account.root_account }
-
-        before {account.update(root_account: root_account)}
 
         it "succeeds if the tool is installed in the current account's root account" do
           tool_proxy.update(context: root_account)

spec/models/access_token_spec.rb

@@ -323,8 +323,7 @@ describe AccessToken do
       let(:root_account_key) { DeveloperKey.create!(account: root_account) }
       let(:site_admin_key) { DeveloperKey.create! }
       let(:sub_account) do
-        account = account_model
+        account = account_model(root_account: root_account)
-        account.update!(root_account: root_account)
         account
       end
 

spec/models/account_spec.rb

@@ -1539,9 +1539,23 @@ describe Account do
       account3 = account2.sub_accounts.create!
       account4 = account3.sub_accounts.create!
 
-      expect(account4.account_chain).to eq [account4, account3, account2, account1]
+      chain = account4.account_chain
-    end
+      expect(chain).to eq [account4, account3, account2, account1]
+      # ensure pre-loading worked correctly
+      expect(chain.map { |a| a.association(:parent_account).loaded? }).to eq [true, true, true, true]
+      expect(chain.map(&:parent_account)).to eq [account3, account2, account1, nil]
+      expect(chain.map { |a| a.association(:root_account).loaded? }).to eq [true, true, true, false]
+      expect(chain.map(&:root_account)).to eq [account1, account1, account1, account1]
 
+      chain = account4.account_chain(include_site_admin: true)
+      sa = Account.site_admin
+      expect(chain).to eq [account4, account3, account2, account1, sa]
+      # ensure pre-loading worked correctly
+      expect(chain.map { |a| a.association(:parent_account).loaded? }).to eq [true, true, true, true, true]
+      expect(chain.map(&:parent_account)).to eq [account3, account2, account1, nil, nil]
+      expect(chain.map { |a| a.association(:root_account).loaded? }).to eq [true, true, true, false, false]
+      expect(chain.map(&:root_account)).to eq [account1, account1, account1, account1, sa]
+    end
   end
 
   describe "#can_see_admin_tools_tab?" do
@@ -1766,17 +1780,19 @@ describe Account do
       enable_cache do
         account = account_model
 
-        account.default_storage_quota = 10.megabytes
+        sub1 = account.sub_accounts.create!
-        account.save!
+        sub2 = account.sub_accounts.create!
+        sub2.update(default_storage_quota: 10.megabytes)
 
-        to_be_subaccount = Account.create!
+        to_be_subaccount = sub1.sub_accounts.create!
         expect(to_be_subaccount.default_storage_quota).to eq Account.default_storage_quota
 
         # should clear caches
-        to_be_subaccount.parent_account = account
+        Timecop.travel(1.second.from_now) do
-        to_be_subaccount.root_account = account
+          to_be_subaccount.update(parent_account: sub2)
-        to_be_subaccount.save!
+          to_be_subaccount = Account.find(to_be_subaccount.id)
-        expect(to_be_subaccount.default_storage_quota).to eq 10.megabytes
+          expect(to_be_subaccount.default_storage_quota).to eq 10.megabytes
+        end
       end
     end
   end

spec/models/developer_key_account_binding_spec.rb

@@ -219,12 +219,7 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
         end
 
         context 'when account is not root account' do
-          let(:account) {
+          let(:account) { account_model(root_account: Account.create!) }
-            a = account_model
-            a.root_account = Account.create!
-            a.save!
-            a
-          }
 
           it 'sets root account equal to account\'s root account' do
             dev_key_binding.account = account

spec/models/developer_key_spec.rb

@@ -664,12 +664,7 @@ describe DeveloperKey do
     describe 'after_save' do
       describe 'set_root_account' do
         context 'when account is not root account' do
-          let(:account) {
+          let(:account) {account_model(root_account: Account.create!) }
-            a = account_model
-            a.root_account = Account.create!
-            a.save!
-            a
-          }
 
           it 'sets root account equal to account\'s root account' do
             expect(developer_key_not_saved.root_account).to be_nil

spec/models/lti/lti_account_creator_spec.rb

@@ -38,9 +38,7 @@ describe Lti::LtiAccountCreator do
   end
   let(:canvas_user) { user_factory(name: 'Shorty McLongishname') }
   let(:canvas_account) do
-    Account.create!.tap do |account|
+    root_account.sub_accounts.create!(name: 'account_name').tap do |account|
-      account.name = 'account_name'
-      account.root_account = root_account
       allow(account).to receive(:id).and_return(123)
       account.sis_source_id = 'sis_id'
     end

spec/models/lti/lti_context_creator_spec.rb

@@ -113,8 +113,7 @@ describe Lti::LtiContextCreator do
 
     describe "for a canvas account" do
       let(:canvas_account) do
-        Account.create!.tap do |account|
+        root_account.sub_accounts.create!(name: 'account name').tap do |account|
-          account.name = 'account name'
           account.root_account = root_account
           allow(account).to receive(:id).and_return(123)
           account.sis_source_id = 'sis_id'

spec/models/lti/lti_integration_spec.rb

@@ -58,12 +58,7 @@ describe "LTI integration tests" do
     end
   }
 
-  let_once(:root_account) {
+  let_once(:root_account) { Account.create!(name: 'root_account') }
-    Account.new.tap do |account|
-      account.name = 'root_account'
-      account.save!
-    end
-  }
 
   let(:controller) do
     request_mock = double('request')
@@ -80,9 +75,7 @@ describe "LTI integration tests" do
   it "generates the correct post payload" do
     canvas_user.email = 'user@email.com'
 
-    sub_account = Account.create!
+    sub_account = root_account.sub_accounts.create!
-    sub_account.root_account = root_account
-    sub_account.save!
     pseudonym = pseudonym(canvas_user, account: root_account, username: 'login_id')
 
     teacher_in_course(user: canvas_user, course: canvas_course, active_enrollment: true)

spec/models/lti/message_handler_spec.rb

@@ -250,12 +250,11 @@ module Lti
         end
 
         it 'finds message handlers when tool is installed in account root account' do
-          root_account = Account.create!
+          sub = account.sub_accounts.create!
-          account.update(root_account: root_account)
           mh = MessageHandler.by_resource_codes(vendor_code: jwt_body[:vendor_code],
                                                 product_code: jwt_body[:product_code],
                                                 resource_type_code: jwt_body[:resource_type_code],
-                                                context: account)
+                                                context: sub)
           expect(mh).to eq message_handler
         end
       end

spec/models/user_spec.rb

@@ -125,16 +125,13 @@ describe User do
     expect(@user.associated_accounts.length).to eql(1)
     expect(@user.associated_accounts.first).to eql(account1)
 
-    account2 = account_model
+    account2 = account_model(root_account: account1)
-    account1.parent_account = account2
+    @course.update(account: account2)
-    account1.root_account = account2
-    account1.save!
-    @course.reload
     @user.reload
 
     expect(@user.associated_accounts.length).to eql(2)
-    expect(@user.associated_accounts[0]).to eql(account1)
+    expect(@user.associated_accounts[0]).to eql(account2)
-    expect(@user.associated_accounts[1]).to eql(account2)
+    expect(@user.associated_accounts[1]).to eql(account1)
   end
 
   it "should update account associations when a user is associated to an account just by pseudonym" do
@@ -158,15 +155,6 @@ describe User do
     user.reload
     expect(user.associated_accounts.length).to eql(1)
     expect(user.associated_accounts.first).to eql(account1)
-
-    account1.parent_account = account2
-    account1.root_account = account2
-    account1.save!
-
-    user.reload
-    expect(user.associated_accounts.length).to eql(2)
-    expect(user.associated_accounts[0]).to eql(account1)
-    expect(user.associated_accounts[1]).to eql(account2)
   end
 
   it "should update account associations when a user is associated to an account just by account_users" do