diff --git a/app/components/npq_separation/navigation_structures/admin_navigation_structure.rb b/app/components/npq_separation/navigation_structures/admin_navigation_structure.rb index 170ccfc6ec..b7de7cb204 100644 --- a/app/components/npq_separation/navigation_structures/admin_navigation_structure.rb +++ b/app/components/npq_separation/navigation_structures/admin_navigation_structure.rb @@ -57,9 +57,9 @@ def structure prefix: "/npq-separation/admin/lead_providers", ) => [], Node.new( - name: "Settings", - href: "#", - prefix: "/npq-separation/admin/settings", + name: "Admins", + href: npq_separation_admin_admins_path, + prefix: "/npq-separation/admin/admins", ) => [], } end diff --git a/app/controllers/npq_separation/admin/admins_controller.rb b/app/controllers/npq_separation/admin/admins_controller.rb index 0ee251e99c..3a75dddb85 100644 --- a/app/controllers/npq_separation/admin/admins_controller.rb +++ b/app/controllers/npq_separation/admin/admins_controller.rb @@ -1,3 +1,61 @@ -class NpqSeparation::Admin::AdminsController < ApplicationController - def index = head(:method_not_allowed) +class NpqSeparation::Admin::AdminsController < NpqSeparation::AdminController + before_action :require_super_admin + + def index + @admins = Admin.all + end + + def new + @admin = Admin.new + end + + def create + @admin = Admin.new(admin_params) + + if @admin.save + flash[:success] = t(".success", email: @admin.email) + redirect_to action: :index + else + render :new, status: :unprocessable_entity + end + end + + def update + @admin = Admin.find(params[:id]) + + if !@admin.super_admin? && @admin.update(super_admin: true) + flash[:success] = t(".success", email: @admin.email) + else + flash[:error] = t(".failure", email: @admin.email) + end + + redirect_to action: :index + end + + def destroy + @admin = Admin.find(params[:id]) + + if @admin.super_admin? + flash[:error] = t(".forbidden") + elsif @admin.destroy + flash[:success] = t(".success", email: @admin.email) + else + flash[:error] = t(".failure", email: @admin.email) + end + + redirect_to action: :index + end + +private + + def require_super_admin + unless current_admin.super_admin? + flash[:negative] = { title: "Unauthorized", text: "Sign in with your admininstrator account" } + redirect_to sign_in_path + end + end + + def admin_params + params.require(:admin).permit(:email, :full_name) + end end diff --git a/app/helpers/admin_management_helper.rb b/app/helpers/admin_management_helper.rb index db8881ac01..394ffc4afa 100644 --- a/app/helpers/admin_management_helper.rb +++ b/app/helpers/admin_management_helper.rb @@ -20,4 +20,16 @@ def elevate_to_super_admin_cell_contents(user) govuk_link_to(t(".buttons.elevate"), admin_super_admin_path(user), method: :patch) end + + def npq_separation_destroy_admin_cell_contents(admin) + return "" if admin.super_admin? + + govuk_button_link_to(t(".buttons.delete"), npq_separation_admin_admin_path(admin), method: :delete, warning: true) + end + + def npq_separation_elevate_to_super_admin_cell_contents(admin) + return "" if admin.super_admin? + + govuk_link_to(t(".buttons.elevate"), npq_separation_admin_admin_path(admin), method: :patch) + end end diff --git a/app/models/admin.rb b/app/models/admin.rb index d5cf7e26bc..67da23c551 100644 --- a/app/models/admin.rb +++ b/app/models/admin.rb @@ -5,7 +5,8 @@ class Admin < ApplicationRecord validates :email, presence: { message: "Enter an email address" }, - length: { maximum: 64, message: "Email must be shorter than 64 characters" } + length: { maximum: 64, message: "Email must be shorter than 64 characters" }, + uniqueness: { message: "An admin with this email address already exists" } # Whether this user has admin access to the feature flagging interface def flipper_access? diff --git a/app/views/npq_separation/admin/admins/index.html.erb b/app/views/npq_separation/admin/admins/index.html.erb new file mode 100644 index 0000000000..33467dd649 --- /dev/null +++ b/app/views/npq_separation/admin/admins/index.html.erb @@ -0,0 +1,33 @@ +

Admins

