diff --git a/app/controllers/duplicate_logs_controller.rb b/app/controllers/duplicate_logs_controller.rb index e5ae95bbf..bc7579e17 100644 --- a/app/controllers/duplicate_logs_controller.rb +++ b/app/controllers/duplicate_logs_controller.rb @@ -1,8 +1,11 @@ class DuplicateLogsController < ApplicationController + include DuplicateLogsHelper + before_action :authenticate_user! before_action :find_resource_by_named_id - before_action :find_duplicate_logs + before_action :find_duplicates_for_a_log before_action :find_original_log + before_action :find_all_duplicates, only: [:index] def show if @log @@ -22,6 +25,13 @@ class DuplicateLogsController < ApplicationController render "logs/delete_duplicates" end + def index + render_not_found if @duplicates.blank? + + @duplicate_sets_count = @duplicates[:lettings].count + @duplicates[:sales].count + render_not_found if @duplicate_sets_count.zero? + end + private def find_resource_by_named_id @@ -32,7 +42,7 @@ private end end - def find_duplicate_logs + def find_duplicates_for_a_log return unless @log @duplicate_logs = if @log.lettings? @@ -42,6 +52,15 @@ private end end + def find_all_duplicates + return @duplicates = duplicates_for_user(current_user) if current_user.data_provider? + + organisation = current_user.support? ? Organisation.find(params[:organisation_id]) : current_user.organisation + return unless organisation + + @duplicates = duplicates_for_organisation(organisation) + end + def duplicate_check_question_ids if @log.lettings? ["owning_organisation_id", diff --git a/app/controllers/lettings_logs_controller.rb b/app/controllers/lettings_logs_controller.rb index 64e07efcd..058240b67 100644 --- a/app/controllers/lettings_logs_controller.rb +++ b/app/controllers/lettings_logs_controller.rb @@ -1,4 +1,6 @@ class LettingsLogsController < LogsController + include DuplicateLogsHelper + rescue_from ActiveRecord::RecordNotFound, with: :render_not_found before_action :find_resource, only: %i[update show] @@ -11,20 +13,17 @@ class LettingsLogsController < LogsController before_action :redirect_if_bulk_upload_resolved, only: [:index] def index - respond_to do |format| - format.html do - all_logs = current_user.lettings_logs.visible - unpaginated_filtered_logs = filter_manager.filtered_logs(all_logs, search_term, session_filters) - - @delete_logs_path = delete_logs_lettings_logs_path(search: search_term) - @pagy, @logs = pagy(unpaginated_filtered_logs) - @searched = search_term.presence - @total_count = all_logs.size - @unresolved_count = all_logs.unresolved.created_by(current_user).count - @filter_type = "lettings_logs" - render "logs/index" - end - end + all_logs = current_user.lettings_logs.visible + unpaginated_filtered_logs = filter_manager.filtered_logs(all_logs, search_term, session_filters) + + @delete_logs_path = delete_logs_lettings_logs_path(search: search_term) + @pagy, @logs = pagy(unpaginated_filtered_logs) + @searched = search_term.presence + @total_count = all_logs.size + @unresolved_count = all_logs.unresolved.created_by(current_user).count + @filter_type = "lettings_logs" + @duplicate_sets_count = FeatureToggle.duplicate_summary_enabled? && !current_user.support? ? duplicate_sets_count(current_user, current_user.organisation) : 0 + render "logs/index" end def create diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index b01ea2b0f..d94b7460b 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 = FeatureToggle.duplicate_summary_enabled? ? duplicate_sets_count(current_user, @organisation) : 0 + 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 = FeatureToggle.duplicate_summary_enabled? ? duplicate_sets_count(current_user, @organisation) : 0 render "logs", layout: "application" end diff --git a/app/controllers/sales_logs_controller.rb b/app/controllers/sales_logs_controller.rb index f7c69ec87..d97ed8b2a 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 = FeatureToggle.duplicate_summary_enabled? && !current_user.support? ? duplicate_sets_count(current_user, current_user.organisation) : 0 + render "logs/index" end def show diff --git a/app/helpers/duplicate_logs_helper.rb b/app/helpers/duplicate_logs_helper.rb index f1efec21b..e296cab1e 100644 --- a/app/helpers/duplicate_logs_helper.rb +++ b/app/helpers/duplicate_logs_helper.rb @@ -27,4 +27,27 @@ module DuplicateLogsHelper first_remaining_duplicate_id = all_duplicates.map(&:id).reject { |id| id == log.id }.first send("#{log.model_name.param_key}_#{page_id}_path", log, referrer: "duplicate_logs", first_remaining_duplicate_id:, original_log_id:) end + + def duplicates_for_user(user) + { + lettings: user.duplicate_lettings_logs_sets, + sales: user.duplicate_sales_logs_sets, + } + end + + def duplicates_for_organisation(organisation) + { + lettings: organisation.duplicate_lettings_logs_sets, + sales: organisation.duplicate_sales_logs_sets, + } + end + + def duplicate_sets_count(user, organisation) + duplicates = user.data_provider? ? duplicates_for_user(user) : duplicates_for_organisation(organisation) + duplicates[:lettings].count + duplicates[:sales].count + end + + def duplicate_list_header(duplicate_sets_count) + duplicate_sets_count > 1 ? "Review these #{duplicate_sets_count} sets of logs" : "Review this #{duplicate_sets_count} set of logs" + end end diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 6104ee3d5..9ab6329d2 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -45,31 +45,58 @@ class LettingsLog < Log scope :filter_by_location_postcode, ->(postcode_full) { left_joins(:location).where("REPLACE(locations.postcode, ' ', '') ILIKE ?", "%#{postcode_full.delete(' ')}%") } scope :search_by, lambda { |param| filter_by_location_postcode(param) - .or(filter_by_tenant_code(param)) - .or(filter_by_propcode(param)) - .or(filter_by_postcode(param)) - .or(filter_by_id(param)) + .or(filter_by_tenant_code(param)) + .or(filter_by_propcode(param)) + .or(filter_by_postcode(param)) + .or(filter_by_id(param)) } scope :after_date, ->(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, ->(owning_organisation, _user = nil) { where(owning_organisation:) } scope :filter_by_managing_organisation, ->(managing_organisation, _user = nil) { where(managing_organisation:) } - + scope :age1_answered, -> { where.not(age1: nil).or(where(age1_known: 1)) } + scope :tcharge_answered, -> { where.not(tcharge: nil).or(where(household_charge: 1)).or(where(is_carehome: 1)) } + scope :chcharge_answered, -> { where.not(chcharge: nil).or(where(is_carehome: [nil, 0])) } + scope :location_for_log_answered, ->(log) { where(location_id: log.location_id).or(where(needstype: 1)) } + scope :postcode_for_log_answered, ->(log) { where(postcode_full: log.postcode_full).or(where(needstype: 2)) } + scope :location_answered, -> { where.not(location_id: nil).or(where(needstype: 1)) } + scope :postcode_answered, -> { where.not(postcode_full: nil).or(where(needstype: 2)) } scope :duplicate_logs, lambda { |log| visible - .where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) .where.not(id: log.id) .where.not(startdate: nil) .where.not(sex1: nil) .where.not(ecstat1: nil) .where.not(needstype: nil) - .where("age1 IS NOT NULL OR age1_known = 1") - .where("tcharge IS NOT NULL OR household_charge = 1 OR is_carehome = 1") - .where("chcharge IS NOT NULL OR is_carehome IS NULL OR is_carehome = 0") - .where("location_id = ? OR needstype = 1", log.location_id) - .where("postcode_full = ? OR needstype = 2", log.postcode_full) + .age1_answered + .tcharge_answered + .chcharge_answered + .location_for_log_answered(log) + .postcode_for_log_answered(log) + .where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) + } + + scope :duplicate_sets, lambda { |created_by_id = nil| + scope = visible + .group(*DUPLICATE_LOG_ATTRIBUTES, :postcode_full, :location_id) + .where.not(startdate: nil) + .where.not(sex1: nil) + .where.not(ecstat1: nil) + .where.not(needstype: nil) + .age1_answered + .tcharge_answered + .chcharge_answered + .location_answered + .postcode_answered + .having( + "COUNT(*) > 1", + ) + + if created_by_id + scope = scope.having("MAX(CASE WHEN created_by_id = ? THEN 1 ELSE 0 END) >= 1", created_by_id) + end + scope.pluck("ARRAY_AGG(id)") } AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze diff --git a/app/models/log.rb b/app/models/log.rb index dd2cc8016..024470b21 100644 --- a/app/models/log.rb +++ b/app/models/log.rb @@ -81,6 +81,10 @@ class Log < ApplicationRecord collection_start_year end + def setup_completed? + form.setup_sections.all? { |sections| sections.subsections.all? { |subsection| subsection.status(self) == :completed } } + end + def lettings? false end diff --git a/app/models/organisation.rb b/app/models/organisation.rb index dac2b0ff5..77b5af50c 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -132,4 +132,12 @@ class Organisation < ApplicationRecord :active end + + def duplicate_lettings_logs_sets + lettings_logs.duplicate_sets.map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end + + def duplicate_sales_logs_sets + sales_logs.duplicate_sets.map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end end diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 4e829d760..283485d11 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -42,6 +42,7 @@ class SalesLog < Log } scope :filter_by_organisation, ->(owning_organisation, _user = nil) { where(owning_organisation:) } scope :filter_by_owning_organisation, ->(owning_organisation, _user = nil) { where(owning_organisation:) } + scope :age1_answered, -> { where.not(age1: nil).or(where(age1_known: [1, 2])) } scope :duplicate_logs, lambda { |log| visible.where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) .where.not(id: log.id) @@ -49,10 +50,27 @@ 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) } + scope :duplicate_sets, lambda { |created_by_id = nil| + scope = visible + .group(*DUPLICATE_LOG_ATTRIBUTES) + .where.not(saledate: nil) + .where.not(sex1: nil) + .where.not(ecstat1: nil) + .where.not(postcode_full: nil) + .age1_answered + .having("COUNT(*) > 1") + + if created_by_id + scope = scope.having("MAX(CASE WHEN created_by_id = ? THEN 1 ELSE 0 END) >= 1", created_by_id) + end + + scope.pluck("ARRAY_AGG(id)") + } + OPTIONAL_FIELDS = %w[purchid othtype].freeze RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze DUPLICATE_LOG_ATTRIBUTES = %w[owning_organisation_id purchid saledate age1_known age1 sex1 ecstat1 postcode_full].freeze @@ -120,10 +138,6 @@ class SalesLog < Log collection_start_year < 2023 end - def setup_completed? - form.setup_sections.all? { |sections| sections.subsections.all? { |subsection| subsection.status(self) == :completed } } - end - def unresolved false end diff --git a/app/models/user.rb b/app/models/user.rb index eccdaec47..5b7ec3e2f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -203,6 +203,14 @@ class User < ApplicationRecord super && active? end + def duplicate_lettings_logs_sets + lettings_logs.duplicate_sets(id).map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end + + def duplicate_sales_logs_sets + sales_logs.duplicate_sets(id).map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end + protected # Checks whether a password is needed or not. For validations only. diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index d7df829ad..0f5e03a79 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -33,4 +33,8 @@ class FeatureToggle def self.deduplication_flow_enabled? !Rails.env.production? && !Rails.env.staging? end + + def self.duplicate_summary_enabled? + !Rails.env.production? + end end diff --git a/app/views/duplicate_logs/index.html.erb b/app/views/duplicate_logs/index.html.erb new file mode 100644 index 000000000..f8a81cc29 --- /dev/null +++ b/app/views/duplicate_logs/index.html.erb @@ -0,0 +1,41 @@ +
+
+

