Cldc 1275 organisations users page (#627)

* added empty feature for viewing user specific organisation

* refactored contexts in features

* correctly failing feature

* code to have user link

* spec to test we are at correct sub menu for Users

* added view for users view

* edited text to pass remaining tests

* fixed hidde3n field view

* removed hidden field in the table

* correct context

* added extra field to test hidden content

* test TabNavHelper with correct property

* fixed tab highlights

* spec for sub nav logs

* spec for all sub menus

* testing users inside org

* better test for listing users

* all test for search inside org users

* pressing button

* passing feature

* better test with multiple orgs

* almost done

* fixed failing test

* rubocop

* expanded tests

* typo

* selecting subset due to null present in the list as well

Co-authored-by: Ted <ted.booton@madetech.com>

app/components/search_component.rb

@@ -9,10 +9,12 @@ class SearchComponent < ViewComponent::Base
   end
 
   def path(current_user)
-    if request.path.include?("users")
+    if request.path.include?("organisations") && request.path.include?("users")
-      user_path(current_user)
+      request.path
     elsif request.path.include?("organisations") && request.path.include?("logs")
       request.path
+    elsif request.path.include?("users")
+      user_path(current_user)
     elsif request.path.include?("organisations")
       organisations_path
     elsif request.path.include?("logs")

app/controllers/organisations_controller.rb

@@ -24,7 +24,11 @@ class OrganisationsController < ApplicationController
     @pagy, @users = pagy(filtered_users(@organisation.users.sorted_by_organisation_and_role, search_term))
     @searched = search_term.presence
     @total_count = @organisation.users.size
-    render "users/index"
+    if current_user.support?
+      render "users", layout: "application"
+    else
+      render "/users/index"
+    end
   end
 
   def details

app/controllers/users_controller.rb

@@ -62,6 +62,7 @@ class UsersController < ApplicationController
   end
 
   def new
+    @organisation_id = params["organisation_id"]
     @resource = User.new
   end
 

app/helpers/navigation_items_helper.rb

@@ -4,23 +4,24 @@ module NavigationItemsHelper
   def primary_items(path, current_user)
     if current_user.support?
       [
-        NavigationItem.new("Organisations", organisations_path, organisation_current?(path)),
+        NavigationItem.new("Organisations", organisations_path, organisations_current?(path)),
         NavigationItem.new("Users", "/users", users_current?(path)),
         NavigationItem.new("Logs", case_logs_path, logs_current?(path)),
       ]
     else
       [
         NavigationItem.new("Logs", case_logs_path, logs_current?(path)),
-        NavigationItem.new("Users", users_organisation_path(current_user.organisation), users_current?(path)),
+        NavigationItem.new("Users", users_organisation_path(current_user.organisation), subnav_users_path?(path)),
-        NavigationItem.new("About your organisation", "/organisations/#{current_user.organisation.id}", organisation_current?(path)),
+        NavigationItem.new("About your organisation", "/organisations/#{current_user.organisation.id}", subnav_details_path?(path)),
       ]
     end
   end
 
   def secondary_items(path, current_organisation_id)
     [
-      NavigationItem.new("Logs", "/organisations/#{current_organisation_id}/logs", organisation_logs_current?(path, current_organisation_id)),
+      NavigationItem.new("Logs", "/organisations/#{current_organisation_id}/logs", subnav_logs_path?(path)),
-      NavigationItem.new("About this organisation", "/organisations/#{current_organisation_id}", about_organisation_current?(path, current_organisation_id)),
+      NavigationItem.new("Users", "/organisations/#{current_organisation_id}/users", subnav_users_path?(path)),
+      NavigationItem.new("About this organisation", "/organisations/#{current_organisation_id}", subnav_details_path?(path)),
     ]
   end
 
@@ -31,18 +32,22 @@ private
   end
 
   def users_current?(path)
-    path.include?("/users")
+    path == "/users"
   end
 
-  def organisation_current?(path)
+  def organisations_current?(path)
-    path.include?("/organisations") && !path.include?("/users")
+    path == "/organisations" || subnav_users_path?(path) || subnav_logs_path?(path) || subnav_details_path?(path)
   end
 
-  def about_organisation_current?(path, organisation_id)
+  def subnav_users_path?(path)
-    path.include?("/organisations/#{organisation_id}/details") || path.include?("/organisations/#{organisation_id}/edit")
+    path.include?("/organisations") && path.include?("/users")
   end
 
-  def organisation_logs_current?(path, organisation_id)
+  def subnav_logs_path?(path)
-    path == "/organisations/#{organisation_id}/logs"
+    path.include?("/organisations") && path.include?("/logs")
+  end
+
+  def subnav_details_path?(path)
+    path.include?("/organisations") && path.include?("/details")
   end
 end

app/helpers/tab_nav_helper.rb

@@ -3,7 +3,7 @@ module TabNavHelper
 
   def user_cell(user)
     link_text = user.name.presence || user.email
-    [govuk_link_to(link_text, user), "<span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{user.email}</span>"].join("\n")
+    [govuk_link_to(link_text, user), "<span class=\"govuk-visually-hidden\">User </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{user.email}</span>"].join("\n")
   end
 
   def org_cell(user)

app/views/organisations/users.html.erb

@@ -0,0 +1,27 @@
+<% item_label = @pagy.count > 1 ? "users" : "user" %>
+<% if @searched.present? %>
+  <% title = "Your organisation (#{@pagy.count} #{item_label} matching ‘#{@searched}’ of #{@total_count} total users)" %>
+<% else %>
+  <% title = "Your organisation (User)" %>
+<% end %>
+
+<% content_for :title, title %>
+
+<% content_for :tab_title do %>
+  <%= "Users" %>
+<% end %>
+
+<%= render SubNavigationComponent.new(
+  items: secondary_items(request.path, @organisation.id),
+) %>
+
+<% if current_user.data_coordinator? || current_user.support? %>
+  <%= govuk_button_link_to "Invite user", new_user_path(organisation_id: @organisation.id), html: { method: :get } %>
+<% end %>
+
+<%= render SearchComponent.new(current_user:, search_label: "Search by name or email address", value: @searched) %>
+
+<hr class="govuk-section-break govuk-section-break--visible govuk-section-break--m">
+
+<%= render partial: "users/user_list", locals: { users: @users, title:, pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %>
+<%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "users" } %>

app/views/users/_user_list.html.erb

@@ -49,6 +49,9 @@
           <% end %>
           <% row.cell(text: simple_format(org_cell(user), {}, wrapper_tag: "div")) %>
           <% row.cell(text: user.active? ? user.last_sign_in_at&.to_formatted_s(:govuk_date) : "Deactivated") %>
+          <%= govuk_link_to users_path(user) do %>
+            <span class="govuk-visually-hidden">User </span><%= user.id %>
+          <% end %>
         <% end %>
       <% end %>
     <% end %>

app/views/users/new.html.erb

@@ -30,13 +30,17 @@
         <% 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 %>
+        <% if @organisation_id %>
+          <% organisation = Organisation.find(@organisation_id) %>
+          <% answer_options = [OpenStruct.new(id: organisation.id, name: organisation.name)] %>
+        <% end %>
 
           <%= f.govuk_collection_select :organisation_id,
             answer_options,
             :id,
             :name,
             label: { text: "Organisation", size: "m" },
-            options: { disabled: [""], selected: "" } %>
+            options: { disabled: [""], selected: @organisation_id ? answer_options.first : "" } %>
       <% end %>
 
       <% roles = current_user.assignable_roles.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize) } %>

