From 780c731edb62d1e2d078813d559888baa77c98df Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Mon, 17 Jul 2023 15:29:21 +0100 Subject: [PATCH] CLDC-2243 Add owning organisation filter (#1772) * Add owning organisation filter * fix tests * Update filter content * Remove organisation filter --- app/helpers/filters_helper.rb | 8 ++--- app/models/lettings_log.rb | 1 + app/models/organisation.rb | 4 +++ app/models/sales_log.rb | 1 + app/models/user.rb | 4 +-- app/services/filter_manager.rb | 7 ++-- app/views/logs/_log_filters.html.erb | 16 ++++----- spec/features/organisation_spec.rb | 4 +-- spec/features/user_spec.rb | 18 +++++----- spec/helpers/filters_helper_spec.rb | 36 +++++++++---------- spec/models/lettings_log_spec.rb | 19 ++++++++++ spec/models/user_spec.rb | 10 +++--- .../requests/lettings_logs_controller_spec.rb | 23 +++++------- spec/requests/sales_logs_controller_spec.rb | 6 ---- 14 files changed, 86 insertions(+), 71 deletions(-) diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index f54e91fc9..4667310ef 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -4,8 +4,8 @@ module FiltersHelper selected_filters = JSON.parse(session[session_name_for(filter_type)]) return true if selected_filters.blank? && filter == "user" && value == :all - return true if !selected_filters.key?("organisation") && filter == "organisation_select" && value == :all - return true if selected_filters["organisation"].present? && filter == "organisation_select" && value == :specific_org + return true if !selected_filters.key?("owning_organisation") && filter == "owning_organisation_select" && value == :all + return true if selected_filters["owning_organisation"].present? && filter == "owning_organisation_select" && value == :specific_org return false if selected_filters[filter].blank? selected_filters[filter].include?(value.to_s) @@ -37,8 +37,8 @@ module FiltersHelper JSON.parse(session[session_name_for(filter_type)])[filter] || "" end - def organisations_filter_options(user) - organisation_options = user.support? ? Organisation.all : [user.organisation] + user.organisation.managing_agents + def owning_organisations_filter_options(user) + organisation_options = user.support? ? Organisation.all : [user.organisation] + user.organisation.stock_owners [OpenStruct.new(id: "", name: "Select an option")] + organisation_options.map { |org| OpenStruct.new(id: org.id, name: org.name) } end diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index a1094ee37..d1525590d 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -53,6 +53,7 @@ class LettingsLog < Log scope :filter_by_before_startdate, ->(date) { where("lettings_logs.startdate >= ?", date) } scope :unresolved, -> { where(unresolved: true) } scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org).or(where(managing_organisation: org)) } + scope :filter_by_owning_organisation, ->(org, _user = nil) { where(owning_organisation: org) } scope :duplicate_logs, lambda { |log| visible .where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) diff --git a/app/models/organisation.rb b/app/models/organisation.rb index ad74ac3e6..91c091720 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -118,4 +118,8 @@ class Organisation < ApplicationRecord def has_managing_agents? managing_agents.count.positive? end + + def has_stock_owners? + stock_owners.count.positive? + end end diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index ae76e32c3..47a3ef9f9 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -41,6 +41,7 @@ class SalesLog < Log .or(filter_by_id(param)) } scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org) } + scope :filter_by_owning_organisation, ->(org, _user = nil) { where(owning_organisation: org) } scope :duplicate_logs, lambda { |log| visible.where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) .where.not(id: log.id) diff --git a/app/models/user.rb b/app/models/user.rb index 2cf45de45..14b71be64 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -143,8 +143,8 @@ class User < ApplicationRecord end def logs_filters(specific_org: false) - if (support? && !specific_org) || organisation.has_managing_agents? - %w[status years user organisation bulk_upload_id] + if (support? && !specific_org) || organisation.has_stock_owners? + %w[status years user owning_organisation bulk_upload_id] else %w[status years user bulk_upload_id] end diff --git a/app/services/filter_manager.rb b/app/services/filter_manager.rb index 39cd7cdd3..005201031 100644 --- a/app/services/filter_manager.rb +++ b/app/services/filter_manager.rb @@ -21,7 +21,7 @@ class FilterManager filters.each do |category, values| next if Array(values).reject(&:empty?).blank? - next if category == "organisation" && all_orgs + next if category == "owning_organisation" && all_orgs logs = logs.public_send("filter_by_#{category}", values, user) end @@ -53,11 +53,12 @@ class FilterManager new_filters[filter] = params[filter] if params[filter].present? end end - params["organisation_select"] == "all" ? new_filters.except("organisation") : new_filters + new_filters = new_filters.except("owning_organisation") if params["owning_organisation_select"] == "all" + new_filters end def filtered_logs(logs, search_term, filters) - all_orgs = params["organisation_select"] == "all" + all_orgs = params["organisation_select"] == "all" && params["owning_organisation_select"] == "all" FilterManager.filter_logs(logs, search_term, filters, all_orgs, current_user) end diff --git a/app/views/logs/_log_filters.html.erb b/app/views/logs/_log_filters.html.erb index d52e25409..9b22348a1 100644 --- a/app/views/logs/_log_filters.html.erb +++ b/app/views/logs/_log_filters.html.erb @@ -52,23 +52,23 @@ category: "user", } %> - <% if (@current_user.support? || @current_user.organisation.has_managing_agents?) && request.path == "/lettings-logs" %> + <% if @current_user.support? || @current_user.organisation.stock_owners.count > 1 && request.path == "/lettings-logs" %> <%= render partial: "filters/radio_filter", locals: { f:, options: { - "all": { label: "All" }, + "all": { label: "Any owning organisation" }, "specific_org": { - label: "Specific organisation", + label: "Specific owning organisation", conditional_filter: { type: "select", - label: "Organisation", - category: "organisation", - options: organisations_filter_options(@current_user), + label: "Owning Organisation", + category: "owning_organisation", + options: owning_organisations_filter_options(@current_user), }, }, }, - label: "Organisation", - category: "organisation_select", + label: "Owned by", + category: "owning_organisation_select", } %> <% end %> diff --git a/spec/features/organisation_spec.rb b/spec/features/organisation_spec.rb index 53b8a75cc..23c0dad13 100644 --- a/spec/features/organisation_spec.rb +++ b/spec/features/organisation_spec.rb @@ -194,7 +194,7 @@ RSpec.describe "User Features" do end check("years-2021-field") click_button("Apply filters") - expect(page).to have_current_path("/organisations/#{org_id}/lettings-logs?years[]=&years[]=2021&status[]=&user=all") + expect(page).to have_current_path("/organisations/#{org_id}/lettings-logs?years[]=&years[]=2021&status[]=&user=all&owning_organisation_select=all&owning_organisation=") expect(page).not_to have_link first_log.id.to_s, href: "/lettings-logs/#{first_log.id}" end end @@ -227,7 +227,7 @@ RSpec.describe "User Features" do end check("years-2021-field") click_button("Apply filters") - expect(page).to have_current_path("/organisations/#{org_id}/sales-logs?years[]=&years[]=2021&status[]=&user=all") + expect(page).to have_current_path("/organisations/#{org_id}/sales-logs?years[]=&years[]=2021&status[]=&user=all&owning_organisation_select=all&owning_organisation=") expect(page).not_to have_link first_log.id.to_s, href: "/sales-logs/#{first_log.id}" end end diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index 62e185b2e..e7cff9f43 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -708,10 +708,12 @@ RSpec.describe "User Features" do end context "when viewing logs" do - context "when filtering by organisation and then switching back to all organisations", js: true do - let!(:organisation) { FactoryBot.create(:organisation, name: "Filtered Org") } + context "when filtering by owning organisation and then switching back to all organisations", js: true do + let!(:organisation) { FactoryBot.create(:organisation) } + let(:parent_organisation) { FactoryBot.create(:organisation, name: "Filtered Org") } before do + create(:organisation_relationship, child_organisation: organisation, parent_organisation:) allow(SecureRandom).to receive(:random_number).and_return(otp) click_button("Sign in") fill_in("code", with: otp) @@ -720,14 +722,14 @@ RSpec.describe "User Features" do it "clears the previously selected organisation value" do visit("/lettings-logs") - choose("organisation-select-specific-org-field", allow_label_click: true) - expect(page).to have_field("organisation-field", with: "") - find("#organisation-field").click.native.send_keys("F", "i", "l", "t", :down, :enter) + choose("owning-organisation-select-specific-org-field", allow_label_click: true) + expect(page).to have_field("owning-organisation-field", with: "") + find("#owning-organisation-field").click.native.send_keys("F", "i", "l", "t", :down, :enter) click_button("Apply filters") - expect(page).to have_current_path("/lettings-logs?%5Byears%5D%5B%5D=&%5Bstatus%5D%5B%5D=&user=all&organisation_select=specific_org&organisation=#{organisation.id}") - choose("organisation-select-all-field", allow_label_click: true) + expect(page).to have_current_path("/lettings-logs?%5Byears%5D%5B%5D=&%5Bstatus%5D%5B%5D=&user=all&owning_organisation_select=specific_org&owning_organisation=#{parent_organisation.id}") + choose("owning-organisation-select-all-field", allow_label_click: true) click_button("Apply filters") - expect(page).to have_current_path("/lettings-logs?%5Byears%5D%5B%5D=&%5Bstatus%5D%5B%5D=&user=all&organisation_select=all") + expect(page).to have_current_path("/lettings-logs?%5Byears%5D%5B%5D=&%5Bstatus%5D%5B%5D=&user=all&owning_organisation_select=all") end end end diff --git a/spec/helpers/filters_helper_spec.rb b/spec/helpers/filters_helper_spec.rb index 65d0c2c6b..44ef8e3ac 100644 --- a/spec/helpers/filters_helper_spec.rb +++ b/spec/helpers/filters_helper_spec.rb @@ -38,34 +38,34 @@ RSpec.describe FiltersHelper do context "when support user is using the organisation filter" do before do - session[:lettings_logs_filters] = { "organisation": "1" }.to_json + session[:lettings_logs_filters] = { "owning_organisation": "1" }.to_json end - it "returns true for the parent organisation_select filter" do - expect(filter_selected?("organisation_select", :specific_org, "lettings_logs")).to be true - expect(filter_selected?("organisation_select", :all, "lettings_logs")).to be false + it "returns true for the parent owning_organisation_select filter" do + expect(filter_selected?("owning_organisation_select", :specific_org, "lettings_logs")).to be true + expect(filter_selected?("owning_organisation_select", :all, "lettings_logs")).to be false end end - context "when support user has not set the organisation_select filter" do + context "when support user has not set the owning_organisation_select filter" do before do session[:lettings_logs_filters] = {}.to_json end it "defaults to all organisations" do - expect(filter_selected?("organisation_select", :all, "lettings_logs")).to be true - expect(filter_selected?("organisation_select", :specific_org, "lettings_logs")).to be false + expect(filter_selected?("owning_organisation_select", :all, "lettings_logs")).to be true + expect(filter_selected?("owning_organisation_select", :specific_org, "lettings_logs")).to be false end end - context "when the specific organisation filter is not set" do + context "when the specific owning organisation filter is not set" do before do session[:lettings_logs_filters] = { "status" => [""], "years" => [""], "user" => "all" }.to_json end it "marks the all options as checked" do - expect(filter_selected?("organisation_select", :all, "lettings_logs")).to be true - expect(filter_selected?("organisation_select", :specific_org, "lettings_logs")).to be false + expect(filter_selected?("owning_organisation_select", :all, "lettings_logs")).to be true + expect(filter_selected?("owning_organisation_select", :specific_org, "lettings_logs")).to be false end end end @@ -85,7 +85,7 @@ RSpec.describe FiltersHelper do end context "when organisation and user are set to all" do - let(:filters) { { "organisation_select" => "all", "user" => "all" } } + let(:filters) { { "owning_organisation_select" => "all", "user" => "all" } } it "returns false" do expect(result).to be_falsey @@ -160,7 +160,7 @@ RSpec.describe FiltersHelper do end end - describe "#organisations_filter_options" do + describe "#owning_organisations_filter_options" do let(:parent_organisation) { FactoryBot.create(:organisation, name: "Parent organisation") } let(:child_organisation) { FactoryBot.create(:organisation, name: "Child organisation") } @@ -170,10 +170,10 @@ RSpec.describe FiltersHelper do end context "with a support user" do - let(:user) { FactoryBot.create(:user, :support, organisation: parent_organisation) } + let(:user) { FactoryBot.create(:user, :support, organisation: child_organisation) } it "returns a list of all organisations" do - expect(organisations_filter_options(user)).to eq([ + expect(owning_organisations_filter_options(user)).to eq([ OpenStruct.new(id: "", name: "Select an option"), OpenStruct.new(id: parent_organisation.id, name: "Parent organisation"), OpenStruct.new(id: child_organisation.id, name: "Child organisation"), @@ -183,13 +183,13 @@ RSpec.describe FiltersHelper do end context "with a data coordinator user" do - let(:user) { FactoryBot.create(:user, :data_coordinator, organisation: parent_organisation) } + let(:user) { FactoryBot.create(:user, :data_coordinator, organisation: child_organisation) } - it "returns a list of managing agents and your own organisation" do - expect(organisations_filter_options(user)).to eq([ + it "returns a list of paret orgs and your own organisation" do + expect(owning_organisations_filter_options(user)).to eq([ OpenStruct.new(id: "", name: "Select an option"), - OpenStruct.new(id: parent_organisation.id, name: "Parent organisation"), OpenStruct.new(id: child_organisation.id, name: "Child organisation"), + OpenStruct.new(id: parent_organisation.id, name: "Parent organisation"), ]) end end diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index e2a15874a..80ac0d510 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -2769,6 +2769,25 @@ RSpec.describe LettingsLog do end end + context "when filtering by owning organisation" do + let(:organisation_1) { create(:organisation) } + let(:organisation_2) { create(:organisation) } + let(:organisation_3) { create(:organisation) } + + before do + create(:lettings_log, :in_progress, owning_organisation: organisation_1, managing_organisation: organisation_1, created_by: nil) + create(:lettings_log, :completed, owning_organisation: organisation_1, managing_organisation: organisation_2, created_by: nil) + create(:lettings_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_1, created_by: nil) + create(:lettings_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_2, created_by: nil) + end + + it "filters by given owning organisation" do + expect(described_class.filter_by_owning_organisation([organisation_1]).count).to eq(2) + expect(described_class.filter_by_owning_organisation([organisation_1, organisation_2]).count).to eq(4) + expect(described_class.filter_by_owning_organisation([organisation_3]).count).to eq(0) + end + end + context "when filtering on status" do it "allows filtering on a single status" do expect(described_class.filter_by_status(%w[in_progress]).count).to eq(2) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b9fb91c06..dae075b06 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -148,11 +148,11 @@ RSpec.describe User, type: :model do context "and their organisation has managing agents" do before do - create(:organisation_relationship, parent_organisation: user.organisation) + create(:organisation_relationship, child_organisation: user.organisation) end - it "can filter lettings logs by user, year, status and organisation" do - expect(user.logs_filters).to eq(%w[status years user organisation bulk_upload_id]) + it "can filter lettings logs by user, owning_organisation, year and status" do + expect(user.logs_filters).to eq(%w[status years user owning_organisation bulk_upload_id]) end end end @@ -192,8 +192,8 @@ RSpec.describe User, type: :model do }) end - it "can filter lettings logs by user, year, status and organisation" do - expect(user.logs_filters).to eq(%w[status years user organisation bulk_upload_id]) + it "can filter lettings logs by user, owning_organisation, year and status" do + expect(user.logs_filters).to eq(%w[status years user owning_organisation bulk_upload_id]) end end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 46f4027a9..688639339 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -354,12 +354,18 @@ RSpec.describe LettingsLogsController, type: :request do expect(page).not_to have_link(completed_lettings_log.id.to_s) end - it "filters on organisation" do - get "/lettings-logs?organisation[]=#{organisation_2.id}", headers:, params: {} + it "filters on owning organisation" do + get "/lettings-logs?owning_organisation[]=#{organisation_2.id}", headers:, params: {} expect(page).to have_link(completed_lettings_log.id.to_s) expect(page).not_to have_link(in_progress_lettings_log.id.to_s) end + it "filtering on owning organisation does not return managed orgs" do + get "/lettings-logs?owning_organisation[]=#{organisation.id}", headers:, params: {} + expect(page).not_to have_link(completed_lettings_log.id.to_s) + expect(page).to have_link(in_progress_lettings_log.id.to_s) + end + it "does not reset the filters" do get "/lettings-logs?status[]=in_progress", headers:, params: {} expect(page).to have_link(in_progress_lettings_log.id.to_s) @@ -780,25 +786,12 @@ RSpec.describe LettingsLogsController, type: :request do sign_in user end - it "does show the organisation filter" do - get "/lettings-logs", headers:, params: {} - expect(page).to have_field("organisation-field") - end - it "shows all logs by default" do get "/lettings-logs", headers:, params: {} expect(page).to have_content(tenant_code_1) expect(page).to have_content(tenant_code_2) end - context "when filtering by organisation" do - it "only show the selected organisations logs" do - get "/lettings-logs?organisation_select=specific_org&organisation=#{org_1.id}", headers:, params: {} - expect(page).to have_content(tenant_code_1) - expect(page).not_to have_content(tenant_code_2) - end - end - context "when the support user has filtered by organisation, then switches back to all organisations" do it "shows all organisations" do get "http://localhost:3000/lettings-logs?%5Byears%5D%5B%5D=&%5Bstatus%5D%5B%5D=&user=all&organisation_select=all&organisation=#{org_1.id}", headers:, params: {} diff --git a/spec/requests/sales_logs_controller_spec.rb b/spec/requests/sales_logs_controller_spec.rb index 9583cf197..7bac712df 100644 --- a/spec/requests/sales_logs_controller_spec.rb +++ b/spec/requests/sales_logs_controller_spec.rb @@ -250,12 +250,6 @@ RSpec.describe SalesLogsController, type: :request do expect(page).not_to have_link(completed_sales_log.id.to_s) end - it "filters on organisation" do - get "/sales-logs?organisation[]=#{organisation_2.id}", headers: headers, params: {} - expect(page).to have_link(completed_sales_log.id.to_s) - expect(page).not_to have_link(not_started_sales_log.id.to_s) - end - it "does not reset the filters" do get "/sales-logs?status[]=not_started", headers: headers, params: {} expect(page).to have_link(not_started_sales_log.id.to_s)