From 6c33210510a6554e291bdb700e6c58332c1a4822 Mon Sep 17 00:00:00 2001
From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com>
Date: Tue, 20 Aug 2024 08:11:20 +0100
Subject: [PATCH 1/8] Bump fugit from 1.10.0 to 1.11.1 (#2595)

Bumps [fugit](https://github.com/floraison/fugit) from 1.10.0 to 1.11.1.
- [Changelog](https://github.com/floraison/fugit/blob/master/CHANGELOG.md)
- [Commits](https://github.com/floraison/fugit/compare/v1.10.0...v1.11.1)

---
updated-dependencies:
- dependency-name: fugit
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
---
 Gemfile.lock | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index 7fdef3710..2f51d6a29 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -141,7 +141,7 @@ GEM
     coderay (1.1.3)
     coercible (1.0.0)
       descendants_tracker (~> 0.0.1)
-    concurrent-ruby (1.3.3)
+    concurrent-ruby (1.3.4)
     connection_pool (2.4.1)
     crack (1.0.0)
       bigdecimal
@@ -180,7 +180,7 @@ GEM
       rubocop
       smart_properties
     erubi (1.13.0)
-    et-orbi (1.2.7)
+    et-orbi (1.2.11)
       tzinfo
     event_stream_parser (1.0.0)
     excon (0.109.0)
@@ -198,8 +198,8 @@ GEM
     faraday-net_http (3.1.0)
       net-http
     ffi (1.16.3)
-    fugit (1.10.0)
-      et-orbi (~> 1, >= 1.2.7)
+    fugit (1.11.1)
+      et-orbi (~> 1, >= 1.2.11)
       raabro (~> 1.4)
     globalid (1.2.1)
       activesupport (>= 6.1)

From eb355d6829cf9584ed1355341f1c22cccad80a02 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Wed, 21 Aug 2024 09:19:29 +0100
Subject: [PATCH 2/8] Update equity validation for resale (#2559)

---
 app/models/sales_log.rb                                |  4 ++++
 app/models/validations/sales/financial_validations.rb  |  3 ++-
 .../validations/sales/financial_validations_spec.rb    | 10 +++++++++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb
index 6ac2979c8..ad6984e07 100644
--- a/app/models/sales_log.rb
+++ b/app/models/sales_log.rb
@@ -542,4 +542,8 @@ class SalesLog < Log
   def address_search_given?
     address_line1_input.present? && postcode_full_input.present?
   end
+
+  def is_resale?
+    resale == 1
+  end
 end
diff --git a/app/models/validations/sales/financial_validations.rb b/app/models/validations/sales/financial_validations.rb
index 3e1d3dfb8..9a119475f 100644
--- a/app/models/validations/sales/financial_validations.rb
+++ b/app/models/validations/sales/financial_validations.rb
@@ -96,9 +96,10 @@ module Validations::Sales::FinancialValidations
     if record.equity < range.min
       record.errors.add :type, I18n.t("validations.financial.equity.under_min", min_equity: range.min)
       record.errors.add :equity, :under_min, message: I18n.t("validations.financial.equity.under_min", min_equity: range.min)
-    elsif record.equity > range.max
+    elsif !record.is_resale? && record.equity > range.max
       record.errors.add :type, I18n.t("validations.financial.equity.over_max", max_equity: range.max)
       record.errors.add :equity, :over_max, message: I18n.t("validations.financial.equity.over_max", max_equity: range.max)
+      record.errors.add :resale, I18n.t("validations.financial.equity.over_max", max_equity: range.max)
     end
   end
 
diff --git a/spec/models/validations/sales/financial_validations_spec.rb b/spec/models/validations/sales/financial_validations_spec.rb
index dd71d0b24..d9f47d39a 100644
--- a/spec/models/validations/sales/financial_validations_spec.rb
+++ b/spec/models/validations/sales/financial_validations_spec.rb
@@ -338,7 +338,7 @@ RSpec.describe Validations::Sales::FinancialValidations do
   end
 
   describe "#validate_equity_in_range_for_year_and_type" do
-    let(:record) { FactoryBot.build(:sales_log, saledate:) }
+    let(:record) { FactoryBot.build(:sales_log, saledate:, resale: nil) }
 
     context "with a log in the 22/23 collection year" do
       let(:saledate) { Time.zone.local(2023, 1, 1) }
@@ -373,6 +373,14 @@ RSpec.describe Validations::Sales::FinancialValidations do
         expect(record.errors["equity"]).to include(match I18n.t("validations.financial.equity.over_max", max_equity: 75))
         expect(record.errors["type"]).to include(match I18n.t("validations.financial.equity.over_max", max_equity: 75))
       end
+
+      it "does not add an error if it's a resale" do
+        record.type = 2
+        record.equity = 90
+        record.resale = 1
+        financial_validator.validate_equity_in_range_for_year_and_type(record)
+        expect(record.errors).to be_empty
+      end
     end
 
     context "with a log in 23/24 collection year" do

From 573e6302bc4bccfe71e6a8e4014c27d309f21da6 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Wed, 21 Aug 2024 09:50:58 +0100
Subject: [PATCH 3/8] CLDC-3600 Clear unconfirmed emails for deactivated users
 (#2587)

* Discard email changes when deactivating user

* Clear existing unconfirmed emails
---
 app/models/user.rb                            |  1 +
 lib/tasks/clear_unconfirmed_emails.rake       |  4 +++
 .../tasks/clear_unconfirmed_emails_spec.rb    | 36 +++++++++++++++++++
 spec/requests/users_controller_spec.rb        |  7 +++-
 4 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 lib/tasks/clear_unconfirmed_emails.rake
 create mode 100644 spec/lib/tasks/clear_unconfirmed_emails_spec.rb

diff --git a/app/models/user.rb b/app/models/user.rb
index c79ceb0d9..58e806bc5 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -142,6 +142,7 @@ class User < ApplicationRecord
       sign_in_count: 0,
       initial_confirmation_sent: false,
       reactivate_with_organisation:,
+      unconfirmed_email: nil,
     )
   end
 
diff --git a/lib/tasks/clear_unconfirmed_emails.rake b/lib/tasks/clear_unconfirmed_emails.rake
new file mode 100644
index 000000000..f53a0c893
--- /dev/null
+++ b/lib/tasks/clear_unconfirmed_emails.rake
@@ -0,0 +1,4 @@
+desc "Clear unconfimed emails for deactivated users"
+task clear_unconfirmed_emails: :environment do
+  User.deactivated.where.not(unconfirmed_email: nil).update(unconfirmed_email: nil)
+end
diff --git a/spec/lib/tasks/clear_unconfirmed_emails_spec.rb b/spec/lib/tasks/clear_unconfirmed_emails_spec.rb
new file mode 100644
index 000000000..0ff7a3315
--- /dev/null
+++ b/spec/lib/tasks/clear_unconfirmed_emails_spec.rb
@@ -0,0 +1,36 @@
+require "rails_helper"
+require "rake"
+
+RSpec.describe "clear_unconfirmed_emails" do
+  describe ":clear_unconfirmed_emails", type: :task do
+    subject(:task) { Rake::Task["clear_unconfirmed_emails"] }
+
+    before do
+      Rake.application.rake_require("tasks/clear_unconfirmed_emails")
+      Rake::Task.define_task(:environment)
+      task.reenable
+    end
+
+    context "when the rake task is run" do
+      context "and there are deactivated users with unconfirmed emails" do
+        let!(:user) { create(:user, active: false, unconfirmed_email: "some_email@example.com") }
+
+        it "clears unconfirmed_email" do
+          task.invoke
+
+          expect(user.reload.unconfirmed_email).to eq(nil)
+        end
+      end
+
+      context "and there are active users with unconfirmed emails" do
+        let!(:user) { create(:user, active: true, unconfirmed_email: "some_email@example.com") }
+
+        it "does not clear unconfirmed_email" do
+          task.invoke
+
+          expect(user.reload.unconfirmed_email).not_to eq(nil)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 8e87f7f28..c2240a77d 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -434,7 +434,7 @@ RSpec.describe UsersController, type: :request do
 
   context "when user is signed in as a data coordinator" do
     let(:user) { create(:user, :data_coordinator, email: "coordinator@example.com", organisation: create(:organisation, :without_dpc)) }
-    let!(:other_user) { create(:user, organisation: user.organisation, name: "filter name", email: "filter@example.com") }
+    let!(:other_user) { create(:user, organisation: user.organisation, name: "filter name", email: "filter@example.com", unconfirmed_email: "email@something.com") }
 
     before do
       sign_in user
@@ -885,6 +885,11 @@ RSpec.describe UsersController, type: :request do
                 expect { patch "/users/#{other_user.id}", headers:, params: }
                   .to change { other_user.reload.active }.from(true).to(false)
               end
+
+              it "discards unconfirmed email" do
+                expect { patch "/users/#{other_user.id}", headers:, params: }
+                  .to change { other_user.reload.unconfirmed_email }.from("email@something.com").to(nil)
+              end
             end
 
             context "and tries to activate deactivated user" do

From daff1d637b95c322a059f2fa7ece742ae3dc17df Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Wed, 21 Aug 2024 12:40:02 +0100
Subject: [PATCH 4/8] Remove old email template (#2591)

---
 app/models/user.rb                             | 7 -------
 spec/features/user_spec.rb                     | 4 ++--
 spec/mailers/resend_invitation_mailer_spec.rb  | 2 +-
 spec/requests/organisations_controller_spec.rb | 2 +-
 4 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 58e806bc5..23e787268 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -157,7 +157,6 @@ class User < ApplicationRecord
   RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze
   CONFIRMABLE_TEMPLATE_ID = "3fc2e3a7-0835-4b84-ab7a-ce51629eb614".freeze
   RECONFIRMABLE_TEMPLATE_ID = "bcdec787-f0a7-46e9-8d63-b3e0a06ee455".freeze
-  BETA_ONBOARDING_TEMPLATE_ID = "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd".freeze
   USER_REACTIVATED_TEMPLATE_ID = "ac45a899-490e-4f59-ae8d-1256fc0001f9".freeze
   FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "3eb80517-1051-4dfc-b4cc-cb18228a3829".freeze
   FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "0cdd0be1-7fa5-4808-8225-ae4c5a002352".freeze
@@ -169,8 +168,6 @@ class User < ApplicationRecord
   def confirmable_template
     if last_sign_in_at.present? && (unconfirmed_email.blank? || unconfirmed_email == email)
       USER_REACTIVATED_TEMPLATE_ID
-    elsif was_migrated_from_softwire? && last_sign_in_at.blank?
-      BETA_ONBOARDING_TEMPLATE_ID
     elsif initial_confirmation_sent && !confirmed?
       RECONFIRMABLE_TEMPLATE_ID
     else
@@ -178,10 +175,6 @@ class User < ApplicationRecord
     end
   end
 
-  def was_migrated_from_softwire?
-    legacy_users.any? || old_user_id.present?
-  end
-
   def send_confirmation_instructions
     return unless active?
 
diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb
index c30abe1e9..119dbfa52 100644
--- a/spec/features/user_spec.rb
+++ b/spec/features/user_spec.rb
@@ -600,8 +600,8 @@ RSpec.describe "User Features" do
         visit(user_path(other_user))
       end
 
-      it "sends beta onboarding email to be sent when user is legacy" do
-        expect(notify_client).to receive(:send_email).with(email_address: "new_user@example.com", template_id: User::BETA_ONBOARDING_TEMPLATE_ID, personalisation:).once
+      it "sends initial confirmable template email when user is legacy" do
+        expect(notify_client).to receive(:send_email).with(email_address: "new_user@example.com", template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once
         click_button("Resend invite link")
       end
     end
diff --git a/spec/mailers/resend_invitation_mailer_spec.rb b/spec/mailers/resend_invitation_mailer_spec.rb
index 2b76484c3..9dec88ace 100644
--- a/spec/mailers/resend_invitation_mailer_spec.rb
+++ b/spec/mailers/resend_invitation_mailer_spec.rb
@@ -42,7 +42,7 @@ RSpec.describe ResendInvitationMailer do
 
       it "sends an initial invitation" do
         FactoryBot.create(:legacy_user, old_user_id: new_active_migrated_user.old_user_id, user: new_active_migrated_user)
-        expect(notify_client).to receive(:send_email).with(email_address: "new_active_migrated_user@example.com", template_id: User::BETA_ONBOARDING_TEMPLATE_ID, personalisation:).once
+        expect(notify_client).to receive(:send_email).with(email_address: "new_active_migrated_user@example.com", template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once
         described_class.new.resend_invitation_email(new_active_migrated_user)
       end
     end
diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb
index d3e8a8155..cb74ca3ab 100644
--- a/spec/requests/organisations_controller_spec.rb
+++ b/spec/requests/organisations_controller_spec.rb
@@ -1688,7 +1688,7 @@ RSpec.describe OrganisationsController, type: :request do
           end
 
           it "sends invitation emails" do
-            expect(notify_client).to have_received(:send_email).with(email_address: user_to_reactivate.email, template_id: User::BETA_ONBOARDING_TEMPLATE_ID, personalisation: expected_personalisation).once
+            expect(notify_client).to have_received(:send_email).with(email_address: user_to_reactivate.email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation: expected_personalisation).once
           end
         end
       end

From 14d63bd5c41ee2db2d0720544ed572491c9e7405 Mon Sep 17 00:00:00 2001
From: Rachael Booth <Rachael.Booth@softwire.com>
Date: Thu, 22 Aug 2024 10:21:35 +0100
Subject: [PATCH 5/8] CLDC-3577: Add optional phone extension to users (#2597)

* CLDC-3577: Add optional phone extension to users

* CLDC-3577: Validate that phone extension is a number
---
 app/controllers/users_controller.rb           |  8 +++---
 app/models/user.rb                            |  7 ++++++
 app/views/users/edit.html.erb                 |  7 +++++-
 app/views/users/new.html.erb                  |  8 +++++-
 app/views/users/show.html.erb                 |  2 +-
 ...0819143150_add_phone_extension_to_users.rb |  5 ++++
 db/schema.rb                                  |  3 ++-
 spec/requests/users_controller_spec.rb        | 25 ++++++++++++++++---
 8 files changed, 53 insertions(+), 12 deletions(-)
 create mode 100644 db/migrate/20240819143150_add_phone_extension_to_users.rb

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 2f7bb2bd6..c5c5241f9 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -192,14 +192,14 @@ private
   def user_params
     if @user == current_user
       if current_user.data_coordinator? || current_user.support?
-        params.require(:user).permit(:email, :phone, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent)
+        params.require(:user).permit(:email, :phone, :phone_extension, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent)
       else
-        params.require(:user).permit(:email, :phone, :name, :password, :password_confirmation, :initial_confirmation_sent)
+        params.require(:user).permit(:email, :phone, :phone_extension, :name, :password, :password_confirmation, :initial_confirmation_sent)
       end
     elsif current_user.data_coordinator?
-      params.require(:user).permit(:email, :phone, :name, :role, :is_dpo, :is_key_contact, :active, :initial_confirmation_sent)
+      params.require(:user).permit(:email, :phone, :phone_extension, :name, :role, :is_dpo, :is_key_contact, :active, :initial_confirmation_sent)
     elsif current_user.support?
-      params.require(:user).permit(:email, :phone, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active, :initial_confirmation_sent)
+      params.require(:user).permit(:email, :phone, :phone_extension, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active, :initial_confirmation_sent)
     end
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 23e787268..31956e54d 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -19,6 +19,7 @@ class User < ApplicationRecord
   validates :password, presence: { if: :password_required? }
   validates :password, length: { within: Devise.password_length, allow_blank: true }
   validates :password, confirmation: { if: :password_required? }
+  validates :phone_extension, format: { with: /\A\d+\z/, allow_blank: true, message: I18n.t("validations.numeric.format", field: "") }
 
   after_validation :send_data_protection_confirmation_reminder, if: :is_dpo_changed?
 
@@ -263,6 +264,12 @@ class User < ApplicationRecord
     save!(validate: false)
   end
 
+  def phone_with_extension
+    return phone if phone_extension.blank?
+
+    "#{phone}, Ext. #{phone_extension}"
+  end
+
 protected
 
   # Checks whether a password is needed or not. For validations only.
diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb
index 3f7259bc3..6fdfdb5aa 100644
--- a/app/views/users/edit.html.erb
+++ b/app/views/users/edit.html.erb
@@ -24,7 +24,12 @@
 
       <%= f.govuk_phone_field :phone,
         label: { text: "Telephone number", size: "m" },
-        autocomplete: "phone",
+        autocomplete: "tel-national",
+        spellcheck: "false" %>
+
+      <%= f.govuk_phone_field :phone_extension,
+        label: { text: "Extension number (optional)", size: "m" },
+        autocomplete: "tel-extension",
         spellcheck: "false" %>
 
       <% if current_user.data_coordinator? || current_user.support? %>
diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb
index 925f1ada7..f9f9b1e48 100644
--- a/app/views/users/new.html.erb
+++ b/app/views/users/new.html.erb
@@ -25,10 +25,16 @@
 
       <%= f.govuk_phone_field :phone,
         label: { text: "Telephone number", size: "m" },
-        autocomplete: "phone",
+        autocomplete: "tel-national",
         spellcheck: "false",
         value: @user.phone %>
 
+      <%= f.govuk_phone_field :phone_extension,
+        label: { text: "Extension number (optional)", size: "m" },
+        autocomplete: "tel-extension",
+        spellcheck: "false",
+        value: @user.phone_extension %>
+
       <% if current_user.support? %>
         <% null_option = [OpenStruct.new(id: "", name: "Select an option")] %>
         <% organisations = Organisation.filter_by_active.map { |org| OpenStruct.new(id: org.id, name: org.name) } %>
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index acb53d51d..df8c0e915 100644
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -43,7 +43,7 @@
 
       <%= summary_list.with_row do |row|
             row.with_key { "Telephone number" }
-            row.with_value { @user.phone }
+            row.with_value { @user.phone_with_extension }
             if UserPolicy.new(current_user, @user).edit_telephone_numbers?
               row.with_action(visually_hidden_text: "telephone number", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-telephone-number" })
             else
diff --git a/db/migrate/20240819143150_add_phone_extension_to_users.rb b/db/migrate/20240819143150_add_phone_extension_to_users.rb
new file mode 100644
index 000000000..0dcd8d0c5
--- /dev/null
+++ b/db/migrate/20240819143150_add_phone_extension_to_users.rb
@@ -0,0 +1,5 @@
+class AddPhoneExtensionToUsers < ActiveRecord::Migration[7.0]
+  def change
+    add_column :users, :phone_extension, :string
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 50073d2d6..a76cf9eea 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_07_15_082338) do
+ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -788,6 +788,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_07_15_082338) do
     t.boolean "initial_confirmation_sent"
     t.boolean "reactivate_with_organisation"
     t.datetime "discarded_at"
+    t.string "phone_extension"
     t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["encrypted_otp_secret_key"], name: "index_users_on_encrypted_otp_secret_key", unique: true
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index c2240a77d..dac2806a5 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -1002,6 +1002,7 @@ RSpec.describe UsersController, type: :request do
             email: "new_user@example.com",
             role: "data_coordinator",
             phone: "12345678910",
+            phone_extension: "1234",
           },
         }
       end
@@ -1025,12 +1026,14 @@ RSpec.describe UsersController, type: :request do
         request
       end
 
-      it "creates a new scheme for user organisation with valid params" do
+      it "creates a new user for user organisation with valid params" do
         request
 
         expect(User.last.name).to eq("new user")
         expect(User.last.email).to eq("new_user@example.com")
         expect(User.last.role).to eq("data_coordinator")
+        expect(User.last.phone).to eq("12345678910")
+        expect(User.last.phone_extension).to eq("1234")
       end
 
       it "redirects back to organisation users page" do
@@ -1617,11 +1620,12 @@ RSpec.describe UsersController, type: :request do
           expect(page).to have_content("Change your personal details")
         end
 
-        it "has fields for name, email, role, dpo and key contact" do
+        it "has fields for name, email, role, phone number and phone extension" do
           expect(page).to have_field("user[name]")
           expect(page).to have_field("user[email]")
           expect(page).to have_field("user[role]")
           expect(page).to have_field("user[phone]")
+          expect(page).to have_field("user[phone_extension]")
         end
 
         it "allows setting the role to `support`" do
@@ -1643,10 +1647,12 @@ RSpec.describe UsersController, type: :request do
             expect(page).to have_content("Change #{other_user.name}’s personal details")
           end
 
-          it "has fields for name, email, role, dpo and key contact" do
+          it "has fields for name, email, role, phone number and phone extension" do
             expect(page).to have_field("user[name]")
             expect(page).to have_field("user[email]")
             expect(page).to have_field("user[role]")
+            expect(page).to have_field("user[phone]")
+            expect(page).to have_field("user[phone_extension]")
           end
         end
 
@@ -1661,10 +1667,12 @@ RSpec.describe UsersController, type: :request do
             expect(page).to have_content("Change #{other_user.name}’s personal details")
           end
 
-          it "has fields for name, email, role, dpo and key contact" do
+          it "has fields for name, email, role, phone number and phone extension" do
             expect(page).to have_field("user[name]")
             expect(page).to have_field("user[email]")
             expect(page).to have_field("user[role]")
+            expect(page).to have_field("user[phone]")
+            expect(page).to have_field("user[phone_extension]")
           end
         end
 
@@ -1994,6 +2002,7 @@ RSpec.describe UsersController, type: :request do
             email:,
             role: "data_coordinator",
             phone: "12345612456",
+            phone_extension: "1234",
             organisation_id: organisation.id,
           },
         }
@@ -2009,6 +2018,14 @@ RSpec.describe UsersController, type: :request do
         expect(User.find_by(email:).organisation).to eq(organisation)
       end
 
+      it "sets expected values on the user" do
+        request
+        user = User.find_by(email:)
+        expect(user.name).to eq("new user")
+        expect(user.phone).to eq("12345612456")
+        expect(user.phone_extension).to eq("1234")
+      end
+
       it "redirects back to users page" do
         request
         expect(response).to redirect_to("/users")

From ea853c484dcd29c1f5a28ade09652be7ea0dc9bb Mon Sep 17 00:00:00 2001
From: Rachael Booth <Rachael.Booth@softwire.com>
Date: Thu, 22 Aug 2024 11:46:25 +0100
Subject: [PATCH 6/8] 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 = "<a class=\"govuk-notification-banner__link govuk-!-font-weight-bold\" href=\"#{send("#{@log.class.name.underscore}_path", @log)}\">Log #{@log.id}</a>"
     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

From 4690fb446e9d3a090414a9bade894b976010d13e Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Thu, 22 Aug 2024 17:14:20 +0100
Subject: [PATCH 7/8] CLDC-3595 Remove duplicate errors for check errors page
 (#2560)

* Remove duplicate errors for check errors page

* Remove unused line

* Format some missed errors correctly
---
 app/helpers/form_page_error_helper.rb          |  9 +++++++++
 .../sales/sale_information_validations.rb      |  4 ++--
 app/views/form/check_errors.html.erb           |  1 +
 spec/helpers/form_page_error_helper_spec.rb    | 18 ++++++++++++++++++
 4 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/app/helpers/form_page_error_helper.rb b/app/helpers/form_page_error_helper.rb
index 8a46accca..c2c13e0c6 100644
--- a/app/helpers/form_page_error_helper.rb
+++ b/app/helpers/form_page_error_helper.rb
@@ -4,6 +4,15 @@ module FormPageErrorHelper
     other_page_error_ids.each { |id| lettings_log.errors.delete(id) }
   end
 
+  def remove_duplicate_page_errors(lettings_log)
+    lettings_log.errors.group_by(&:message).each do |_, errors|
+      next if errors.size == 1
+
+      errors.shift
+      errors.each { |error| lettings_log.errors.delete(error.attribute) }
+    end
+  end
+
   def all_questions_affected_by_errors(log)
     log.errors.map(&:attribute) - [:base]
   end
diff --git a/app/models/validations/sales/sale_information_validations.rb b/app/models/validations/sales/sale_information_validations.rb
index 42c6a6172..8bfa46783 100644
--- a/app/models/validations/sales/sale_information_validations.rb
+++ b/app/models/validations/sales/sale_information_validations.rb
@@ -222,7 +222,7 @@ module Validations::Sales::SaleInformationValidations
                                           value: record.field_formatted_as_currency("value"),
                                           equity: "#{record.equity}%",
                                           mortgage_and_deposit_total: record.field_formatted_as_currency("mortgage_and_deposit_total"),
-                                          expected_shared_ownership_deposit_value: record.field_formatted_as_currency("expected_shared_ownership_deposit_value"))
+                                          expected_shared_ownership_deposit_value: record.field_formatted_as_currency("expected_shared_ownership_deposit_value")).html_safe
         end
         record.errors.add :type, :skip_bu_error, message: I18n.t("validations.sale_information.non_staircasing_mortgage.mortgage_used",
                                                                  mortgage: record.field_formatted_as_currency("mortgage"),
@@ -230,7 +230,7 @@ module Validations::Sales::SaleInformationValidations
                                                                  value: record.field_formatted_as_currency("value"),
                                                                  equity: "#{record.equity}%",
                                                                  mortgage_and_deposit_total: record.field_formatted_as_currency("mortgage_and_deposit_total"),
-                                                                 expected_shared_ownership_deposit_value: record.field_formatted_as_currency("expected_shared_ownership_deposit_value"))
+                                                                 expected_shared_ownership_deposit_value: record.field_formatted_as_currency("expected_shared_ownership_deposit_value")).html_safe
       end
     elsif record.mortgage_not_used?
       if over_tolerance?(record.deposit, record.expected_shared_ownership_deposit_value, 1)
diff --git a/app/views/form/check_errors.html.erb b/app/views/form/check_errors.html.erb
index 27fa3ea0c..bab4858a5 100644
--- a/app/views/form/check_errors.html.erb
+++ b/app/views/form/check_errors.html.erb
@@ -2,6 +2,7 @@
   <div class="govuk-grid-column-three-quarters-from-desktop">
 
     <%= form_with model: @log, url: send("#{@log.model_name.param_key}_confirm_clear_answer_path", @log), method: "post", local: true do |f| %>
+      <% remove_duplicate_page_errors(@log) %>
       <%= f.govuk_error_summary %>
       <%= f.hidden_field :page_id, value: @page.id %>
 
diff --git a/spec/helpers/form_page_error_helper_spec.rb b/spec/helpers/form_page_error_helper_spec.rb
index 5f86e1648..067739e16 100644
--- a/spec/helpers/form_page_error_helper_spec.rb
+++ b/spec/helpers/form_page_error_helper_spec.rb
@@ -21,4 +21,22 @@ RSpec.describe FormPageErrorHelper do
       end
     end
   end
+
+  describe "#remove_duplicate_page_errors" do
+    context "when non base other questions are removed" do
+      let!(:lettings_log) { FactoryBot.create(:lettings_log, :in_progress) }
+
+      before do
+        lettings_log.errors.add :layear, "error"
+        lettings_log.errors.add :period, "error_one"
+        lettings_log.errors.add :base, "error_one"
+      end
+
+      it "returns details and user tabs" do
+        remove_duplicate_page_errors(lettings_log)
+        expect(lettings_log.errors.count).to eq(2)
+        expect(lettings_log.errors.map(&:message)).to match_array(%w[error_one error])
+      end
+    end
+  end
 end

From caba5e7fdd56430b4debe93d7ee13d5f438ed1e4 Mon Sep 17 00:00:00 2001
From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com>
Date: Fri, 23 Aug 2024 08:30:01 +0100
Subject: [PATCH 8/8] Bump rexml from 3.3.3 to 3.3.6 in /docs (#2603)

Bumps [rexml](https://github.com/ruby/rexml) from 3.3.3 to 3.3.6.
- [Release notes](https://github.com/ruby/rexml/releases)
- [Changelog](https://github.com/ruby/rexml/blob/master/NEWS.md)
- [Commits](https://github.com/ruby/rexml/compare/v3.3.3...v3.3.6)

---
updated-dependencies:
- dependency-name: rexml
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
---
 docs/Gemfile.lock | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/Gemfile.lock b/docs/Gemfile.lock
index 849732bc5..2a406a953 100644
--- a/docs/Gemfile.lock
+++ b/docs/Gemfile.lock
@@ -226,7 +226,7 @@ GEM
     rb-fsevent (0.11.2)
     rb-inotify (0.10.1)
       ffi (~> 1.0)
-    rexml (3.3.3)
+    rexml (3.3.6)
       strscan
     rouge (3.26.0)
     ruby2_keywords (0.0.5)