From 9dca9422978a73f1320f77edac0ad6762f414a53 Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Wed, 14 Jul 2021 14:25:00 -0600 Subject: [PATCH] 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 Reviewed-by: Jacob Burroughs QA-Review: Cody Cutrer Product-Review: Cody Cutrer --- ...veloper_key_account_bindings_controller.rb | 12 +---- app/models/developer_key.rb | 2 +- app/models/developer_key_account_binding.rb | 53 ++++++------------- ...per_key_account_bindings_workflow_state.rb | 35 ++++++++++++ .../backfill_dev_key_account_bindings.rb | 2 +- ...v_key_account_bindings_for_deleted_keys.rb | 2 +- lib/data_fixup/set_existing_binding_state.rb | 4 +- spec/apis/lti/lti_app_api_spec.rb | 2 +- .../advantage_services_shared_context.rb | 2 +- .../advantage_services_shared_examples.rb | 2 +- ..._account_bindings_for_deleted_keys_spec.rb | 8 +-- .../backfill_dev_key_account_bindings_spec.rb | 4 +- .../set_existing_binding_state_spec.rb | 36 +++++-------- .../developer_key_account_binding_spec.rb | 25 +++++---- spec/models/developer_key_spec.rb | 4 +- 15 files changed, 91 insertions(+), 102 deletions(-) create mode 100644 db/migrate/20210714203723_set_default_on_developer_key_account_bindings_workflow_state.rb diff --git a/app/controllers/developer_key_account_bindings_controller.rb b/app/controllers/developer_key_account_bindings_controller.rb index 6d1d48c8e9e..cead480a037 100644 --- a/app/controllers/developer_key_account_bindings_controller.rb +++ b/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 - 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 + @_account ||= api_find(Account, params[:account_id]) end def existing_binding diff --git a/app/models/developer_key.rb b/app/models/developer_key.rb index 55220a48f8f..b74146a24ba 100644 --- a/app/models/developer_key.rb +++ b/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) diff --git a/app/models/developer_key_account_binding.rb b/app/models/developer_key_account_binding.rb index a4a29179a3d..0995b67156a 100644 --- a/app/models/developer_key_account_binding.rb +++ b/app/models/developer_key_account_binding.rb @@ -19,26 +19,25 @@ # class DeveloperKeyAccountBinding < ApplicationRecord - ALLOW_STATE = 'allow'.freeze - ON_STATE = 'on'.freeze - OFF_STATE = 'off'.freeze - DEFAULT_STATE = OFF_STATE + include Workflow + workflow do + state :off + 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(workflow_state: ON_STATE) + where(account_id: account.account_chain_ids, workflow_state: 'on') 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) - SELECT b.* - FROM - unnest('#{account_ids_string}'::int8[]) WITH ordinality AS i (id, ord) - JOIN #{DeveloperKeyAccountBinding.quoted_table_name} b ON i.id = b.account_id - WHERE - 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) + relation = DeveloperKeyAccountBinding + .joins("JOIN unnest('#{account_ids_string}'::int8[]) WITH ordinality AS i (id, ord) ON i.id=account_id") + .where(developer_key_id: developer_key_id) + .order(:ord) + relation = relation.where.not(workflow_state: 'allow') if explicitly_set + relation.take 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? - self.workflow_state == ON_STATE - end - - def off? - self.workflow_state == OFF_STATE - end - - def allowed? - self.workflow_state == ALLOW_STATE - end + alias allowed? allow? 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 diff --git a/db/migrate/20210714203723_set_default_on_developer_key_account_bindings_workflow_state.rb b/db/migrate/20210714203723_set_default_on_developer_key_account_bindings_workflow_state.rb new file mode 100644 index 00000000000..bf67611665f --- /dev/null +++ b/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 . +# + +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 diff --git a/lib/data_fixup/backfill_dev_key_account_bindings.rb b/lib/data_fixup/backfill_dev_key_account_bindings.rb index b0aa8801908..a43bf5cb508 100644 --- a/lib/data_fixup/backfill_dev_key_account_bindings.rb +++ b/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 diff --git a/lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys.rb b/lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys.rb index d8ab7293596..c95b93b68c9 100644 --- a/lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys.rb +++ b/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 diff --git a/lib/data_fixup/set_existing_binding_state.rb b/lib/data_fixup/set_existing_binding_state.rb index e0ebe34cb97..b22873c7fee 100644 --- a/lib/data_fixup/set_existing_binding_state.rb +++ b/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 diff --git a/spec/apis/lti/lti_app_api_spec.rb b/spec/apis/lti/lti_app_api_spec.rb index 2c87f1f52a9..c261b31830b 100644 --- a/spec/apis/lti/lti_app_api_spec.rb +++ b/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 diff --git a/spec/controllers/lti/ims/concerns/advantage_services_shared_context.rb b/spec/controllers/lti/ims/concerns/advantage_services_shared_context.rb index 987b98a3fd8..4aa1ce014e4 100644 --- a/spec/controllers/lti/ims/concerns/advantage_services_shared_context.rb +++ b/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 diff --git a/spec/controllers/lti/ims/concerns/advantage_services_shared_examples.rb b/spec/controllers/lti/ims/concerns/advantage_services_shared_examples.rb index 5766e54df82..2ea244fd8e1 100644 --- a/spec/controllers/lti/ims/concerns/advantage_services_shared_examples.rb +++ b/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 diff --git a/spec/lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys_spec.rb b/spec/lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys_spec.rb index fc86bceb8f6..fa09a99ad45 100644 --- a/spec/lib/data_fixup/backfill_dev_key_account_bindings_for_deleted_keys_spec.rb +++ b/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( - key.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::OFF_STATE) + expect(key.developer_key_account_bindings.first).to be_off 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( - key.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::OFF_STATE) + expect(key.developer_key_account_bindings.first).to be_off end it "does not backfill when no binding is present for an active key" do diff --git a/spec/lib/data_fixup/backfill_dev_key_account_bindings_spec.rb b/spec/lib/data_fixup/backfill_dev_key_account_bindings_spec.rb index 0c6bdacb281..35b47541333 100644 --- a/spec/lib/data_fixup/backfill_dev_key_account_bindings_spec.rb +++ b/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( - key.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::ON_STATE) + expect(key.developer_key_account_bindings.first).to be_on end it "does not backfill when a binding is present" do diff --git a/spec/lib/data_fixup/set_existing_binding_state_spec.rb b/spec/lib/data_fixup/set_existing_binding_state_spec.rb index 71dd57bff6d..3854c19dcb1 100644 --- a/spec/lib/data_fixup/set_existing_binding_state_spec.rb +++ b/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_2.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::OFF_STATE) - inactive_key_1.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::ON_STATE) - inactive_key_2.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::OFF_STATE) - deleted_key_1.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::ON_STATE) - deleted_key_2.developer_key_account_bindings.first.update(workflow_state: DeveloperKeyAccountBinding::OFF_STATE) + active_key_1.developer_key_account_bindings.first.update(workflow_state: 'on') + active_key_2.developer_key_account_bindings.first.update(workflow_state: 'off') + inactive_key_1.developer_key_account_bindings.first.update(workflow_state: 'on') + inactive_key_2.developer_key_account_bindings.first.update(workflow_state:'off') + deleted_key_1.developer_key_account_bindings.first.update(workflow_state: 'on') + deleted_key_2.developer_key_account_bindings.first.update(workflow_state: 'off') # Update binding state described_class.run # Verify - expect( - active_key_1.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::ON_STATE) + expect(active_key_1.developer_key_account_bindings.first).to be_on - expect( - active_key_2.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::ON_STATE) + expect(active_key_2.developer_key_account_bindings.first).to be_on - expect( - inactive_key_1.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::OFF_STATE) + expect(inactive_key_1.developer_key_account_bindings.first).to be_off - expect( - inactive_key_2.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::OFF_STATE) + expect(inactive_key_2.developer_key_account_bindings.first).to be_off - expect( - deleted_key_1.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::OFF_STATE) + expect(deleted_key_1.developer_key_account_bindings.first).to be_off - expect( - deleted_key_2.developer_key_account_bindings.first.workflow_state - ).to eq(DeveloperKeyAccountBinding::OFF_STATE) + expect(deleted_key_2.developer_key_account_bindings.first).to be_off end end end diff --git a/spec/models/developer_key_account_binding_spec.rb b/spec/models/developer_key_account_binding_spec.rb index a9be6168a5e..c225fba4084 100644 --- a/spec/models/developer_key_account_binding_spec.rb +++ b/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 - let(:workflow_state) { described_class::ALLOW_STATE } + context 'with visible dev keys but no "on" keys' do + 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( - [:workflow_state] - ) + # it automatically flips it to the default 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!) diff --git a/spec/models/developer_key_spec.rb b/spec/models/developer_key_spec.rb index 08b849fdaae..59c76007979 100644 --- a/spec/models/developer_key_spec.rb +++ b/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