spec/features/log_spec.rb

@@ -2,7 +2,7 @@ require "rails_helper"
 
 RSpec.describe "Log Features" do
   context "when searching for specific logs" do
-    context "when I am logged in and there are logs in the database" do
+    context "when I am signed in and there are logs in the database" do
       let(:user) { FactoryBot.create(:user, last_sign_in_at: Time.zone.now) }
       let!(:log_to_search) { FactoryBot.create(:case_log, owning_organisation: user.organisation) }
       let!(:same_organisation_log) { FactoryBot.create(:case_log, owning_organisation: user.organisation) }

spec/features/organisation_spec.rb

@@ -5,6 +5,7 @@ RSpec.describe "User Features" do
   include Helpers
   let(:organisation) { user.organisation }
   let(:org_id) { organisation.id }
+  let(:org_name) { organisation.name }
   let(:set_password_template_id) { User::CONFIRMABLE_TEMPLATE_ID }
   let(:notify_client) { instance_double(Notifications::Client) }
   let(:confirmation_token) { "MCDH5y6Km-U7CFPgAMVS" }
@@ -15,7 +16,10 @@ RSpec.describe "User Features" do
     allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client)
     allow(Devise).to receive(:friendly_token).and_return(confirmation_token)
     allow(notify_client).to receive(:send_email).and_return(true)
