From ea853c484dcd29c1f5a28ade09652be7ea0dc9bb Mon Sep 17 00:00:00 2001 From: Rachael Booth Date: Thu, 22 Aug 2024 11:46:25 +0100 Subject: [PATCH] CLDC-3566: Clear duplicate set ids when a log ceases to be a duplicate in normal update flow - with better performance (#2573) * Revert "Revert "CLDC-3566: Clear duplicate set ids when a log ceases to be a duplicate through normal update flow (#2534)" (#2563)" This reverts commit 53ae216bae9f7d7be5b563fe9f02e8634f61c3a8. * CLDC-3566: Avoid unnecessary queries for duplicates --- app/controllers/form_controller.rb | 80 ++++--- app/models/lettings_log.rb | 4 +- app/models/sales_log.rb | 4 +- app/services/feature_toggle.rb | 4 - spec/requests/form_controller_spec.rb | 311 +++++++++++++++++++++++--- 5 files changed, 323 insertions(+), 80 deletions(-) diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index d84f8642a..8a95464a7 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -26,6 +26,8 @@ class FormController < ApplicationController flash[:notice] = "You have successfully updated #{updated_question_string}" end + update_duplication_tracking + pages_requiring_update = pages_requiring_update(shown_page_ids_with_unanswered_questions_before_update) redirect_to(successful_redirect_path(pages_requiring_update)) else @@ -192,24 +194,49 @@ private params[@log.model_name.param_key]["interruption_page_referrer_type"].presence end - def successful_redirect_path(pages_to_check) - if FeatureToggle.deduplication_flow_enabled? - if is_referrer_type?("duplicate_logs") || is_referrer_type?("duplicate_logs_banner") - return correcting_duplicate_logs_redirect_path + def page_has_duplicate_check_question + @page.questions.any? { |q| @log.duplicate_check_question_ids.include?(q.id) } + end + + def update_duplication_tracking + return unless page_has_duplicate_check_question + + class_name = @log.class.name.underscore + dynamic_duplicates = current_user.send(class_name.pluralize).duplicate_logs(@log) + + if dynamic_duplicates.any? + saved_duplicates = @log.duplicates + if saved_duplicates.none? || duplicates_changed?(dynamic_duplicates, saved_duplicates) + duplicate_set_id = dynamic_duplicates.first.duplicate_set_id || new_duplicate_set_id(@log) + update_logs_with_duplicate_set_id(@log, dynamic_duplicates, duplicate_set_id) + saved_duplicates.first.update!(duplicate_set_id: nil) if saved_duplicates.count == 1 end + else + remove_fixed_duplicate_set_ids(@log) + end + end - dynamic_duplicates = @log.lettings? ? current_user.lettings_logs.duplicate_logs(@log) : current_user.sales_logs.duplicate_logs(@log) - if dynamic_duplicates.any? - saved_duplicates = @log.duplicates - if saved_duplicates.none? || duplicates_changed?(dynamic_duplicates, saved_duplicates) - duplicate_set_id = dynamic_duplicates.first.duplicate_set_id || new_duplicate_set_id(@log) - update_logs_with_duplicate_set_id(@log, dynamic_duplicates, duplicate_set_id) - saved_duplicates.first.update!(duplicate_set_id: nil) if saved_duplicates.count == 1 + def successful_redirect_path(pages_to_check) + class_name = @log.class.name.underscore + + if is_referrer_type?("duplicate_logs") || is_referrer_type?("duplicate_logs_banner") + original_log = current_user.send(class_name.pluralize).find_by(id: from_referrer_query("original_log_id")) + + if original_log.present? && current_user.send(class_name.pluralize).duplicate_logs(original_log).any? + if @log.duplicate_set_id.nil? + flash[:notice] = deduplication_success_banner end - return send("#{@log.class.name.underscore}_duplicate_logs_path", @log, original_log_id: @log.id) + return send("#{class_name}_duplicate_logs_path", original_log, original_log_id: original_log.id, referrer: params[:referrer], organisation_id: params[:organisation_id]) + else + flash[:notice] = deduplication_success_banner + return send("#{class_name}_duplicate_logs_path", "#{class_name}_id".to_sym => from_referrer_query("first_remaining_duplicate_id"), original_log_id: from_referrer_query("original_log_id"), referrer: params[:referrer], organisation_id: params[:organisation_id]) end end + unless @log.duplicate_set_id.nil? + return send("#{@log.class.name.underscore}_duplicate_logs_path", @log, original_log_id: @log.id) + end + if is_referrer_type?("check_answers") next_page_id = form.next_page_id(@page, @log, current_user) next_page = form.get_page(next_page_id) @@ -315,35 +342,6 @@ private CONFIRMATION_PAGE_IDS = %w[uprn_confirmation uprn_selection].freeze - def correcting_duplicate_logs_redirect_path - class_name = @log.class.name.underscore - - original_log = current_user.send(class_name.pluralize).find_by(id: from_referrer_query("original_log_id")) - dynamic_duplicates = current_user.send(class_name.pluralize).duplicate_logs(@log) - - if dynamic_duplicates.any? - saved_duplicates = @log.duplicates - if duplicates_changed?(dynamic_duplicates, saved_duplicates) - duplicate_set_id = dynamic_duplicates.first.duplicate_set_id || new_duplicate_set_id(@log) - update_logs_with_duplicate_set_id(@log, dynamic_duplicates, duplicate_set_id) - saved_duplicates.first.update!(duplicate_set_id: nil) if saved_duplicates.count == 1 - end - else - remove_fixed_duplicate_set_ids(@log) - end - - if original_log.present? && current_user.send(class_name.pluralize).duplicate_logs(original_log).any? - if dynamic_duplicates.none? - flash[:notice] = deduplication_success_banner - end - send("#{class_name}_duplicate_logs_path", original_log, original_log_id: original_log.id, referrer: params[:referrer], organisation_id: params[:organisation_id]) - else - remove_fixed_duplicate_set_ids(original_log) - flash[:notice] = deduplication_success_banner - send("#{class_name}_duplicate_logs_path", "#{class_name}_id".to_sym => from_referrer_query("first_remaining_duplicate_id"), original_log_id: from_referrer_query("original_log_id"), referrer: params[:referrer], organisation_id: params[:organisation_id]) - end - end - def deduplication_success_banner deduplicated_log_link = "Log #{@log.id}" changed_labels = { diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 10ab612cd..7bf963212 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -701,7 +701,9 @@ class LettingsLog < Log end def duplicates - LettingsLog.where.not(duplicate_set_id: nil).where(duplicate_set_id:).where.not(id:) + return LettingsLog.none if duplicate_set_id.nil? + + LettingsLog.where(duplicate_set_id:).where.not(id:) end def address_search_given? diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index ad6984e07..8312b8bff 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -514,7 +514,9 @@ class SalesLog < Log end def duplicates - SalesLog.where.not(duplicate_set_id: nil).where(duplicate_set_id:).where.not(id:) + return SalesLog.none if duplicate_set_id.nil? + + SalesLog.where(duplicate_set_id:).where.not(id:) end def nationality2_uk_or_prefers_not_to_say? diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index 810bfb845..91989ff86 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -11,10 +11,6 @@ class FeatureToggle !Rails.env.development? end - def self.deduplication_flow_enabled? - true - end - def self.duplicate_summary_enabled? true end diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index 020dba3a4..80157e992 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -763,39 +763,162 @@ RSpec.describe FormController, type: :request do } end - before do - post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: - end + context "when the log will not be a duplicate" do + before do + post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + end - it "re-renders the same page with errors if validation fails" do - expect(response).to have_http_status(:redirect) - end + it "re-renders the same page with errors if validation fails" do + expect(response).to have_http_status(:redirect) + end - it "only updates answers that apply to the page being submitted" do - lettings_log.reload - expect(lettings_log.age1).to eq(answer) - expect(lettings_log.age2).to be nil + it "only updates answers that apply to the page being submitted" do + lettings_log.reload + expect(lettings_log.age1).to eq(answer) + expect(lettings_log.age2).to be nil + end + + it "tracks who updated the record" do + lettings_log.reload + whodunnit_actor = lettings_log.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end end - it "tracks who updated the record" do - lettings_log.reload - whodunnit_actor = lettings_log.versions.last.actor - expect(whodunnit_actor).to be_a(User) - expect(whodunnit_actor.id).to eq(user.id) + context "when the answer makes the log a duplicate" do + context "with one other log" do + let(:new_duplicate) { create(:lettings_log) } + + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicate.id)) + post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + end + + it "sets a new duplicate set id on both logs" do + lettings_log.reload + new_duplicate.reload + expect(lettings_log.duplicate_set_id).not_to be_nil + expect(lettings_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id) + end + + it "redirects to the duplicate logs page" do + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + follow_redirect! + expect(page).to have_content("These logs are duplicates") + end + end + + context "with a set of other logs" do + let(:duplicate_set_id) { 100 } + let(:new_duplicates) { create_list(:lettings_log, 2, duplicate_set_id:) } + + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicates.pluck(:id))) + post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + end + + it "sets the logs duplicate set id to that of the set it is now part of" do + lettings_log.reload + expect(lettings_log.duplicate_set_id).to eql(duplicate_set_id) + new_duplicates.each do |log| + log.reload + expect(log.duplicate_set_id).to eql(duplicate_set_id) + end + end + + it "redirects to the duplicate logs page" do + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + follow_redirect! + expect(page).to have_content("These logs are duplicates") + end + end + + context "when the log was previously in a different duplicate set" do + context "with a single other log" do + let(:old_duplicate_set_id) { 110 } + let!(:old_duplicate) { create(:lettings_log, duplicate_set_id: old_duplicate_set_id) } + let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } + let(:new_duplicate) { create(:lettings_log) } + + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicate.id)) + post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + lettings_log.reload + old_duplicate.reload + new_duplicate.reload + expect(old_duplicate.duplicate_set_id).to be_nil + expect(lettings_log.duplicate_set_id).not_to be_nil + expect(lettings_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id) + end + end + + context "with multiple other logs" do + let(:old_duplicate_set_id) { 120 } + let!(:old_duplicates) { create_list(:lettings_log, 2, duplicate_set_id: old_duplicate_set_id) } + let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } + let(:new_duplicate) { create(:lettings_log) } + + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicate.id)) + post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + lettings_log.reload + new_duplicate.reload + old_duplicates.each do |log| + log.reload + expect(log.duplicate_set_id).to eql(old_duplicate_set_id) + end + expect(lettings_log.duplicate_set_id).not_to be_nil + expect(lettings_log.duplicate_set_id).not_to eql(old_duplicate_set_id) + expect(lettings_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id) + end + end + end end - context "and duplicate logs" do - let(:duplicate_logs) { create_list(:lettings_log, 2) } + context "when the answer makes the log stop being a duplicate" do + context "when the log had one duplicate" do + let(:old_duplicate_set_id) { 130 } + let!(:old_duplicate) { create(:lettings_log, duplicate_set_id: old_duplicate_set_id) } + let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } - before do - allow(LettingsLog).to receive(:duplicate_logs).and_return(duplicate_logs) - post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.none) + post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + lettings_log.reload + old_duplicate.reload + expect(old_duplicate.duplicate_set_id).to be_nil + expect(lettings_log.duplicate_set_id).to be_nil + end end - it "redirects to the duplicate logs page" do - expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") - follow_redirect! - expect(page).to have_content("These logs are duplicates") + context "when the log had multiple duplicates" do + let(:old_duplicate_set_id) { 140 } + let!(:old_duplicates) { create_list(:lettings_log, 2, duplicate_set_id: old_duplicate_set_id) } + let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } + + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.none) + post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + lettings_log.reload + old_duplicates.each do |log| + log.reload + expect(log.duplicate_set_id).to eql(old_duplicate_set_id) + end + expect(lettings_log.duplicate_set_id).to be_nil + end end end end @@ -816,19 +939,141 @@ RSpec.describe FormController, type: :request do }, } end + let(:page_id) { "buyer_1_age" } - context "and duplicate logs" do - let!(:duplicate_logs) { create_list(:sales_log, 2) } + context "when the answer makes the log a duplicate" do + context "with one other log" do + let(:new_duplicate) { create(:sales_log) } - before do - allow(SalesLog).to receive(:duplicate_logs).and_return(duplicate_logs) - post "/sales-logs/#{sales_log.id}/buyer-1-age", params: + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicate.id)) + post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params: + end + + it "sets a new duplicate set id on both logs" do + sales_log.reload + new_duplicate.reload + expect(sales_log.duplicate_set_id).not_to be_nil + expect(sales_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id) + end + + it "redirects to the duplicate logs page" do + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + follow_redirect! + expect(page).to have_content("These logs are duplicates") + end end - it "redirects to the duplicate logs page" do - expect(response).to redirect_to("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") - follow_redirect! - expect(page).to have_content("These logs are duplicates") + context "with a set of other logs" do + let(:duplicate_set_id) { 100 } + let(:new_duplicates) { create_list(:sales_log, 2, duplicate_set_id:) } + + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicates.pluck(:id))) + post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params: + end + + it "sets the logs duplicate set id to that of the set it is now part of" do + sales_log.reload + expect(sales_log.duplicate_set_id).to eql(duplicate_set_id) + new_duplicates.each do |log| + log.reload + expect(log.duplicate_set_id).to eql(duplicate_set_id) + end + end + + it "redirects to the duplicate logs page" do + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + follow_redirect! + expect(page).to have_content("These logs are duplicates") + end + end + + context "when the log was previously in a different duplicate set" do + context "with a single other log" do + let(:old_duplicate_set_id) { 110 } + let!(:old_duplicate) { create(:sales_log, duplicate_set_id: old_duplicate_set_id) } + let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } + let(:new_duplicate) { create(:sales_log) } + + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicate.id)) + post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + sales_log.reload + old_duplicate.reload + new_duplicate.reload + expect(old_duplicate.duplicate_set_id).to be_nil + expect(sales_log.duplicate_set_id).not_to be_nil + expect(sales_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id) + end + end + + context "with multiple other logs" do + let(:old_duplicate_set_id) { 120 } + let!(:old_duplicates) { create_list(:sales_log, 2, duplicate_set_id: old_duplicate_set_id) } + let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } + let(:new_duplicate) { create(:sales_log) } + + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicate.id)) + post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + sales_log.reload + new_duplicate.reload + old_duplicates.each do |log| + log.reload + expect(log.duplicate_set_id).to eql(old_duplicate_set_id) + end + expect(sales_log.duplicate_set_id).not_to be_nil + expect(sales_log.duplicate_set_id).not_to eql(old_duplicate_set_id) + expect(sales_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id) + end + end + end + end + + context "when the answer makes the log stop being a duplicate" do + context "when the log had one duplicate" do + let(:old_duplicate_set_id) { 130 } + let!(:old_duplicate) { create(:sales_log, duplicate_set_id: old_duplicate_set_id) } + let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } + + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.none) + post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + sales_log.reload + old_duplicate.reload + expect(old_duplicate.duplicate_set_id).to be_nil + expect(sales_log.duplicate_set_id).to be_nil + end + end + + context "when the log had multiple duplicates" do + let(:old_duplicate_set_id) { 140 } + let!(:old_duplicates) { create_list(:sales_log, 2, duplicate_set_id: old_duplicate_set_id) } + let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) } + + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.none) + post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params: + end + + it "updates the relevant duplicate set ids" do + sales_log.reload + old_duplicates.each do |log| + log.reload + expect(log.duplicate_set_id).to eql(old_duplicate_set_id) + end + expect(sales_log.duplicate_set_id).to be_nil + end end end end