From 0cb81e061e8300914025a26d61c0c4316fdde663 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 11:42:43 +0000
Subject: [PATCH 01/26] Avoid unnecessary metaprogramming

---
 lib/roles.rb | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 84d2e63b1..6fb460c2b 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -17,11 +17,13 @@ def self.included(base)
 
   module ClassMethods
     def role_classes
-      class_names = Roles.constants.select { |c| Roles.const_get(c).is_a?(Class) && Roles.const_get(c) != Roles::Base }
-
-      class_names.map do |role_class|
-        "Roles::#{role_class}".constantize
-      end
+      [
+        Roles::Superadmin,
+        Roles::Admin,
+        Roles::SuperOrganisationAdmin,
+        Roles::OrganisationAdmin,
+        Roles::Normal,
+      ]
     end
 
     def admin_role_classes

From 595124743e40afb7194d84fcbffe5e171f1f7ed6 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 11:45:02 +0000
Subject: [PATCH 02/26] Remove Roles::<class>#level

This was only being used to order them but we now
hardcode the order.
---
 lib/roles.rb                          | 2 +-
 lib/roles/admin.rb                    | 4 ----
 lib/roles/normal.rb                   | 4 ----
 lib/roles/organisation_admin.rb       | 4 ----
 lib/roles/super_organisation_admin.rb | 4 ----
 lib/roles/superadmin.rb               | 4 ----
 6 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 6fb460c2b..5d609be56 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -35,7 +35,7 @@ def admin_roles
     end
 
     def roles
-      role_classes.sort_by(&:level).map(&:role_name)
+      role_classes.map(&:role_name)
     end
   end
 
diff --git a/lib/roles/admin.rb b/lib/roles/admin.rb
index c7ec2edc3..2ef3eef58 100644
--- a/lib/roles/admin.rb
+++ b/lib/roles/admin.rb
@@ -19,10 +19,6 @@ def self.role_name
       "admin"
     end
 
-    def self.level
-      1
-    end
-
     def self.manageable_roles
       %w[normal organisation_admin super_organisation_admin admin]
     end
diff --git a/lib/roles/normal.rb b/lib/roles/normal.rb
index 8fd67cfff..6025463c2 100644
--- a/lib/roles/normal.rb
+++ b/lib/roles/normal.rb
@@ -8,10 +8,6 @@ def self.role_name
       "normal"
     end
 
-    def self.level
-      4
-    end
-
     def self.manageable_roles
       []
     end
diff --git a/lib/roles/organisation_admin.rb b/lib/roles/organisation_admin.rb
index 68617ebf6..366b0295f 100644
--- a/lib/roles/organisation_admin.rb
+++ b/lib/roles/organisation_admin.rb
@@ -18,10 +18,6 @@ def self.role_name
       "organisation_admin"
     end
 
-    def self.level
-      3
-    end
-
     def self.manageable_roles
       %w[normal organisation_admin]
     end
diff --git a/lib/roles/super_organisation_admin.rb b/lib/roles/super_organisation_admin.rb
index f8afe40b2..ee9d13364 100644
--- a/lib/roles/super_organisation_admin.rb
+++ b/lib/roles/super_organisation_admin.rb
@@ -18,10 +18,6 @@ def self.role_name
       "super_organisation_admin"
     end
 
-    def self.level
-      2
-    end
-
     def self.manageable_roles
       %w[normal organisation_admin super_organisation_admin]
     end
diff --git a/lib/roles/superadmin.rb b/lib/roles/superadmin.rb
index 44a5def39..5a0400c48 100644
--- a/lib/roles/superadmin.rb
+++ b/lib/roles/superadmin.rb
@@ -20,10 +20,6 @@ def self.role_name
       "superadmin"
     end
 
-    def self.level
-      0
-    end
-
     def self.manageable_roles
       User.roles
     end

From f0ec51448b90a3d41c9c74f009cf7ce41066e458 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 11:47:29 +0000
Subject: [PATCH 03/26] Explicitly require roles

---
 lib/roles.rb | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 5d609be56..78786adbe 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -1,4 +1,9 @@
-Dir["#{File.dirname(__FILE__)}/roles/*.rb"].sort.each { |file| require file }
+require "roles/base"
+require "roles/superadmin"
+require "roles/admin"
+require "roles/super_organisation_admin"
+require "roles/organisation_admin"
+require "roles/normal"
 
 module Roles
   def self.included(base)

From 10fd8cd2a498f52c82d0e924cda78c8307eb6f4e Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 11:48:52 +0000
Subject: [PATCH 04/26] Remove unused Roles::Base

---
 lib/roles.rb                          | 3 +--
 lib/roles/admin.rb                    | 2 +-
 lib/roles/base.rb                     | 3 ---
 lib/roles/normal.rb                   | 2 +-
 lib/roles/organisation_admin.rb       | 2 +-
 lib/roles/super_organisation_admin.rb | 2 +-
 lib/roles/superadmin.rb               | 2 +-
 7 files changed, 6 insertions(+), 10 deletions(-)
 delete mode 100644 lib/roles/base.rb

diff --git a/lib/roles.rb b/lib/roles.rb
index 78786adbe..b59e2132c 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -1,4 +1,3 @@
-require "roles/base"
 require "roles/superadmin"
 require "roles/admin"
 require "roles/super_organisation_admin"
@@ -32,7 +31,7 @@ def role_classes
     end
 
     def admin_role_classes
-      role_classes - [Roles::Normal, Roles::Base]
+      role_classes - [Roles::Normal]
     end
 
     def admin_roles