-    sign_in user
+    visit("/organisations")
+    fill_in("user[email]", with: user.email)
+    fill_in("user[password]", with: user.password)
+    click_button("Sign in")
   end
 
   context "when user is a data coordinator" do
@@ -82,21 +86,28 @@ RSpec.describe "User Features" do
   end
 
   context "when user is support user" do
+    let(:otp) { "999111" }
+    let(:user) { FactoryBot.create(:user, :support) }
+
+    before do
+      allow(SecureRandom).to receive(:random_number).and_return(otp)
+      click_link("Sign out")
+      visit("/organisations")
+      fill_in("user[email]", with: user.email)
+      fill_in("user[password]", with: user.password)
+      click_button("Sign in")
+      fill_in("code", with: otp)
+      click_button("Submit")
+    end
+
     context "when viewing logs for specific organisation" do
-      let(:user) { FactoryBot.create(:user, :support) }
       let(:first_log) { organisation.case_logs.first }
-      let(:otp) { "999111" }
       let!(:log_to_search) { FactoryBot.create(:case_log, owning_organisation: user.organisation, managing_organisation_id: organisation.id) }
       let!(:other_logs) { FactoryBot.create_list(:case_log, 4, owning_organisation_id: organisation.id, managing_organisation_id: organisation.id) }
       let(:number_of_case_logs) { CaseLog.count }
 
       before do
         first_log.update!(startdate: Time.utc(2022, 6, 2, 10, 36, 49))
-        allow(SecureRandom).to receive(:random_number).and_return(otp)
-        click_link("Sign out")
-        sign_in user
-        fill_in("code", with: otp)
-        click_button("Submit")
         visit("/organisations/#{org_id}/logs")
       end
 
@@ -153,5 +164,51 @@ RSpec.describe "User Features" do
         expect(page).not_to have_link first_log.id.to_s, href: "/logs/#{first_log.id}"
       end
     end
+
+    context "when I search for users belonging to a specific organisation" do
+      context "when I am signed in and there are users in the database" do
+        let!(:user_list) { FactoryBot.create_list(:user, 4, organisation: user.organisation) }
+
+        context "when I visit the organisation page" do
+          before do
+            visit("/organisations/#{org_id}")
+          end
+
+          it "has link to the organisations users tab" do
+            expect(page).to have_link("Users", href: "/organisations/#{org_id}/users")
+          end
+
+          context "when I click users link in submenu" do
+            before do
+              click_link("Users", href: "/organisations/#{org_id}/users")
+            end
+
+            it "shows list of users belonging to the same organisation" do
+              user_list.each do |user|
+                expect(page).to have_content(user.email)
+              end
+            end
+
+            it "shows submenu for selected orgnisation" do
+              expect(page).to have_css('[aria-current="page"]', text: "Users")
+              expect(page).to have_current_path("/organisations/#{org_id}/users")
+              expect(page).to have_link("Logs")
+              expect(page).to have_link("About this organisation")
+            end
+
+            context "when I click on Invite user and there are multiple organisations in the database" do
+              before do
+                FactoryBot.create_list(:organisation, 5)
+                click_link(text: "Invite user")
+              end
+
+              it "has only specific organisation name in the dropdown" do
+                expect(page).to have_select("user-organisation-id-field", options: [org_name])
+              end
+            end
+          end
+        end
+      end
+    end
   end
 end

