From fb0c6b8bd47c2de9e50af68a25e994708bbc6358 Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Mon, 11 Apr 2022 13:34:19 +0100
Subject: [PATCH] Allow support to edit users (#463)

* Allow support to edit users

* Refactor assignable roles

* Better spec descriptions
---
 app/controllers/users_controller.rb    |  13 +-
 app/helpers/user_helper.rb             |  24 ++
 app/views/organisations/users.html.erb |   2 +-
 app/views/users/show.html.erb          |  12 +-
 config/locales/en.yml                  |   2 +
 spec/helpers/user_helper_spec.rb       | 101 ++++++
 spec/requests/users_controller_spec.rb | 464 ++++++++++++++++++++++++-
 7 files changed, 595 insertions(+), 23 deletions(-)

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index bf4130bf9..0c44b679b 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -34,6 +34,8 @@ class UsersController < ApplicationController
       @resource.errors.add :email, I18n.t("validations.email.blank")
     elsif !email_valid?(user_params["email"])
       @resource.errors.add :email, I18n.t("validations.email.invalid")
+    elsif user_params[:role] && !current_user.assignable_roles.key?(user_params[:role].to_sym)
+      @resource.errors.add :role, I18n.t("validations.role.invalid")
     end
     if @resource.errors.present?
       render :new, status: :unprocessable_entity
@@ -78,12 +80,12 @@ private
 
   def user_params
     if @user == current_user
-      if current_user.data_coordinator?
+      if current_user.data_coordinator? || current_user.support?
         params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact)
       else
         params.require(:user).permit(:email, :name, :password, :password_confirmation)
       end
-    elsif current_user.data_coordinator?
+    elsif current_user.data_coordinator? || current_user.support?
       params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact)
     end
   end
@@ -94,11 +96,12 @@ private
 
   def authenticate_scope!
     if action_name == "create"
-      head :unauthorized and return unless current_user.data_coordinator?
+      head :unauthorized and return unless current_user.data_coordinator? || current_user.support?
     else
-      render_not_found and return unless current_user.organisation == @user.organisation
+      render_not_found and return unless (current_user.organisation == @user.organisation) || current_user.support?
       render_not_found and return if action_name == "edit_password" && current_user != @user
-      render_not_found and return unless action_name == "show" || current_user.data_coordinator? || current_user == @user
+      render_not_found and return unless action_name == "show" ||
+        current_user.data_coordinator? || current_user.support? || current_user == @user
     end
   end
 end
diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb
index a9f6c920d..329920346 100644
--- a/app/helpers/user_helper.rb
+++ b/app/helpers/user_helper.rb
@@ -6,4 +6,28 @@ module UserHelper
   def pronoun(user, current_user)
     current_user == user ? "you" : "they"
   end
+
+  def can_edit_names?(user, current_user)
+    current_user == user || current_user.data_coordinator? || current_user.support?
+  end
+
+  def can_edit_emails?(user, current_user)
+    current_user == user || current_user.data_coordinator? || current_user.support?
+  end
+
+  def can_edit_password?(user, current_user)
+    current_user == user
+  end
+
+  def can_edit_roles?(_user, current_user)
+    current_user.data_coordinator? || current_user.support?
+  end
+
+  def can_edit_dpo?(_user, current_user)
+    current_user.data_coordinator? || current_user.support?
+  end
+
+  def can_edit_key_contact?(_user, current_user)
+    current_user.data_coordinator? || current_user.support?
+  end
 end
diff --git a/app/views/organisations/users.html.erb b/app/views/organisations/users.html.erb
index 33c7bc5d0..304528016 100644
--- a/app/views/organisations/users.html.erb
+++ b/app/views/organisations/users.html.erb
@@ -4,7 +4,7 @@
   <%= "Users" %>
 <% end %>
 
-<% if current_user.data_coordinator? %>
+<% if current_user.data_coordinator? || current_user.support? %>
   <%= govuk_button_link_to "Invite user", new_user_path, html: { method: :get } %>
 <% end %>
 <%= govuk_table do |table| %>
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index 6222fc471..846472ca1 100644
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -14,7 +14,7 @@
       <%= summary_list.row do |row|
         row.key { "Name" }
         row.value { @user.name }
