Merge pull request #2670 from alphagov/refactor-roles

Refactor roles

app/controllers/account/roles_controller.rb

@@ -8,7 +8,7 @@ def update
     role = params[:user][:role]
 
     if UserUpdate.new(current_user, { role: }, current_user, user_ip_address).call
-      redirect_to account_path, notice: "Your role is now #{role.humanize}"
+      redirect_to account_path, notice: "Your role is now #{Roles.find(role).display_name}"
     else
       flash[:alert] = "There was a problem changing your role."
       render :edit

app/controllers/api_users_controller.rb

@@ -48,7 +48,7 @@ def api_user_params_for_create
   def sanitise(permitted_user_params)
     UserParameterSanitiser.new(
       user_params: permitted_user_params.to_h,
-      current_user_role: current_user.role.to_sym,
+      current_user_role: current_user.role_name.to_sym,
     ).sanitise
   end
 

app/controllers/invitations_controller.rb

@@ -62,7 +62,7 @@ def organisation(params)
   end
 
   def invitee_requires_2sv(params)
-    organisation(params)&.require_2sv? || User.admin_roles.include?(params[:role])
+    organisation(params)&.require_2sv? || Roles.find(params[:role])&.require_2sv? || false
   end
 
   def redirect_if_invitee_already_exists

app/controllers/users_controller.rb

@@ -70,7 +70,7 @@ def export
   def user_params
     UserParameterSanitiser.new(
       user_params: params.require(:user).permit(:require_2sv).to_h,
-      current_user_role: current_user.role.to_sym,
+      current_user_role: current_user.role_name.to_sym,
     ).sanitise
   end
 

app/helpers/users_helper.rb

@@ -62,18 +62,14 @@ def filtered_users_heading(users)
     end
   end
 
-  def assignable_user_roles
-    current_user.manageable_roles
-  end
-
   def user_name(user)
     anchor_tag = link_to(user.name, edit_user_path(user), class: "govuk-link")
     user.suspended? ? content_tag(:del, anchor_tag) : anchor_tag
   end
 
   def options_for_role_select(selected: nil)
-    assignable_user_roles.map do |role|
-      { text: role.humanize, value: role }.tap do |option|
+    current_user.manageable_roles.map do |role|
+      { text: role.display_name, value: role.name }.tap do |option|
         option[:selected] = true if option[:value] == selected
       end
     end
@@ -134,7 +130,7 @@ def summary_list_item_for_organisation(user)
   end
 
   def summary_list_item_for_role(user)
-    item = { field: "Role", value: user.role.humanize.capitalize }
+    item = { field: "Role", value: user.role_display_name }
     item[:edit] = { href: edit_user_role_path(user) } if policy(user).assign_role?
     item
   end

app/models/batch_invitation_user.rb

@@ -94,7 +94,7 @@ def invite_user_with_attributes(sanitised_attributes, inviting_user)
   def sanitise_attributes_for_inviting_user_role(raw_attributes, inviting_user)
     UserParameterSanitiser.new(
       user_params: raw_attributes,
-      current_user_role: inviting_user.role.to_sym,
+      current_user_role: inviting_user.role_name.to_sym,
     ).sanitise
   end
 

app/models/doorkeeper/application.rb

@@ -39,7 +39,7 @@ def self.policy_class
   end
 
   def supported_permission_strings(user = nil)
-    if user && %w[organisation_admin super_organisation_admin].include?(user.role)
+    if user && user.publishing_manager?
       supported_permissions.delegatable.pluck(:name) & user.permissions_for(self)
     else
       supported_permissions.pluck(:name)

app/models/user.rb

@@ -43,13 +43,16 @@ class User < ApplicationRecord
          :confirmable,
          :password_archivable # in signon/lib/devise/models/password_archivable.rb
 
-  delegate :manageable_roles, to: :role_class
+  delegate :manageable_roles, to: :role
+  delegate :display_name, to: :role, prefix: true
+  delegate :name, to: :role, prefix: true, allow_nil: true
 
   encrypts :otp_secret_key
 
   validates :name, presence: true
   validates :email, reject_non_governmental_email_addresses: true
   validates :reason_for_suspension, presence: true, if: proc { |u| u.suspended? }
+  validates :role, inclusion: { in: Roles.all }
   validate :user_can_be_exempted_from_2sv
   validate :organisation_admin_belongs_to_organisation
   validate :email_is_ascii_only
@@ -126,6 +129,10 @@ def self.with_default_permissions
     new(supported_permissions: SupportedPermission.default)
   end
 
+  def role
+    Roles.find(self[:role])
+  end
+
   def require_2sv?
     return require_2sv unless organisation
 
@@ -235,7 +242,7 @@ def unusable_account?
 
   # Required for devise_invitable to set role and permissions
   def self.inviter_role(inviter)