spec/helpers/navigation_items_helper_spec.rb

@@ -81,6 +81,80 @@ RSpec.describe NavigationItemsHelper do
           expect(primary_items("/account", current_user)).to eq(expected_navigation_items)
         end
       end
+
+      context "when the user is on the specific organisation's page" do
+        context "when the user is on organisation logs page" do
+          let(:required_sub_path) { "logs" }
+          let(:expected_navigation_items) do
+            [
+              NavigationItemsHelper::NavigationItem.new("Organisations", "/organisations", true),
+              NavigationItemsHelper::NavigationItem.new("Users", "/users", false),
+              NavigationItemsHelper::NavigationItem.new("Logs", "/logs", false),
+            ]
+          end
+
+          let(:expected_secondary_navigation_items) do
+            [
+              NavigationItemsHelper::NavigationItem.new("Logs", "/organisations/#{current_user.organisation.id}/logs", true),
+              NavigationItemsHelper::NavigationItem.new("Users", "/organisations/#{current_user.organisation.id}/users", false),
+              NavigationItemsHelper::NavigationItem.new("About this organisation", "/organisations/#{current_user.organisation.id}", false),
+            ]
+          end
+
+          it "returns navigation items with the logs item set as current" do
+            expect(primary_items("/organisations/#{current_user.organisation.id}/#{required_sub_path}", current_user)).to eq(expected_navigation_items)
+            expect(secondary_items("/organisations/#{current_user.organisation.id}/#{required_sub_path}", current_user.organisation.id)).to eq(expected_secondary_navigation_items)
+          end
+        end
+
+        context "when the user is on organisation users page" do
+          let(:required_sub_path) { "users" }
+          let(:expected_navigation_items) do
+            [
+              NavigationItemsHelper::NavigationItem.new("Organisations", "/organisations", true),
+              NavigationItemsHelper::NavigationItem.new("Users", "/users", false),
+              NavigationItemsHelper::NavigationItem.new("Logs", "/logs", false),
+            ]
+          end
+
+          let(:expected_secondary_navigation_items) do
+            [
+              NavigationItemsHelper::NavigationItem.new("Logs", "/organisations/#{current_user.organisation.id}/logs", false),
+              NavigationItemsHelper::NavigationItem.new("Users", "/organisations/#{current_user.organisation.id}/users", true),
+              NavigationItemsHelper::NavigationItem.new("About this organisation", "/organisations/#{current_user.organisation.id}", false),
+            ]
+          end
+
+          it "returns navigation items with the logs item set as current" do
+            expect(primary_items("/organisations/#{current_user.organisation.id}/#{required_sub_path}", current_user)).to eq(expected_navigation_items)
+            expect(secondary_items("/organisations/#{current_user.organisation.id}/#{required_sub_path}", current_user.organisation.id)).to eq(expected_secondary_navigation_items)
+          end
+        end
+
+        context "when the user is on organisation details page" do
+          let(:required_sub_path) { "details" }
+          let(:expected_navigation_items) do
+            [
+              NavigationItemsHelper::NavigationItem.new("Organisations", "/organisations", true),
+              NavigationItemsHelper::NavigationItem.new("Users", "/users", false),
+              NavigationItemsHelper::NavigationItem.new("Logs", "/logs", false),
+            ]
+          end
+
+          let(:expected_secondary_navigation_items) do
+            [
+              NavigationItemsHelper::NavigationItem.new("Logs", "/organisations/#{current_user.organisation.id}/logs", false),
+              NavigationItemsHelper::NavigationItem.new("Users", "/organisations/#{current_user.organisation.id}/users", false),
+              NavigationItemsHelper::NavigationItem.new("About this organisation", "/organisations/#{current_user.organisation.id}", true),
+            ]
+          end
+
+          it "returns navigation items with the logs item set as current" do
+            expect(primary_items("/organisations/#{current_user.organisation.id}/#{required_sub_path}", current_user)).to eq(expected_navigation_items)
+            expect(secondary_items("/organisations/#{current_user.organisation.id}/#{required_sub_path}", current_user.organisation.id)).to eq(expected_secondary_navigation_items)
+          end
+        end
+      end
     end
   end
 end

