From 0439696dd329b8dd2963248a48c14057f6370a9b Mon Sep 17 00:00:00 2001
From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com>
Date: Mon, 31 Mar 2025 18:17:53 +0100
Subject: [PATCH] Add error to firststair question and test date changes that
 cross collection years (#3029)

* Adjust staircase difference validation to exclude initial purchase

* Update tests

* Keep validation as is, add error to firststair also

* Add method to generate random dates within collection year for logs

* Revert "Update tests"

This reverts commit e7081871608b51a544fc93a5cfc3655c35aa1458.

* Update financial validations tests to reflect new error messages for firststair

* Update sales log test only pick a date up to 14 days away

* Lint

* Adjust merge date for future dates

* Update La test

* Fix date range logic in generate_different_date_within_collection_year method

* Refactor generate_different_date_within_collection_year to use keyword arguments for start and end date overrides in tests

* Refactor sales_log_spec to use descriptive variable names for date overrides

* Refactor merge_requests_helper_spec to use generated merge dates for consistency in tests

* Refactor generate_different_date_within_collection_year

* Refactor sales_log_spec to use generated sale date for duplicate log creation

* Refactor sales_log_spec to use fixed date values for end date and date after end date, mimics change of year without changing year

* Refactor merge_requests_helper_spec to use a fixed merge date for consistency in tests

* Refactor generate_different_date_within_collection_year to handle edge cases and return nil if no available dates
---
 app/helpers/collection_time_helper.rb              | 10 ++++++++++
 .../validations/sales/financial_validations.rb     |  1 +
 config/locales/validations/sales/financial.en.yml  |  3 +++
 spec/helpers/merge_requests_helper_spec.rb         | 13 ++++++++-----
 spec/models/sales_log_spec.rb                      | 14 +++++++++-----
 .../sales/financial_validations_spec.rb            |  7 ++++---
 spec/requests/merge_requests_controller_spec.rb    |  2 +-
 7 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/app/helpers/collection_time_helper.rb b/app/helpers/collection_time_helper.rb
index acd56378b..f5ffd66bf 100644
--- a/app/helpers/collection_time_helper.rb
+++ b/app/helpers/collection_time_helper.rb
@@ -53,4 +53,14 @@ module CollectionTimeHelper
   def archived_collection_start_year
     current_collection_start_year - 2
   end
+
+  def generate_different_date_within_collection_year(date, start_date_override: nil, end_date_override: nil)
+    start_date = [start_date_override, collection_start_date(date).to_date].compact.max.to_date
+    end_date = [end_date_override, collection_end_date(date).to_date].compact.min.to_date
+
+    return nil if start_date > end_date
+
+    available_dates = (start_date..end_date).to_a - [date.to_date]
+    available_dates.empty? ? nil : available_dates.sample
+  end
 end
diff --git a/app/models/validations/sales/financial_validations.rb b/app/models/validations/sales/financial_validations.rb
index 72b80874b..4d273d042 100644
--- a/app/models/validations/sales/financial_validations.rb
+++ b/app/models/validations/sales/financial_validations.rb
@@ -134,6 +134,7 @@ module Validations::Sales::FinancialValidations
         record.errors.add :stairowned, I18n.t("validations.sales.financial.stairowned.less_than_stairbought_plus_equity_plus_prev_staircasing", equity: formatted_equity, bought: formatted_stairbought, numprevstair: previous_staircasing_transactions, equity_sum:, stair_total: formatted_stairowned)
         record.errors.add :stairbought, I18n.t("validations.sales.financial.stairbought.more_than_stairowned_minus_equity_minus_prev_staircasing", equity: formatted_equity, bought: formatted_stairbought, numprevstair: previous_staircasing_transactions, equity_sum:, stair_total: formatted_stairowned)
         record.errors.add :numstair, I18n.t("validations.sales.financial.numstair.too_high_for_stairowned_minus_stairbought_minus_equity", equity: formatted_equity, bought: formatted_stairbought, numprevstair: previous_staircasing_transactions, equity_sum:, stair_total: formatted_stairowned)
+        record.errors.add :firststair, I18n.t("validations.sales.financial.firststair.invalid_for_stairowned_minus_stairbought_minus_equity", equity: formatted_equity, bought: formatted_stairbought, numprevstair: previous_staircasing_transactions, equity_sum:, stair_total: formatted_stairowned)
       end
     end
   end
diff --git a/config/locales/validations/sales/financial.en.yml b/config/locales/validations/sales/financial.en.yml
index 50d8fe90a..c8a962079 100644
--- a/config/locales/validations/sales/financial.en.yml
+++ b/config/locales/validations/sales/financial.en.yml
@@ -83,6 +83,9 @@ en:
           percentage_bought_equal_percentage_owned: "The percentage bought is %{stairbought}% and the percentage owned in total is %{stairowned}%. These figures cannot be the same."
           more_than_stairowned_minus_equity_minus_prev_staircasing: "The initial equity stake is %{equity}%, the percentage bought is %{bought}%, and there have been %{numprevstair} previous staircasing transactions, totalling at least %{equity_sum}%, which is more than the total percentage owned by the buyers (%{stair_total}%). In a staircasing transaction, the total percentage owned must be at least the initial equity stake plus the percentage bought plus a minimum of 1% for each previous staircasing transaction."
 
+        firststair:
+          invalid_for_stairowned_minus_stairbought_minus_equity: "The initial equity stake is %{equity}%, the percentage bought is %{bought}%, and there have been %{numprevstair} previous staircasing transactions, totalling at least %{equity_sum}%, which is more than the total percentage owned by the buyers (%{stair_total}%). In a staircasing transaction, the total percentage owned must be at least the initial equity stake plus the percentage bought plus a minimum of 1% for each previous staircasing transaction."
+
         numstair:
           too_high_for_stairowned_minus_stairbought_minus_equity: "The initial equity stake is %{equity}%, the percentage bought is %{bought}%, and there have been %{numprevstair} previous staircasing transactions, totalling at least %{equity_sum}%, which is more than the total percentage owned by the buyers (%{stair_total}%). In a staircasing transaction, the total percentage owned must be at least the initial equity stake plus the percentage bought plus a minimum of 1% for each previous staircasing transaction."
 
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index db0598489..c9a71fc8d 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -1,6 +1,8 @@
 require "rails_helper"
 
 RSpec.describe MergeRequestsHelper do
+  include CollectionTimeHelper
+
   describe "#merging_organisations_without_users_text" do
     context "with 1 organisation" do
       let(:organisation) { build(:organisation, name: "Org 1") }
@@ -163,7 +165,8 @@ RSpec.describe MergeRequestsHelper do
     let(:organisation) { create(:organisation, name: "Org 1") }
     let(:merging_organisation) { create(:organisation, name: "Org 2") }
     let(:merging_organisation_2) { create(:organisation, name: "Org 3") }
-    let(:merge_request) { create(:merge_request, absorbing_organisation: organisation, merge_date: Time.zone.today) }
+    let(:merge_date) { Time.zone.local(2025, 1, 1) }
+    let(:merge_request) { create(:merge_request, absorbing_organisation: organisation, merge_date:) }
 
     before do
       create(:merge_request_organisation, merge_request:, merging_organisation:)
@@ -198,10 +201,10 @@ RSpec.describe MergeRequestsHelper do
     context "when merging organisations have logs" do
       before do
         create(:lettings_log, owning_organisation: organisation)
-        create(:lettings_log, owning_organisation: merging_organisation, startdate: Time.zone.tomorrow)
+        create(:lettings_log, owning_organisation: merging_organisation, startdate: generate_different_date_within_collection_year(merge_date, start_date_override: merge_date, end_date_override: Time.zone.now + 14.days))
         create(:lettings_log, owning_organisation: merging_organisation, startdate: Time.zone.yesterday)
         create(:sales_log, owning_organisation: organisation)
-        create(:sales_log, owning_organisation: merging_organisation, saledate: Time.zone.tomorrow)
+        create(:sales_log, owning_organisation: merging_organisation, saledate: generate_different_date_within_collection_year(merge_date, start_date_override: merge_date, end_date_override: Time.zone.now + 14.days))
         create(:sales_log, owning_organisation: merging_organisation, saledate: Time.zone.yesterday)
       end
 
@@ -235,8 +238,8 @@ RSpec.describe MergeRequestsHelper do
         before do
           create(:organisation_relationship, parent_organisation: merging_organisation_2, child_organisation: merging_organisation)
           create(:merge_request_organisation, merge_request:, merging_organisation: merging_organisation_2)
-          create(:lettings_log, assigned_to: merging_organisation_2.users.first, owning_organisation: merging_organisation_2, managing_organisation: merging_organisation, startdate: Time.zone.yesterday)
-          create(:sales_log, assigned_to: merging_organisation_2.users.first, owning_organisation: merging_organisation_2, managing_organisation: merging_organisation, saledate: Time.zone.yesterday)
+          create(:lettings_log, assigned_to: merging_organisation_2.users.first, owning_organisation: merging_organisation_2, managing_organisation: merging_organisation, startdate: generate_different_date_within_collection_year(merge_date, start_date_override: merge_date, end_date_override: Time.zone.now + 14.days))
+          create(:sales_log, assigned_to: merging_organisation_2.users.first, owning_organisation: merging_organisation_2, managing_organisation: merging_organisation, saledate: generate_different_date_within_collection_year(merge_date, start_date_override: merge_date, end_date_override: Time.zone.now + 14.days))
         end
 
         it "returns the correct merging_organisations_lettings_logs_outcomes_text text" do
diff --git a/spec/models/sales_log_spec.rb b/spec/models/sales_log_spec.rb
index 442febddb..c522643c1 100644
--- a/spec/models/sales_log_spec.rb
+++ b/spec/models/sales_log_spec.rb
@@ -3,6 +3,8 @@ require "shared/shared_log_examples"
 
 # rubocop:disable RSpec/MessageChain
 RSpec.describe SalesLog, type: :model do
+  include CollectionTimeHelper
+
   let(:owning_organisation) { create(:organisation) }
   let(:assigned_to_user) { create(:user) }
 
@@ -250,7 +252,7 @@ RSpec.describe SalesLog, type: :model do
     end
 
     context "when there is a log with a different sale date" do
-      let!(:different_sale_date_log) { create(:sales_log, :duplicate, saledate: Time.zone.tomorrow, owning_organisation: organisation) }
+      let!(:different_sale_date_log) { create(:sales_log, :duplicate, saledate: generate_different_date_within_collection_year(Time.zone.now, end_date_override: Time.zone.now + 14.days), owning_organisation: organisation) }
 
       it "does not return a log with a different sale date as a duplicate" do
         expect(described_class.duplicate_logs(log)).not_to include(different_sale_date_log)
@@ -376,7 +378,7 @@ RSpec.describe SalesLog, type: :model do
 
     context "when there is a log with a different sale date" do
       before do
-        create(:sales_log, :duplicate, saledate: Time.zone.tomorrow)
+        create(:sales_log, :duplicate, saledate: generate_different_date_within_collection_year(Time.zone.now, end_date_override: Time.zone.now + 14.days))
       end
 
       it "does not return a log with a different sale date as a duplicate" do
@@ -1085,16 +1087,18 @@ RSpec.describe SalesLog, type: :model do
 
   context "when form year changes and LA is no longer active" do
     let!(:sales_log) { create(:sales_log) }
+    let(:end_date) { Time.zone.local(2025, 3, 30) }
+    let(:date_after_end_date) { Time.zone.local(2025, 3, 31) }
 
     before do
-      LocalAuthority.find_by(code: "E08000003").update!(end_date: Time.zone.today)
+      LocalAuthority.find_by(code: "E08000003").update!(end_date:)
     end
 
     it "removes the LA" do
-      sales_log.update!(saledate: Time.zone.yesterday, la: "E08000003")
+      sales_log.update!(saledate: end_date, la: "E08000003")
       expect(sales_log.reload.la).to eq("E08000003")
 
-      sales_log.update!(saledate: Time.zone.tomorrow)
+      sales_log.update!(saledate: date_after_end_date)
       expect(sales_log.reload.la).to eq(nil)
       expect(sales_log.reload.is_la_inferred).to eq(false)
     end
diff --git a/spec/models/validations/sales/financial_validations_spec.rb b/spec/models/validations/sales/financial_validations_spec.rb
index 3a2a7b7cd..e6f1a2958 100644
--- a/spec/models/validations/sales/financial_validations_spec.rb
+++ b/spec/models/validations/sales/financial_validations_spec.rb
@@ -514,9 +514,10 @@ RSpec.describe Validations::Sales::FinancialValidations do
         record.numstair = 3
         financial_validator.validate_staircase_difference(record)
         expect(record.errors["equity"]).to include(I18n.t("validations.sales.financial.equity.more_than_stairowned_minus_stairbought_minus_prev_staircasing", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
-        expect(record.errors["stairowned"]).to include(I18n.t("validations.sales.financial.equity.more_than_stairowned_minus_stairbought_minus_prev_staircasing", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
-        expect(record.errors["stairbought"]).to include(I18n.t("validations.sales.financial.equity.more_than_stairowned_minus_stairbought_minus_prev_staircasing", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
-        expect(record.errors["numstair"]).to include(I18n.t("validations.sales.financial.equity.more_than_stairowned_minus_stairbought_minus_prev_staircasing", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
+        expect(record.errors["stairowned"]).to include(I18n.t("validations.sales.financial.stairowned.less_than_stairbought_plus_equity_plus_prev_staircasing", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
+        expect(record.errors["stairbought"]).to include(I18n.t("validations.sales.financial.stairbought.more_than_stairowned_minus_equity_minus_prev_staircasing", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
+        expect(record.errors["numstair"]).to include(I18n.t("validations.sales.financial.numstair.too_high_for_stairowned_minus_stairbought_minus_equity", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
+        expect(record.errors["firststair"]).to include(I18n.t("validations.sales.financial.firststair.invalid_for_stairowned_minus_stairbought_minus_equity", equity: 9, bought: 10, numprevstair: 2, equity_sum: 21, stair_total: 20))
       end
 
       it "does not add errors if stairnum is present and stairowned is enough more than stairbought + equity" do
diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb
index 5545d6ca2..c6ddea466 100644
--- a/spec/requests/merge_requests_controller_spec.rb
+++ b/spec/requests/merge_requests_controller_spec.rb
@@ -416,7 +416,7 @@ RSpec.describe MergeRequestsController, type: :request do
         context "when merge date set to a date more than 1 year in the future" do
           let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) }
           let(:params) do
-            { merge_request: { page: "merge_date", "merge_date": [(Time.zone.now.day + 1).to_s, Time.zone.now.month.to_s, (Time.zone.now.year + 1).to_s].join("/") } }
+            { merge_request: { page: "merge_date", "merge_date": (Time.zone.now + 1.year + 1.day).strftime("%d/%m/%Y") } }
           end
 
           let(:request) do