fix Account#find_child for qualified names

and make it more straightforward and not dependent on the total
number of accounts in the tree

Change-Id: If08ccddea067456d17faeb81d71ede12b7630a51
Reviewed-on: https://gerrit.instructure.com/71848
Tested-by: Jenkins
Reviewed-by: Simon Williams <simon@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>

app/controllers/courses_controller.rb

@@ -596,7 +596,7 @@ class CoursesController < ApplicationController
       end
 
       if (sub_account_id = params[:course].delete(:account_id)) && sub_account_id.to_i != @account.id
-        @sub_account = @account.find_child(sub_account_id) || raise(ActiveRecord::RecordNotFound)
+        @sub_account = @account.find_child(sub_account_id)
       end
 
       term_id = params[:course].delete(:term_id).presence || params[:course].delete(:enrollment_term_id).presence

app/models/account.rb

@@ -1463,18 +1463,13 @@ class Account < ActiveRecord::Base
 
   def self.serialization_excludes; [:uuid]; end
 
-  # This could be much faster if we implement a SQL tree for the account tree
-  # structure.
   def find_child(child_id)
-    child_id = child_id.to_i
-    child_ids = self.class.connection.select_values("SELECT id FROM accounts WHERE parent_account_id = #{self.id}").map(&:to_i)
-    until child_ids.empty?
-      if child_ids.include?(child_id)
-        return self.class.find(child_id)
-      end
-      child_ids = self.class.connection.select_values("SELECT id FROM accounts WHERE parent_account_id IN (#{child_ids.join(",")})").map(&:to_i)
-    end
-    return false
+    return all_accounts.find(child_id) if root_account?
+
+    child = Account.find(child_id)
+    raise ActiveRecord::RecordNotFound unless child.account_chain.include?(self)
+
+    child
   end
 
   def manually_created_courses_account

spec/models/account_spec.rb

@@ -981,6 +981,27 @@ describe Account do
     end
   end
 
+  describe '#find_child' do
+    it 'works for root accounts' do
+      sub = Account.default.sub_accounts.create!
+      expect(Account.default.find_child(sub.id)).to eq sub
+    end
+
+    it 'works for children accounts' do
+      sub = Account.default.sub_accounts.create!
+      sub_sub = sub.sub_accounts.create!
+      sub_sub_sub = sub_sub.sub_accounts.create!
+      expect(sub.find_child(sub_sub_sub.id)).to eq sub_sub_sub
+    end
+
+    it 'raises for out-of-tree accounts' do
+      sub = Account.default.sub_accounts.create!
+      sub_sub = sub.sub_accounts.create!
+      sibling = sub.sub_accounts.create!
+      expect { sub_sub.find_child(sibling.id) }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
   context "manually created courses account" do
     it "should still work with existing manually created courses accounts" do
       acct = Account.default