diff --git a/lib/roles/admin.rb b/lib/roles/admin.rb
index 2ef3eef58..5b90c18b7 100644
--- a/lib/roles/admin.rb
+++ b/lib/roles/admin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class Admin < Base
+  class Admin
     def self.permitted_user_params
       [
         :uid,
diff --git a/lib/roles/base.rb b/lib/roles/base.rb
deleted file mode 100644
index 5b0a5cd86..000000000
--- a/lib/roles/base.rb
+++ /dev/null
@@ -1,3 +0,0 @@
-module Roles
-  class Base; end
-end
diff --git a/lib/roles/normal.rb b/lib/roles/normal.rb
index 6025463c2..a3e4bbe73 100644
--- a/lib/roles/normal.rb
+++ b/lib/roles/normal.rb
@@ -1,5 +1,5 @@
 module Roles
-  class Normal < Base
+  class Normal
     def self.permitted_user_params
       %i[uid name email password password_confirmation]
     end
diff --git a/lib/roles/organisation_admin.rb b/lib/roles/organisation_admin.rb
index 366b0295f..e8a228e3c 100644
--- a/lib/roles/organisation_admin.rb
+++ b/lib/roles/organisation_admin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class OrganisationAdmin < Base
+  class OrganisationAdmin
     def self.permitted_user_params
       [
         :uid,
diff --git a/lib/roles/super_organisation_admin.rb b/lib/roles/super_organisation_admin.rb
index ee9d13364..49c486fc6 100644
--- a/lib/roles/super_organisation_admin.rb
+++ b/lib/roles/super_organisation_admin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class SuperOrganisationAdmin < Base
+  class SuperOrganisationAdmin
     def self.permitted_user_params
       [
         :uid,
diff --git a/lib/roles/superadmin.rb b/lib/roles/superadmin.rb
index 5a0400c48..23a2c2e77 100644
--- a/lib/roles/superadmin.rb
+++ b/lib/roles/superadmin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class Superadmin < Base
+  class Superadmin
     def self.permitted_user_params
       [
         :uid,

From b8c557b261badc09524bcb63e25861d7afdcb894 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:00:28 +0000
Subject: [PATCH 05/26] Extract Roles.all

We don't make use of User.role_classes so let's remove it.
---
 lib/roles.rb | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index b59e2132c..58265bae5 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -5,6 +5,16 @@
 require "roles/normal"
 
 module Roles
+  def self.all
+    [
+      Roles::Superadmin,
+      Roles::Admin,
+      Roles::SuperOrganisationAdmin,
+      Roles::OrganisationAdmin,
+      Roles::Normal,
+    ]
+  end
+
   def self.included(base)
     base.extend ClassMethods
 
@@ -20,16 +30,6 @@ def self.included(base)
   end
 
   module ClassMethods
-    def role_classes
-      [
-        Roles::Superadmin,
-        Roles::Admin,
-        Roles::SuperOrganisationAdmin,
-        Roles::OrganisationAdmin,
-        Roles::Normal,
-      ]
-    end
-
     def admin_role_classes
       role_classes - [Roles::Normal]
     end
@@ -39,7 +39,7 @@ def admin_roles
     end
 
     def roles
-      role_classes.map(&:role_name)
+      Roles.all.map(&:role_name)
     end
   end
 

From 12367baaa81ee9dfb4443c7ac0dfe17b3cf23a53 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:01:31 +0000
Subject: [PATCH 06/26] Extract Roles.names

We don't make use of User.roles so let's remove it.
---
 lib/roles.rb             | 12 ++++++------
 lib/roles/superadmin.rb  |  2 +-
 test/models/user_test.rb |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 58265bae5..e49990119 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -15,14 +15,18 @@ def self.all
     ]
   end
 
+  def self.names
+    all.map(&:role_name)
+  end
+
   def self.included(base)
     base.extend ClassMethods
 
     base.instance_eval do
-      validates :role, inclusion: { in: roles }
+      validates :role, inclusion: { in: Roles.names }
     end
 
-    base.roles.each do |role_name|
+    Roles.names.each do |role_name|
       define_method("#{role_name}?") do
         role == role_name
       end
@@ -37,10 +41,6 @@ def admin_role_classes
     def admin_roles
       admin_role_classes.map(&:role_name)
     end
-
-    def roles
-      Roles.all.map(&:role_name)
-    end
   end
 
   def govuk_admin?
diff --git a/lib/roles/superadmin.rb b/lib/roles/superadmin.rb
index 23a2c2e77..ea0b5d318 100644
--- a/lib/roles/superadmin.rb
+++ b/lib/roles/superadmin.rb
@@ -21,7 +21,7 @@ def self.role_name
     end
 
     def self.manageable_roles
-      User.roles
+      Roles.names
     end
 
     def self.manageable_organisations_for(_)
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 3607eff4f..21971d2c3 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -874,7 +874,7 @@ def setup
       assert_equal %w[normal organisation_admin], build(:organisation_admin_user).manageable_roles
       assert_equal %w[normal organisation_admin super_organisation_admin], build(:super_organisation_admin_user).manageable_roles
       assert_equal %w[normal organisation_admin super_organisation_admin admin], build(:admin_user).manageable_roles
-      assert_equal User.roles, build(:superadmin_user).manageable_roles
+      assert_equal Roles.names, build(:superadmin_user).manageable_roles
     end
   end
 

From 82e4f5441260e2fa89cd68f079422034dea8e6fa Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:03:02 +0000
Subject: [PATCH 07/26] Move User#role validation to User class

As this is the only place it's used.
---
 app/models/user.rb | 1 +
 lib/roles.rb       | 4 ----
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 609ec1169..6ef37cc26 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -50,6 +50,7 @@ class User < ApplicationRecord
   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.names }
   validate :user_can_be_exempted_from_2sv
   validate :organisation_admin_belongs_to_organisation
   validate :email_is_ascii_only
diff --git a/lib/roles.rb b/lib/roles.rb
index e49990119..288db4811 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -22,10 +22,6 @@ def self.names
   def self.included(base)
     base.extend ClassMethods
 
-    base.instance_eval do
-      validates :role, inclusion: { in: Roles.names }
-    end
-
     Roles.names.each do |role_name|
       define_method("#{role_name}?") do
         role == role_name

From 15058cd984946a313c59c5bd8a857c916fe26589 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:07:12 +0000
Subject: [PATCH 08/26] Extract Roles.admin

This is not used as User.admin_role_classes
---
 lib/roles.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 288db4811..6275c21d1 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -15,6 +15,10 @@ def self.all
     ]
   end
 
+  def self.admin
+    all - [Roles::Normal]
+  end
+
   def self.names
     all.map(&:role_name)
   end
@@ -30,12 +34,8 @@ def self.included(base)
   end
 
   module ClassMethods
-    def admin_role_classes
-      role_classes - [Roles::Normal]
-    end
-
     def admin_roles
-      admin_role_classes.map(&:role_name)
+      Roles.admin.map(&:role_name)
     end
   end
 

From d6eb388cf36b8cae473583419af6ca4ef58d04ec Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:08:45 +0000
Subject: [PATCH 09/26] Extract Roles.admin_names

---
 app/controllers/invitations_controller.rb |  2 +-
 lib/roles.rb                              | 12 ++++--------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/app/controllers/invitations_controller.rb b/app/controllers/invitations_controller.rb
index b33e6651d..01bfa28d2 100644
--- a/app/controllers/invitations_controller.rb
+++ b/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.admin_names.include?(params[:role])
   end
 
   def redirect_if_invitee_already_exists
diff --git a/lib/roles.rb b/lib/roles.rb
index 6275c21d1..9cbc89927 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -23,9 +23,11 @@ def self.names
     all.map(&:role_name)
   end
 
-  def self.included(base)
-    base.extend ClassMethods
+  def self.admin_names
+    admin.map(&:role_name)
+  end
 
+  def self.included(base)
     Roles.names.each do |role_name|
       define_method("#{role_name}?") do
         role == role_name
@@ -33,12 +35,6 @@ def self.included(base)
     end
   end
 
-  module ClassMethods
-    def admin_roles
-      Roles.admin.map(&:role_name)
-    end
-  end
-
   def govuk_admin?
     [Roles::Superadmin.role_name, Roles::Admin.role_name].include? role
   end

From 981dc13d6709336cb7ad0e13b4e2c74f580e4d66 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:14:00 +0000
Subject: [PATCH 10/26] Avoid metaprogramming

---
 lib/roles.rb | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 9cbc89927..5ddfb5534 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -27,12 +27,24 @@ def self.admin_names
     admin.map(&:role_name)
   end
 
-  def self.included(base)
-    Roles.names.each do |role_name|
-      define_method("#{role_name}?") do
-        role == role_name
-      end
-    end
+  def superadmin?
+    role == Roles::Superadmin.role_name
+  end
+
+  def admin?
+    role == Roles::Admin.role_name
+  end
+
+  def super_organisation_admin?
+    role == Roles::SuperOrganisationAdmin.role_name
+  end
+
+  def organisation_admin?
+    role == Roles::OrganisationAdmin.role_name
+  end
+
+  def normal?
+    role == Roles::Normal.role_name
   end
 
   def govuk_admin?

From 894f955916003a608f97b91120db0e56718cf0b0 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:14:39 +0000
Subject: [PATCH 11/26] Remove duplication in Roles#govuk_admin?

---
 lib/roles.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 5ddfb5534..935f26b58 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -48,7 +48,7 @@ def normal?
   end
 
   def govuk_admin?
-    [Roles::Superadmin.role_name, Roles::Admin.role_name].include? role
+    superadmin? || admin?
   end
 
   def publishing_manager?

From 200d0c8d3a939db842b0cb9625d17bdd34ca6600 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:15:04 +0000
Subject: [PATCH 12/26] Remove duplication in Roles#publishing_manager?

---
 lib/roles.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 935f26b58..b95a6ddc6 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -52,6 +52,6 @@ def govuk_admin?
   end
 
   def publishing_manager?
-    [Roles::SuperOrganisationAdmin.role_name, Roles::OrganisationAdmin.role_name].include? role
+    super_organisation_admin? || organisation_admin?
   end
 end

From 0d344a8dc5a05df517fb14df1f56deb46bb0b3fe Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 12:16:33 +0000
Subject: [PATCH 13/26] TEMP: Remove roles_test while experimenting

---
 test/lib/roles_test.rb | 130 -----------------------------------------
 1 file changed, 130 deletions(-)
 delete mode 100644 test/lib/roles_test.rb

diff --git a/test/lib/roles_test.rb b/test/lib/roles_test.rb
deleted file mode 100644
index 27f3a756b..000000000
--- a/test/lib/roles_test.rb
+++ /dev/null
@@ -1,130 +0,0 @@
-require "test_helper"
-
-class RolesTest < ActiveSupport::TestCase
-  class Subject
-    include ActiveModel::Validations
-    include Roles
-
-    attr_accessor :role
-  end
-
-  context ".role_classes" do
-    should "include all role classes" do
-      expected_role_classes = [
-        Roles::Normal,
-        Roles::Admin,
-        Roles::Superadmin,
-        Roles::OrganisationAdmin,
-        Roles::SuperOrganisationAdmin,
-      ]
-
-      assert_same_elements expected_role_classes, Subject.role_classes
-    end
-  end
-
-  context ".admin_role_classes" do
-    should "only include admin role classes" do
-      expected_role_classes = [
-        Roles::Admin,
-        Roles::Superadmin,
-        Roles::OrganisationAdmin,
-        Roles::SuperOrganisationAdmin,
-      ]
-
-      assert_same_elements expected_role_classes, Subject.admin_role_classes
-    end
-  end
-
-  context ".admin_roles" do
-    should "only include admin role names" do
-      expected_role_names = [
-        Roles::Admin.role_name,
-        Roles::Superadmin.role_name,
-        Roles::OrganisationAdmin.role_name,
-        Roles::SuperOrganisationAdmin.role_name,
-      ]
-
-      assert_same_elements expected_role_names, Subject.admin_roles
-    end
-  end
-
-  context ".roles" do
-    should "order roles by their level" do
-      expected_ordered_roles = %w[
-        superadmin
-        admin
-        super_organisation_admin
-        organisation_admin
-        normal
-      ]
-
-      assert_equal expected_ordered_roles, Subject.roles
-    end
-  end
-
-  Subject.roles.each do |role|
-    context "##{role}?" do
-      setup do
-        @subject = Subject.new
-      end
-
-      should "return true if subject has the #{role} role" do
-        @subject.role = role
-        assert @subject.send("#{role}?")
-      end
-
-      should "return false if subject does not have #{role} role" do
-        @subject.role = "not-#{role}"
-        assert_not @subject.send("#{role}?")
-      end
-    end
-  end
-
-  context "#govuk_admin?" do
-    setup do
-      @subject = Subject.new
-    end
-
-    should "be true if the role is superadmin" do
-      @subject.role = Roles::Superadmin.role_name
-      assert @subject.govuk_admin?
-    end
-
-    should "be true if role is admin" do
-      @subject.role = Roles::Admin.role_name
-      assert @subject.govuk_admin?
-    end
-
-    should "be false if role is anything else" do
-      other_role_classes = Subject.role_classes - [Roles::Superadmin, Roles::Admin]
-      other_role_classes.each do |role_class|
-        @subject.role = role_class.role_name
-        assert_not @subject.govuk_admin?
-      end
-    end
-  end
-
-  context "#publishing_manager?" do
-    setup do
-      @subject = Subject.new
-    end
-
-    should "be true if the role is super_organisation_admin" do
-      @subject.role = Roles::SuperOrganisationAdmin.role_name
-      assert @subject.publishing_manager?
-    end
-
-    should "be true if role is organisation_admin" do
-      @subject.role = Roles::OrganisationAdmin.role_name
-      assert @subject.publishing_manager?
-    end
-
-    should "be false if role is anything else" do
-      other_role_classes = Subject.role_classes - [Roles::SuperOrganisationAdmin, Roles::OrganisationAdmin]
-      other_role_classes.each do |role_class|
-        @subject.role = role_class.role_name
-        assert_not @subject.publishing_manager?
-      end
-    end
-  end
-end

From d8c63dec3cc5457e19d0f801910b4636d4142334 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 14:28:20 +0000
Subject: [PATCH 14/26] Move #role_class to Roles

---
 app/models/user.rb | 4 ----
 lib/roles.rb       | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 6ef37cc26..a9f987189 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -305,10 +305,6 @@ 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)
   end
