From 28a25e71d7f91f5d87a42a0e8f96edacdf50b625 Mon Sep 17 00:00:00 2001 From: Ben Shimmin Date: Tue, 7 Jan 2025 10:58:14 +0000 Subject: [PATCH] Administrative interface for user additional organisations We now have the ability to add additional organisations from the app itself via the create/edit user forms. This replaces the radio list of BEIS + partner organisations with a dropdown, for the primary organisation, and also adds a checkbox list of partner organisations for the additional organisations. Appropriate explanatory hint text is also provided. We also have a sprinkling of JavaScript to enhance the form such that a user's additional organisation selections cannot include their primary organisation. As a safeguard for users who do not have JavaScript enabled, we have validation in the model for this too. --- CHANGELOG.md | 2 ++ app/controllers/users_controller.rb | 15 ++++++--- app/javascript/application.js | 1 + .../src/edit-user-additional-organisations.js | 23 +++++++++++++ app/models/user.rb | 3 +- app/services/create_user.rb | 6 ++-- app/services/update_user.rb | 6 ++-- app/views/users/_form.html.haml | 17 ++++++---- app/views/users/show.html.haml | 6 ++++ config/locales/models/user.en.yml | 6 ++++ .../beis_users_can_edit_a_user_spec.rb | 32 +++++++++++++++++++ .../beis_users_can_invite_new_users_spec.rb | 19 +++++++---- spec/models/user_spec.rb | 8 +++++ spec/services/create_user_spec.rb | 16 ++++++++++ spec/services/update_user_spec.rb | 16 ++++++++++ 15 files changed, 154 insertions(+), 22 deletions(-) create mode 100644 app/javascript/src/edit-user-additional-organisations.js diff --git a/CHANGELOG.md b/CHANGELOG.md index f5d84bc59..a00e185ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ [Full changelog][unreleased] +- Admin interface to enable additional organisations + ## Release 159 - 2024-12-19 [Full changelog][159] diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 62b139f6c..f5b6d39b3 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -28,12 +28,12 @@ def new end def create - @user = User.new(user_params.except(:active)) + @user = User.new(user_params.except(:active, :additional_organisations)) authorize @user @service_owner = service_owner @partner_organisations = partner_organisations - result = CreateUser.new(user: @user, organisation: organisation).call + result = CreateUser.new(user: @user, organisation:, additional_organisations:).call if result.success? flash.now[:notice] = t("action.user.create.success") redirect_to user_path(@user.reload.id) @@ -60,10 +60,11 @@ def update reset_mfa = user_params.delete(:reset_mfa) active = user_params[:active] == "true" - @user.assign_attributes(user_params.except(:reset_mfa, :active)) + @user.assign_attributes(user_params.except(:reset_mfa, :active, :additional_organisations)) + @user.additional_organisations = additional_organisations if @user.valid? - result = UpdateUser.new(user: @user, active: active, organisation: organisation, reset_mfa: reset_mfa).call + result = UpdateUser.new(user: @user, active: active, organisation:, reset_mfa:, additional_organisations:).call if result.success? flash[:notice] = t("action.user.update.success") @@ -78,7 +79,7 @@ def update end def user_params - params.require(:user).permit(:name, :email, :organisation_id, :active, :reset_mfa) + params.require(:user).permit(:name, :email, :organisation_id, :active, :reset_mfa, additional_organisations: []) end def id @@ -93,6 +94,10 @@ def organisation Organisation.find_by(id: organisation_id) end + def additional_organisations + user_params[:additional_organisations].reject(&:blank?).map { |id| Organisation.find_by(id:) } + end + private def service_owner Organisation.service_owner end diff --git a/app/javascript/application.js b/app/javascript/application.js index f6cb747dc..f03b3d7b1 100644 --- a/app/javascript/application.js +++ b/app/javascript/application.js @@ -4,6 +4,7 @@ import "./src/cookie-consent"; import initTableTreeView from "./src/table-tree-view"; import "./src/toggle-providing-org-fields"; import "./src/region-countries-checkbox"; +import "./src/edit-user-additional-organisations"; import * as GOVUKFrontend from "govuk-frontend"; document.addEventListener("DOMContentLoaded", () => { diff --git a/app/javascript/src/edit-user-additional-organisations.js b/app/javascript/src/edit-user-additional-organisations.js new file mode 100644 index 000000000..5527ed817 --- /dev/null +++ b/app/javascript/src/edit-user-additional-organisations.js @@ -0,0 +1,23 @@ +/* + This progressively enhances the "Create/Edit user" form such that a primary + organisation will be hidden from the list of additional organisations (and + also unchecked), because a user's primary organisation can never be a member + of that user's additional organisations. +*/ +document.addEventListener("DOMContentLoaded", function() { + const primaryOrgSelect = document.querySelector("#user_organisation_id"); + + if (!primaryOrgSelect) return; + + const handleCheckboxes = () => { + const val = primaryOrgSelect.querySelector("option:checked").value; + + document.querySelectorAll(".additional-organisations .govuk-checkboxes__item").forEach((checkboxItem) => { + const match = checkboxItem.querySelector(`input[value="${val}"`); + checkboxItem.style.display = match ? (match.checked = false, "none") : "block"; + }); + } + + primaryOrgSelect.addEventListener("change", handleCheckboxes); + handleCheckboxes(); +}); diff --git a/app/models/user.rb b/app/models/user.rb index cc408606d..c13cbc006 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,6 +7,7 @@ class User < ApplicationRecord has_many :historical_events validates_presence_of :name, :email validates :email, with: :email_cannot_be_changed_after_create, on: :update + validates :organisation_id, exclusion: {in: ->(user) { user.additional_organisations.map(&:id) }} before_save :ensure_otp_secret!, if: -> { otp_required_for_login && otp_secret.nil? } @@ -39,7 +40,7 @@ def organisation end def primary_organisation - Organisation.find(organisation_id) + Organisation.find_by_id(organisation_id) end def all_organisations diff --git a/app/services/create_user.rb b/app/services/create_user.rb index ec481c8c4..725fba071 100644 --- a/app/services/create_user.rb +++ b/app/services/create_user.rb @@ -1,15 +1,17 @@ class CreateUser - attr_accessor :user, :organisation + attr_accessor :user, :organisation, :additional_organisations - def initialize(user:, organisation:) + def initialize(user:, organisation:, additional_organisations: []) self.user = user self.organisation = organisation + self.additional_organisations = additional_organisations end def call result = Result.new(true) user.organisation = organisation + user.additional_organisations = additional_organisations # This password will never be used: the user must set a new password upon first login # but it must fulfill the password_complexity requirements we set in # config/initializers/devise_security diff --git a/app/services/update_user.rb b/app/services/update_user.rb index 218bb5cd2..eddac99e2 100644 --- a/app/services/update_user.rb +++ b/app/services/update_user.rb @@ -1,10 +1,11 @@ class UpdateUser - attr_accessor :user, :organisation, :reset_mfa + attr_accessor :user, :organisation, :reset_mfa, :additional_organisations - def initialize(user:, organisation:, active: true, reset_mfa: false) + def initialize(user:, organisation:, active: true, reset_mfa: false, additional_organisations: []) self.user = user self.organisation = organisation self.reset_mfa = reset_mfa + self.additional_organisations = additional_organisations @active = active end @@ -13,6 +14,7 @@ def call User.transaction do user.organisation = organisation + user.additional_organisations = additional_organisations if reset_mfa user.mobile_number = nil diff --git a/app/views/users/_form.html.haml b/app/views/users/_form.html.haml index 716802899..8e0dc5a71 100644 --- a/app/views/users/_form.html.haml +++ b/app/views/users/_form.html.haml @@ -9,18 +9,23 @@ = f.govuk_check_box :reset_mfa, true, multiple: false, label: { text: t("form.label.user.reset_mfa") } - if @service_owner.present? - = f.govuk_radio_buttons_fieldset :organisation, class: "user-organisations" do - = f.govuk_radio_button :organisation_id, @service_owner.id, label: { text: @service_owner.name }, link_errors: true - - if @partner_organisations.any? - = f.govuk_radio_divider - - @partner_organisations.each do |dp| - = f.govuk_radio_button :organisation_id, dp.id, label: { text: dp.name } + .govuk-form-group + = f.govuk_fieldset legend: {text: t("form.legend.user.primary_organisation")}, class: "user-organisations" do + %span.govuk-hint=t("form.hint.user.primary_organisation") + + - opts = [[@service_owner.name, @service_owner.id]] + @partner_organisations.pluck(:name, :id) + =f.select :organisation_id, options_for_select(opts, @user.primary_organisation.try(:id)), {}, class: "govuk-select" + - else .govuk-inset-text = succeed "." do = t("page_content.users.new.no_organisations.cta") = link_to t("page_content.users.new.no_organisations.link"), new_organisation_path, class: "govuk-link" + = f.govuk_check_boxes_fieldset :additional_organisations, legend: {text: "Additional organisations"}, class: "additional-organisations", hint: {text: t("form.hint.user.additional_organisations")} do + - @partner_organisations.each do |dp| + = f.govuk_check_box :additional_organisations, dp.id, label: { text: dp.name }, checked: @user.additional_organisations.include?(dp) + = f.govuk_collection_radio_buttons :active, user_active_options, :id, diff --git a/app/views/users/show.html.haml b/app/views/users/show.html.haml index a6212c00b..8502fcc50 100644 --- a/app/views/users/show.html.haml +++ b/app/views/users/show.html.haml @@ -25,6 +25,12 @@ = t("summary.label.user.organisation") %dd.govuk-summary-list__value = @user.organisation.name + - if @user.additional_organisations? + .govuk-summary-list__row + %dt.govuk-summary-list__key + = t("summary.label.user.additional_organisations") + %dd.govuk-summary-list__value + = @user.additional_organisations.map(&:name).to_sentence .govuk-summary-list__row %dt.govuk-summary-list__key = t("summary.label.user.active") diff --git a/config/locales/models/user.en.yml b/config/locales/models/user.en.yml index 0c935f6bc..e74401baa 100644 --- a/config/locales/models/user.en.yml +++ b/config/locales/models/user.en.yml @@ -22,11 +22,14 @@ en: active: What is the user's status? organisation_id: What organisation does this user belong to? reset_mfa: Reset the user's mobile number? + primary_organisation: Primary organisation hint: user: active: Deactivated users cannot log in new_password: Minimum 15 characters; must contain at least one digit, one lowercase letter, one uppercase letter, and one punctuation mark or symbol reset_mfa: The user will have to provide their mobile number on their next log in attempt + primary_organisation: This is the main organisation the user belongs to + additional_organisations: Select one or more additional organisations that this user can supply data on behalf of. They will not receive notifications for these organisations user: active: active: Activate @@ -48,6 +51,7 @@ en: name: Full name email: Email address organisation: Organisation + additional_organisations: Additional organisations active: Active? confirmed_for_mfa: label: Mobile number confirmed for authentication? @@ -88,6 +92,8 @@ en: attributes: organisation: required: Select the user's organisation + organisation_id: + exclusion: Additional organisations cannot include the primary organisation. name: blank: Enter a full name email: diff --git a/spec/features/beis_users_can_edit_a_user_spec.rb b/spec/features/beis_users_can_edit_a_user_spec.rb index f38e893a2..49b8e809e 100644 --- a/spec/features/beis_users_can_edit_a_user_spec.rb +++ b/spec/features/beis_users_can_edit_a_user_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" RSpec.feature "BEIS users can edit other users" do + include HideFromBullet + let!(:user) { create(:partner_organisation_user, organisation: create(:partner_organisation)) } after { logout } @@ -99,6 +101,36 @@ expect(user.reload.active).to be(true) end + scenario "a user can have additional organisations" do + administrator_user = create(:beis_user) + authenticate!(user: administrator_user) + + target_user = create(:partner_organisation_user) + + additional_orgs = [] + + # Navigate to the users page + skip_bullet do + visit users_index_path(user_state: "active") + + find("tr", text: target_user.name).click_link("Edit") + + # Set a couple of random partner organisations which *aren't* the + # primary organisation + Organisation.partner_organisations + .reject { |org| org.id == target_user.primary_organisation.id } + .pluck(:name).sample(3).each do |org| + additional_orgs << org + check org + end + + click_on t("default.button.submit") + end + + expect(page).to have_content("User successfully updated") + expect(page).to have_content(additional_orgs.to_sentence) + end + context "editing a user with a non-lowercase email address" do before do # Given a non-lowercase email address exists diff --git a/spec/features/beis_users_can_invite_new_users_spec.rb b/spec/features/beis_users_can_invite_new_users_spec.rb index 78d008cce..00416d85a 100644 --- a/spec/features/beis_users_can_invite_new_users_spec.rb +++ b/spec/features/beis_users_can_invite_new_users_spec.rb @@ -12,15 +12,17 @@ scenario "a new user can be created" do organisation = create(:partner_organisation) second_organisation = create(:partner_organisation) + additional_organisation = create(:partner_organisation) new_user_name = "Foo Bar" new_user_email = "email@example.com" perform_enqueued_jobs do - create_user(organisation, new_user_name, new_user_email) + create_user(organisation, additional_organisation, new_user_name, new_user_email) end expect(page).to have_content(organisation.name) expect(page).not_to have_content(second_organisation.name) + expect(page).to have_content(additional_organisation.name) new_user = User.where(email: new_user_email).first reset_password_link_regex = %r{http://test.local/users/password/edit\?reset_password_token=.*} @@ -56,7 +58,7 @@ end end - def create_user(organisation, new_user_name, new_user_email) + def create_user(organisation, additional_organisation, new_user_name, new_user_email) # Navigate from the landing page visit organisation_path(organisation) click_on("Users") @@ -67,11 +69,15 @@ def create_user(organisation, new_user_name, new_user_email) # Create a new user click_on("Add user") - # We expect to see BEIS separately on this page + # We expect to see BEIS on this page in the dropdown within(".user-organisations") do beis_identifier = Organisation.service_owner.id - expect(page).to have_css("input[type='radio'][value='#{beis_identifier}']:first-child") - expect(page).to have_css(".govuk-radios__divider:nth-child(2)") + expect(page).to have_css("select option[value='#{beis_identifier}']") + end + + # We expect to see the additional organisation too + within(".additional-organisations") do + expect(page).to have_css("input[value='#{additional_organisation.id}']") end # Fill out the form @@ -79,7 +85,8 @@ def create_user(organisation, new_user_name, new_user_email) expect(page).to have_content("Create user") fill_in "user[name]", with: new_user_name fill_in "user[email]", with: new_user_email - choose organisation.name + select organisation.name + check additional_organisation.name # Submit the form click_button "Submit" diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e597c9947..ae586f34c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -25,6 +25,14 @@ expect(user).to be_valid end + + it "won't allow a user to have its primary organisation also as an additional organisation" do + user = create(:administrator) + org = user.organisation + user.organisation = org + user.additional_organisations << org + expect(user).to be_invalid + end end describe "associations" do diff --git a/spec/services/create_user_spec.rb b/spec/services/create_user_spec.rb index 1d85919b5..d42512fef 100644 --- a/spec/services/create_user_spec.rb +++ b/spec/services/create_user_spec.rb @@ -33,5 +33,21 @@ expect(user.reload.organisation).to eql(organisation) end end + + context "when additional organisations are provided" do + it "associates the additional organsations to it" do + organisation = create(:partner_organisation) + org1 = create(:partner_organisation) + org2 = create(:partner_organisation) + + described_class.new( + user: user, + organisation: organisation, + additional_organisations: [org1, org2] + ).call + + expect(user.reload.additional_organisations).to include(org1, org2) + end + end end end diff --git a/spec/services/update_user_spec.rb b/spec/services/update_user_spec.rb index 796c7f29b..a53c5c3d0 100644 --- a/spec/services/update_user_spec.rb +++ b/spec/services/update_user_spec.rb @@ -27,6 +27,22 @@ end end + context "when additional organisations are provided" do + it "associates the additional organsations to it" do + organisation = create(:partner_organisation) + org1 = create(:partner_organisation) + org2 = create(:partner_organisation) + + described_class.new( + user: user, + organisation: organisation, + additional_organisations: [org1, org2] + ).call + + expect(user.reload.additional_organisations).to include(org1, org2) + end + end + context "when reset MFA is requested" do it "resets the user's mobile number and its confirmation time" do described_class.new(