spec/helpers/tab_nav_helper_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe TabNavHelper do
 
   describe "#user_cell" do
     it "returns user link and email separated by a newline character" do
-      expected_html = "<a class=\"govuk-link\" href=\"/users\">Danny Rojas</a>\n<span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{user.email}</span>"
+      expected_html = "<a class=\"govuk-link\" href=\"/users\">#{user.name}</a>\n<span class=\"govuk-visually-hidden\">User </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{user.email}</span>"
       expect(user_cell(user)).to match(expected_html)
     end
   end

spec/requests/organisations_controller_spec.rb

@@ -131,9 +131,11 @@ RSpec.describe OrganisationsController, type: :request do
             expect(response.body).to include(user.email)
           end
 
-          it "has a hidden header title" do
+          it "shows hidden accesibility fields only for active users in the current user's organisation" do
-            expected_html = "<h2 class=\"govuk-visually-hidden\">  Users"
+            expected_case_row_log = "<span class=\"govuk-visually-hidden\">User </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{user.email}</span>"
-            expect(response.body).to include(expected_html)
+            unauthorized_case_row_log = "<span class=\"govuk-visually-hidden\">User </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{other_org_user.email}</span>"
+            expect(CGI.unescape_html(response.body)).to include(expected_case_row_log)
+            expect(CGI.unescape_html(response.body)).not_to include(unauthorized_case_row_log)
           end
 
           it "shows only active users in the current user's organisation" do
@@ -517,6 +519,138 @@ RSpec.describe OrganisationsController, type: :request do
         end
       end
 
