clean up developer key account bindings

* use api_find in developer key account bindings controller
 * use workflow module instead of writing a bunch of stuff manually for the same
   concept
 * set some columns NOT NULL
 * collapse a hand-written query that actually takes two queries into
   using Rails relations and only one
 * take out a condition in that query entirely, instead of making a non-sensical
   comparison

Change-Id: Ifdf88a6c398c1d6353ec273712383bbd3c9c7907
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/269133
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

app/controllers/developer_key_account_bindings_controller.rb

@@ -93,7 +93,7 @@ class DeveloperKeyAccountBindingsController < ApplicationController
     paginated_bindings = Api.paginate(
       account_chain_bindings,
       self,
-      pagination_url,
+      url_for(action: :index, account_id: account.id),
       pagination_args
     )
     render json: index_serializer(paginated_bindings)
@@ -107,20 +107,12 @@ class DeveloperKeyAccountBindingsController < ApplicationController
     end
   end
 
-  def pagination_url
-    url_for(action: :index, account_id: account.id)
-  end
-
   def pagination_args
     params[:limit] ? { per_page: params[:limit] } : {}
   end
 
   def account
-    @_account ||= begin
+    @_account ||= api_find(Account, params[:account_id])
-      a = Account.site_admin if params[:account_id] == 'site_admin'
-      a = @domain_root_account if params[:account_id] == 'self'
-      a || Account.find(params[:account_id])
-    end
   end
 
   def existing_binding

app/models/developer_key.rb

@@ -290,7 +290,7 @@ class DeveloperKey < ActiveRecord::Base
   end
 
   def binding_on_in_account?(target_account)
-    account_binding_for(target_account)&.workflow_state == DeveloperKeyAccountBinding::ON_STATE
+    account_binding_for(target_account)&.on?
   end
 
   def disable_external_tools!(binding_account)

app/models/developer_key_account_binding.rb

@@ -19,26 +19,25 @@
 #
 
 class DeveloperKeyAccountBinding < ApplicationRecord
-  ALLOW_STATE = 'allow'.freeze
+  include Workflow
-  ON_STATE = 'on'.freeze
+  workflow do
-  OFF_STATE = 'off'.freeze
+    state :off
-  DEFAULT_STATE = OFF_STATE
+    state :on
+    state :allow
+  end
 
   belongs_to :account
   belongs_to :developer_key
   belongs_to :root_account, class_name: 'Account'
 
   validates :account, :developer_key, presence: true
-  validates :workflow_state, inclusion: { in: [OFF_STATE, ALLOW_STATE, ON_STATE] }
 
-  before_validation :infer_workflow_state
   after_update :clear_cache_if_site_admin
   after_update :update_tools!
   before_save :set_root_account
 
   scope :active_in_account, -> (account) do
-    where(account_id: account.account_chain_ids).
+    where(account_id: account.account_chain_ids, workflow_state: 'on')
-      where(workflow_state: ON_STATE)
   end
 
   # run this once on the local shard and again on site_admin to get all avaiable dev_keys with
@@ -68,28 +67,24 @@ class DeveloperKeyAccountBinding < ApplicationRecord
   # account 2.
   def self.find_in_account_priority(account_ids, developer_key_id, explicitly_set = true)
     raise 'Account ids must be integers' if account_ids.any? { |id| !id.is_a?(Integer) }
+
     account_ids_string = "{#{account_ids.join(',')}}"
-    binding_id = DeveloperKeyAccountBinding.connection.select_values(<<~SQL)
+    relation = DeveloperKeyAccountBinding
-      SELECT b.*
+      .joins("JOIN unnest('#{account_ids_string}'::int8[]) WITH ordinality AS i (id, ord) ON i.id=account_id")
-      FROM
+      .where(developer_key_id: developer_key_id)
-          unnest('#{account_ids_string}'::int8[]) WITH ordinality AS i (id, ord)
+      .order(:ord)
-          JOIN #{DeveloperKeyAccountBinding.quoted_table_name} b ON i.id = b.account_id
+    relation = relation.where.not(workflow_state: 'allow') if explicitly_set
-      WHERE
+    relation.take
-          b."developer_key_id" = #{developer_key_id}
-      AND
-          b."workflow_state" <> '#{explicitly_set.present? ? ALLOW_STATE : "NULL"}'
-      ORDER BY i.ord ASC LIMIT 1
-    SQL
-    self.find_by(id: binding_id)
   end
 
   def self.find_site_admin_cached(developer_key)
     # Site admin bindings don't exists for non-site admin developer keys
     return nil if developer_key.account_id.present?