diff --git a/lib/roles.rb b/lib/roles.rb
index b95a6ddc6..ed34eb76a 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -27,6 +27,10 @@ def self.admin_names
     admin.map(&:role_name)
   end
 
+  def role_class
+    Roles.const_get(role.classify)
+  end
+
   def superadmin?
     role == Roles::Superadmin.role_name
   end

From 2cd32d945bbcff66198689a2c09c88d11fe2d1d5 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 14:40:26 +0000
Subject: [PATCH 15/26] Extract Roles::<class>.can_manage?

---
 app/models/user.rb                    | 2 +-
 lib/roles/admin.rb                    | 4 ++++
 lib/roles/normal.rb                   | 4 ++++
 lib/roles/organisation_admin.rb       | 4 ++++
 lib/roles/super_organisation_admin.rb | 4 ++++
 lib/roles/superadmin.rb               | 4 ++++
 6 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index a9f987189..38762eee7 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -306,7 +306,7 @@ def not_setup_2sv?
   end
 
   def can_manage?(other_user)
-    manageable_roles.include?(other_user.role)
+    role_class.can_manage?(other_user.role)
   end
 
   def manageable_organisations
diff --git a/lib/roles/admin.rb b/lib/roles/admin.rb
index 5b90c18b7..c88b01c9c 100644
--- a/lib/roles/admin.rb
+++ b/lib/roles/admin.rb
@@ -23,6 +23,10 @@ def self.manageable_roles
       %w[normal organisation_admin super_organisation_admin admin]
     end
 