+ +<%= govuk_button_link_to("Add new admin", new_npq_separation_admin_admin_path) %> + +<%= govuk_details(summary_text: t('.super_admins.summary_text'), text: t('.super_admins.text_html')) %> + +<%= + govuk_table do |table| + table.with_head do |head| + head.with_row do |row| + row.with_cell(header: true, text: "Name") + row.with_cell(header: true, text: "Email") + row.with_cell(header: true, text: "") + row.with_cell(header: true, text: "") + row.with_cell(header: true, text: "") + end + end + + table.with_body do |body| + @admins.each do |user| + body.with_row do |row| + row.with_cell(text: user.full_name.presence || "-") + row.with_cell(text: user.email.presence || "No Email Found") + row.with_cell(text: admin_type_cell_contents(user)) + row.with_cell(text: npq_separation_destroy_admin_cell_contents(user)) + row.with_cell(text: npq_separation_elevate_to_super_admin_cell_contents(user)) + end + end + end + end +%> + +<%== govuk_pagination(pagy: @pagy) %> diff --git a/app/views/npq_separation/admin/admins/new.html.erb b/app/views/npq_separation/admin/admins/new.html.erb new file mode 100644 index 0000000000..c1338e43a0 --- /dev/null +++ b/app/views/npq_separation/admin/admins/new.html.erb @@ -0,0 +1,22 @@ +<% content_for :before_content do %> + <%= render GovukComponent::BackLinkComponent.new( + text: "Back", + href: :back + ) %> +<% end %> + +

<%= t(".title") %>

