From 0aa46ca8be65739e4244214e38140645ed62eccf Mon Sep 17 00:00:00 2001 From: J G <7750475+moarpheus@users.noreply.github.com> Date: Wed, 1 Jun 2022 10:22:27 +0100 Subject: [PATCH] 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 --- app/components/search_component.rb | 6 +- app/controllers/organisations_controller.rb | 6 +- app/controllers/users_controller.rb | 1 + app/helpers/navigation_items_helper.rb | 29 ++-- app/helpers/tab_nav_helper.rb | 2 +- app/views/organisations/users.html.erb | 27 ++++ app/views/users/_user_list.html.erb | 3 + app/views/users/new.html.erb | 6 +- spec/features/log_spec.rb | 2 +- spec/features/organisation_spec.rb | 73 ++++++++- spec/helpers/navigation_items_helper_spec.rb | 74 +++++++++ spec/helpers/tab_nav_helper_spec.rb | 2 +- .../requests/organisations_controller_spec.rb | 140 +++++++++++++++++- spec/requests/users_controller_spec.rb | 29 +++- 14 files changed, 363 insertions(+), 37 deletions(-) create mode 100644 app/views/organisations/users.html.erb diff --git a/app/components/search_component.rb b/app/components/search_component.rb index 676257263..eefdc5309 100644 --- a/app/components/search_component.rb +++ b/app/components/search_component.rb @@ -9,10 +9,12 @@ class SearchComponent < ViewComponent::Base end def path(current_user) - if request.path.include?("users") - user_path(current_user) + if request.path.include?("organisations") && request.path.include?("users") + 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") diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 921953779..4452fc4a0 100644 --- a/app/controllers/organisations_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 7c52cdc0d..e0df2c2e6 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -62,6 +62,7 @@ class UsersController < ApplicationController end def new + @organisation_id = params["organisation_id"] @resource = User.new end diff --git a/app/helpers/navigation_items_helper.rb b/app/helpers/navigation_items_helper.rb index 0115d6a24..1ebf17d11 100644 --- a/app/helpers/navigation_items_helper.rb +++ b/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("About your organisation", "/organisations/#{current_user.organisation.id}", organisation_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}", 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("About this organisation", "/organisations/#{current_organisation_id}", about_organisation_current?(path, current_organisation_id)), + NavigationItem.new("Logs", "/organisations/#{current_organisation_id}/logs", subnav_logs_path?(path)), + 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) - path.include?("/organisations") && !path.include?("/users") + def organisations_current?(path) + path == "/organisations" || subnav_users_path?(path) || subnav_logs_path?(path) || subnav_details_path?(path) end - def about_organisation_current?(path, organisation_id) - path.include?("/organisations/#{organisation_id}/details") || path.include?("/organisations/#{organisation_id}/edit") + def subnav_users_path?(path) + path.include?("/organisations") && path.include?("/users") end - def organisation_logs_current?(path, organisation_id) - path == "/organisations/#{organisation_id}/logs" + def subnav_logs_path?(path) + path.include?("/organisations") && path.include?("/logs") + end + + def subnav_details_path?(path) + path.include?("/organisations") && path.include?("/details") end end diff --git a/app/helpers/tab_nav_helper.rb b/app/helpers/tab_nav_helper.rb index bc3e10f86..1157b83e6 100644 --- a/app/helpers/tab_nav_helper.rb +++ b/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), "#{user.email}"].join("\n") + [govuk_link_to(link_text, user), "User #{user.email}"].join("\n") end def org_cell(user) diff --git a/app/views/organisations/users.html.erb b/app/views/organisations/users.html.erb new file mode 100644 index 000000000..67df43109 --- /dev/null +++ b/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) %> + +
+ +<%= 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" } %> diff --git a/app/views/users/_user_list.html.erb b/app/views/users/_user_list.html.erb index 780d481f0..fb370c914 100644 --- a/app/views/users/_user_list.html.erb +++ b/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 %> + User <%= user.id %> + <% end %> <% end %> <% end %> <% end %> diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index 642337fc4..fa9fc26fc 100644 --- a/app/views/users/new.html.erb +++ b/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) } %> diff --git a/spec/features/log_spec.rb b/spec/features/log_spec.rb index 2e8812b63..31b556a5b 100644 --- a/spec/features/log_spec.rb +++ b/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) } diff --git a/spec/features/organisation_spec.rb b/spec/features/organisation_spec.rb index 8d00bf2dd..99e3657c6 100644 --- a/spec/features/organisation_spec.rb +++ b/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 diff --git a/spec/helpers/navigation_items_helper_spec.rb b/spec/helpers/navigation_items_helper_spec.rb index 9a060c759..1bcf23f1a 100644 --- a/spec/helpers/navigation_items_helper_spec.rb +++ b/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 diff --git a/spec/helpers/tab_nav_helper_spec.rb b/spec/helpers/tab_nav_helper_spec.rb index 4930d33b5..b59bb5d05 100644 --- a/spec/helpers/tab_nav_helper_spec.rb +++ b/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 = "Danny Rojas\n#{user.email}" + expected_html = "#{user.name}\nUser #{user.email}" expect(user_cell(user)).to match(expected_html) end end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index d49e6a61a..5647cfe7a 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/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 - expected_html = "

Users" - expect(response.body).to include(expected_html) + it "shows hidden accesibility fields only for active users in the current user's organisation" do + expected_case_row_log = "User #{user.email}" + unauthorized_case_row_log = "User #{other_org_user.email}" + 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: {} diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 9b951b742..7831a45d9 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 - get "/users/new" - expect(page).to have_field("user-role-support-field") - end + context "when support user" do + it "can assign support role to the new user" 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") + it "can assign organisation to the new user" do + get "/users/new" + 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