-        if current_user == @user || current_user.data_coordinator?
+        if can_edit_names?(@user, current_user)
           row.action(visually_hidden_text: "name", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-name" })
         else
           row.action()
@@ -24,7 +24,7 @@
       <%= summary_list.row() do |row|
         row.key { "Email address" }
         row.value { @user.email }
-        if current_user == @user || current_user.data_coordinator?
+        if can_edit_emails?(@user, current_user)
           row.action(visually_hidden_text: "email address", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-email-address" })
         else
           row.action()
@@ -34,7 +34,7 @@
       <%= summary_list.row do |row|
         row.key { "Password" }
         row.value { "••••••••" }
-        if current_user == @user
+        if can_edit_password?(@user, current_user)
           row.action(visually_hidden_text: "password", href: edit_password_account_path, html_attributes: { "data-qa": "change-password" })
         else
           row.action()
@@ -50,7 +50,7 @@
       <%= summary_list.row do |row|
         row.key { "Role" }
         row.value { @user.role.humanize }
-        if current_user.data_coordinator?
+        if can_edit_roles?(@user, current_user)
           row.action(visually_hidden_text: "role", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-role" })
         else
           row.action()
@@ -60,7 +60,7 @@
       <%= summary_list.row do |row|
         row.key { "Data protection officer" }
         row.value { @user.is_data_protection_officer? ? "Yes" : "No" }
-        if current_user.data_coordinator?
+        if can_edit_dpo?(@user, current_user)
           row.action(visually_hidden_text: "are #{pronoun(@user, current_user)} a data protection officer?", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-are-#{pronoun(@user, current_user)}-a-data-protection-officer" })
         else
           row.action()
@@ -70,7 +70,7 @@
       <%= summary_list.row do |row|
         row.key { "Key contact" }
         row.value { @user.is_key_contact? ? "Yes" : "No" }
-        if current_user.data_coordinator?
+        if can_edit_key_contact?(@user, current_user)
           row.action(visually_hidden_text: "are #{pronoun(@user, current_user)} a key contact?", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-are-#{pronoun(@user, current_user)}-a-key-contact" })
         else
           row.action()
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 2837d1ea7..d73de31f6 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -47,6 +47,8 @@ en:
       taken: "Email already exists"
       invalid: "Enter an email address in the correct format, like name@example.com"
       blank: "Enter an email address"
+    role:
+      invalid: "Role must be data accessor, data provider or data coordinator"
 
     setup:
       intermediate_rent_product_name:
diff --git a/spec/helpers/user_helper_spec.rb b/spec/helpers/user_helper_spec.rb
index 6386ddd4f..5ec525c69 100644
--- a/spec/helpers/user_helper_spec.rb
+++ b/spec/helpers/user_helper_spec.rb
@@ -35,4 +35,105 @@ RSpec.describe UserHelper do
       end
     end
   end
+
+  describe "change button permissions" do
+    context "when the user is a data provider viewing their own details" do
+      let(:current_user) { FactoryBot.create(:user, :data_provider) }
+      let(:user) { current_user }
+
+      it "allows changing name" do
+        expect(can_edit_names?(user, current_user)).to be true
+      end
+
+      it "allows changing email" do
+        expect(can_edit_emails?(user, current_user)).to be true
+      end
+
+      it "allows changing password" do
+        expect(can_edit_password?(user, current_user)).to be true
+      end
+
+      it "does not allow changing roles" do
+        expect(can_edit_roles?(user, current_user)).to be false
+      end
+
+      it "does not allow changing dpo" do
+        expect(can_edit_dpo?(user, current_user)).to be false
+      end
+
+      it "does not allow changing key contact" do
+        expect(can_edit_key_contact?(user, current_user)).to be false
+      end
+    end
+
+    context "when the user is a data coordinator viewing another user's details" do
+      it "allows changing name" do
+        expect(can_edit_names?(user, current_user)).to be true
+      end
+
+      it "allows changing email" do
+        expect(can_edit_emails?(user, current_user)).to be true
+      end
+
+      it "allows changing password" do
+        expect(can_edit_password?(user, current_user)).to be false
+      end
+
+      it "does not allow changing roles" do
+        expect(can_edit_roles?(user, current_user)).to be true
+      end
+
+      it "does not allow changing dpo" do
+        expect(can_edit_dpo?(user, current_user)).to be true
+      end
+
+      it "does not allow changing key contact" do
+        expect(can_edit_key_contact?(user, current_user)).to be true
+      end
+
+      context "when the user is a data coordinator viewing their own details" do
+        let(:user) { current_user }
+
+        it "allows changing password" do
+          expect(can_edit_password?(user, current_user)).to be true
+        end
+      end
+    end
+
+    context "when the user is a support user viewing another user's details" do
+      let(:current_user) { FactoryBot.create(:user, :support) }
+
+      it "allows changing name" do
+        expect(can_edit_names?(user, current_user)).to be true
+      end
+
+      it "allows changing email" do
+        expect(can_edit_emails?(user, current_user)).to be true
+      end
+
+      it "allows changing password" do
+        expect(can_edit_password?(user, current_user)).to be false
+      end
+
+      it "does not allow changing roles" do
+        expect(can_edit_roles?(user, current_user)).to be true
+      end
+
+      it "does not allow changing dpo" do
+        expect(can_edit_dpo?(user, current_user)).to be true
+      end
+
+      it "does not allow changing key contact" do
+        expect(can_edit_key_contact?(user, current_user)).to be true
+      end
+
+      context "when the user is a support user viewing their own details" do
+        let(:user) { current_user }
+
+        it "allows changing password" do
+          expect(can_edit_password?(user, current_user)).to be true
+        end
+      end
+    end
+  end
 end
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index bef6baa13..3da36083b 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -132,7 +132,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         before do
           sign_in user
           get "/users/#{other_user.id}", headers: headers, params: {}
@@ -188,7 +188,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         before do
           sign_in user
           get "/users/#{other_user.id}/edit", headers: headers, params: {}
@@ -216,7 +216,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         before do
           sign_in user
           get "/users/#{other_user.id}/edit", headers: headers, params: {}
@@ -272,7 +272,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         let(:params) { { id: other_user.id, user: { name: new_name } } }
 
         before do
@@ -356,7 +356,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         before do
           sign_in user
           get "/users/#{other_user.id}", headers: headers, params: {}
@@ -419,7 +419,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         before do
           sign_in user
           get "/users/#{other_user.id}/edit", headers: headers, params: {}
@@ -469,7 +469,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         before do
           sign_in user
         end
@@ -531,7 +531,7 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
-      context "when the current user does not matches the user ID" do
+      context "when the current user does not match the user ID" do
         before do
           sign_in user
         end
@@ -566,7 +566,7 @@ RSpec.describe UsersController, type: :request do
             expect(page).to have_content(other_user.reload.email.to_s)
           end
 
-          context "when we try to update the user password" do
+          context "when the data coordinator tries to update the user’s password" do
             let(:params) do
               {
                 id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" }
@@ -585,7 +585,7 @@ RSpec.describe UsersController, type: :request do
           end
         end
 
-        context "when the current user does not matches the user ID" do
+        context "when the current user does not match the user ID" do
           context "when the user is not part of the same organisation as the current user" do
             let(:other_user) { FactoryBot.create(:user) }
             let(:params) { { id: other_user.id, user: { name: new_name } } }
@@ -642,10 +642,425 @@ RSpec.describe UsersController, type: :request do
         expect(response).to redirect_to("/organisations/#{user.organisation.id}/users")
       end
 
+      context "when the email is already taken" do
+        before do
+          FactoryBot.create(:user, email: "new_user@example.com")
+        end
+
+        it "shows an error" do
+          request
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(page).to have_content(I18n.t("validations.email.taken"))
+        end
+      end
+
+      context "when trying to assign support role" do
+        let(:params) do
+          {
+            "user": {
+              name: "new user",
+              email: "new_user@example.com",
+              role: "support",
+            },
+          }
+        end
+
+        it "shows an error" do
+          request
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(page).to have_content(I18n.t("validations.role.invalid"))
+        end
+      end
+    end
+
+    describe "#new" do
+      before do
+        sign_in user
+      end
+
       it "cannot assign support role to the new user" do
-        request
+        get "/users/new"
         expect(page).not_to have_field("user-role-support-field")
       end
+    end
+  end
+
+  context "when user is signed in as a support user" do
+    let(:user) { FactoryBot.create(:user, :support) }
+    let(:other_user) { FactoryBot.create(:user, organisation: user.organisation) }
+
+    before do
+      allow(user).to receive(:need_two_factor_authentication?).and_return(false)
+    end
+
+    describe "#show" do
+      context "when the current user matches the user ID" do
+        before do
+          sign_in user
+          get "/users/#{user.id}", headers: headers, params: {}
+        end
+
+        it "show the user details" do
+          expect(page).to have_content("Your account")
+        end
+
+        it "allows changing name, email, password, role, dpo and key contact" do
+          expect(page).to have_link("Change", text: "name")
+          expect(page).to have_link("Change", text: "email address")
+          expect(page).to have_link("Change", text: "password")
+          expect(page).to have_link("Change", text: "role")
+          expect(page).to have_link("Change", text: "are you a data protection officer?")
+          expect(page).to have_link("Change", text: "are you a key contact?")
+        end
+      end
+
+      context "when the current user does not match the user ID" do
+        before do
+          sign_in user
+          get "/users/#{other_user.id}", headers: headers, params: {}
+        end
+
+        context "when the user is part of the same organisation as the current user" do
+          it "returns 200" do
+            expect(response).to have_http_status(:ok)
+          end
+
+          it "shows the user details page" do
+            expect(page).to have_content("#{other_user.name}’s account")
+          end
+
+          it "allows changing name, email, role, dpo and key contact" do
+            expect(page).to have_link("Change", text: "name")
+            expect(page).to have_link("Change", text: "email address")
+            expect(page).not_to have_link("Change", text: "password")
+            expect(page).to have_link("Change", text: "role")
+            expect(page).to have_link("Change", text: "are they a data protection officer?")
+            expect(page).to have_link("Change", text: "are they a key contact?")
+          end
+        end
+
+        context "when the user is not part of the same organisation as the current user" do
+          let(:other_user) { FactoryBot.create(:user) }
+
+          it "returns 200" do
+            expect(response).to have_http_status(:ok)
+          end
+
+          it "shows the user details page" do
+            expect(page).to have_content("#{other_user.name}’s account")
+          end
+
+          it "allows changing name, email, role, dpo and key contact" do
+            expect(page).to have_link("Change", text: "name")
+            expect(page).to have_link("Change", text: "email address")
+            expect(page).not_to have_link("Change", text: "password")
+            expect(page).to have_link("Change", text: "role")
+            expect(page).to have_link("Change", text: "are they a data protection officer?")
+            expect(page).to have_link("Change", text: "are they a key contact?")
+          end
+        end
+      end
+    end
+
+    describe "#edit" do
+      context "when the current user matches the user ID" do
+        before do
+          sign_in user
+          get "/users/#{user.id}/edit", headers: headers, params: {}
+        end
+
+        it "show the edit personal details page" do
+          expect(page).to have_content("Change your personal details")
+        end
+
+        it "has fields for name, email, role, dpo and key contact" do
+          expect(page).to have_field("user[name]")
+          expect(page).to have_field("user[email]")
+          expect(page).to have_field("user[role]")
+          expect(page).to have_field("user[is_dpo]")
+          expect(page).to have_field("user[is_key_contact]")
+        end
+
+        it "allows setting the role to `support`" do
+          expect(page).to have_field("user-role-support-field")
+        end
+      end
+
+      context "when the current user does not match the user ID" do
+        before do
+          sign_in user
+          get "/users/#{other_user.id}/edit", headers: headers, params: {}
+        end
+
+        context "when the user is part of the same organisation as the current user" do
+          it "returns 200" do
+            expect(response).to have_http_status(:ok)
+          end
+
+          it "shows the user details page" do
+            expect(page).to have_content("Change #{other_user.name}’s personal details")
+          end
+
+          it "has fields for name, email, role, dpo and key contact" do
+            expect(page).to have_field("user[name]")
+            expect(page).to have_field("user[email]")
+            expect(page).to have_field("user[role]")
+            expect(page).to have_field("user[is_dpo]")
+            expect(page).to have_field("user[is_key_contact]")
+          end
+        end
+
+        context "when the user is not part of the same organisation as the current user" do
+          let(:other_user) { FactoryBot.create(:user) }
+
+          it "returns 200" do
+            expect(response).to have_http_status(:ok)
+          end
+
+          it "shows the user details page" do
+            expect(page).to have_content("Change #{other_user.name}’s personal details")
+          end
+
+          it "has fields for name, email, role, dpo and key contact" do
+            expect(page).to have_field("user[name]")
+            expect(page).to have_field("user[email]")
+            expect(page).to have_field("user[role]")
+            expect(page).to have_field("user[is_dpo]")
+            expect(page).to have_field("user[is_key_contact]")
+          end
+        end
+      end
+    end
+
+    describe "#edit_password" do
+      context "when the current user matches the user ID" do
+        before do
+          sign_in user
+          get "/account/edit/password", headers: headers, params: {}
+        end
+
+        it "shows the edit password page" do
+          expect(page).to have_content("Change your password")
+        end
+
+        it "shows the password requirements hint" do
+          expect(page).to have_css("#user-password-hint")
+        end
+      end
+
+      context "when the current user does not match the user ID" do
+        before do
+          sign_in user
+        end
+
+        it "there is no route" do
+          expect {
+            get "/users/#{other_user.id}/password/edit", headers: headers, params: {}
+          }.to raise_error(ActionController::RoutingError)
+        end
+      end
+    end
+
+    describe "#update" do
+      context "when the current user matches the user ID" do
+        before do
+          sign_in user
+          patch "/users/#{user.id}", headers: headers, params: params
+        end
+
+        it "updates the user" do
+          user.reload
+          expect(user.name).to eq(new_name)
+        end
+
+        it "tracks who updated the record" do
+          user.reload
+          whodunnit_actor = user.versions.last.actor
+          expect(whodunnit_actor).to be_a(User)
+          expect(whodunnit_actor.id).to eq(user.id)
+        end
+
+        context "when user changes email, dpo and key contact" do
+          let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } }
+
+          it "allows changing email and dpo" do
+            user.reload
+            expect(user.email).to eq(new_email)
+            expect(user.is_data_protection_officer?).to be true
+            expect(user.is_key_contact?).to be true
+          end
+        end
+
+        context "when we update the user password" do
+          let(:params) do
+            {
+              id: user.id, user: { password: new_name, password_confirmation: "something_else" }
+            }
+          end
+
+          before do
+            sign_in user
+            patch "/users/#{user.id}", headers: headers, params: params
+          end
+
+          it "shows an error if passwords don't match" do
+            expect(response).to have_http_status(:unprocessable_entity)
+            expect(page).to have_selector("#error-summary-title")
+          end
+        end
+      end
+
+      context "when the current user does not match the user ID" do
+        before do
+          sign_in user
+        end
+
+        context "when the user is part of the same organisation as the current user" do
+          it "updates the user" do
+            expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+              .to change { other_user.reload.name }.from(other_user.name).to(new_name)
+          end
+
+          it "tracks who updated the record" do
+            expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+              .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id)
+          end
+
+          context "when user changes email, dpo, key_contact" do
+            let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } }
+
+            it "allows changing email, dpo, key_contact" do
+              patch "/users/#{other_user.id}", headers: headers, params: params
+              other_user.reload
+              expect(other_user.email).to eq(new_email)
+              expect(other_user.is_data_protection_officer?).to be true
+              expect(other_user.is_key_contact?).to be true
+            end
+          end
+
+          it "does not bypass sign in for the support user" do
+            patch "/users/#{other_user.id}", headers: headers, params: params
+            follow_redirect!
+            expect(page).to have_content("#{other_user.reload.name}’s account")
+            expect(page).to have_content(other_user.reload.email.to_s)
+          end
+
+          context "when the support user tries to update the user’s password" do
+            let(:params) do
+              {
+                id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" }
+              }
+            end
+
+            it "does not update the password" do
+              expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+                .not_to change(other_user, :encrypted_password)
+            end
+
+            it "does update other values" do
+              expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+                .to change { other_user.reload.name }.from("Danny Rojas").to("new name")
+            end
+          end
+        end
+
+        context "when the current user does not match the user ID" do
+          context "when the user is not part of the same organisation as the current user" do
+            let(:other_user) { FactoryBot.create(:user) }
+            let(:params) { { id: other_user.id, user: { name: new_name } } }
+
+            before do
+              sign_in user
+            end
+
+            it "updates the user" do
+              expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+                .to change { other_user.reload.name }.from(other_user.name).to(new_name)
+            end
+
+            it "tracks who updated the record" do
+              expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+                .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id)
+            end
+
+            context "when user changes email, dpo, key_contact" do
+              let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } }
+
+              it "allows changing email, dpo, key_contact" do
+                patch "/users/#{other_user.id}", headers: headers, params: params
+                other_user.reload
+                expect(other_user.email).to eq(new_email)
+                expect(other_user.is_data_protection_officer?).to be true
+                expect(other_user.is_key_contact?).to be true
+              end
+            end
+
+            it "does not bypass sign in for the support user" do
+              patch "/users/#{other_user.id}", headers: headers, params: params
+              follow_redirect!
+              expect(page).to have_content("#{other_user.reload.name}’s account")
+              expect(page).to have_content(other_user.reload.email.to_s)
+            end
+
+            context "when the support user tries to update the user’s password" do
+              let(:params) do
+                {
+                  id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" }
+                }
+              end
+
+              it "does not update the password" do
+                expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+                  .not_to change(other_user, :encrypted_password)
+              end
+
+              it "does update other values" do
+                expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+                  .to change { other_user.reload.name }.from("Danny Rojas").to("new name")
+              end
+            end
+          end
+        end
+      end
+
+      context "when the update fails to persist" do
+        before do
+          sign_in user
+          allow(User).to receive(:find_by).and_return(user)
+          allow(user).to receive(:update).and_return(false)
+          patch "/users/#{user.id}", headers: headers, params: params
+        end
+
+        it "show an error" do
+          expect(response).to have_http_status(:unprocessable_entity)
+        end
+      end
+    end
+
+    describe "#create" do
+      let(:params) do
+        {
+          "user": {
+            name: "new user",
+            email: "new_user@example.com",
+            role: "data_coordinator",
+          },
+        }
+      end
+      let(:request) { post "/users/", headers: headers, params: params }
+
+      before do
+        sign_in user
+      end
+
+      it "invites a new user" do
+        expect { request }.to change(User, :count).by(1)
+      end
+
+      it "redirects back to organisation users page" do
+        request
+        expect(response).to redirect_to("/organisations/#{user.organisation.id}/users")
+      end
 
       context "when the email is already taken" do
         before do
@@ -658,6 +1073,33 @@ RSpec.describe UsersController, type: :request do
           expect(page).to have_content(I18n.t("validations.email.taken"))
         end
       end
+
+      context "when trying to assign support role" do
+        let(:params) do
+          {
+            "user": {
+              name: "new user",
+              email: "new_user@example.com",
+              role: "support",
+            },
+          }
+        end
+
+        it "creates a new support user" do
+          expect(User.last.role).to eq("support")
+        end
+      end
+    end
+
+    describe "#new" do
+      before do
+        sign_in user
+      end
+
+      it "can assign support role to the new user" do
+        get "/users/new"
+        expect(page).to have_field("user-role-support-field")
+      end
     end
   end