Merge pull request #3087 from alphagov/1184-refactor

Refactor supported permissions method

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?

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

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
 

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