CLDC-3566: Clear duplicate set ids when a log ceases to be a duplicate through normal update flow (#2534)

* CLDC-3566: Clear duplicate set ids when a log ceases to be a duplicate in normal updates

* Refactor

* Ensure post not run twice in form controller tests

* Remove deduplication flow feature toggle

* Ensure class_name is defined in redirect path function

* Add tests for sales logs

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,43 @@ 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 update_duplication_tracking
+    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
+
+  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"))
 
-      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
+      if original_log.present? && current_user.send(class_name.pluralize).duplicate_logs(original_log).any?
+        if @log.duplicates.none?
+          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
 
+    if @log.duplicates.any?
+      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 +336,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 = "<a class=\"govuk-notification-banner__link govuk-!-font-weight-bold\" href=\"#{send("#{@log.class.name.underscore}_path", @log)}\">Log #{@log.id}</a>"
     changed_labels = {

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

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