+ +<%= form_with model: @admin, url: npq_separation_admin_admins_path do |f| %> + <%= f.govuk_error_summary %> + + <%= f.govuk_email_field :email, + label: { text: "Email address" }, + width: "three-quarters" %> + + <%= f.govuk_email_field :full_name, + label: { text: "Full name" }, + width: "three-quarters" %> + + <%= f.govuk_submit "Add admin" %> +<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 877f26a288..a7e95eefa9 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -468,7 +468,7 @@ en: update: success: Super admin permissions granted to %{email} failure: Failed to grant super admin permissions to %{email}, please contact technical support if this problem persists. - admins: + admins: &admins new: title: "Add a new admin" index: @@ -575,7 +575,7 @@ en: not_eligible_for_funding: "If your provider does not confirm you've started the course before %{date}, your registration will expire. You can register again later." helpers: - warnings: + warnings: registration_wizard: check_provider_is_open: "Before selecting your provider, you must check that they are currently open to accepting NPQ applications." title: @@ -807,3 +807,18 @@ en: national_insurance_number: "National Insurance number (optional)" date_of_birth: What is your date of birth? + + npq_separation: + admin: + admins: + <<: *admins + create: + success: Created admin %{email} + failure: Failed to create admin for %{email}, please contact technical support if this problem persists. + update: + success: Super admin permissions granted to %{email} + failure: Failed to grant super admin permissions to %{email}, please contact technical support if this problem persists. + destroy: + forbidden: You cannot remove admin permissions from yourself or another super admin. + success: Deleted admin %{email} + failure: Failed to remove admin permissions from %{email}, please contact technical support if this problem persists. diff --git a/config/routes.rb b/config/routes.rb index 9860b2b55e..1def27eedc 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -244,7 +244,7 @@ end resources :lead_providers, only: %i[index show], path: "lead-providers" - resources :admins, only: %i[index] + resources :admins, except: :edit end end diff --git a/db/migrate/20241001141442_add_unique_index_to_admins.rb b/db/migrate/20241001141442_add_unique_index_to_admins.rb new file mode 100644 index 0000000000..5d500319e7 --- /dev/null +++ b/db/migrate/20241001141442_add_unique_index_to_admins.rb @@ -0,0 +1,5 @@ +class AddUniqueIndexToAdmins < ActiveRecord::Migration[7.1] + def change + add_index :admins, :email, unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 99e6edfc6a..0d77360fd6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_09_18_145749) do +ActiveRecord::Schema[7.1].define(version: 2024_10_01_141442) do # These are extensions that must be enabled in order to support this database enable_extension "btree_gin" enable_extension "citext" @@ -39,6 +39,7 @@ t.datetime "otp_expires_at", precision: nil t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.index ["email"], name: "index_admins_on_email", unique: true end create_table "api_tokens", force: :cascade do |t| diff --git a/spec/components/npq_separation/navigation_structures/admin_navigation_structure_spec.rb b/spec/components/npq_separation/navigation_structures/admin_navigation_structure_spec.rb index 57d8aaeeeb..717bd2b7e4 100644 --- a/spec/components/npq_separation/navigation_structures/admin_navigation_structure_spec.rb +++ b/spec/components/npq_separation/navigation_structures/admin_navigation_structure_spec.rb @@ -12,7 +12,7 @@ "Finance" => "/npq-separation/admin/finance/statements", "Schools" => "/npq-separation/admin/schools", "Lead providers" => "/npq-separation/admin/lead-providers", - "Settings" => "#", + "Admins" => "/npq-separation/admin/admins", }.each_with_index do |(name, href), i| it "#{name} with href #{href} is at position #{i + 1}" do expect(subject[i].name).to eql(name) diff --git a/spec/controllers/npq_separation/admin/admins_controller_spec.rb b/spec/controllers/npq_separation/admin/admins_controller_spec.rb index e28fdb039e..2d6f164fb8 100644 --- a/spec/controllers/npq_separation/admin/admins_controller_spec.rb +++ b/spec/controllers/npq_separation/admin/admins_controller_spec.rb @@ -1,9 +1,104 @@ require "rails_helper" RSpec.describe NpqSeparation::Admin::AdminsController, type: :request do - describe("index") do - before { get(npq_separation_admin_admins_path) } + include Helpers::NPQSeparationAdminLogin - specify { expect(response).to(be_method_not_allowed) } + let!(:admin) { create :admin } + let!(:super_admin) { create :super_admin } + + shared_examples "unauthorised" do + specify "index is inaccessible" do + get npq_separation_admin_admins_path + expect(response).to redirect_to(sign_in_path) + end + + specify "new is inaccessible" do + get new_npq_separation_admin_admin_path + expect(response).to redirect_to(sign_in_path) + end + + specify "create is inaccessible" do + post npq_separation_admin_admins_path + expect(response).to redirect_to(sign_in_path) + end + + specify "update is inaccessible" do + patch npq_separation_admin_admin_path(0) + expect(response).to redirect_to(sign_in_path) + end + + it "is inaccessible" do + delete npq_separation_admin_admin_path(0) + expect(response).to redirect_to(sign_in_path) + end + end + + context "when not signed in" do + include_examples "unauthorised" + end + + context "when signed in as a normal admin" do + before { sign_in_as_admin } + + include_examples "unauthorised" + end + + context "when signed in as a super admin" do + before { sign_in_as_admin super_admin: true } + + specify "index is successful" do + get npq_separation_admin_admins_path + expect(response).to have_http_status(:success) + end + + specify "new is successful" do + get new_npq_separation_admin_admin_path + expect(response).to have_http_status(:success) + end + + specify "create is successful" do + expect { + post npq_separation_admin_admins_path, params: { admin: { email: "foo@example.org", full_name: "Name" } } + }.to change(Admin, :count).by(1) + + expect(response).to redirect_to(npq_separation_admin_admins_path) + expect(flash[:success]).to be_present + end + + specify "update updates regular admin to super admin" do + expect { + patch npq_separation_admin_admin_path(admin) + }.to change { admin.reload.super_admin? }.from(false).to(true) + + expect(response).to redirect_to(npq_separation_admin_admins_path) + expect(flash[:success]).to be_present + end + + specify "update does not change super admins" do + expect { + patch npq_separation_admin_admin_path(super_admin) + }.not_to(change { super_admin.reload.super_admin? }) + + expect(response).to redirect_to(npq_separation_admin_admins_path) + expect(flash[:error]).to be_present + end + + specify "destroy deletes regular admins" do + expect { + delete npq_separation_admin_admin_path(admin) + }.to change(Admin, :count).by(-1) + + expect(response).to redirect_to(npq_separation_admin_admins_path) + expect(flash[:success]).to be_present + end + + specify "destroy does nothing for super admins" do + expect { + delete npq_separation_admin_admin_path(super_admin) + }.not_to change(Admin, :count) + + expect(response).to redirect_to(npq_separation_admin_admins_path) + expect(flash[:error]).to be_present + end end end diff --git a/spec/features/npq_separation/admin/admins_spec.rb b/spec/features/npq_separation/admin/admins_spec.rb new file mode 100644 index 0000000000..2b8152cb76 --- /dev/null +++ b/spec/features/npq_separation/admin/admins_spec.rb @@ -0,0 +1,67 @@ +require "rails_helper" + +RSpec.feature "Administering admins", type: :feature do + include Helpers::AdminLogin + + let(:super_admin) { create :super_admin } + + scenario "is not authorized for regular admins" do + sign_in_as(create(:admin)) + + visit(npq_separation_admin_admins_path) + expect(page).to have_text("Unauthorized") + end + + scenario "listing current admins" do + admins = create_list(:admin, 2) + super_admins = create_list(:super_admin, 2) + + sign_in_as(super_admin) + visit(npq_separation_admin_admins_path) + + (admins + super_admins).each do |admin| + expect(page).to have_text(admin.full_name) + end + end + + scenario "creating a new admin" do + sign_in_as(super_admin) + visit(npq_separation_admin_admins_path) + click_on "Add new admin" + + fill_in "Email address", with: "new@example.com" + fill_in "Full name", with: "New Admin" + click_on "Add admin" + + expect(page).to have_text("new@example.com") + expect(page).to have_text("New Admin") + end + + scenario "promoting an admin to super admin" do + admin = create :admin + sign_in_as(super_admin) + visit(npq_separation_admin_admins_path) + + expect { click_on "Make Super Admin" }.to change { admin.reload.super_admin? }.from(false).to(true) + end + + scenario "deleting an admin" do + admin = create :admin + sign_in_as(super_admin) + visit(npq_separation_admin_admins_path) + + within "tr", text: admin.email do + expect { click_on "Delete" }.to change(Admin, :count).by(-1) + end + end + + scenario "no link to delete a super admin" do + another_super_admin = create :super_admin + sign_in_as(super_admin) + visit(npq_separation_admin_admins_path) + + within "tr", text: another_super_admin.email do + expect(page).not_to have_link("Delete") + end + end +end