From bd88eadb3584d89b8f43efa195ee1b292ba94b3c Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Fri, 13 May 2022 13:58:12 +0100
Subject: [PATCH] Hide inactive users and allow support users to view all users
(#576)
* Hide inactive users and allow support users to view all users
* Enable support users to invite users to any organisation
* Add pagination to user views
---
app/controllers/organisations_controller.rb | 10 ++--
app/controllers/users_controller.rb | 25 +++++++--
app/views/organisations/index.html.erb | 1 +
app/views/organisations/users.html.erb | 27 ----------
app/views/users/index.html.erb | 33 ++++++++++++
app/views/users/new.html.erb | 13 +++++
.../requests/organisations_controller_spec.rb | 15 ++++++
spec/requests/users_controller_spec.rb | 53 +++++++++++++++++--
8 files changed, 138 insertions(+), 39 deletions(-)
delete mode 100644 app/views/organisations/users.html.erb
diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb
index 25c75146f..9401fae3a 100644
--- a/app/controllers/organisations_controller.rb
+++ b/app/controllers/organisations_controller.rb
@@ -1,12 +1,13 @@
class OrganisationsController < ApplicationController
+ include Pagy::Backend
before_action :authenticate_user!, except: [:index]
before_action :find_resource, except: [:index]
before_action :authenticate_scope!
def index
- unless current_user.support?
- redirect_to user_path(current_user)
- end
+ redirect_to organisation_path(current_user.organisation) unless current_user.support?
+
+ @pagy, @organisations = pagy(Organisation.all)
end
def show
@@ -14,7 +15,8 @@ class OrganisationsController < ApplicationController
end
def users
- render "users"
+ @pagy, @users = pagy(@organisation.users.where(active: true))
+ render "users/index"
end
def details
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index f687b0b6a..cd4c3254e 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1,4 +1,5 @@
class UsersController < ApplicationController
+ include Pagy::Backend
include Devise::Controllers::SignInOut
include Helpers::Email
before_action :authenticate_user!
@@ -6,11 +7,13 @@ class UsersController < ApplicationController
before_action :authenticate_scope!, except: %i[new]
def index
- unless current_user.support?
- redirect_to user_path(@user)
- end
+ redirect_to users_organisation_path(current_user.organisation) unless current_user.support?
+
+ @pagy, @users = pagy(User.all.where(active: true))
end
+ def show; end
+
def update
if @user.update(user_params)
if @user == current_user
@@ -49,7 +52,7 @@ class UsersController < ApplicationController
user = User.create(user_params.merge(org_params).merge(password_params))
if user.persisted?
user.send_reset_password_instructions
- redirect_to users_organisation_path(current_user.organisation)
+ redirect_to created_user_redirect_path
else
@resource.errors.add :email, I18n.t("validations.email.taken")
render :new, status: :unprocessable_entity
@@ -81,6 +84,8 @@ private
end
def org_params
+ return {} if current_user.support?
+
{ organisation: current_user.organisation }
end
@@ -91,8 +96,18 @@ private
else
params.require(:user).permit(:email, :name, :password, :password_confirmation)
end
- elsif current_user.data_coordinator? || current_user.support?
+ elsif current_user.data_coordinator?
params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact)
+ elsif current_user.support?
+ params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact, :organisation_id)
+ end
+ end
+
+ def created_user_redirect_path
+ if current_user.support?
+ users_path
+ else
+ users_organisation_path(current_user.organisation)
end
end
diff --git a/app/views/organisations/index.html.erb b/app/views/organisations/index.html.erb
index e69de29bb..8b1378917 100644
--- a/app/views/organisations/index.html.erb
+++ b/app/views/organisations/index.html.erb
@@ -0,0 +1 @@
+
diff --git a/app/views/organisations/users.html.erb b/app/views/organisations/users.html.erb
deleted file mode 100644
index 8c0e5e5d0..000000000
--- a/app/views/organisations/users.html.erb
+++ /dev/null
@@ -1,27 +0,0 @@
-<% content_for :title, "Your organisation (Users)" %>
-
-<% content_for :tab_title do %>
- <%= "Users" %>
-<% end %>
-
-<% 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| %>
- <%= table.head do |head| %>
- <%= head.row do |row| %>
- <% row.cell(header: true, text: "Name and email adress") %>
- <% row.cell(header: true, text: "Organisation and role") %>
- <% row.cell(header: true, text: "Last logged in") %>
- <% end %>
- <% end %>
- <% @organisation.users.each do |user| %>
- <%= table.body do |body| %>
- <%= body.row do |row| %>
- <% row.cell(text: simple_format(user_cell(user), {}, wrapper_tag: "div")) %>
- <% row.cell(text: simple_format(org_cell(user), {}, wrapper_tag: "div")) %>
- <% row.cell(text: user.last_sign_in_at&.to_formatted_s(:govuk_date)) %>
- <% end %>
- <% end %>
- <% end %>
-<% end %>
diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb
index e69de29bb..cff822745 100644
--- a/app/views/users/index.html.erb
+++ b/app/views/users/index.html.erb
@@ -0,0 +1,33 @@
+<% content_for :title, "Your organisation (Users)" %>
+
+<% content_for :tab_title do %>
+ <%= "Users" %>
+<% end %>
+
+<% 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| %>
+ <%= table.caption(size: "s", classes: %w[govuk-!-text-align-left govuk-!-margin-top-4 govuk-!-margin-bottom-4]) do |caption| %>
+
+ <%= @pagy.count %> total users
+
+ <% end %>
+ <%= table.head do |head| %>
+ <%= head.row do |row| %>
+ <% row.cell(header: true, text: "Name and email adress") %>
+ <% row.cell(header: true, text: "Organisation and role") %>
+ <% row.cell(header: true, text: "Last logged in") %>
+ <% end %>
+ <% end %>
+ <% @users.each do |user| %>
+ <%= table.body do |body| %>
+ <%= body.row do |row| %>
+ <% row.cell(text: simple_format(user_cell(user), {}, wrapper_tag: "div")) %>
+ <% row.cell(text: simple_format(org_cell(user), {}, wrapper_tag: "div")) %>
+ <% row.cell(text: user.last_sign_in_at&.to_formatted_s(:govuk_date)) %>
+ <% end %>
+ <% end %>
+ <% end %>
+<% end %>
+<%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "users" } %>
diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb
index a84f0d634..980281f11 100644
--- a/app/views/users/new.html.erb
+++ b/app/views/users/new.html.erb
@@ -26,6 +26,19 @@
spellcheck: "false",
value: @resource.email %>
+ <% if current_user.support? %>
+ <% null_option = [OpenStruct.new(id: "", name: "Select an option")] %>
+ <% organisations = Organisation.all.map { |org| OpenStruct.new(id: org.id, name: org.name) } %>
+ <% answer_options = null_option + organisations %>
+
+ <%= f.govuk_collection_select :organisation_id,
+ answer_options,
+ :id,
+ :name,
+ label: { text: "Organisation", size: "m" },
+ options: { disabled: [""], selected: "" } %>
+ <% end %>
+
<% roles = current_user.assignable_roles.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize) } %>
<%= f.govuk_collection_radio_buttons :role,
diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb
index 8d4e22690..f9e164b38 100644
--- a/spec/requests/organisations_controller_spec.rb
+++ b/spec/requests/organisations_controller_spec.rb
@@ -101,6 +101,10 @@ RSpec.describe OrganisationsController, type: :request do
context "when accessing the users tab" do
context "with an organisation that the user belongs to" do
+ let!(:other_user) { FactoryBot.create(:user, organisation: user.organisation, name: "User 2") }
+ let!(:inactive_user) { FactoryBot.create(:user, organisation: user.organisation, active: false, name: "User 3") }
+ let!(:other_org_user) { FactoryBot.create(:user, name: "User 4") }
+
before do
sign_in user
get "/organisations/#{organisation.id}/users", headers:, params: {}
@@ -125,6 +129,17 @@ RSpec.describe OrganisationsController, type: :request do
expected_html = " Users"
expect(response.body).to include(expected_html)
end
+
+ it "shows only active users in the current user's organisation" do
+ expect(page).to have_content(user.name)
+ expect(page).to have_content(other_user.name)
+ expect(page).not_to have_content(inactive_user.name)
+ expect(page).not_to have_content(other_org_user.name)
+ end
+
+ it "shows the pagination count" do
+ expect(page).to have_content("2 total users")
+ end
end
context "with an organisation that are not in scope for the user, i.e. that they do not belong to" do
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 2c2902094..b75cc5b4f 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -338,6 +338,18 @@ RSpec.describe UsersController, type: :request do
let(:user) { FactoryBot.create(:user, :data_coordinator) }
let(:other_user) { FactoryBot.create(:user, organisation: user.organisation) }
+ describe "#index" do
+ before do
+ sign_in user
+ get "/users", headers:, params: {}
+ end
+
+ it "redirects to the organisation user path" do
+ follow_redirect!
+ expect(path).to match("/organisations/#{user.organisation.id}/users")
+ end
+ end
+
describe "#show" do
context "when the current user matches the user ID" do
before do
@@ -696,6 +708,28 @@ RSpec.describe UsersController, type: :request do
allow(user).to receive(:need_two_factor_authentication?).and_return(false)
end
+ describe "#index" do
+ let!(:other_user) { FactoryBot.create(:user, organisation: user.organisation, name: "User 2") }
+ let!(:inactive_user) { FactoryBot.create(:user, organisation: user.organisation, active: false, name: "User 3") }
+ let!(:other_org_user) { FactoryBot.create(:user, name: "User 4") }
+
+ before do
+ sign_in user
+ get "/users", headers:, params: {}
+ end
+
+ it "shows all active users" do
+ expect(page).to have_content(user.name)
+ expect(page).to have_content(other_user.name)
+ expect(page).not_to have_content(inactive_user.name)
+ expect(page).to have_content(other_org_user.name)
+ end
+
+ it "shows the pagination count" do
+ expect(page).to have_content("3 total users")
+ end
+ end
+
describe "#show" do
context "when the current user matches the user ID" do
before do
@@ -1041,12 +1075,15 @@ RSpec.describe UsersController, type: :request do
end
describe "#create" do
+ let(:organisation) { FactoryBot.create(:organisation) }
+ let(:email) { "new_user@example.com" }
let(:params) do
{
"user": {
name: "new user",
- email: "new_user@example.com",
+ email:,
role: "data_coordinator",
+ organisation_id: organisation.id,
},
}
end
@@ -1060,9 +1097,14 @@ RSpec.describe UsersController, type: :request do
expect { request }.to change(User, :count).by(1)
end
- it "redirects back to organisation users page" do
+ it "adds the user to the correct organisation" do
request
- expect(response).to redirect_to("/organisations/#{user.organisation.id}/users")
+ expect(User.find_by(email:).organisation).to eq(organisation)
+ end
+
+ it "redirects back to users page" do
+ request
+ expect(response).to redirect_to("/users")
end
context "when the email is already taken" do
@@ -1103,6 +1145,11 @@ RSpec.describe UsersController, type: :request do
get "/users/new"
expect(page).to have_field("user-role-support-field")
end
+
+ it "can assign organisation to the new user" do
+ get "/users/new"
+ expect(page).to have_field("user-organisation-id-field")
+ end
end
end