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

@@ -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

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
 

app/views/organisations/index.html.erb

@@ -0,0 +1 @@
+

app/views/organisations/users.html.erb

@@ -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 %>

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| %>
+    <span class="govuk-!-margin-right-4">
+      <strong><%= @pagy.count %></strong><span style="font-weight: normal"> total users</span>
+    </span>
+  <% 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" } %>

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,

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 = "<h2 class=\"govuk-visually-hidden\">  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

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