From 4032b678599f67cde93fa3046753c91d41341448 Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Thu, 27 Jul 2023 12:23:42 +0100 Subject: [PATCH] temp --- app/controllers/duplicate_logs_controller.rb | 16 ++-- app/controllers/lettings_logs_controller.rb | 5 +- app/controllers/organisations_controller.rb | 23 +++--- app/controllers/sales_logs_controller.rb | 27 ++++--- app/helpers/duplicate_logs_helper.rb | 64 +++++++++++----- app/models/sales_log.rb | 2 +- app/views/duplicate_logs/index.html.erb | 4 +- app/views/logs/index.html.erb | 4 +- app/views/organisations/logs.html.erb | 6 ++ config/routes.rb | 2 + spec/helpers/duplicate_logs_helper_spec.rb | 79 ++++++++++---------- 11 files changed, 135 insertions(+), 97 deletions(-) diff --git a/app/controllers/duplicate_logs_controller.rb b/app/controllers/duplicate_logs_controller.rb index 34656f4a8..c22be0a06 100644 --- a/app/controllers/duplicate_logs_controller.rb +++ b/app/controllers/duplicate_logs_controller.rb @@ -2,7 +2,6 @@ class DuplicateLogsController < ApplicationController include DuplicateLogsHelper before_action :authenticate_user! - before_action :authenticate_scope!, only: [:index] before_action :find_resource_by_named_id before_action :find_duplicate_logs before_action :find_original_log @@ -26,12 +25,19 @@ class DuplicateLogsController < ApplicationController end def index - @duplicates = params.permit(duplicates: {})[:duplicates]&.to_h || duplicates_for_user(current_user) - return render_not_found unless @duplicates + if current_user.data_provider? + @duplicates = duplicates_for_user(current_user) + elsif current_user.support? + organisation = Organisation.find(params[:organisation_id]) + render_not_found unless organisation + @duplicates = duplicates_for_organisation(organisation) + elsif current_user.data_coordinator? + @duplicates = duplicates_for_organisation(current_user.organisation) + end - @duplicates[:lettings] ||= {} - @duplicates[:sales] ||= {} @duplicate_sets_count = @duplicates[:lettings].count + @duplicates[:sales].count + + render_not_found if @duplicate_sets_count.zero? end private diff --git a/app/controllers/lettings_logs_controller.rb b/app/controllers/lettings_logs_controller.rb index 3d72c36ce..bc606d4c6 100644 --- a/app/controllers/lettings_logs_controller.rb +++ b/app/controllers/lettings_logs_controller.rb @@ -13,10 +13,6 @@ class LettingsLogsController < LogsController before_action :redirect_if_bulk_upload_resolved, only: [:index] def index - if current_user.data_provider? && (@duplicates = duplicates_for_user(current_user)) - @duplicate_sets_count = @duplicates[:lettings].count + @duplicates[:sales].count - end - all_logs = current_user.lettings_logs.visible unpaginated_filtered_logs = filter_manager.filtered_logs(all_logs, search_term, session_filters) @@ -26,6 +22,7 @@ class LettingsLogsController < LogsController @total_count = all_logs.size @unresolved_count = all_logs.unresolved.created_by(current_user).count @filter_type = "lettings_logs" + @duplicate_sets_count = duplicate_sets_count(current_user, nil) render "logs/index" end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index b01ea2b0f..6f6d39e1d 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -1,6 +1,7 @@ class OrganisationsController < ApplicationController include Pagy::Backend include Modules::SearchFilter + include DuplicateLogsHelper before_action :authenticate_user! before_action :find_resource, except: %i[index new create] @@ -96,18 +97,15 @@ class OrganisationsController < ApplicationController organisation_logs = LettingsLog.visible.where(owning_organisation_id: @organisation.id) unpaginated_filtered_logs = filter_manager.filtered_logs(organisation_logs, search_term, session_filters) - respond_to do |format| - format.html do - @search_term = search_term - @pagy, @logs = pagy(unpaginated_filtered_logs) - @delete_logs_path = delete_lettings_logs_organisation_path(search: @search_term) - @searched = search_term.presence - @total_count = organisation_logs.size - @log_type = :lettings - @filter_type = "lettings_logs" - render "logs", layout: "application" - end - end + @search_term = search_term + @pagy, @logs = pagy(unpaginated_filtered_logs) + @delete_logs_path = delete_lettings_logs_organisation_path(search: @search_term) + @searched = search_term.presence + @total_count = organisation_logs.size + @log_type = :lettings + @filter_type = "lettings_logs" + @duplicate_sets_count = duplicate_sets_count(current_user, @organisation) + render "logs", layout: "application" end def download_lettings_csv @@ -136,6 +134,7 @@ class OrganisationsController < ApplicationController @total_count = organisation_logs.size @log_type = :sales @filter_type = "sales_logs" + @duplicate_sets_count = duplicate_sets_count(current_user, @organisation) render "logs", layout: "application" end diff --git a/app/controllers/sales_logs_controller.rb b/app/controllers/sales_logs_controller.rb index f7c69ec87..7ddc1054d 100644 --- a/app/controllers/sales_logs_controller.rb +++ b/app/controllers/sales_logs_controller.rb @@ -1,4 +1,6 @@ class SalesLogsController < LogsController + include DuplicateLogsHelper + rescue_from ActiveRecord::RecordNotFound, with: :render_not_found before_action :session_filters, if: :current_user, only: %i[index email_csv download_csv] @@ -13,20 +15,17 @@ class SalesLogsController < LogsController end def index - respond_to do |format| - format.html do - all_logs = current_user.sales_logs.visible - unpaginated_filtered_logs = filter_manager.filtered_logs(all_logs, search_term, session_filters) - - @delete_logs_path = delete_logs_sales_logs_path(search: search_term) - @search_term = search_term - @pagy, @logs = pagy(unpaginated_filtered_logs) - @searched = search_term.presence - @total_count = all_logs.size - @filter_type = "sales_logs" - render "logs/index" - end - end + all_logs = current_user.sales_logs.visible + unpaginated_filtered_logs = filter_manager.filtered_logs(all_logs, search_term, session_filters) + + @delete_logs_path = delete_logs_sales_logs_path(search: search_term) + @search_term = search_term + @pagy, @logs = pagy(unpaginated_filtered_logs) + @searched = search_term.presence + @total_count = all_logs.size + @filter_type = "sales_logs" + @duplicate_sets_count = duplicate_sets_count(current_user, nil) + render "logs/index" end def show diff --git a/app/helpers/duplicate_logs_helper.rb b/app/helpers/duplicate_logs_helper.rb index a97c1ac4e..81c699b0d 100644 --- a/app/helpers/duplicate_logs_helper.rb +++ b/app/helpers/duplicate_logs_helper.rb @@ -29,38 +29,64 @@ module DuplicateLogsHelper end def duplicates_for_user(user) - duplicate_sets = { lettings: {}, sales: {} } - lettings_count = 0 - sales_count = 0 - duplicate_lettings_ids = Set.new - duplicate_sales_ids = Set.new + { + lettings: lettings_duplicate_sets_from_collection(LettingsLog.created_by(user), user.organisation), + sales: sales_duplicate_sets_from_collection(SalesLog.created_by(user), user.organisation), + } + end + + def duplicates_for_organisation(organisation) + { + lettings: lettings_duplicate_sets_from_collection(LettingsLog.filter_by_organisation(organisation), organisation), + sales: sales_duplicate_sets_from_collection(SalesLog.filter_by_organisation(organisation), organisation), + } + end - LettingsLog.created_by(user).visible.each do |log| - next if duplicate_lettings_ids.include? log.id + def sales_duplicate_sets_from_collection(logs, organisation) + duplicate_sets = [] + duplicate_ids_seen = Set.new - duplicates = user.lettings_logs.duplicate_logs(log) + logs.visible.each do |log| + next if duplicate_ids_seen.include? log.id + + duplicates = SalesLog.filter_by_organisation(organisation).duplicate_logs(log) next if duplicates.none? duplicate_ids = [log.id, *duplicates.map(&:id)] - duplicate_sets[:lettings][lettings_count] = duplicate_ids - lettings_count += 1 - duplicate_lettings_ids << duplicate_ids + duplicate_sets << duplicate_ids + duplicate_ids_seen.merge duplicate_ids end - SalesLog.created_by(user).visible.each do |log| - next if duplicate_sales_ids.include? log.id + duplicate_sets + end + + def lettings_duplicate_sets_from_collection(logs, organisation) + duplicate_sets = [] + duplicate_ids_seen = Set.new + + logs.visible.each do |log| + next if duplicate_ids_seen.include? log.id - duplicates = user.sales_logs.duplicate_logs(log) + duplicates = LettingsLog.filter_by_organisation(organisation).duplicate_logs(log) next if duplicates.none? duplicate_ids = [log.id, *duplicates.map(&:id)] - duplicate_sets[:sales][sales_count] = duplicate_ids - sales_count += 1 - duplicate_sales_ids << duplicate_ids + duplicate_sets << duplicate_ids + duplicate_ids_seen.merge duplicate_ids end - return if duplicate_lettings_ids.empty? && duplicate_sales_ids.empty? - duplicate_sets end + + def duplicate_sets_count(user, organisation) + duplicates = if user.support? + duplicates_for_organisation(organisation) + elsif user.data_coordinator? + duplicates_for_organisation(user.organisation) + elsif user.data_provider? + duplicates_for_user(user) + end + + duplicates[:lettings].count + duplicates[:sales].count + end end diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index fa5f623bd..99f5f3b46 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -49,7 +49,7 @@ class SalesLog < Log .where.not(sex1: nil) .where.not(ecstat1: nil) .where.not(postcode_full: nil) - .where("age1 IS NOT NULL OR age1_known = 1 OR age1_known = 2") + .age1_answered } scope :after_date, ->(date) { where("saledate >= ?", date) } diff --git a/app/views/duplicate_logs/index.html.erb b/app/views/duplicate_logs/index.html.erb index ccc1f983c..00687a2f6 100644 --- a/app/views/duplicate_logs/index.html.erb +++ b/app/views/duplicate_logs/index.html.erb @@ -7,7 +7,7 @@ <% end %> <% end %> <%= table.body do |body| %> - <% @duplicates[:lettings].each do |_i, duplicate_set| %> + <% @duplicates[:lettings].each do |duplicate_set| %> <% body.row do |row| %> <% row.cell text: "Lettings" %> <% row.cell text: duplicate_set.map { |id| "Log #{id}" }.join(", ") %> @@ -16,7 +16,7 @@ <% end %> <% end %> <% end %> - <% @duplicates[:sales].each do |_i, duplicate_set| %> + <% @duplicates[:sales].each do |duplicate_set| %> <% body.row do |row| %> <% row.cell text: "Sales" %> <% row.cell text: duplicate_set.map { |id| "Log #{id}" }.join(", ") %> diff --git a/app/views/logs/index.html.erb b/app/views/logs/index.html.erb index 97fa361f5..75086a24f 100644 --- a/app/views/logs/index.html.erb +++ b/app/views/logs/index.html.erb @@ -8,8 +8,8 @@ organisation: @organisation, ) %> -<% if @duplicates %> - <%= govuk_notification_banner(title_text: "Important", text: govuk_link_to("Review logs", duplicate_logs_path(duplicates: @duplicates))) do |banner| +<% if @duplicate_sets_count&.positive? %> + <%= govuk_notification_banner(title_text: "Important", text: govuk_link_to("Review logs", duplicate_logs_path)) do |banner| banner.with_heading(text: I18n.t("notification.duplicate_sets", count: @duplicate_sets_count)) end %> <% end %> diff --git a/app/views/organisations/logs.html.erb b/app/views/organisations/logs.html.erb index 89f54fb1d..3914baaf3 100644 --- a/app/views/organisations/logs.html.erb +++ b/app/views/organisations/logs.html.erb @@ -10,6 +10,12 @@ organisation: @organisation, ) %> +<% if @duplicate_sets_count&.positive? %> + <%= govuk_notification_banner(title_text: "Important", text: govuk_link_to("Review logs", organisation_duplicates_path(@organisation))) do |banner| + banner.with_heading(text: I18n.t("notification.duplicate_sets", count: @duplicate_sets_count)) + end %> +<% end %> + <% if current_user.support? %> <%= render SubNavigationComponent.new( items: secondary_items(request.path, @organisation.id), diff --git a/config/routes.rb b/config/routes.rb index 43d0c7232..9ba1a2f18 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -119,6 +119,8 @@ Rails.application.routes.draw do end resources :organisations do + get "duplicates", to: "duplicate_logs#index" + member do get "details", to: "organisations#details" get "data-sharing-agreement", to: "organisations#data_sharing_agreement" diff --git a/spec/helpers/duplicate_logs_helper_spec.rb b/spec/helpers/duplicate_logs_helper_spec.rb index 388a5a76b..0eeca38d8 100644 --- a/spec/helpers/duplicate_logs_helper_spec.rb +++ b/spec/helpers/duplicate_logs_helper_spec.rb @@ -6,61 +6,64 @@ RSpec.describe DuplicateLogsHelper do let(:current_user) { create(:user, organisation: org) } let(:user_same_org) { create(:user, organisation: org) } let(:user_other_org) { create(:user, organisation: other_org) } - - let(:result) { duplicates_for_user(current_user) } + let(:empty_duplicates) { { lettings: [], sales: [] } } let!(:lettings_log) { create(:lettings_log, :duplicate, created_by: current_user) } let!(:sales_log) { create(:sales_log, :duplicate, created_by: current_user) } - context "when there are no duplicates" do - it "returns nil" do - expect(result).to be nil - end - end + describe "#duplicates_for_user" do + let(:result) { duplicates_for_user(current_user) } - context "when there are duplicates in another org" do - before do - create(:lettings_log, :duplicate, created_by: user_other_org) - create(:sales_log, :duplicate, created_by: user_other_org) + context "when there are no duplicates" do + it "returns nil" do + expect(result).to eq empty_duplicates + end end - it "returns nil" do - expect(result).to be nil + context "when there are duplicates in another org" do + before do + create(:lettings_log, :duplicate, created_by: user_other_org) + create(:sales_log, :duplicate, created_by: user_other_org) + end + + it "does not locate duplicates" do + expect(result).to eq empty_duplicates + end end - end - context "when another user in the same org has created a duplicate lettings log" do - let!(:duplicate_lettings_log) { create(:lettings_log, :duplicate, created_by: user_same_org) } + context "when another user in the same org has created a duplicate lettings log" do + let!(:duplicate_lettings_log) { create(:lettings_log, :duplicate, created_by: user_same_org) } - it "returns the ids of the duplicates in a hash under the lettings key" do - expect(result).to be_a Hash - expect(result[:lettings].values).to match_array [[lettings_log.id, duplicate_lettings_log.id]] + it "returns the ids of the duplicates in a hash under the lettings key" do + expect(result).to be_a Hash + expect(result[:lettings]).to match_array [[lettings_log.id, duplicate_lettings_log.id]] + end end - end - context "when another user in the same org has created a duplicate sales log" do - let!(:duplicate_sales_log) { create(:sales_log, :duplicate, created_by: user_same_org) } + context "when another user in the same org has created a duplicate sales log" do + let!(:duplicate_sales_log) { create(:sales_log, :duplicate, created_by: user_same_org) } - it "returns the ids of the duplicates in a hash under the sales key" do - expect(result).to be_a Hash - expect(result[:sales].values).to match_array [[sales_log.id, duplicate_sales_log.id]] + it "returns the ids of the duplicates in a hash under the sales key" do + expect(result).to be_a Hash + expect(result[:sales]).to match_array [[sales_log.id, duplicate_sales_log.id]] + end end - end - context "when there are multiple sets of duplicates across lettings and sales" do - let!(:duplicate_lettings_log) { create(:lettings_log, :duplicate, created_by: user_same_org) } - let!(:duplicate_sales_log) { create(:sales_log, :duplicate, created_by: user_same_org) } - let!(:further_sales_log) { create(:sales_log, :duplicate, purchid: "further", created_by: current_user) } - let!(:further_duplicate_sales_logs) { create_list(:sales_log, 2, :duplicate, purchid: "further", created_by: user_same_org) } + context "when there are multiple sets of duplicates across lettings and sales" do + let!(:duplicate_lettings_log) { create(:lettings_log, :duplicate, created_by: user_same_org) } + let!(:duplicate_sales_log) { create(:sales_log, :duplicate, created_by: user_same_org) } + let!(:further_sales_log) { create(:sales_log, :duplicate, purchid: "further", created_by: current_user) } + let!(:further_duplicate_sales_logs) { create_list(:sales_log, 2, :duplicate, purchid: "further", created_by: user_same_org) } - it "returns them all with no repeats" do - expected_sales_duplicates_result = [ - [sales_log.id, duplicate_sales_log.id], - [further_sales_log.id, *further_duplicate_sales_logs.map(&:id)], - ] + it "returns them all with no repeats" do + expected_sales_duplicates_result = [ + [sales_log.id, duplicate_sales_log.id], + [further_sales_log.id, *further_duplicate_sales_logs.map(&:id)], + ] - expect(result[:lettings].values).to match_array [[lettings_log.id, duplicate_lettings_log.id]] - expect(result[:sales].values).to match_array expected_sales_duplicates_result + expect(result[:lettings]).to match_array [[lettings_log.id, duplicate_lettings_log.id]] + expect(result[:sales]).to match_array expected_sales_duplicates_result + end end end end