Skip to content

Commit

Permalink
Extract User#web_user? predicate method
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
floehopper committed Dec 12, 2023
1 parent 2a8b4cf commit c006e59
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 7 deletions.
6 changes: 5 additions & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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?
Expand Down
2 changes: 1 addition & 1 deletion app/policies/user_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions app/views/shared/_user_permissions.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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 %>
Expand Down
6 changes: 3 additions & 3 deletions app/views/users/_app_permissions.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down
10 changes: 10 additions & 0 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit c006e59

Please sign in to comment.