+    def self.can_manage?(other_role)
+      manageable_roles.include?(other_role)
+    end
+
     def self.manageable_organisations_for(_)
       Organisation.all
     end
diff --git a/lib/roles/normal.rb b/lib/roles/normal.rb
index a3e4bbe73..7bab1856d 100644
--- a/lib/roles/normal.rb
+++ b/lib/roles/normal.rb
@@ -12,6 +12,10 @@ def self.manageable_roles
       []
     end
 
+    def self.can_manage?(other_role)
+      manageable_roles.include?(other_role)
+    end
+
     def self.manageable_organisations_for(_)
       Organisation.where("false")
     end
diff --git a/lib/roles/organisation_admin.rb b/lib/roles/organisation_admin.rb
index e8a228e3c..08d78bc54 100644
--- a/lib/roles/organisation_admin.rb
+++ b/lib/roles/organisation_admin.rb
@@ -22,6 +22,10 @@ def self.manageable_roles
       %w[normal organisation_admin]
     end
 
+    def self.can_manage?(other_role)
+      manageable_roles.include?(other_role)
+    end
+
     def self.manageable_organisations_for(user)
       Organisation.where(id: user.organisation)
     end
diff --git a/lib/roles/super_organisation_admin.rb b/lib/roles/super_organisation_admin.rb
index 49c486fc6..60c95e5d0 100644
--- a/lib/roles/super_organisation_admin.rb
+++ b/lib/roles/super_organisation_admin.rb
@@ -22,6 +22,10 @@ def self.manageable_roles
       %w[normal organisation_admin super_organisation_admin]
     end
 
+    def self.can_manage?(other_role)
+      manageable_roles.include?(other_role)
+    end
+
     def self.manageable_organisations_for(user)
       Organisation.where(id: user.organisation.subtree)
     end
diff --git a/lib/roles/superadmin.rb b/lib/roles/superadmin.rb
index ea0b5d318..3190ebaf5 100644
--- a/lib/roles/superadmin.rb
+++ b/lib/roles/superadmin.rb
@@ -24,6 +24,10 @@ def self.manageable_roles
       Roles.names
     end
 
+    def self.can_manage?(other_role)
+      manageable_roles.include?(other_role)
+    end
+
     def self.manageable_organisations_for(_)
       Organisation.all
     end

From 8800e3f29ee8ee18ad6bf0717f8294722e61a689 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 14:42:25 +0000
Subject: [PATCH 16/26] Extract Roles::Base.can_manage?

---
 lib/roles/admin.rb                    | 6 +-----
 lib/roles/base.rb                     | 7 +++++++
 lib/roles/normal.rb                   | 6 +-----
 lib/roles/organisation_admin.rb       | 6 +-----
 lib/roles/super_organisation_admin.rb | 6 +-----
 lib/roles/superadmin.rb               | 6 +-----
 6 files changed, 12 insertions(+), 25 deletions(-)
 create mode 100644 lib/roles/base.rb