-    inviter.nil? ? :default : inviter.role.to_sym
+    inviter.nil? ? :default : inviter.role_name.to_sym
   end
 
   def invite!(*args)
@@ -304,16 +311,12 @@ def not_setup_2sv?
     two_step_status == TWO_STEP_STATUS_NOT_SET_UP
   end
 
-  def role_class
-    Roles.const_get(role.classify)
-  end
-
   def can_manage?(other_user)
-    manageable_roles.include?(other_user.role)
+    role.can_manage?(other_user.role)
   end
 
   def manageable_organisations
-    role_class.manageable_organisations_for(self).order(:name)
+    role.manageable_organisations_for(self).order(:name)
   end
 
   # Make devise send all emails using ActiveJob
@@ -328,7 +331,7 @@ def need_two_step_verification?
   def set_2sv_for_admin_roles
     return unless GovukEnvironment.production?
 
-    self.require_2sv = true if role_changed? && (govuk_admin? || publishing_manager?)
+    self.require_2sv = true if role_changed? && role.require_2sv?
   end
 
   def reset_2sv_exemption_reason
@@ -427,12 +430,12 @@ def mark_two_step_mandated_changed
   end
 
   def user_can_be_exempted_from_2sv
-    errors.add(:reason_for_2sv_exemption, "#{role} users cannot be exempted from 2SV. Remove the user's exemption to change their role.") if exempt_from_2sv? && !normal?
+    errors.add(:reason_for_2sv_exemption, "cannot be blank for #{role_display_name} users. Remove the user's exemption to change their role.") if exempt_from_2sv? && role.require_2sv?
   end
 
   def organisation_admin_belongs_to_organisation
-    if %w[organisation_admin super_organisation_admin].include?(role) && organisation_id.blank?
-      errors.add(:organisation_id, "can't be 'None' for #{role.titleize}")
+    if publishing_manager? && organisation_id.blank?
+      errors.add(:organisation_id, "can't be 'None' for #{role_display_name}")
     end
   end
 

app/models/users_filter.rb

@@ -51,10 +51,10 @@ def two_step_status_option_select_options(aria_controls_id: nil)
   def role_option_select_options(aria_controls_id: nil)
     @current_user.manageable_roles.map do |role|
       {
-        label: role.humanize.capitalize,
+        label: role.display_name,
         controls: aria_controls_id,
-        value: role,
-        checked: Array(options[:roles]).include?(role),
+        value: role.name,
+        checked: Array(options[:roles]).include?(role.name),
       }.compact
     end
   end

app/policies/user_policy.rb

@@ -1,31 +1,31 @@
 class UserPolicy < BasePolicy
   def index?
-    %w[superadmin admin super_organisation_admin organisation_admin].include? current_user.role
+    current_user.govuk_admin? || current_user.publishing_manager?
   end
 
   # invitations#new
   def new?
-    %w[superadmin admin].include? current_user.role
+    current_user.govuk_admin?
   end
   alias_method :assign_organisation?, :new?
 
   # invitations#create
   alias_method :create?, :new?
 
   def edit?
-    case current_user.role
-    when Roles::Superadmin.role_name
+    case current_user.role_name
+    when Roles::Superadmin.name
       true
-    when Roles::Admin.role_name
+    when Roles::Admin.name
       can_manage?
-    when Roles::SuperOrganisationAdmin.role_name
+    when Roles::SuperOrganisationAdmin.name
       can_manage? && (record_in_own_organisation? || record_in_child_organisation?)
-    when Roles::OrganisationAdmin.role_name
+    when Roles::OrganisationAdmin.name
       can_manage? && record_in_own_organisation?
-    when Roles::Normal.role_name
+    when Roles::Normal.name
       false
     else
-      raise "Unknown role: #{current_user.role}"
+      raise "Unknown role: #{current_user.role_name}"
     end
   end
   alias_method :update?, :edit?
@@ -62,11 +62,11 @@ def resolve
       if current_user.superadmin?
         scope.web_users
       elsif current_user.admin?
-        scope.web_users.where(role: current_user.manageable_roles)
+        scope.web_users.where(role: current_user.manageable_roles.map(&:name))
       elsif current_user.super_organisation_admin?
-        scope.web_users.where(role: current_user.manageable_roles).where(organisation: current_user.organisation.subtree)
+        scope.web_users.where(role: current_user.manageable_roles.map(&:name)).where(organisation: current_user.organisation.subtree)
       elsif current_user.organisation_admin?
-        scope.web_users.where(role: current_user.manageable_roles).where(organisation: current_user.organisation)
+        scope.web_users.where(role: current_user.manageable_roles.map(&:name)).where(organisation: current_user.organisation)
       else
         scope.none
       end

app/presenters/user_export_presenter.rb

