From 7acc7d1591124908334eb46c41e166c6530e2567 Mon Sep 17 00:00:00 2001 From: George Eaton Date: Tue, 13 Aug 2024 12:02:28 +0100 Subject: [PATCH] Refactor supported permissions method The `sorted_supported_permissions_grantable_from_ui` method was getting a little complex, with a lot of conditional logic. This separates out the sorting and filtering, with the former the responsibility of the `SupportedPermission` model, and the latter remaining the responsibility of the `Doorkeeper::Application` model based on the passed in arguments In the `Doorkeeper::Application` tests, we're using [Mocha's with][1] with a block using [Minitest's assert_same_elements][2] to check that the array argument contains the same elements regardless of order Mocha does have various [ParameterMatchers][3], which can be used with `with`, but unfortunately not an equivalent of `assert_same_elements` or something like [Jest's expect.arrayContaining][4] [1]: https://mocha.jamesmead.org/Mocha/Expectation.html#with-instance_method [2]: https://www.rubydoc.info/gems/minitest-rails-shoulda/0.4.1/MiniTest%2FAssertions:assert_same_elements [3]: https://mocha.jamesmead.org/Mocha/ParameterMatchers.html [4]: https://jestjs.io/docs/expect#expectarraycontainingarray Co-authored-by: Ynda Jas --- app/models/doorkeeper/application.rb | 17 +--- app/models/supported_permission.rb | 6 ++ test/models/doorkeeper/application_test.rb | 108 +++++++++------------ test/models/supported_permission_test.rb | 28 ++++++ 4 files changed, 86 insertions(+), 73 deletions(-) diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index e6cebf5aa..05625608f 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -43,20 +43,11 @@ def signin_permission end def sorted_supported_permissions_grantable_from_ui(include_signin: true, only_delegatable: false) - sorted_permissions = if only_delegatable - supported_permissions.grantable_from_ui.delegatable.order(:name) - else - supported_permissions.grantable_from_ui.order(:name) - end - sorted_permissions_without_signin = sorted_permissions - [signin_permission] + permissions = supported_permissions.grantable_from_ui + permissions = permissions.excluding_signin unless include_signin + permissions = permissions.delegatable if only_delegatable - return sorted_permissions_without_signin unless include_signin - - if only_delegatable && !signin_permission.delegatable - sorted_permissions_without_signin - else - ([signin_permission] + sorted_permissions_without_signin).compact - end + SupportedPermission.sort_with_signin_first(permissions) end def has_non_signin_permissions_grantable_from_ui? diff --git a/app/models/supported_permission.rb b/app/models/supported_permission.rb index eac3f3e8f..2b8386a0e 100644 --- a/app/models/supported_permission.rb +++ b/app/models/supported_permission.rb @@ -14,12 +14,18 @@ class SupportedPermission < ApplicationRecord scope :grantable_from_ui, -> { where(grantable_from_ui: true) } scope :default, -> { where(default: true) } scope :signin, -> { where(name: SIGNIN_NAME) } + scope :excluding_signin, -> { where.not(name: SIGNIN_NAME) } scope :excluding_application, ->(application) { where.not(application:) } def signin? name.try(:downcase) == SIGNIN_NAME end + def self.sort_with_signin_first(supported_permissions) + signin_permission = supported_permissions.find(&:signin?) + ([signin_permission] + supported_permissions.excluding_signin.order(:name)).compact + end + private def signin_permission_name_not_changed diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index 131a95836..a3f13e0ab 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -84,77 +84,65 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase end context "#sorted_supported_permissions_grantable_from_ui" do - should "return all of the supported permissions that are grantable from the ui" do - application = create( - :application, - with_delegatable_supported_permissions: %w[delegatable-grantable], - with_delegatable_supported_permissions_not_grantable_from_ui: %w[delegatable-non-grantable], - with_non_delegatable_supported_permissions: ["non-delegatable-grantable", SupportedPermission::SIGNIN_NAME], - with_non_delegatable_supported_permissions_not_grantable_from_ui: %w[non-delegatable-non-grantable], - ) - - permission_names = application.sorted_supported_permissions_grantable_from_ui.map(&:name) + setup do + @application = create(:application) + @delegatable_permission = create(:delegatable_supported_permission, application: @application) + @delegatable_non_grantable_permission = create(:delegatable_supported_permission, application: @application, grantable_from_ui: false) + @non_delegatable_permission = create(:non_delegatable_supported_permission, application: @application) + @non_delegatable_non_grantable_permission = create(:non_delegatable_supported_permission, application: @application, grantable_from_ui: false) - assert_includes permission_names, "delegatable-grantable" - assert_includes permission_names, "non-delegatable-grantable" - assert_includes permission_names, SupportedPermission::SIGNIN_NAME - assert_not_includes permission_names, "delegatable-non-grantable" - assert_not_includes permission_names, "non-delegatable-non-grantable" + @sorted_permissions = "double" end - should "sort the permissions alphabetically by name, but with the signin permission first" do - application = create( - :application, - with_delegatable_supported_permissions: %w[a d], - with_non_delegatable_supported_permissions: ["c", "b", SupportedPermission::SIGNIN_NAME], - ) - - permission_names = application.sorted_supported_permissions_grantable_from_ui.map(&:name) + should "sorts the app's UI-grantable permissions using `SupportedPermission`" do + SupportedPermission + .expects(:sort_with_signin_first) + .with { |value| + assert_same_elements( + [@application.signin_permission, @delegatable_permission, @non_delegatable_permission], + value, + ) + } + .returns(@sorted_permissions) - assert_equal [SupportedPermission::SIGNIN_NAME, "a", "b", "c", "d"], permission_names + assert_equal @sorted_permissions, @application.sorted_supported_permissions_grantable_from_ui end should "exclude the signin permission if requested" do - application = create( - :application, - with_non_delegatable_supported_permissions: ["Writer", "Editor", SupportedPermission::SIGNIN_NAME], - ) + SupportedPermission + .expects(:sort_with_signin_first) + .with { |value| + assert_same_elements( + [@delegatable_permission, @non_delegatable_permission], + value, + ) + } + .returns(@sorted_permissions) - permission_names = application.sorted_supported_permissions_grantable_from_ui(include_signin: false).map(&:name) - - assert_not_includes permission_names, SupportedPermission::SIGNIN_NAME - assert_equal %w[Editor Writer], permission_names + assert_equal @sorted_permissions, @application.sorted_supported_permissions_grantable_from_ui(include_signin: false) end should "exclude non-delegatable permissions if requested" do - application_1 = create( - :application, - with_delegatable_supported_permissions: %w[delegatable], - with_non_delegatable_supported_permissions: ["non-delegatable", SupportedPermission::SIGNIN_NAME], - ) - application_2 = create( - :application, - with_delegatable_supported_permissions: ["delegatable", SupportedPermission::SIGNIN_NAME], - with_non_delegatable_supported_permissions: %w[non-delegatable], - ) - - application_1_permission_names = application_1.sorted_supported_permissions_grantable_from_ui(only_delegatable: true).map(&:name) - application_2_permission_names = application_2.sorted_supported_permissions_grantable_from_ui(only_delegatable: true).map(&:name) - - assert_equal %w[delegatable], application_1_permission_names - assert_equal [SupportedPermission::SIGNIN_NAME, "delegatable"], application_2_permission_names - end - - should "exclude non-delegatable signin permission if requested, even when `include_signin == true`" do - application = create( - :application, - with_non_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME], - ) - - permission_names = application.sorted_supported_permissions_grantable_from_ui(include_signin: true, only_delegatable: true).map(&:name) - - assert_not_includes permission_names, SupportedPermission::SIGNIN_NAME - assert_empty permission_names + SupportedPermission + .expects(:sort_with_signin_first) + .with { |value| + assert_same_elements( + [@application.signin_permission, @delegatable_permission], + value, + ) + } + .returns(@sorted_permissions) + + assert_equal @sorted_permissions, @application.sorted_supported_permissions_grantable_from_ui(only_delegatable: true) + end + + should "exclude signin and non-delegatable permissions if requested" do + SupportedPermission + .expects(:sort_with_signin_first) + .with { |value| assert_same_elements([@delegatable_permission], value) } + .returns(@sorted_permissions) + + assert_equal @sorted_permissions, @application.sorted_supported_permissions_grantable_from_ui(include_signin: false, only_delegatable: true) end end diff --git a/test/models/supported_permission_test.rb b/test/models/supported_permission_test.rb index 5079c38df..e09e4c692 100644 --- a/test/models/supported_permission_test.rb +++ b/test/models/supported_permission_test.rb @@ -77,4 +77,32 @@ class SupportedPermissionTest < ActiveSupport::TestCase assert_same_elements ["included-permission", SupportedPermission::SIGNIN_NAME], SupportedPermission.excluding_application(excluded_application).pluck(:name) end + + context ".sort_with_signin_first" do + setup do + @application = create(:application, with_delegatable_supported_permissions: %w[d a c2 b c1]) + end + + context "without a signin permission" do + should "return the permissions, sorted alphanumerically by name" do + assert_equal( + %w[a b c1 c2 d], + SupportedPermission + .sort_with_signin_first(@application.supported_permissions.excluding_signin) + .map(&:name), + ) + end + end + + context "with a signin permission" do + should "return the permissions, sorted alphanumerically by name, but with signin first" do + assert_equal( + %w[signin a b c1 c2 d], + SupportedPermission + .sort_with_signin_first(@application.supported_permissions) + .map(&:name), + ) + end + end + end end