diff --git a/lib/roles/admin.rb b/lib/roles/admin.rb
index c88b01c9c..2ef3eef58 100644
--- a/lib/roles/admin.rb
+++ b/lib/roles/admin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class Admin
+  class Admin < Base
     def self.permitted_user_params
       [
         :uid,
@@ -23,10 +23,6 @@ def self.manageable_roles
       %w[normal organisation_admin super_organisation_admin admin]
     end
 
-    def self.can_manage?(other_role)
-      manageable_roles.include?(other_role)
-    end
-
     def self.manageable_organisations_for(_)
       Organisation.all
     end
diff --git a/lib/roles/base.rb b/lib/roles/base.rb
new file mode 100644
index 000000000..774658e51
--- /dev/null
+++ b/lib/roles/base.rb
@@ -0,0 +1,7 @@
+module Roles
+  class Base
+    def self.can_manage?(other_role)
+      manageable_roles.include?(other_role)
+    end
+  end
+end
diff --git a/lib/roles/normal.rb b/lib/roles/normal.rb
index 7bab1856d..6025463c2 100644
--- a/lib/roles/normal.rb
+++ b/lib/roles/normal.rb
@@ -1,5 +1,5 @@
 module Roles
-  class Normal
+  class Normal < Base
     def self.permitted_user_params
       %i[uid name email password password_confirmation]
     end
@@ -12,10 +12,6 @@ def self.manageable_roles
       []
     end
 
-    def self.can_manage?(other_role)
-      manageable_roles.include?(other_role)
-    end
-
     def self.manageable_organisations_for(_)
       Organisation.where("false")
     end
diff --git a/lib/roles/organisation_admin.rb b/lib/roles/organisation_admin.rb
index 08d78bc54..366b0295f 100644
--- a/lib/roles/organisation_admin.rb
+++ b/lib/roles/organisation_admin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class OrganisationAdmin
+  class OrganisationAdmin < Base
     def self.permitted_user_params
       [
         :uid,
@@ -22,10 +22,6 @@ def self.manageable_roles
       %w[normal organisation_admin]
     end
 
-    def self.can_manage?(other_role)
-      manageable_roles.include?(other_role)
-    end
-
     def self.manageable_organisations_for(user)
       Organisation.where(id: user.organisation)
     end
diff --git a/lib/roles/super_organisation_admin.rb b/lib/roles/super_organisation_admin.rb
index 60c95e5d0..ee9d13364 100644
--- a/lib/roles/super_organisation_admin.rb
+++ b/lib/roles/super_organisation_admin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class SuperOrganisationAdmin
+  class SuperOrganisationAdmin < Base
     def self.permitted_user_params
       [
         :uid,
@@ -22,10 +22,6 @@ def self.manageable_roles
       %w[normal organisation_admin super_organisation_admin]
     end
 
-    def self.can_manage?(other_role)
-      manageable_roles.include?(other_role)
-    end
-
     def self.manageable_organisations_for(user)
       Organisation.where(id: user.organisation.subtree)
     end
diff --git a/lib/roles/superadmin.rb b/lib/roles/superadmin.rb
index 3190ebaf5..6c2d72330 100644
--- a/lib/roles/superadmin.rb
+++ b/lib/roles/superadmin.rb
@@ -1,5 +1,5 @@
 module Roles
-  class Superadmin
+  class Superadmin < Base
     def self.permitted_user_params
       [
         :uid,
@@ -24,10 +24,6 @@ def self.manageable_roles
       Roles.names
     end
 
-    def self.can_manage?(other_role)
-      manageable_roles.include?(other_role)
-    end
-
     def self.manageable_organisations_for(_)
       Organisation.all
     end

From 33c50fbedc15df53fbca5498bd1ed1857ac9d8a9 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 14:53:29 +0000
Subject: [PATCH 17/26] Remove explicit requires

These should not be necessary now that we reference the classes directly
from the Roles module.
---
 lib/roles.rb | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index ed34eb76a..6c527038e 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -1,9 +1,3 @@
-require "roles/superadmin"
-require "roles/admin"
-require "roles/super_organisation_admin"
-require "roles/organisation_admin"
-require "roles/normal"
-
 module Roles
   def self.all
     [

From 85ca5d204e40fb27d6ffe3a81f0c98c1d8808ae8 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:03:04 +0000
Subject: [PATCH 18/26] Use User#govuk_admin? where possible

---
 app/policies/user_policy.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
index 9843ca486..49897c8c5 100644
--- a/app/policies/user_policy.rb
+++ b/app/policies/user_policy.rb
@@ -1,11 +1,11 @@
 class UserPolicy < BasePolicy
   def index?
-    %w[superadmin admin super_organisation_admin organisation_admin].include? current_user.role
+    current_user.govuk_admin? || %w[super_organisation_admin organisation_admin].include? current_user.role
   end
 
   # invitations#new
   def new?
-    %w[superadmin admin].include? current_user.role
+    current_user.govuk_admin?
   end
   alias_method :assign_organisation?, :new?
 

From 3cd82da213d0beaa34f5fbc58b93afb069e5ac7a Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:05:01 +0000
Subject: [PATCH 19/26] Use User#publishing_manager? where possible

---
 app/models/doorkeeper/application.rb | 2 +-
 app/models/user.rb                   | 2 +-
 app/policies/user_policy.rb          | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb
index dd2f1f5ca..ad1211248 100644
--- a/app/models/doorkeeper/application.rb
+++ b/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)
diff --git a/app/models/user.rb b/app/models/user.rb
index 38762eee7..c2f15a1ed 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -428,7 +428,7 @@ def user_can_be_exempted_from_2sv
   end
 
   def organisation_admin_belongs_to_organisation
-    if %w[organisation_admin super_organisation_admin].include?(role) && organisation_id.blank?
+    if publishing_manager? && organisation_id.blank?
       errors.add(:organisation_id, "can't be 'None' for #{role.titleize}")
     end
   end
diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
index 49897c8c5..85190869a 100644
--- a/app/policies/user_policy.rb
+++ b/app/policies/user_policy.rb
@@ -1,6 +1,6 @@
 class UserPolicy < BasePolicy
   def index?
-    current_user.govuk_admin? || %w[super_organisation_admin organisation_admin].include? current_user.role
+    current_user.govuk_admin? || current_user.publishing_manager?
   end
 
   # invitations#new

From 7580e967a5a47a78b820ce27188b3b7ab9108d41 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:34:43 +0000
Subject: [PATCH 20/26] Extract Roles.find

---
 lib/roles.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index 6c527038e..a0c7c60f9 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -9,6 +9,10 @@ def self.all
     ]
   end
 
+  def self.find(role_name)
+    Roles.const_get(role_name.classify)
+  end
+
   def self.admin
     all - [Roles::Normal]
   end
@@ -22,7 +26,7 @@ def self.admin_names
   end
 
   def role_class
-    Roles.const_get(role.classify)
+    Roles.find(role)
   end
 
   def superadmin?

From 592d8e53ee61e88e4c1b4afad4315ba5d384364b Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:37:54 +0000
Subject: [PATCH 21/26] Add #require_2sv? to roles

---
 lib/roles/admin.rb                    | 4 ++++
 lib/roles/normal.rb                   | 4 ++++
 lib/roles/organisation_admin.rb       | 4 ++++
 lib/roles/super_organisation_admin.rb | 4 ++++
 lib/roles/superadmin.rb               | 4 ++++
 5 files changed, 20 insertions(+)

diff --git a/lib/roles/admin.rb b/lib/roles/admin.rb
index 2ef3eef58..a3a16262a 100644
--- a/lib/roles/admin.rb
+++ b/lib/roles/admin.rb
@@ -26,5 +26,9 @@ def self.manageable_roles
     def self.manageable_organisations_for(_)
       Organisation.all
     end
+
+    def self.require_2sv?
+      true
+    end
   end
 end
diff --git a/lib/roles/normal.rb b/lib/roles/normal.rb
index 6025463c2..65fa6a6fb 100644
--- a/lib/roles/normal.rb
+++ b/lib/roles/normal.rb
@@ -15,5 +15,9 @@ def self.manageable_roles
     def self.manageable_organisations_for(_)
       Organisation.where("false")
     end
+
+    def self.require_2sv?
+      false
+    end
   end
 end
diff --git a/lib/roles/organisation_admin.rb b/lib/roles/organisation_admin.rb
index 366b0295f..dc0a73a3e 100644
--- a/lib/roles/organisation_admin.rb
+++ b/lib/roles/organisation_admin.rb
@@ -25,5 +25,9 @@ def self.manageable_roles
     def self.manageable_organisations_for(user)
       Organisation.where(id: user.organisation)
     end
+
+    def self.require_2sv?
+      true
+    end
   end
 end
diff --git a/lib/roles/super_organisation_admin.rb b/lib/roles/super_organisation_admin.rb
index ee9d13364..d0053b1d9 100644
--- a/lib/roles/super_organisation_admin.rb
+++ b/lib/roles/super_organisation_admin.rb
@@ -25,5 +25,9 @@ def self.manageable_roles
     def self.manageable_organisations_for(user)
       Organisation.where(id: user.organisation.subtree)
     end
+
+    def self.require_2sv?
+      true
+    end
   end
 end
diff --git a/lib/roles/superadmin.rb b/lib/roles/superadmin.rb
index 6c2d72330..d51cd87ea 100644
--- a/lib/roles/superadmin.rb
+++ b/lib/roles/superadmin.rb
@@ -27,5 +27,9 @@ def self.manageable_roles
     def self.manageable_organisations_for(_)
       Organisation.all
     end
+
+    def self.require_2sv?
+      true
+    end
   end
 end

From 60b42f31ffe21db91147f7cc43e4b99487466290 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:38:30 +0000
Subject: [PATCH 22/26] Use Roles::<class>.require_2sv?

Have to fallback to `false` in `invitee_requires_2sv` to prevent a
database constraint violation attempting to insert `null` into
`require_2sv`.
---
 app/controllers/invitations_controller.rb | 2 +-
 app/models/user.rb                        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/invitations_controller.rb b/app/controllers/invitations_controller.rb
index 01bfa28d2..9a38f7d0c 100644
--- a/app/controllers/invitations_controller.rb
+++ b/app/controllers/invitations_controller.rb
@@ -62,7 +62,7 @@ def organisation(params)
   end
 
   def invitee_requires_2sv(params)
-    organisation(params)&.require_2sv? || Roles.admin_names.include?(params[:role])
+    organisation(params)&.require_2sv? || Roles.find(params[:role])&.require_2sv? || false
   end
 
   def redirect_if_invitee_already_exists
diff --git a/app/models/user.rb b/app/models/user.rb
index c2f15a1ed..a83319671 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -325,7 +325,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_class.require_2sv?
   end
 
   def reset_2sv_exemption_reason
@@ -424,7 +424,7 @@ 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, "#{role} users cannot be exempted from 2SV. Remove the user's exemption to change their role.") if exempt_from_2sv? && role_class.require_2sv?
   end
 
   def organisation_admin_belongs_to_organisation

From 8288ff90776a07af72262741a34cf9fc9ab53da1 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:39:51 +0000
Subject: [PATCH 23/26] Remove Roles.admin and .admin_names

No longer used.
---
 lib/roles.rb | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index a0c7c60f9..ebc80b423 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -13,18 +13,10 @@ def self.find(role_name)
     Roles.const_get(role_name.classify)
   end
 
-  def self.admin
-    all - [Roles::Normal]
-  end
-
   def self.names
     all.map(&:role_name)
   end
 
-  def self.admin_names
-    admin.map(&:role_name)
-  end
-
   def role_class
     Roles.find(role)
   end

From 2bc23818c2bffa06ff1f2da7fec555f23599024b Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:43:17 +0000
Subject: [PATCH 24/26] Simplify Roles.find

---
 lib/roles.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index ebc80b423..a12f3a7e3 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -10,7 +10,7 @@ def self.all
   end
 
   def self.find(role_name)
-    Roles.const_get(role_name.classify)
+    all.find { |role| role.role_name == role_name }
   end
 
   def self.names

From 8d6a54470a900c38304fb701e82901154361c2ab Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 15:54:04 +0000
Subject: [PATCH 25/26] Use metaprogramming to define predicate methods

---
 lib/roles.rb | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/lib/roles.rb b/lib/roles.rb
index a12f3a7e3..31c7deae1 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -21,24 +21,10 @@ def role_class
     Roles.find(role)
   end
 
-  def superadmin?
-    role == Roles::Superadmin.role_name
-  end
-
-  def admin?
-    role == Roles::Admin.role_name
-  end
-
-  def super_organisation_admin?
-    role == Roles::SuperOrganisationAdmin.role_name
-  end
-
-  def organisation_admin?
-    role == Roles::OrganisationAdmin.role_name
-  end
-
-  def normal?
-    role == Roles::Normal.role_name
+  all.each do |role_class|
+    define_method("#{role_class.role_name}?") do
+      role == role_class.role_name
+    end
   end
 
   def govuk_admin?

From f9035b421802017dd6df4c1f99f7105fc1482c31 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 25 Jan 2024 16:54:48 +0000
Subject: [PATCH 26/26] Return Role class from User#role

---
 app/controllers/api_users_controller.rb        |  2 +-
 app/controllers/users_controller.rb            |  2 +-
 app/helpers/users_helper.rb                    |  2 +-
 app/models/batch_invitation_user.rb            |  2 +-
 app/models/user.rb                             | 18 +++++++++++-------
 app/policies/user_policy.rb                    |  2 +-
 app/presenters/user_export_presenter.rb        |  2 +-
 app/views/account/roles/edit.html.erb          |  2 +-
 app/views/devise/invitations/new.html.erb      |  2 +-
 .../users_with_access.html.erb                 |  2 +-
 app/views/users/index.html.erb                 |  2 +-
 app/views/users/roles/edit.html.erb            |  4 ++--
 lib/roles.rb                                   |  2 +-
 lib/roles/base.rb                              |  2 +-
 .../controllers/invitations_controller_test.rb |  4 ++--
 .../controllers/users/roles_controller_test.rb |  8 ++++----
 test/integration/change_user_role_test.rb      |  4 ++--
 test/lib/tasks/permissions_promoter_test.rb    |  6 +++---
 test/models/user_test.rb                       | 12 ++++++------
 19 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/app/controllers/api_users_controller.rb b/app/controllers/api_users_controller.rb
index 0ccf00250..e0a58c9b3 100644
--- a/app/controllers/api_users_controller.rb
+++ b/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.role_name.to_sym,
     ).sanitise
   end
 
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index dc8587e91..a9a414f1e 100644
--- a/app/controllers/users_controller.rb
+++ b/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.role_name.to_sym,
     ).sanitise
   end
 
diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb
index 894e69f6d..741988605 100644
--- a/app/helpers/users_helper.rb
+++ b/app/helpers/users_helper.rb
@@ -134,7 +134,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.role_name.humanize.capitalize }
     item[:edit] = { href: edit_user_role_path(user) } if policy(user).assign_role?
     item
   end
diff --git a/app/models/batch_invitation_user.rb b/app/models/batch_invitation_user.rb
index c487e207a..2550c84c9 100644
--- a/app/models/batch_invitation_user.rb
+++ b/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.role_name.to_sym,
     ).sanitise
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index a83319671..830588cdd 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -43,14 +43,14 @@ 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
 
   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.names }
+  validates :role, inclusion: { in: Roles.all }
   validate :user_can_be_exempted_from_2sv
   validate :organisation_admin_belongs_to_organisation
   validate :email_is_ascii_only
@@ -127,6 +127,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
 
@@ -306,11 +310,11 @@ def not_setup_2sv?
   end
 
   def can_manage?(other_user)
-    role_class.can_manage?(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
@@ -325,7 +329,7 @@ def need_two_step_verification?
   def set_2sv_for_admin_roles
     return unless GovukEnvironment.production?
 
-    self.require_2sv = true if role_changed? && role_class.require_2sv?
+    self.require_2sv = true if role_changed? && role.require_2sv?
   end
 
   def reset_2sv_exemption_reason
@@ -424,12 +428,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? && role_class.require_2sv?
+    errors.add(:reason_for_2sv_exemption, "#{role.role_name} users cannot be exempted from 2SV. Remove the user's exemption to change their role.") if exempt_from_2sv? && role.require_2sv?
   end
 
   def organisation_admin_belongs_to_organisation
     if publishing_manager? && organisation_id.blank?
-      errors.add(:organisation_id, "can't be 'None' for #{role.titleize}")
+      errors.add(:organisation_id, "can't be 'None' for #{role.role_name.titleize}")
     end
   end
 
diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
index 85190869a..0cbabfc36 100644
--- a/app/policies/user_policy.rb
+++ b/app/policies/user_policy.rb
@@ -13,7 +13,7 @@ def new?
   alias_method :create?, :new?
 
   def edit?
-    case current_user.role
+    case current_user.role.role_name
     when Roles::Superadmin.role_name
       true
     when Roles::Admin.role_name
diff --git a/app/presenters/user_export_presenter.rb b/app/presenters/user_export_presenter.rb
index a4789d4f6..72289209e 100644
--- a/app/presenters/user_export_presenter.rb
+++ b/app/presenters/user_export_presenter.rb
@@ -35,7 +35,7 @@ def row(user)
     [
       user.name,
       user.email,
-      user.role.humanize,
+      user.role.role_name.humanize,
       user.organisation.try(:name),
       user.sign_in_count,
       user.current_sign_in_at.try(:to_formatted_s, :db),
diff --git a/app/views/account/roles/edit.html.erb b/app/views/account/roles/edit.html.erb
index 45dc98380..beb5a77c7 100644
--- a/app/views/account/roles/edit.html.erb
+++ b/app/views/account/roles/edit.html.erb
@@ -32,7 +32,7 @@
       <% end %>
     <% else %>
       <%= render "govuk_publishing_components/components/inset_text", {
-        text: current_user.role.humanize,
+        text: current_user.role.role_name.humanize,
       } %>
     <% end %>
   </div>
diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb
index 89e8a0fc1..2edd5d93b 100644
--- a/app/views/devise/invitations/new.html.erb
+++ b/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.role_name),
         } %>
       <% end %>
 