@@ -35,7 +35,7 @@ def row(user)
     [
       user.name,
       user.email,
-      user.role.humanize,
+      user.role_display_name,
       user.organisation.try(:name),
       user.sign_in_count,
       user.current_sign_in_at.try(:to_formatted_s, :db),

app/views/account/roles/edit.html.erb

@@ -24,15 +24,15 @@
             id: "user_role",
             name: "user[role]",
             label: "Role",
-            options: current_user.manageable_roles.map { |role| { text: role.humanize, value: role, selected: current_user.role == role } }
+            options: current_user.manageable_roles.map { |role| { text: role.display_name, value: role.name, selected: current_user.role == role.name } }
             } %>
         <%= render "govuk_publishing_components/components/button", {
           text: "Change role"
         } %>
       <% end %>
     <% else %>
       <%= render "govuk_publishing_components/components/inset_text", {
-        text: current_user.role.humanize,
+        text: current_user.role_display_name,
       } %>
     <% end %>
   </div>

app/views/devise/invitations/new.html.erb

@@ -62,7 +62,7 @@
           name: "user[role]",
           label: "Role",
           hint: user_role_select_hint,
-          options: options_for_role_select(selected: f.object.role),
+          options: options_for_role_select(selected: f.object.role_name),
         } %>
       <% end %>
 

app/views/doorkeeper_applications/users_with_access.html.erb

@@ -34,7 +34,7 @@
     [
       { text: formatted_user_name(user), format: user_name_format(user) },
       { text: user.email, format: 'email' },
-      { text: user.role.humanize },
+      { text: user.role_display_name },
       { text: user.organisation_name },
       { text: user.sign_in_count },
       { text: formatted_last_sign_in(user) },

app/views/users/index.html.erb

@@ -39,7 +39,7 @@
           [
             { text: user_name(user) },
             { text: user.email },
-            { text: user.role.humanize },
+            { text: user.role_display_name },
             { text: status(user) },
             { text: two_step_status(user) },
           ]

app/views/users/roles/edit.html.erb

@@ -40,15 +40,15 @@
     <%= form_for @user, url: user_role_path(@user) do %>
       <% if @user.exempt_from_2sv? %>
         <%= render "govuk_publishing_components/components/inset_text", {
-          text: "This user's role is set to #{@user.role.humanize}. They are currently exempted from 2-step verification, meaning that their role cannot be changed as admins are required to have 2-step verification.",
+          text: "This user's role is set to #{@user.role_display_name}. They are currently exempted from 2-step verification, meaning that their role cannot be changed as admins are required to have 2-step verification.",
         } %>
       <% else %>
         <%= render "govuk_publishing_components/components/select", {
           id: "user_role",
           name: "user[role]",
           label: "Role",
           hint: user_role_select_hint,
-          options: current_user.manageable_roles.map { |role| { text: role.humanize, value: role, selected: @user.role == role } },
+          options: current_user.manageable_roles.map { |role| { text: role.display_name, value: role.name, selected: @user.role == role.name } },
           error_message: @user.errors[:role].any? ? @user.errors.full_messages_for(:role).to_sentence : nil
         } %>
         <div class="govuk-button-group">

db/seeds.rb

@@ -11,7 +11,7 @@
   name: "Test Superadmin",
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::Superadmin.role_name,
+  role: Roles::Superadmin.name,
   confirmed_at: Time.current,
   organisation: gds,
 )
@@ -20,7 +20,7 @@
   name: "Test Admin",
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::Admin.role_name,
+  role: Roles::Admin.name,
   confirmed_at: Time.current,
   organisation: gds,
 )
@@ -29,7 +29,7 @@
   name: "Test Super Organisation Admin",
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::SuperOrganisationAdmin.role_name,
+  role: Roles::SuperOrganisationAdmin.name,
   confirmed_at: Time.current,
   organisation: gds,
 )
@@ -38,7 +38,7 @@
   name: "Test Organisation Admin",
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::OrganisationAdmin.role_name,
+  role: Roles::OrganisationAdmin.name,
   confirmed_at: Time.current,
   organisation: gds,
 )
@@ -63,7 +63,7 @@
   name: "Test User",
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::Normal.role_name,
+  role: Roles::Normal.name,
   confirmed_at: Time.current,
   organisation: test_organisation_without_2sv,
 )
@@ -99,7 +99,7 @@
   name: "Test User with 2SV enabled",
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::Normal.role_name,
+  role: Roles::Normal.name,
   confirmed_at: Time.current,
   organisation: test_organisation_without_2sv,
   require_2sv: true,
@@ -110,7 +110,7 @@
   name: "Test User from organisation with mandatory 2SV",
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::Normal.role_name,
+  role: Roles::Normal.name,
   confirmed_at: Time.current,
   organisation: test_organisation_with_2sv,
   require_2sv: true,
@@ -122,7 +122,7 @@
   api_user: true,
   email: "[email protected]",
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
-  role: Roles::Normal.role_name,
+  role: Roles::Normal.name,
   confirmed_at: Time.current,
   require_2sv: false,
 )