+
     Shard.default.activate do
       MultiCache.fetch(site_admin_cache_key(developer_key)) do
         GuardRail.activate(:secondary) do
-          binding = self.where.not(workflow_state: ALLOW_STATE).find_by(
+          binding = self.where.not(workflow_state: 'allow').find_by(
             account: Account.site_admin,
             developer_key: developer_key
           )
@@ -109,17 +104,7 @@ class DeveloperKeyAccountBinding < ApplicationRecord
     "accounts/site_admin/developer_key_account_bindings/#{developer_key.global_id}"
   end
 
-  def on?
+  alias allowed? allow?
-    self.workflow_state == ON_STATE
-  end
-
-  def off?
-    self.workflow_state == OFF_STATE
-  end
-
-  def allowed?
-    self.workflow_state == ALLOW_STATE
-  end
 
   private
 
@@ -152,8 +137,4 @@ class DeveloperKeyAccountBinding < ApplicationRecord
   def clear_cache_if_site_admin
     self.class.clear_site_admin_cache(developer_key) if account.site_admin?
   end
-
-  def infer_workflow_state
-    self.workflow_state ||= DEFAULT_STATE
-  end
 end

db/migrate/20210714203723_set_default_on_developer_key_account_bindings_workflow_state.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+#
+# Copyright (C) 2021 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+class SetDefaultOnDeveloperKeyAccountBindingsWorkflowState < ActiveRecord::Migration[6.0]
+  tag :predeploy
+
+  def up
+    change_column_default :developer_key_account_bindings, :workflow_state, 'off'
+    change_column_null :developer_key_account_bindings, :created_at, false
+    change_column_null :developer_key_account_bindings, :updated_at, false
+  end
+
+  def down
+    change_column_default :developer_key_account_bindings, :workflow_state, nil
+    change_column_null :developer_key_account_bindings, :created_at, true
+    change_column_null :developer_key_account_bindings, :updated_at, true
+  end
+end

lib/data_fixup/backfill_dev_key_account_bindings.rb

@@ -29,7 +29,7 @@ module DataFixup
       return if developer_key.owner_account.developer_key_account_bindings.where(developer_key: developer_key).exists?
 
       developer_key.owner_account.developer_key_account_bindings.create!(
-        workflow_state: DeveloperKeyAccountBinding::ON_STATE,
+        workflow_state: 'on',
         developer_key: developer_key
       )
     end

lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys.rb

@@ -29,7 +29,7 @@ module DataFixup
       return if developer_key.owner_account.developer_key_account_bindings.where(developer_key: developer_key).exists?
 
       developer_key.owner_account.developer_key_account_bindings.create!(
-        workflow_state: DeveloperKeyAccountBinding::OFF_STATE,
+        workflow_state: 'off',
         developer_key: developer_key
       )
     end

lib/data_fixup/set_existing_binding_state.rb

@@ -23,10 +23,10 @@ module DataFixup::SetExistingBindingState
       # This shouldn't ever happen, but strange things occur
       next if binding.developer_key.blank?
 
-      new_workflow_state = DeveloperKeyAccountBinding::ON_STATE
+      new_workflow_state = 'on'
 
       if binding.developer_key.workflow_state == 'deleted' || binding.developer_key.workflow_state == 'inactive'
-        new_workflow_state = DeveloperKeyAccountBinding::OFF_STATE
+        new_workflow_state = 'off'
       end
 
       binding.update!(workflow_state: new_workflow_state) unless binding.workflow_state == new_workflow_state

spec/apis/lti/lti_app_api_spec.rb

@@ -228,7 +228,7 @@ module Lti
       context 'lti 1.1 and 2.0, and 1.3 tools' do
         let(:dev_key) { DeveloperKey.create! account: account }
         let(:tool_config) { dev_key.create_tool_configuration! settings: settings }
-        let(:enable_binding) { dev_key.developer_key_account_bindings.first.update! workflow_state: DeveloperKeyAccountBinding::ON_STATE }
+        let(:enable_binding) { dev_key.developer_key_account_bindings.first.update! workflow_state: 'on' }
         let(:advantage_tool) do
           t = new_valid_external_tool(account)
           t.use_1_3 = true

spec/controllers/lti/ims/concerns/advantage_services_shared_context.rb

@@ -29,7 +29,7 @@ shared_context 'advantage services context' do
   end
   let_once(:developer_key) do
     dk = DeveloperKey.create!(account: root_account)
-    dk.developer_key_account_bindings.first.update! workflow_state: DeveloperKeyAccountBinding::ON_STATE
+    dk.developer_key_account_bindings.first.update! workflow_state: 'on'
     dk
   end
   let(:access_token_scopes) do

spec/controllers/lti/ims/concerns/advantage_services_shared_examples.rb

@@ -94,7 +94,7 @@ shared_examples_for "advantage services" do
     context 'with unbound developer key' do
       let(:before_send_request) do
         -> {
-          developer_key.developer_key_account_bindings.first.update! workflow_state: DeveloperKeyAccountBinding::OFF_STATE
+          developer_key.developer_key_account_bindings.first.update! workflow_state: 'off'
         }
       end
 

spec/lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys_spec.rb

@@ -39,9 +39,7 @@ describe DataFixup::BackfillDevKeyAccountBindingsForDeletedKeys do
 
       # Verify
       expect(key.developer_key_account_bindings.count).to eq(1)
-      expect(
+      expect(key.developer_key_account_bindings.first).to be_off
-        key.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::OFF_STATE)
     end
 
     it "backfills when no binding is present for inactive key" do
@@ -58,9 +56,7 @@ describe DataFixup::BackfillDevKeyAccountBindingsForDeletedKeys do
 
       # Verify
       expect(key.developer_key_account_bindings.count).to eq(1)
-      expect(
+      expect(key.developer_key_account_bindings.first).to be_off
-        key.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::OFF_STATE)
     end
 
     it "does not backfill when no binding is present for an active key" do

spec/lib/data_fixup/backfill_dev_key_account_bindings_spec.rb

@@ -35,9 +35,7 @@ describe DataFixup::BackfillDevKeyAccountBindings do
       # Verify
       key.reload
       expect(key.developer_key_account_bindings.count).to eq(1)
-      expect(
+      expect(key.developer_key_account_bindings.first).to be_on
-        key.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::ON_STATE)
     end
 
     it "does not backfill when a binding is present" do

spec/lib/data_fixup/set_existing_binding_state_spec.rb

@@ -32,40 +32,28 @@ describe DataFixup::SetExistingBindingState do
       deleted_key_2  = DeveloperKey.create!.tap(&:destroy)
 
       # Set bindings' workflow state
-      active_key_1.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::ON_STATE)
+      active_key_1.developer_key_account_bindings.first.update(workflow_state: 'on')
-      active_key_2.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::OFF_STATE)
+      active_key_2.developer_key_account_bindings.first.update(workflow_state: 'off')
-      inactive_key_1.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::ON_STATE)
+      inactive_key_1.developer_key_account_bindings.first.update(workflow_state: 'on')
-      inactive_key_2.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::OFF_STATE)
+      inactive_key_2.developer_key_account_bindings.first.update(workflow_state:'off')
-      deleted_key_1.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::ON_STATE)
+      deleted_key_1.developer_key_account_bindings.first.update(workflow_state: 'on')
-      deleted_key_2.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::OFF_STATE)
+      deleted_key_2.developer_key_account_bindings.first.update(workflow_state: 'off')
 
       # Update binding state
       described_class.run
 
       # Verify