diff --git a/app/views/doorkeeper_applications/users_with_access.html.erb b/app/views/doorkeeper_applications/users_with_access.html.erb
index d7f17e255..fa66bf94b 100644
--- a/app/views/doorkeeper_applications/users_with_access.html.erb
+++ b/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.role_name.humanize },
       { text: user.organisation_name },
       { text: user.sign_in_count },
       { text: formatted_last_sign_in(user) },
diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb
index 01fca829a..611e5c9d8 100644
--- a/app/views/users/index.html.erb
+++ b/app/views/users/index.html.erb
@@ -39,7 +39,7 @@
           [
             { text: user_name(user) },
             { text: user.email },
-            { text: user.role.humanize },
+            { text: user.role.role_name.humanize },
             { text: status(user) },
             { text: two_step_status(user) },
           ]
diff --git a/app/views/users/roles/edit.html.erb b/app/views/users/roles/edit.html.erb
index 1ac254240..5b10cd324 100644
--- a/app/views/users/roles/edit.html.erb
+++ b/app/views/users/roles/edit.html.erb
@@ -40,7 +40,7 @@
     <%= 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.role_name.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.",
         } %>
       <% else %>
         <%= render "govuk_publishing_components/components/select", {
@@ -48,7 +48,7 @@
           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.humanize, value: role, selected: @user.role&.role_name == role } },
           error_message: @user.errors[:role].any? ? @user.errors.full_messages_for(:role).to_sentence : nil
         } %>
         <div class="govuk-button-group">
diff --git a/lib/roles.rb b/lib/roles.rb
index 31c7deae1..fb3658e8d 100644
--- a/lib/roles.rb
+++ b/lib/roles.rb
@@ -23,7 +23,7 @@ def role_class
 
   all.each do |role_class|
     define_method("#{role_class.role_name}?") do
