From c006e59b95d0c1abb2a1351179db0bf14309baed Mon Sep 17 00:00:00 2001
From: James Mead <james.mead@digital.cabinet-office.gov.uk>
Date: Tue, 12 Dec 2023 12:25:20 +0000
Subject: [PATCH] Extract User#web_user? predicate method

To be the inverse of `User#api_user?` and corresponding with the
existing `User.web_users` scope.

We can use `User#web_user?` to avoid unnecessary inverse logic and make
the code more readable by using `if user.web_user?` vs `unless
user.api_user?` and `user.web_user?` vs `!user.api_user?`.

Also the callsite in `UserPolicy#exempt_from_two_step_verification?` was
using the non-predicate attribute accessor which wasn't very idiomatic.
---
 app/models/user.rb                          |  6 +++++-
 app/policies/user_policy.rb                 |  2 +-
 app/views/shared/_user_permissions.html.erb |  4 ++--
 app/views/users/_app_permissions.html.erb   |  6 +++---
 test/models/user_test.rb                    | 10 ++++++++++
 5 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index eb7483c4a..5b1b7237a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -285,7 +285,7 @@ def lock_access!(opts = {})
   def status
     return USER_STATUS_SUSPENDED if suspended?
 
-    if !api_user? && invited_but_not_yet_accepted?
+    if web_user? && invited_but_not_yet_accepted?
       return USER_STATUS_INVITED
     end
 
@@ -419,6 +419,10 @@ def organisation_name
     organisation.present? ? organisation.name : Organisation::NONE
   end
 
+  def web_user?
+    !api_user?
+  end
+
 private
 
   def two_step_mandated_changed?
diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
index 30c4cccc7..9843ca486 100644
--- a/app/policies/user_policy.rb
+++ b/app/policies/user_policy.rb
@@ -48,7 +48,7 @@ def exempt_from_two_step_verification?
     current_user.belongs_to_gds? &&
       current_user.govuk_admin? &&
       record.normal? &&
-      !record.api_user
+      record.web_user?
   end
 
 private
diff --git a/app/views/shared/_user_permissions.html.erb b/app/views/shared/_user_permissions.html.erb
index 4745dc2d7..a7961e9bf 100644
--- a/app/views/shared/_user_permissions.html.erb
+++ b/app/views/shared/_user_permissions.html.erb
@@ -2,10 +2,10 @@
   <thead>
     <tr class="table-header">
       <th>Application</th>
-      <% unless user_object.api_user? %>
+      <% if user_object.web_user? %>
         <th>Has access?</th>
       <% end %>
-      <th><%= "Other" unless user_object.api_user? %> Permissions</th>
+      <th><%= "Other" if user_object.web_user? %> Permissions</th>
       <% if user_object.persisted? %>
         <th>Last synced at</th>
       <% end %>
diff --git a/app/views/users/_app_permissions.html.erb b/app/views/users/_app_permissions.html.erb
index 73d42c938..b61d73fde 100644
--- a/app/views/users/_app_permissions.html.erb
+++ b/app/views/users/_app_permissions.html.erb
@@ -2,10 +2,10 @@
   <thead>
     <tr class="table-header">
       <th>Application</th>
-      <% unless user_object.api_user? %>
+      <% if user_object.web_user? %>
           <th>Has access?</th>
       <% end %>
-      <th><%= "Other" unless user_object.api_user? %> Permissions</th>
+      <th><%= "Other" if user_object.web_user? %> Permissions</th>
     </tr>
   </thead>
   <tbody>
@@ -16,7 +16,7 @@
         <td>
           <%= application.name %>
         </td>
-        <% unless user_object.api_user? %>
+        <% if user_object.web_user? %>
           <td>
             <%= permission_names.include?(SupportedPermission::SIGNIN_NAME) ? 'Yes' : 'No' %>
           </td>
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index d1d8145bd..facb5a397 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -1276,6 +1276,16 @@ def setup
     end
   end
 
+  context "#web_user?" do
+    should "return true for non-API user" do
+      assert build(:user).web_user?
+    end
+
+    should "return false for API user" do
+      assert_not build(:api_user).web_user?
+    end
+  end
+
   def authenticate_access(user, app)
     Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id)
   end