-      expect(
+      expect(active_key_1.developer_key_account_bindings.first).to be_on
-        active_key_1.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::ON_STATE)
 
-      expect(
+      expect(active_key_2.developer_key_account_bindings.first).to be_on
-        active_key_2.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::ON_STATE)
 
-      expect(
+      expect(inactive_key_1.developer_key_account_bindings.first).to be_off
-        inactive_key_1.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::OFF_STATE)
 
-      expect(
+      expect(inactive_key_2.developer_key_account_bindings.first).to be_off
-        inactive_key_2.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::OFF_STATE)
 
-      expect(
+      expect(deleted_key_1.developer_key_account_bindings.first).to be_off
-        deleted_key_1.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::OFF_STATE)
 
-      expect(
+      expect(deleted_key_2.developer_key_account_bindings.first).to be_off
-        deleted_key_2.developer_key_account_bindings.first.workflow_state
-      ).to eq(DeveloperKeyAccountBinding::OFF_STATE)
     end
   end
 end

spec/models/developer_key_account_binding_spec.rb

@@ -44,7 +44,7 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
     end
 
     let(:params) { { visible: true } }
-    let(:workflow_state) { described_class::ON_STATE }
+    let(:workflow_state) { 'on' }
 
     before do
       dev_keys = []
@@ -60,17 +60,17 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
       it { is_expected.to be_empty }
     end
 