-      role == role_class.role_name
+      role == role_class
     end
   end
 
diff --git a/lib/roles/base.rb b/lib/roles/base.rb
index 774658e51..448cd13dc 100644
--- a/lib/roles/base.rb
+++ b/lib/roles/base.rb
@@ -1,7 +1,7 @@
 module Roles
   class Base
     def self.can_manage?(other_role)
-      manageable_roles.include?(other_role)
+      manageable_roles.include?(other_role.role_name)
     end
   end
 end
diff --git a/test/controllers/invitations_controller_test.rb b/test/controllers/invitations_controller_test.rb
index 7f56fb42e..b9361e3d7 100644
--- a/test/controllers/invitations_controller_test.rb
+++ b/test/controllers/invitations_controller_test.rb
@@ -198,7 +198,7 @@ class InvitationsControllerTest < ActionController::TestCase
         assert_equal "invitee", invitee.name
         assert_equal "invitee@gov.uk", invitee.email
         assert_equal @organisation, invitee.organisation
-        assert_equal Roles::OrganisationAdmin.role_name, invitee.role
+        assert_equal Roles::OrganisationAdmin, invitee.role
         assert_equal [permission], invitee.supported_permissions
       end
 
@@ -439,7 +439,7 @@ class InvitationsControllerTest < ActionController::TestCase
 
         invitee = User.last
         assert invitee.present?
-        default_role = Roles::Normal.role_name
+        default_role = Roles::Normal
         assert_equal default_role, invitee.role
       end
     end
diff --git a/test/controllers/users/roles_controller_test.rb b/test/controllers/users/roles_controller_test.rb
index 51d48f2cf..6ba748f94 100644
--- a/test/controllers/users/roles_controller_test.rb
+++ b/test/controllers/users/roles_controller_test.rb
@@ -98,7 +98,7 @@ class Users::RolesControllerTest < ActionController::TestCase
 
         put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } }
 
-        assert_equal Roles::OrganisationAdmin.role_name, user.reload.role
+        assert_equal Roles::OrganisationAdmin, user.reload.role
       end
 
       should "record account updated & role change events" do
@@ -154,7 +154,7 @@ class Users::RolesControllerTest < ActionController::TestCase
 
         put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } }
 
-        assert_equal Roles::OrganisationAdmin.role_name, user.reload.role
+        assert_equal Roles::OrganisationAdmin, user.reload.role
       end
 
       should "not update user role if UserPolicy#assign_role? returns false" do
@@ -164,7 +164,7 @@ class Users::RolesControllerTest < ActionController::TestCase
 
         put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } }
 
-        assert_equal Roles::Normal.role_name, user.reload.role
+        assert_equal Roles::Normal, user.reload.role
         assert_not_authorised
       end
 
@@ -173,7 +173,7 @@ class Users::RolesControllerTest < ActionController::TestCase
 
         put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } }
 
-        assert_equal Roles::Normal.role_name, user.reload.role
+        assert_equal Roles::Normal, user.reload.role
 
         assert_select ".govuk-error-summary" do
           assert_select "li", text: /#{Roles::OrganisationAdmin.role_name} users cannot be exempted from 2SV/
diff --git a/test/integration/change_user_role_test.rb b/test/integration/change_user_role_test.rb
index 5789a494d..4a94af09f 100644
--- a/test/integration/change_user_role_test.rb
+++ b/test/integration/change_user_role_test.rb
@@ -31,7 +31,7 @@ def sign_in_as_and_edit_user(sign_in_as, user_to_edit)
 
       assert page.has_no_select?("Role")
 
-      assert page.has_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."
+      assert page.has_text? "This user's role is set to #{user.role.role_name.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."
     end
   end
 
@@ -41,7 +41,7 @@ def sign_in_as_and_edit_user(sign_in_as, user_to_edit)
       sign_in_as_and_edit_user(create(:admin_user, organisation: user.organisation), user)
 
       assert page.has_no_select?("Role")
-      assert page.has_no_text? "This user's role is set to #{user.role}. They are currently exempted from 2sv, meaning that their role cannot be changed as admins are required to have 2sv."
+      assert page.has_no_text? "This user's role is set to #{user.role.role_name.humanize}. They are currently exempted from 2sv, meaning that their role cannot be changed as admins are required to have 2sv."
     end
   end
 end
diff --git a/test/lib/tasks/permissions_promoter_test.rb b/test/lib/tasks/permissions_promoter_test.rb
index 73e76c658..50b10dc6b 100644
--- a/test/lib/tasks/permissions_promoter_test.rb
+++ b/test/lib/tasks/permissions_promoter_test.rb
@@ -36,7 +36,7 @@ class PermissionsPromoterTest < ActiveSupport::TestCase
 
     @task.invoke
 
-    assert non_gds_user.reload.role == "normal"
+    assert non_gds_user.reload.role == Roles::Normal
   end
 
   should "not update a non-GDS user who already has an admin role" do
@@ -44,7 +44,7 @@ class PermissionsPromoterTest < ActiveSupport::TestCase
 
     @task.invoke
 
-    assert admin_user.reload.role == Roles::Admin.role_name
+    assert admin_user.reload.role == Roles::Admin
   end
 
   should "not update a non-GDS user who is suspended" do
@@ -53,7 +53,7 @@ class PermissionsPromoterTest < ActiveSupport::TestCase
 
     @task.invoke
 
-    assert user.reload.role == "normal"
+    assert user.reload.role == Roles::Normal
   end
 
   should "not update GDS users" do
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 21971d2c3..9a90c1ed1 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -858,13 +858,13 @@ def setup
     end
   end
 
-  context "#role_class" do
+  context "#role" do
     should "return the role class" do
-      assert_equal Roles::Normal, build(:user).role_class
-      assert_equal Roles::OrganisationAdmin, build(:organisation_admin_user).role_class
-      assert_equal Roles::SuperOrganisationAdmin, build(:super_organisation_admin_user).role_class
-      assert_equal Roles::Admin, build(:admin_user).role_class
-      assert_equal Roles::Superadmin, build(:superadmin_user).role_class
+      assert_equal Roles::Normal, build(:user).role
+      assert_equal Roles::OrganisationAdmin, build(:organisation_admin_user).role
+      assert_equal Roles::SuperOrganisationAdmin, build(:super_organisation_admin_user).role
+      assert_equal Roles::Admin, build(:admin_user).role
+      assert_equal Roles::Superadmin, build(:superadmin_user).role
     end
   end