+      context "when viewing a specific organisation users" do
+        let!(:users) { FactoryBot.create_list(:user, 5, organisation:) }
+        let!(:different_org_users) { FactoryBot.create_list(:user, 5) }
+
+        before do
+          get "/organisations/#{organisation.id}/users", headers:, params: {}
+        end
+
+        it "displays the name of the organisation" do
+          expect(page).to have_content(organisation.name)
+        end
+
+        it "has a sub-navigation with correct tabs" do
+          expect(page).to have_css(".app-sub-navigation")
+          expect(page).to have_content("Users")
+        end
+
+        it "displays users for this organisation" do
+          expect(page).to have_content(user.email)
+          users.each do |user|
+            expect(page).to have_content(user.email)
+          end
+        end
+
+        it "doesn't display users for other organisations" do
+          different_org_users.each do |different_org_user|
+            expect(page).not_to have_content(different_org_user.email)
+          end
+        end
+
+        context "when a search parameter is passed" do
+          let!(:matching_user) { FactoryBot.create(:user, organisation:, name: "joe", email: "matching@example.com") }
+          let(:org_user_count) { User.where(organisation:).count }
+
+          before do
+            get "/organisations/#{user.organisation.id}/users?search=#{search_param}"
+          end
+
+          context "when our search string matches case" do
+            let(:search_param) { "joe" }
+
+            it "returns only matching results" do
+              expect(page).to have_content(matching_user.name)
+              expect(page).not_to have_link(user.name)
+
+              different_org_users.each do |different_org_user|
+                expect(page).not_to have_content(different_org_user.email)
+              end
+
+              users.each do |org_user|
+                expect(page).not_to have_content(org_user.email)
+              end
+            end
+
+            it "updates the table caption" do
+              expect(page).to have_content("1 user found matching ‘#{search_param}’ of #{org_user_count} total users.")
+            end
+
+            context "when we need case insensitive search" do
+              let(:search_param) { "Joe" }
+
+              it "returns only matching results" do
+                expect(page).to have_content(matching_user.name)
+                expect(page).not_to have_link(user.name)
+
+                different_org_users.each do |different_org_user|
+                  expect(page).not_to have_content(different_org_user.email)
+                end
+
+                users.each do |org_user|
+                  expect(page).not_to have_content(org_user.email)
+                end
+              end
+
+              it "updates the table caption" do
+                expect(page).to have_content("1 user found matching ‘#{search_param}’ of #{org_user_count} total users.")
+              end
+            end
+          end
+
+          context "when our search term matches an email" do
+            let(:search_param) { "matching@example.com" }
+
+            it "returns only matching results" do
+              expect(page).to have_content(matching_user.name)
+              expect(page).not_to have_link(user.name)
+
+              different_org_users.each do |different_org_user|
+                expect(page).not_to have_content(different_org_user.email)
+              end
+
+              users.each do |org_user|
+                expect(page).not_to have_content(org_user.email)
+              end
+            end
+
+            it "updates the table caption" do
+              expect(page).to have_content("1 user found matching ‘#{search_param}’ of #{org_user_count} total users.")
+            end
+
+            context "when our search term matches an email and a name" do
+              let!(:matching_user) { FactoryBot.create(:user, organisation:, name: "Foobar", email: "some@example.com") }
+              let!(:another_matching_user) { FactoryBot.create(:user, organisation:, name: "Joe", email: "foobar@example.com") }
+              let!(:org_user_count) { User.where(organisation:).count }
+              let(:search_param) { "Foobar" }
+
+              before do
+                get "/organisations/#{user.organisation.id}/users?search=#{search_param}"
+              end
+
+              it "returns only matching results" do
+                expect(page).to have_link(matching_user.name)
+                expect(page).to have_link(another_matching_user.name)
+                expect(page).not_to have_link(user.name)
+
+                different_org_users.each do |different_org_user|
+                  expect(page).not_to have_content(different_org_user.email)
+                end
+
+                users.each do |org_user|
+                  expect(page).not_to have_content(org_user.email)
+                end
+              end
+
+              it "updates the table caption" do
+                expect(page).to have_content("2 users found matching ‘#{search_param}’ of #{org_user_count} total users.")
+              end
+            end
+          end
+        end
+      end
+
       context "when viewing a specific organisation details" do
         before do
           get "/organisations/#{organisation.id}/details", headers:, params: {}

spec/requests/users_controller_spec.rb

@@ -1593,16 +1593,31 @@ RSpec.describe UsersController, type: :request do
     describe "#new" do
       before do
         sign_in user
+        FactoryBot.create(:organisation, name: "other org")
       end
 
-      it "can assign support role to the new user" do
+      context "when support user" do
-        get "/users/new"
+        it "can assign support role to the new user" do
-        expect(page).to have_field("user-role-support-field")
+          get "/users/new"
-      end
+          expect(page).to have_field("user-role-support-field")
+        end
 
-      it "can assign organisation to the new user" do
+        it "can assign organisation to the new user" do
-        get "/users/new"
+          get "/users/new"
-        expect(page).to have_field("user-organisation-id-field")
+          expect(page).to have_field("user-organisation-id-field")
+        end
+
+        it "has all organisation names in the dropdown" do
+          get "/users/new"
+          expect(page).to have_select("user-organisation-id-field", with_options: Organisation.pluck(:name))
+        end
+
+        context "when organisation id is present in params and there are multiple organisations in the database" do
+          it "has only specific organisation name in the dropdown" do
+            get "/users/new", params: { organisation_id: user.organisation.id }
+            expect(page).to have_select("user-organisation-id-field", options: [user.organisation.name])
+          end
+        end
       end
     end
   end