-    context 'with visible dev keys but no ON_STATE keys' do
+    context 'with visible dev keys but no "on" keys' do
-      let(:workflow_state) { described_class::ALLOW_STATE }
+      let(:workflow_state) { 'allow' }
 
       it { is_expected.to be_empty }
     end
 
-    context 'with visible dev keys in ON_STATE but no tool_configurations' do
+    context 'with visible dev keys in "on" but no tool_configurations' do
       it { is_expected.to be_empty }
     end
 
-    context 'with visible dev keys in ON_STATE and tool_configurations' do
+    context 'with visible dev keys in "on" and tool_configurations' do
       let(:first_key) { DeveloperKey.first }
       before do
         first_key.create_tool_configuration! settings: settings
@@ -120,9 +120,8 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
       it 'does not allow invalid workflow states' do
         dev_key_binding.workflow_state = 'invalid_state'
         dev_key_binding.validate
-        expect(dev_key_binding.errors.keys).to match_array(
+        # it automatically flips it to the default state
-          [:workflow_state]
+        expect(dev_key_binding.workflow_state).to eq 'off'
-        )
       end
 
       it 'defaults to "off"' do
@@ -157,7 +156,7 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
         before { site_admin_binding.update!(workflow_state: 'on') }
 
         context 'when the new workflow state is "off"' do
-          let(:workflow_state) { DeveloperKeyAccountBinding::OFF_STATE }
+          let(:workflow_state) { 'off' }
 
           it 'disables associated external tools' do
             expect(site_admin_key).to receive(:disable_external_tools!)
@@ -166,7 +165,7 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
         end
 
         context 'when the new workflow state is "on"' do
-          let(:workflow_state) { DeveloperKeyAccountBinding::ON_STATE }
+          let(:workflow_state) { 'on' }
 
           it 'does not disable associated external tools' do
             expect(site_admin_key).not_to receive(:disable_external_tools!)
@@ -175,7 +174,7 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
         end
 
         context 'when the new workflow state is "allow"' do
-          let(:workflow_state) { DeveloperKeyAccountBinding::ALLOW_STATE }
+          let(:workflow_state) { 'allow' }
 
           it 'restores associated external tools' do
             expect(site_admin_key).to receive(:restore_external_tools!)
@@ -188,7 +187,7 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
         before { site_admin_binding.update!(workflow_state: 'off') }
 
         context 'when the new workflow state is "on"' do
-          let(:workflow_state) { DeveloperKeyAccountBinding::ON_STATE }
+          let(:workflow_state) { 'on' }
 
           it 'enables external tools' do
             expect(site_admin_key).not_to receive(:disable_external_tools!)
@@ -197,7 +196,7 @@ RSpec.describe DeveloperKeyAccountBinding, type: :model do
         end
 
         context 'when the new workflow state is "allow"' do
-          let(:workflow_state) { DeveloperKeyAccountBinding::ALLOW_STATE }
+          let(:workflow_state) { 'allow' }
 
           it 'restores associated external tools' do
             expect(site_admin_key).to receive(:restore_external_tools!)

spec/models/developer_key_spec.rb

@@ -743,7 +743,7 @@ describe DeveloperKey do
       context 'when binding state is "allow"' do
         before do
           site_admin_key.developer_key_account_bindings.create!(
-            account: root_account, workflow_state: DeveloperKeyAccountBinding::ALLOW_STATE
+            account: root_account, workflow_state: 'allow'
           )
         end
 
@@ -753,7 +753,7 @@ describe DeveloperKey do
         end
 
         it 'finds the root account binding when requesting root account' do
-          site_admin_key.developer_key_account_bindings.first.update!(workflow_state: DeveloperKeyAccountBinding::ALLOW_STATE)
+          site_admin_key.developer_key_account_bindings.first.update!(workflow_state: 'allow')
           binding = site_admin_key.account_binding_for(root_account)
           expect(binding.account).to eq root_account
         end