<%= duplicate_list_header(@duplicates[:lettings].count + @duplicates[:sales].count) %>

+

+ These logs are duplicates because they have the same answers for certain fields. +

+

+ Review each set of logs and either delete any duplicates or change any incorrect answers. +

+
+
+ +<%= govuk_table do |table| %> + <%= table.head do |head| %> + <%= head.row do |row| %> + <% row.cell header: true, text: "Type of logs" %> + <% row.cell header: true, text: "Log IDs" %> + <% row.cell header: true %> + <% end %> + <% end %> + <%= table.body do |body| %> + <% @duplicates[:lettings].each do |duplicate_set| %> + <% body.row do |row| %> + <% row.cell text: "Lettings" %> + <% row.cell text: duplicate_set.map { |id| "Log #{id}" }.join(", ") %> + <% row.cell do %> + <%= govuk_link_to "Review logs", lettings_log_duplicate_logs_path(duplicate_set.first, original_log_id: duplicate_set.first) %> + <% end %> + <% end %> + <% end %> + <% @duplicates[:sales].each do |duplicate_set| %> + <% body.row do |row| %> + <% row.cell text: "Sales" %> + <% row.cell text: duplicate_set.map { |id| "Log #{id}" }.join(", ") %> + <% row.cell do %> + <%= govuk_link_to "Review logs", sales_log_duplicate_logs_path(duplicate_set.first, original_log_id: duplicate_set.first) %> + <% end %> + <% end %> + <% end %> + <% end %> +<% end %> diff --git a/app/views/logs/index.html.erb b/app/views/logs/index.html.erb index 12bddff79..b7c6ee4b0 100644 --- a/app/views/logs/index.html.erb +++ b/app/views/logs/index.html.erb @@ -8,6 +8,12 @@ organisation: @organisation, ) %> +<% 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 %> + <% if current_page?(controller: 'lettings_logs', action: 'index') %> <% if @unresolved_count > 0 %> <%= govuk_notification_banner( diff --git a/app/views/organisations/logs.html.erb b/app/views/organisations/logs.html.erb index 89f54fb1d..241621996 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/locales/en.yml b/config/locales/en.yml index 7ad173de8..0cf265b08 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -189,6 +189,9 @@ en: other: "%{log_ids} have been deleted." duplicate_logs: deduplication_success_banner: "%{log_link} is no longer a duplicate and has been removed from the list.

You changed the %{changed_question_label}.

" + duplicate_sets: + one: "There is %{count} set of duplicate logs" + other: "There are %{count} sets of duplicate logs" validations: organisation: diff --git a/config/routes.rb b/config/routes.rb index d509fc392..9ba1a2f18 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -105,6 +105,8 @@ Rails.application.routes.draw do end end + resources :duplicate_logs, only: [:index], path: "/duplicate-logs" + resources :users do get "edit-dpo", to: "users#dpo" get "edit-key-contact", to: "users#key_contact" @@ -117,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/factories/lettings_log.rb b/spec/factories/lettings_log.rb index ae203b79e..59eb48da2 100644 --- a/spec/factories/lettings_log.rb +++ b/spec/factories/lettings_log.rb @@ -35,12 +35,13 @@ FactoryBot.define do hhmemb { 1 } end trait :duplicate do + setup_completed status { 1 } - needstype { 1 } tenancycode { "same tenancy code" } postcode_full { "A1 1AA" } uprn_known { 0 } declaration { 1 } + age1_known { 0 } age1 { 18 } sex1 { "M" } ecstat1 { 0 } @@ -51,8 +52,6 @@ FactoryBot.define do supcharg { 35 } tcharge { 325 } propcode { "same property code" } - renewal { 1 } - rent_type { 1 } startdate { Time.zone.today } end trait :completed do diff --git a/spec/factories/sales_log.rb b/spec/factories/sales_log.rb index 54687c928..177ed600c 100644 --- a/spec/factories/sales_log.rb +++ b/spec/factories/sales_log.rb @@ -41,6 +41,7 @@ FactoryBot.define do type { 8 } jointpur { 2 } saledate { Time.zone.today } + age1_known { 1 } age1 { 20 } sex1 { "F" } ecstat1 { 1 } diff --git a/spec/helpers/duplicate_logs_helper_spec.rb b/spec/helpers/duplicate_logs_helper_spec.rb new file mode 100644 index 000000000..8cb971832 --- /dev/null +++ b/spec/helpers/duplicate_logs_helper_spec.rb @@ -0,0 +1,136 @@ +require "rails_helper" + +RSpec.describe DuplicateLogsHelper do + describe "#duplicates_for_user" do + let(:org) { create(:organisation) } + let(:other_org) { create(:organisation) } + 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!(:lettings_log) { create(:lettings_log, :duplicate, created_by: current_user) } + let!(:sales_log) { create(:sales_log, :duplicate, created_by: current_user) } + let(:result) { duplicates_for_user(current_user) } + + context "when there are no duplicates" do + it "returns empty duplicates" do + expect(result).to eq({ lettings: [], sales: [] }) + end + end + + 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({ lettings: [], sales: [] }) + 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) } + + 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 + + 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]).to match_array [[sales_log.id, duplicate_sales_log.id]] + end + end + + context "when there is a set of duplicate lettings logs not associated with the user" do + before do + create_list(:lettings_log, 3, :duplicate, tenancycode: "other", owning_organisation: org) + end + + 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 be_empty + end + end + + context "when there is a set of duplicate sales logs not associated with the user" do + before do + create_list(:sales_log, 3, :duplicate, purchid: "other", owning_organisation: org) + end + + 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 be_empty + 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) } + + 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]).to match_array [[lettings_log.id, duplicate_lettings_log.id]] + expect(result[:sales]).to match_array expected_sales_duplicates_result + end + end + end + + describe "#duplicates_for_organisation" do + let(:organisation) { create(:organisation) } + let(:sales_logs) { SalesLog.filter_by_organisation(organisation) } + + context "when there are no duplicates" do + it "returns empty duplicates" do + expect(duplicates_for_organisation(organisation)).to eq({ lettings: [], sales: [] }) + end + end + + context "when there are multiple sets of sales duplicates" do + let!(:duplicate_sales_logs) { create_list(:sales_log, 4, :duplicate, purchid: "set 1", owning_organisation: organisation) } + let!(:duplicate_sales_logs_too) { create_list(:sales_log, 5, :duplicate, postcode_full: "B1 1BB", owning_organisation: organisation) } + let!(:duplicate_sales_logs_3) { create_list(:sales_log, 3, :duplicate, age1: 38, owning_organisation: organisation) } + + let!(:duplicate_lettings_logs) { create_list(:lettings_log, 4, :duplicate, tenancycode: "set 1", owning_organisation: organisation) } + let!(:duplicate_lettings_logs_too) { create_list(:lettings_log, 5, :duplicate, postcode_full: "B1 1BB", owning_organisation: organisation) } + let!(:duplicate_lettings_logs_3) { create_list(:lettings_log, 3, :duplicate, age1: 38, owning_organisation: organisation) } + + before do + create_list(:sales_log, 3, :duplicate, discarded_at: Time.zone.now, status: 4, owning_organisation: organisation) + create_list(:lettings_log, 3, :duplicate, discarded_at: Time.zone.now, status: 4, owning_organisation: organisation) + end + + it "returns them all with no repeats" do + expected_sales_duplicates_result = [ + duplicate_sales_logs.map(&:id), + duplicate_sales_logs_too.map(&:id), + duplicate_sales_logs_3.map(&:id), + ] + + expected_lettings_duplicates_result = [ + duplicate_lettings_logs.map(&:id), + duplicate_lettings_logs_too.map(&:id), + duplicate_lettings_logs_3.map(&:id), + ] + + expect(duplicates_for_organisation(organisation)[:lettings]).to match_array( + expected_lettings_duplicates_result.map { |nested_array| match_array(nested_array) }, + ) + expect(duplicates_for_organisation(organisation)[:sales]).to match_array( + expected_sales_duplicates_result.map { |nested_array| match_array(nested_array) }, + ) + end + end + end +end diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index 36c793c79..e89015233 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -2980,6 +2980,189 @@ RSpec.describe LettingsLog do end end end + + context "when getting list of duplicate logs" do + let(:organisation) { create(:organisation) } + let!(:log) { create(:lettings_log, :duplicate, owning_organisation: organisation) } + let!(:duplicate_log) { create(:lettings_log, :duplicate, owning_organisation: organisation) } + let(:duplicate_sets) { described_class.duplicate_sets } + + it "returns a list of duplicates in the same organisation" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + + context "when there is a deleted duplicate log" do + before do + create(:lettings_log, :duplicate, discarded_at: Time.zone.now, status: 4) + end + + it "does not return the deleted log as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different start date" do + before do + create(:lettings_log, :duplicate, startdate: Time.zone.tomorrow) + end + + it "does not return a log with a different start date as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different age1" do + before do + create(:lettings_log, :duplicate, age1: 50) + end + + it "does not return a log with a different age1 as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different sex1" do + before do + create(:lettings_log, :duplicate, sex1: "F") + end + + it "does not return a log with a different sex1 as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different ecstat1" do + before do + create(:lettings_log, :duplicate, ecstat1: 1) + end + + it "does not return a log with a different ecstat1 as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different tcharge" do + before do + create(:lettings_log, :duplicate, brent: 100) + end + + it "does not return a log with a different tcharge as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(duplicate_log.id, log.id) + end + end + + context "when there is a log with a different tenancycode" do + before do + create(:lettings_log, :duplicate, tenancycode: "different") + end + + it "does not return a log with a different tenancycode as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different postcode_full" do + before do + create(:lettings_log, :duplicate, postcode_full: "B1 1AA") + end + + it "does not return a log with a different postcode_full as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with nil values for duplicate check fields" do + before do + create(:lettings_log, :duplicate, age1: nil, sex1: nil, ecstat1: nil, postcode_known: 2, postcode_full: nil) + end + + it "does not return a log with nil values as a duplicate" do + log.update!(age1: nil, sex1: nil, ecstat1: nil, postcode_known: 2, postcode_full: nil) + expect(duplicate_sets).to be_empty + end + end + + context "when there is a log with nil values for tenancycode" do + let!(:tenancycode_not_given) { create(:lettings_log, :duplicate, tenancycode: nil, owning_organisation: organisation) } + + it "returns the log as a duplicate if tenancy code is nil" do + log.update!(tenancycode: nil) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, tenancycode_not_given.id) + end + end + + context "when there is a log with age1 not known" do + let!(:age1_not_known) { create(:lettings_log, :duplicate, age1_known: 1, age1: nil, owning_organisation: organisation) } + + it "returns the log as a duplicate if age1 is not known" do + log.update!(age1_known: 1, age1: nil) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(age1_not_known.id, log.id) + end + end + + context "when there is a duplicate supported housing log" do + let(:scheme) { create(:scheme) } + let(:location) { create(:location, scheme:) } + let(:location_2) { create(:location, scheme:) } + let!(:supported_housing_log) { create(:lettings_log, :duplicate, needstype: 2, location:, scheme:, owning_organisation: organisation) } + let!(:duplicate_supported_housing_log) { create(:lettings_log, :duplicate, needstype: 2, location:, scheme:, owning_organisation: organisation) } + + it "returns the log as a duplicate" do + expect(duplicate_sets.count).to eq(2) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + expect(duplicate_sets.second).to contain_exactly(duplicate_supported_housing_log.id, supported_housing_log.id) + end + + it "does not return the log if the locations are different" do + duplicate_supported_housing_log.update!(location: location_2) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + + it "does not compare tcharge if there are no household charges" do + supported_housing_log.update!(household_charge: 1, supcharg: nil, brent: nil, scharge: nil, pscharge: nil, tcharge: nil, owning_organisation: organisation) + duplicate_supported_housing_log.update!(household_charge: 1, supcharg: nil, brent: nil, scharge: nil, pscharge: nil, tcharge: nil, owning_organisation: organisation) + expect(duplicate_sets.count).to eq(2) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + expect(duplicate_sets.second).to contain_exactly(supported_housing_log.id, duplicate_supported_housing_log.id) + end + + it "does not return logs not associated with the user if user is given" do + user = create(:user) + supported_housing_log.update!(created_by: user, owning_organisation: user.organisation) + duplicate_supported_housing_log.update!(owning_organisation: user.organisation) + duplicate_sets = described_class.duplicate_sets(user.id) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(supported_housing_log.id, duplicate_supported_housing_log.id) + end + + it "compares chcharge if it's a carehome" do + supported_housing_log.update!(is_carehome: 1, chcharge: 100, supcharg: nil, brent: nil, scharge: nil, pscharge: nil, tcharge: nil, owning_organisation: organisation) + duplicate_supported_housing_log.update!(is_carehome: 1, chcharge: 100, supcharg: nil, brent: nil, scharge: nil, pscharge: nil, tcharge: nil, owning_organisation: organisation) + expect(duplicate_sets.count).to eq(2) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + expect(duplicate_sets.second).to contain_exactly(supported_housing_log.id, duplicate_supported_housing_log.id) + end + + it "does not return a duplicate if carehome charge is not given" do + supported_housing_log.update!(is_carehome: 1, chcharge: nil, supcharg: nil, brent: nil, scharge: nil, pscharge: nil, tcharge: nil, owning_organisation: organisation) + duplicate_supported_housing_log.update!(is_carehome: 1, chcharge: nil, supcharg: nil, brent: nil, scharge: nil, pscharge: nil, tcharge: nil, owning_organisation: organisation) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + end end describe "#retirement_age_for_person" do diff --git a/spec/models/sales_log_spec.rb b/spec/models/sales_log_spec.rb index 1aa27cbd3..15f4e6518 100644 --- a/spec/models/sales_log_spec.rb +++ b/spec/models/sales_log_spec.rb @@ -279,6 +279,163 @@ RSpec.describe SalesLog, type: :model do end end + context "when getting list of duplicate logs" do + let(:organisation) { create(:organisation) } + let!(:log) { create(:sales_log, :duplicate, owning_organisation: organisation) } + let!(:duplicate_log) { create(:sales_log, :duplicate, owning_organisation: organisation) } + let(:duplicate_sets) { described_class.duplicate_sets } + + it "returns a list of duplicates in the same organisation" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + + context "when there is a deleted duplicate log" do + before do + create(:sales_log, :duplicate, discarded_at: Time.zone.now, status: 4) + end + + it "does not return the deleted log as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different sale date" do + before do + create(:sales_log, :duplicate, saledate: Time.zone.tomorrow) + end + + it "does not return a log with a different sale date as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different age1" do + before do + create(:sales_log, :duplicate, age1: 50) + end + + it "does not return a log with a different age1 as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different sex1" do + before do + create(:sales_log, :duplicate, sex1: "X") + end + + it "does not return a log with a different sex1 as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different ecstat1" do + before do + create(:sales_log, :duplicate, ecstat1: 9) + end + + it "does not return a log with a different ecstat1 as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different purchid" do + before do + create(:sales_log, :duplicate, purchid: "different") + end + + it "does not return a log with a different purchid as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with a different postcode_full" do + before do + create(:sales_log, :duplicate, postcode_full: "B1 1AA") + end + + it "does not return a log with a different postcode_full as a duplicate" do + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + + context "when there is a log with nil values for duplicate check fields" do + before do + create(:sales_log, :duplicate, age1: nil, sex1: nil, ecstat1: nil, pcodenk: 1, postcode_full: nil) + end + + it "does not return a log with nil values as a duplicate" do + log.update!(age1: nil, sex1: nil, ecstat1: nil, pcodenk: 1, postcode_full: nil) + expect(duplicate_sets).to be_empty + end + end + + context "when there is a log with nil values for purchid" do + let!(:purchid_not_given) { create(:sales_log, :duplicate, purchid: nil, owning_organisation: organisation) } + + it "returns the log as a duplicate if tenancy code is nil" do + log.update!(purchid: nil) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, purchid_not_given.id) + end + end + + context "when there is a log with age1 not known" do + let!(:age1_not_known) { create(:sales_log, :duplicate, age1_known: 1, age1: nil, owning_organisation: organisation) } + + it "returns the log as a duplicate if age1 is not known" do + log.update!(age1_known: 1, age1: nil) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(age1_not_known.id, log.id) + end + end + + context "when there is a log with age1 prefers not to say" do + let!(:age1_prefers_not_to_say) { create(:sales_log, :duplicate, age1_known: 2, age1: nil, owning_organisation: organisation) } + + it "returns the log as a duplicate if age1 is prefers not to say" do + log.update!(age1_known: 2, age1: nil) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(age1_prefers_not_to_say.id, log.id) + end + end + + context "when there is a log with age1 not known and prefers not to say" do + before do + create(:sales_log, :duplicate, age1_known: 2, age1: nil) + end + + it "doe not return the log as a duplicate" do + log.update!(age1_known: 1, age1: nil) + expect(duplicate_sets).to be_empty + end + end + + context "when user is given" do + let(:user) { create(:user) } + + before do + create_list(:sales_log, 2, :duplicate, purchid: "other duplicates") + log.update!(created_by: user, owning_organisation: user.organisation) + end + + it "does not return logs not associated with the given user" do + duplicate_log.update!(owning_organisation: user.organisation) + duplicate_sets = described_class.duplicate_sets(user.id) + expect(duplicate_sets.count).to eq(1) + expect(duplicate_sets.first).to contain_exactly(log.id, duplicate_log.id) + end + end + end + describe "derived variables" do let(:sales_log) { create(:sales_log, :completed) } diff --git a/spec/requests/duplicate_logs_controller_spec.rb b/spec/requests/duplicate_logs_controller_spec.rb index 096b5ed61..fa989e836 100644 --- a/spec/requests/duplicate_logs_controller_spec.rb +++ b/spec/requests/duplicate_logs_controller_spec.rb @@ -313,4 +313,78 @@ RSpec.describe DuplicateLogsController, type: :request do end end end + + describe "GET #index" do + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end + + context "when the user is support" do + let(:user) { create(:user, :support) } + + it "renders not found" do + get duplicate_logs_path(organisation_id: user.organisation.id) + expect(response).to have_http_status(:not_found) + end + end + + context "when the user is a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + before do + allow(user.organisation).to receive(:duplicate_lettings_logs_sets).and_return([[1, 2], [3, 4, 5]]) + allow(user.organisation).to receive(:duplicate_sales_logs_sets).and_return([[11, 12]]) + end + + it "gets organisation duplicates" do + expect(user.organisation).to receive(:duplicate_lettings_logs_sets) + expect(user.organisation).to receive(:duplicate_sales_logs_sets) + get duplicate_logs_path(organisation_id: user.organisation.id) + end + end + + context "when the user is a provider" do + let(:user) { create(:user) } + + before do + allow(user).to receive(:duplicate_lettings_logs_sets).and_return([[1, 2], [3, 4, 5]]) + allow(user).to receive(:duplicate_sales_logs_sets).and_return([[11, 12]]) + end + + it "calls the helper method to retrieve duplicates for the current user" do + expect(user).to receive(:duplicate_lettings_logs_sets) + expect(user).to receive(:duplicate_sales_logs_sets) + get duplicate_logs_path + end + + describe "viewing the page" do + before do + get duplicate_logs_path + end + + it "has the correct headers" do + expect(page).to have_content("Type of logs") + expect(page).to have_content("Log IDs") + end + + it "has the correct number of rows for each log type" do + expect(page).to have_selector("tbody tr td", text: "Lettings", count: 2) + expect(page).to have_selector("tbody tr td", text: "Sales", count: 1) + end + + it "shows the log ids for each set of duplicates" do + expect(page).to have_content("Log 1, Log 2") + expect(page).to have_content("Log 3, Log 4, Log 5") + expect(page).to have_content("Log 11, Log 12") + end + + it "shows links for each set of duplciates" do + expect(page).to have_link("Review logs", href: lettings_log_duplicate_logs_path(1, original_log_id: 1)) + expect(page).to have_link("Review logs", href: lettings_log_duplicate_logs_path(3, original_log_id: 3)) + expect(page).to have_link("Review logs", href: sales_log_duplicate_logs_path(11, original_log_id: 11)) + end + end + end + end end diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index 8e8a965cb..5f0e7b273 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -797,7 +797,7 @@ RSpec.describe FormController, type: :request do end it "redirects back to the duplicates page for remaining duplicates" do - expect(response).to have_http_status(:ok) + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") end end end @@ -805,13 +805,13 @@ RSpec.describe FormController, type: :request do context "when the sales question was accessed from a duplicate logs screen" do let(:sales_log) { create(:sales_log, :duplicate, created_by: user) } let(:duplicate_log) { create(:sales_log, :duplicate, created_by: user) } - let(:referrer) { "/sales-logs/#{sales_log.id}/buyer-1-age?referrer=duplicate_logs&first_remaining_duplicate_id=#{duplicate_log.id}&original_log_id=#{sales_log.id}" } + let(:referrer) { "/sales-logs/#{sales_log.id}/buyer-1-age?referrer=duplicate_logs&first_remaining_duplicate_id=#{duplicate_log.id}&original_log_id=#{sales_log.id}&referrer=duplicate_logs" } let(:params) do { id: sales_log.id, sales_log: { page: "buyer_1_age", - age1: 20, + age1: 29, age1_known: 1, }, } @@ -838,7 +838,7 @@ RSpec.describe FormController, type: :request do end it "redirects back to the duplicates page for remaining duplicates" do - expect(response).to have_http_status(:ok) + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") end end end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 6cf53acc9..b4701517f 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -238,8 +238,6 @@ RSpec.describe LettingsLogsController, type: :request do let(:headers) { { "Accept" => "text/html" } } context "when you visit the index page" do - let(:user) { FactoryBot.create(:user, :support) } - before do allow(user).to receive(:need_two_factor_authentication?).and_return(false) sign_in user @@ -308,6 +306,18 @@ RSpec.describe LettingsLogsController, type: :request do expect(page).to have_link("Download (CSV, codes only)", href: "/lettings-logs/csv-download?codes_only=true") end + context "when there are duplicate logs for this user" do + before do + FactoryBot.create_list(:lettings_log, 2, :duplicate, owning_organisation: user.organisation, created_by: user) + end + + it "does not show a notification banner even if there are duplicate logs for this user" do + get lettings_logs_path + expect(page).not_to have_content "duplicate logs" + expect(page).not_to have_link "Review logs" + end + end + context "when there are no logs in the database" do before do LettingsLog.destroy_all @@ -615,6 +625,12 @@ RSpec.describe LettingsLogsController, type: :request do expect(page).not_to have_link("Download (CSV, codes only)") end + it "does not show a notification banner even if there are duplicate logs for this user" do + get lettings_logs_path + expect(page).not_to have_content "duplicate logs" + expect(page).not_to have_link "Review logs" + end + context "when using a search query" do let(:logs) { FactoryBot.create_list(:lettings_log, 3, :completed, owning_organisation: user.organisation, created_by: user) } let(:log_to_search) { FactoryBot.create(:lettings_log, :completed, owning_organisation: user.organisation, created_by: user) } @@ -871,6 +887,36 @@ RSpec.describe LettingsLogsController, type: :request do end end end + + context "and there are duplicate logs for this user" do + before do + FactoryBot.create_list(:lettings_log, 2, :duplicate, owning_organisation: user.organisation, created_by: user) + end + + it "displays a notification banner with a link to review logs" do + get lettings_logs_path + expect(page).to have_content "duplicate logs" + expect(page).to have_link "Review logs" # add an href when routing done + end + + context "when there is one set of duplicates" do + it "displays the correct copy in the banner" do + get lettings_logs_path + expect(page).to have_content "There is 1 set of duplicate logs" + end + end + + context "when there are multiple sets of duplicates" do + before do + FactoryBot.create_list(:sales_log, 2, :duplicate, owning_organisation: user.organisation, created_by: user) + end + + it "displays the correct copy in the banner" do + get lettings_logs_path + expect(page).to have_content "There are 2 sets of duplicate logs" + end + end + end end end