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.

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

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", () => {

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();
+});

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

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

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

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,

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")

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:

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

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 protected]"
 
       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,19 +69,24 @@ 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
     expect(page).not_to have_content("Reset the user's mobile number?")
     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"

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

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

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(