Merge branch 'main' into CLDC-868-purchase-price-validations

# Conflicts:
#	app/models/validations/sales/sale_information_validations.rb
#	app/models/validations/sales/soft_validations.rb
#	config/locales/en.yml
#	db/schema.rb
#	spec/models/form_handler_spec.rb

app/helpers/collection_time_helper.rb

@@ -0,0 +1,16 @@
+module CollectionTimeHelper
+  def current_collection_start_year
+    today = Time.zone.now
+    window_end_date = Time.zone.local(today.year, 4, 1)
+    today < window_end_date ? today.year - 1 : today.year
+  end
+
+  def collection_start_date(date)
+    window_end_date = Time.zone.local(date.year, 4, 1)
+    date < window_end_date ? Time.zone.local(date.year - 1, 4, 1) : Time.zone.local(date.year, 4, 1)
+  end
+
+  def current_collection_start_date
+    Time.zone.local(current_collection_start_year, 4, 1)
+  end
+end

app/mailers/bulk_upload_mailer.rb

@@ -63,30 +63,44 @@ class BulkUploadMailer < NotifyMailer
     )
   end
 
-  def send_bulk_upload_failed_service_error_mail(user, bulk_upload)
+  def send_bulk_upload_failed_service_error_mail(bulk_upload:)
+    bulk_upload_link = if bulk_upload.lettings?
+                         start_bulk_upload_lettings_logs_url
+                       else
+                         start_bulk_upload_sales_logs_url
+                       end
+
     send_email(
-      user.email,
+      bulk_upload.user.email,
       BULK_UPLOAD_FAILED_SERVICE_ERROR_TEMPLATE_ID,
       {
-        filename: "[#{bulk_upload} filename]",
-        upload_timestamp: "[#{bulk_upload} upload_timestamp]",
-        lettings_or_sales: "[#{bulk_upload} lettings_or_sales]",
-        year_combo: "[#{bulk_upload} year_combo]",
-        bulk_upload_link: "[#{bulk_upload} bulk_upload_link]",
+        filename: bulk_upload.filename,
+        upload_timestamp: bulk_upload.created_at,
+        lettings_or_sales: bulk_upload.log_type,
+        year_combo: bulk_upload.year_combo,
+        bulk_upload_link:,
       },
     )
   end
 
-  def send_bulk_upload_with_errors_mail(user, bulk_upload)
+  def send_bulk_upload_with_errors_mail(bulk_upload:)
+    count = bulk_upload.logs.where.not(status: %w[completed]).count
+
+    n_logs = pluralize(count, "log")
+
+    title = "We found #{n_logs} with errors"
+
+    error_description = "We created logs from your #{bulk_upload.year_combo} #{bulk_upload.log_type} data. There was a problem with #{count} of the logs. Click the below link to fix these logs."
+
     send_email(
-      user.email,
+      bulk_upload.user.email,
       BULK_UPLOAD_WITH_ERRORS_TEMPLATE_ID,
       {
-        title: "[#{bulk_upload} title]",
-        filename: "[#{bulk_upload} filename]",
-        upload_timestamp: "[#{bulk_upload} upload_timestamp]",
-        error_description: "[#{bulk_upload} error_description]",
-        summary_report_link: "[#{bulk_upload} summary_report_link]",
+        title:,
+        filename: bulk_upload.filename,
+        upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time),
+        error_description:,
+        summary_report_link: resume_bulk_upload_lettings_result_url(bulk_upload),
       },
     )
   end

app/models/form/lettings/questions/managing_organisation.rb

@@ -1,48 +1,45 @@
 class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question
-  attr_accessor :current_user, :log
-
   def initialize(id, hsh, page)
     super
     @id = "managing_organisation_id"
     @check_answer_label = "Managing agent"
     @header = "Which organisation manages this letting?"
     @type = "select"
-    @answer_options = answer_options
   end
 
-  def answer_options
+  def answer_options(log = nil, user = nil)
     opts = { "" => "Select an option" }
 
     return opts unless ActiveRecord::Base.connected?
-    return opts unless current_user
+    return opts unless user
     return opts unless log
 
     if log.managing_organisation.present?
       opts = opts.merge({ log.managing_organisation.id => log.managing_organisation.name })
     end
 
-    if current_user.support?
+    if user.support?
       if log.owning_organisation.holds_own_stock?
         opts[log.owning_organisation.id] = "#{log.owning_organisation.name} (Owning organisation)"
       end
     else
-      opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)"
+      opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)"
     end
 
-    opts.merge(managing_organisations_answer_options)
+    orgs = if user.support?
+             log.owning_organisation
+           else
+             user.organisation
+           end.managing_agents.pluck(:id, :name).to_h
+
+    opts.merge(orgs)
   end
 
   def displayed_answer_options(log, user)
-    @current_user = user
-    @log = log
-
-    answer_options
+    answer_options(log, user)
   end
 
-  def label_from_value(value, log = nil, user = nil)
-    @log = log
-    @current_user = user
-
+  def label_from_value(value, _log = nil, _user = nil)
     return unless value
 
     answer_options[value]
@@ -53,25 +50,20 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question
   end
 
   def hidden_in_check_answers?(log, user = nil)
-    @current_user = user
-    @current_user.nil? || !@page.routed_to?(log, user)
+    user.nil? || !@page.routed_to?(log, user)
   end
 
   def enabled
     true
   end
 
+  def answer_label(log, _current_user = nil)
+    Organisation.find_by(id: log.managing_organisation_id)&.name
+  end
+
 private
 
   def selected_answer_option_is_derived?(_log)
     true
   end
-
-  def managing_organisations_answer_options
-    if current_user.support?
-      log.owning_organisation
-    else
-      current_user.organisation
-    end.managing_agents.pluck(:id, :name).to_h
-  end
 end

app/models/form/lettings/questions/stock_owner.rb

@@ -1,6 +1,4 @@
 class Form::Lettings::Questions::StockOwner < ::Form::Question
-  attr_accessor :current_user, :log
-
   def initialize(id, hsh, page)
     super
     @id = "owning_organisation_id"
@@ -9,38 +7,38 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question
     @type = "select"
   end
 
-  def answer_options
+  def answer_options(log = nil, user = nil)
     answer_opts = { "" => "Select an option" }
 
     return answer_opts unless ActiveRecord::Base.connected?
-    return answer_opts unless current_user
+    return answer_opts unless user
     return answer_opts unless log
 
     if log.owning_organisation_id.present?
       answer_opts = answer_opts.merge({ log.owning_organisation.id => log.owning_organisation.name })
     end
 
-    if !current_user.support? && current_user.organisation.holds_own_stock?
-      answer_opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)"
+    if !user.support? && user.organisation.holds_own_stock?
+      answer_opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)"
     end
 
+    stock_owners_answer_options = if user.support?
+                                    Organisation
+                                  else
+                                    user.organisation.stock_owners
+                                  end.pluck(:id, :name).to_h
+
     answer_opts.merge(stock_owners_answer_options)
   end
 
   def displayed_answer_options(log, user = nil)
-    @current_user = user
-    @log = log
-
-    answer_options
+    answer_options(log, user)
   end
 
   def label_from_value(value, log = nil, user = nil)
-    @log = log
-    @current_user = user
-
     return unless value
 
-    answer_options[value]
+    answer_options(log, user)[value]
   end
 
   def derived?
@@ -48,13 +46,11 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question
   end
 
   def hidden_in_check_answers?(_log, user = nil)
-    @current_user = user
+    return false if user.support?
 
-    return false if current_user.support?
+    stock_owners = user.organisation.stock_owners
 
-    stock_owners = current_user.organisation.stock_owners
-
-    if current_user.organisation.holds_own_stock?
+    if user.organisation.holds_own_stock?
       stock_owners.count.zero?
     else
       stock_owners.count <= 1
@@ -70,12 +66,4 @@ private
   def selected_answer_option_is_derived?(_log)
     true
   end
-
-  def stock_owners_answer_options
-    if current_user.support?
-      Organisation
-    else
-      current_user.organisation.stock_owners
-    end.pluck(:id, :name).to_h
-  end
 end

app/models/form/sales/pages/about_price_not_rtb.rb

@@ -11,7 +11,7 @@ class Form::Sales::Pages::AboutPriceNotRtb < ::Form::Page
 
   def questions
     @questions ||= [
-      Form::Sales::Questions::Value.new(nil, nil, self),
+      Form::Sales::Questions::PurchasePrice.new(nil, nil, self),
       Form::Sales::Questions::Grant.new(nil, nil, self),
     ]
   end

app/models/form/sales/pages/about_price_rtb.rb

@@ -10,7 +10,7 @@ class Form::Sales::Pages::AboutPriceRtb < ::Form::Page
 
   def questions
     @questions ||= [
-      Form::Sales::Questions::Value.new(nil, nil, self),
+      Form::Sales::Questions::PurchasePrice.new(nil, nil, self),
       Form::Sales::Questions::Discount.new(nil, nil, self),
     ]
   end

app/models/form/sales/pages/handover_date_check.rb

@@ -1,8 +1,14 @@
 class Form::Sales::Pages::HandoverDateCheck < ::Form::Page
   def initialize(id, hsh, subsection)
     super
-    @depends_on = [{ "hodate_3_years_or_more_saledate?" => true }]
+    @id = "handover_date_check"
+    @depends_on = [{ "saledate_check" => nil, "hodate_3_years_or_more_saledate?" => true },
+                   { "saledate_check" => 1, "hodate_3_years_or_more_saledate?" => true }]
     @informative_text = {}
+    @title_text = {
+      "translation" => "validations.sale_information.hodate.must_be_less_than_3_years_from_saledate",
+      "arguments" => [],
+    }
   end
 
   def questions

app/models/form/sales/pages/purchase_price.rb

@@ -2,8 +2,7 @@ class Form::Sales::Pages::PurchasePrice < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @depends_on = [
-      { "ownershipsch" => 3 },
-      { "rent_to_buy_full_ownership?" => true },
+      { "ownershipsch" => 2, "rent_to_buy_full_ownership?" => false },
     ]
   end
 

app/models/form/sales/pages/purchase_price_outright_ownership.rb

@@ -0,0 +1,14 @@
+class Form::Sales::Pages::PurchasePriceOutrightOwnership < ::Form::Page
+  def initialize(id, hsh, subsection)
+    super
+    @depends_on = [
+      { "outright_sale_or_discounted_with_full_ownership?" => true },
+    ]
+  end
+
+  def questions
+    @questions ||= [
+      Form::Sales::Questions::PurchasePriceOutrightOwnership.new(nil, nil, self),
+    ]
+  end
+end

app/models/form/sales/pages/sale_date_check.rb

@@ -0,0 +1,19 @@
+class Form::Sales::Pages::SaleDateCheck < ::Form::Page
+  def initialize(id, hsh, subsection)
+    super
+    @id = "completion_date_check"
+    @depends_on = [{ "hodate_check" => nil, "hodate_3_years_or_more_saledate?" => true },
+                   { "hodate_check" => 1, "hodate_3_years_or_more_saledate?" => true }]
+    @informative_text = {}
+    @title_text = {
+      "translation" => "validations.sale_information.saledate.must_be_less_than_3_years_from_hodate",
+      "arguments" => [],
+    }
+  end
+
+  def questions
+    @questions ||= [
+      Form::Sales::Questions::SaleDateCheck.new(nil, nil, self),
+    ]
+  end
+end

app/models/form/sales/questions/handover_date_check.rb

@@ -3,7 +3,7 @@ class Form::Sales::Questions::HandoverDateCheck < ::Form::Question
     super
     @id = "hodate_check"
     @check_answer_label = "Practical completion or handover date check"
-    @header = "Are you sure practical completion or handover date is more than 3 years before exchange date?"
+    @header = "Are you sure?"
     @type = "interruption_screen"
     @answer_options = {
       "0" => { "value" => "Yes" },
@@ -17,6 +17,12 @@ class Form::Sales::Questions::HandoverDateCheck < ::Form::Question
         {
           "hodate_check" => 1,
         },
+        {
+          "saledate_check" => 0,
+        },
+        {
+          "saledate_check" => 1,
+        },
       ],
     }
   end

app/models/form/sales/questions/number_of_others_in_property.rb

@@ -7,5 +7,7 @@ class Form::Sales::Questions::NumberOfOthersInProperty < ::Form::Question
     @type = "numeric"
     @hint_text = "You can provide details for a maximum of 4 other people."
     @width = 2
+    @min = 0
+    @max = 4
   end
 end

app/models/form/sales/questions/purchase_price.rb

@@ -8,5 +8,6 @@ class Form::Sales::Questions::PurchasePrice < ::Form::Question
     @min = 0
     @width = 5
     @prefix = "£"
+    @hint_text = "For all schemes, including Right to Acquire (RTA), Right to Buy (RTB), Voluntary Right to Buy (VRTB) or Preserved Right to Buy (PRTB) sales, enter the full price of the property without any discount"
   end
 end

app/models/form/sales/questions/purchase_price_outright_ownership.rb

@@ -0,0 +1,12 @@
+class Form::Sales::Questions::PurchasePriceOutrightOwnership < ::Form::Question
+  def initialize(id, hsh, page)
+    super
+    @id = "value"
+    @check_answer_label = "Purchase price"
+    @header = "What is the full purchase price?"
+    @type = "numeric"
+    @min = 0
+    @width = 5
+    @prefix = "£"
+  end
+end

app/models/form/sales/questions/sale_date_check.rb

@@ -0,0 +1,29 @@
+class Form::Sales::Questions::SaleDateCheck < ::Form::Question
+  def initialize(id, hsh, page)
+    super
+    @id = "saledate_check"
+    @check_answer_label = "Sale completion date check"
+    @header = "Are you sure?"
+    @type = "interruption_screen"
+    @answer_options = {
+      "0" => { "value" => "Yes" },
+      "1" => { "value" => "No" },
+    }
+    @hidden_in_check_answers = {
+      "depends_on" => [
+        {
+          "hodate_check" => 0,
+        },
+        {
+          "hodate_check" => 1,
+        },
+        {
+          "saledate_check" => 0,
+        },
+        {
+          "saledate_check" => 1,
+        },
+      ],
+    }
+  end
+end

app/models/form/sales/subsections/discounted_ownership_scheme.rb

@@ -14,6 +14,7 @@ class Form::Sales::Subsections::DiscountedOwnershipScheme < ::Form::Subsection
       Form::Sales::Pages::AboutPriceNotRtb.new(nil, nil, self),
       Form::Sales::Pages::GrantValueCheck.new(nil, nil, self),
       Form::Sales::Pages::PurchasePrice.new("purchase_price_discounted_ownership", nil, self),
+      Form::Sales::Pages::PurchasePriceOutrightOwnership.new("purchase_price_outright_ownership", nil, self),
       Form::Sales::Pages::DepositAndMortgageValueCheck.new("discounted_ownership_deposit_and_mortgage_value_check_after_value_and_discount", nil, self),
       Form::Sales::Pages::Mortgageused.new("mortgage_used_discounted_ownership", nil, self),
       Form::Sales::Pages::MortgageValueCheck.new("discounted_ownership_mortgage_used_mortgage_value_check", nil, self),

app/models/form/sales/subsections/outright_sale.rb

@@ -8,7 +8,7 @@ class Form::Sales::Subsections::OutrightSale < ::Form::Subsection
 
   def pages
     @pages ||= [
-      Form::Sales::Pages::PurchasePrice.new("purchase_price_outright_sale", nil, self),
+      Form::Sales::Pages::PurchasePriceOutrightOwnership.new("purchase_price_outright_sale", nil, self),
       Form::Sales::Pages::Mortgageused.new("mortgage_used_outright_sale", nil, self),
       Form::Sales::Pages::MortgageValueCheck.new("outright_sale_mortgage_used_mortgage_value_check", nil, self),
       Form::Sales::Pages::MortgageAmount.new("mortgage_amount_outright_sale", nil, self),

app/models/form/sales/subsections/setup.rb

@@ -10,6 +10,7 @@ class Form::Sales::Subsections::Setup < ::Form::Subsection
       Form::Common::Pages::Organisation.new(nil, nil, self),
       Form::Common::Pages::CreatedBy.new(nil, nil, self),
       Form::Sales::Pages::SaleDate.new(nil, nil, self),
+      Form::Sales::Pages::SaleDateCheck.new(nil, nil, self),
       Form::Sales::Pages::PurchaserCode.new(nil, nil, self),
       Form::Sales::Pages::OwnershipScheme.new(nil, nil, self),
       Form::Sales::Pages::SharedOwnershipType.new(nil, nil, self),

app/models/form/sales/subsections/shared_ownership_scheme.rb

@@ -15,7 +15,7 @@ class Form::Sales::Subsections::SharedOwnershipScheme < ::Form::Subsection
       Form::Sales::Pages::Resale.new(nil, nil, self),
       Form::Sales::Pages::ExchangeDate.new(nil, nil, self),
       Form::Sales::Pages::HandoverDate.new(nil, nil, self),
-      Form::Sales::Pages::HandoverDateCheck.new("handover_date_check", nil, self),
+      Form::Sales::Pages::HandoverDateCheck.new(nil, nil, self),
       Form::Sales::Pages::LaNominations.new(nil, nil, self),
       Form::Sales::Pages::BuyerPrevious.new(nil, nil, self),
       Form::Sales::Pages::PreviousBedrooms.new(nil, nil, self),

app/models/form_handler.rb

@@ -1,5 +1,6 @@
 class FormHandler
   include Singleton
+  include CollectionTimeHelper
   attr_reader :forms
 
   def initialize
@@ -44,21 +45,6 @@ class FormHandler
     forms
   end
 
-  def current_collection_start_year
-    today = Time.zone.now
-    window_end_date = Time.zone.local(today.year, 4, 1)
-    today < window_end_date ? today.year - 1 : today.year
-  end
-
-  def collection_start_date(date)
-    window_end_date = Time.zone.local(date.year, 4, 1)
-    date < window_end_date ? Time.zone.local(date.year - 1, 4, 1) : Time.zone.local(date.year, 4, 1)
-  end
-
-  def current_collection_start_date
-    Time.zone.local(current_collection_start_year, 4, 1)
-  end
-
   def form_name_from_start_year(year, type)
     form_mappings = { 0 => "current_#{type}", 1 => "previous_#{type}", -1 => "next_#{type}" }
     form_mappings[current_collection_start_year - year]

app/models/sales_log.rb

@@ -35,7 +35,7 @@ class SalesLog < Log
   scope :search_by, ->(param) { filter_by_id(param) }
   scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org) }
 
-  OPTIONAL_FIELDS = %w[purchid monthly_charges_value_check old_persons_shared_ownership_value_check].freeze
+  OPTIONAL_FIELDS = %w[saledate_check purchid monthly_charges_value_check old_persons_shared_ownership_value_check].freeze
   RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze
 
   def startdate
@@ -132,6 +132,10 @@ class SalesLog < Log
     type == 29
   end
 
+  def outright_sale_or_discounted_with_full_ownership?
+    ownershipsch == 3 || (ownershipsch == 2 && rent_to_buy_full_ownership?)
+  end
+
   def is_type_discount?
     type == 18
   end

app/models/validations/date_validations.rb

@@ -84,15 +84,6 @@ private
     @second_collection_end_date ||= FormHandler.instance.forms.map { |_name, form| form.end_date }.compact.max
   end
 
-  def date_valid?(question, record)
-    if record[question].is_a?(ActiveSupport::TimeWithZone) && record[question].year.zero?
-      record.errors.add question, I18n.t("validations.date.invalid_date")
-      false
-    else
-      true
-    end
-  end
-
   def is_rsnvac_first_let?(record)
     [15, 16, 17].include?(record["rsnvac"])
   end

app/models/validations/sales/household_validations.rb

@@ -1,14 +1,6 @@
 module Validations::Sales::HouseholdValidations
   include Validations::SharedValidations
 
-  def validate_number_of_other_people_living_in_the_property(record)
-    return if record.hholdcount.blank?
-
-    unless record.hholdcount >= 0 && record.hholdcount <= 4
-      record.errors.add :hholdcount, I18n.t("validations.numeric.valid", field: "Number of other people living in the property", min: 0, max: 4)
-    end
-  end
-
   def validate_household_number_of_other_members(record)
     (2..6).each do |n|
       validate_person_age_matches_relationship(record, n)

app/models/validations/sales/sale_information_validations.rb

@@ -2,7 +2,7 @@ module Validations::Sales::SaleInformationValidations
   def validate_practical_completion_date_before_saledate(record)
     return if record.saledate.blank? || record.hodate.blank?
 
-    unless record.saledate > record.hodate
+    if record.hodate > record.saledate
       record.errors.add :hodate, I18n.t("validations.sale_information.hodate.must_be_before_saledate")
       record.errors.add :saledate, I18n.t("validations.sale_information.saledate.must_be_after_hodate")
     end

app/models/validations/sales/setup_validations.rb

@@ -1,6 +1,8 @@
 module Validations::Sales::SetupValidations
+  include Validations::SharedValidations
+
   def validate_saledate(record)
-    return unless record.saledate
+    return unless record.saledate && date_valid?("saledate", record)
 
     unless Time.zone.local(2022, 4, 1) <= record.saledate && record.saledate < Time.zone.local(2023, 4, 1)
       record.errors.add :saledate, I18n.t("validations.setup.saledate.financial_year")

app/models/validations/shared_validations.rb

@@ -20,20 +20,16 @@ module Validations::SharedValidations
       next unless question.min || question.max
       next unless record[question.id]
 
-      field = question.check_answer_label || question.id
-      min = [question.prefix, number_with_delimiter(question.min, delimiter: ","), question.suffix].join("")
-      max = [question.prefix, number_with_delimiter(question.max, delimiter: ","), question.suffix].join("")
-
       begin
         answer = Float(record.public_send("#{question.id}_before_type_cast"))
       rescue ArgumentError
-        record.errors.add question.id.to_sym, I18n.t("validations.numeric.valid", field:, min:, max:)
+        add_range_error(record, question)
       end
 
       next unless answer
 
       if (question.min && question.min > answer) || (question.max && question.max < answer)
-        record.errors.add question.id.to_sym, I18n.t("validations.numeric.valid", field:, min:, max:)
+        add_range_error(record, question)
       end
     end
   end
@@ -103,9 +99,30 @@ module Validations::SharedValidations
     end
   end
 
+  def date_valid?(question, record)
+    if record[question].is_a?(ActiveSupport::TimeWithZone) && record[question].year.zero?
+      record.errors.add question, I18n.t("validations.date.invalid_date")
+      false
+    else
+      true
+    end
+  end
+
 private
 
   def person_is_partner?(relationship)
     relationship == "P"
   end
+
+  def add_range_error(record, question)
+    field = question.check_answer_label || question.id
+    min = [question.prefix, number_with_delimiter(question.min, delimiter: ","), question.suffix].join("") if question.min
+    max = [question.prefix, number_with_delimiter(question.max, delimiter: ","), question.suffix].join("") if question.max
+
+    if min && max
+      record.errors.add question.id.to_sym, I18n.t("validations.numeric.within_range", field:, min:, max:)
+    elsif min
+      record.errors.add question.id.to_sym, I18n.t("validations.numeric.above_min", field:, min:)
+    end
+  end
 end

app/services/bulk_upload/lettings/row_parser.rb

@@ -139,13 +139,10 @@ class BulkUpload::Lettings::RowParser
   attribute :field_133, :integer
   attribute :field_134, :integer
 
-  validates :field_1, presence: true, inclusion: { in: (1..12).to_a }
+  validates :field_1, presence: { message: I18n.t("validations.not_answered", question: "letting type") },
+                      inclusion: { in: (1..12).to_a, message: I18n.t("validations.invalid_option", question: "letting type") }
   validates :field_4, presence: { if: proc { [2, 4, 6, 8, 10, 12].include?(field_1) } }
 
-  validates :field_96, presence: true
-  validates :field_97, presence: true
-  validates :field_98, presence: true
-
   def valid?
     errors.clear
 
@@ -176,7 +173,7 @@ private
 
   def validate_data_types
     unless attribute_set["field_1"].value_before_type_cast&.match?(/\A\d+\z/)
-      errors.add(:field_1, :invalid)
+      errors.add(:field_1, I18n.t("validations.invalid_number", question: "letting type"))
     end
   end
 

app/services/bulk_upload/lettings/validator.rb

@@ -162,7 +162,7 @@ class BulkUpload::Lettings::Validator
       row_parser.errors.each do |error|
         bulk_upload.bulk_upload_errors.create!(
           field: error.attribute,
-          error: error.type,
+          error: error.message,
           tenant_code: row_parser.field_7,
           property_ref: row_parser.field_100,
           row:,

app/services/bulk_upload/processor.rb

@@ -7,19 +7,42 @@ class BulkUpload::Processor
 
   def call
     download
+
+    return send_failure_mail if validator.invalid?
+
     validator.call
+
     create_logs if validator.create_logs?
-    send_success_mail
+
+    send_fix_errors_mail if created_logs_but_incompleted?
+    send_success_mail if created_logs_and_all_completed?
+  rescue StandardError => e
+    Sentry.capture_exception(e)
+    send_failure_mail
   ensure
     downloader.delete_local_file!
   end
 
 private
 
+  def send_fix_errors_mail
+    BulkUploadMailer.send_bulk_upload_with_errors_mail(bulk_upload:).deliver_later
+  end
+
   def send_success_mail
-    if validator.create_logs? && bulk_upload.logs.group(:status).count.keys == %w[completed]
-      BulkUploadMailer.send_bulk_upload_complete_mail(user:, bulk_upload:).deliver_later
-    end
+    BulkUploadMailer.send_bulk_upload_complete_mail(user:, bulk_upload:).deliver_later
+  end
+
+  def created_logs_but_incompleted?
+    validator.create_logs? && bulk_upload.logs.where.not(status: %w[completed]).count.positive?
+  end
+
+  def created_logs_and_all_completed?
+    validator.create_logs? && bulk_upload.logs.group(:status).count.keys == %w[completed]
+  end
+
+  def send_failure_mail
+    BulkUploadMailer.send_bulk_upload_failed_service_error_mail(bulk_upload:).deliver_later
   end
 
   def user

app/views/form/page.html.erb

@@ -34,9 +34,26 @@
             <%= govuk_section_break(visible: true, size: "m") %>
           <% end %>
           <% if question.type == "interruption_screen" %>
-            <%= render partial: "form/#{question.type}_question", locals: { question:, caption_text: @subsection.label, page_header: @page.header, lettings_log: @log, title_text: @page.title_text, informative_text: @page.informative_text, form: @form, f:, conditional: false } %>
+            <%= render partial: "form/#{question.type}_question", locals: {
+              question:,
+              caption_text: @subsection.label,
+              page_header: @page.header,
+              lettings_log: @log,
+              title_text: @page.title_text,
+              informative_text: @page.informative_text,
+              form: @form,
+              f:,
+              conditional: false,
+            } %>
           <% else %>
-            <%= render partial: "form/#{question.type}_question", locals: { question:, caption_text: @page.header_partial.present? ? nil : @subsection.label, page_header: @page.header, lettings_log: @log, f:, conditional: false } %>
+            <%= render partial: "form/#{question.type}_question", locals: {
+              question:,
+              caption_text: @page.header_partial.present? ? nil : @subsection.label,
+              page_header: @page.header,
+              lettings_log: @log,
+              f:,
+              conditional: false,
+            } %>
           <% end %>
         </div>
       <% end %>

config/forms/2021_2022.json

@@ -8411,7 +8411,7 @@
                 }
               ],
               "title_text": {
-                "translation": "soft_validations.rent.min.title_text",
+                "translation": "soft_validations.rent.outside_range_title",
                 "arguments": [
                   {
                     "key": "brent",
@@ -8421,7 +8421,7 @@
                 ]
               },
               "informative_text": {
-                "translation": "soft_validations.rent.min.hint_text",
+                "translation": "soft_validations.rent.min_hint_text",
                 "arguments": [
                   {
                     "key": "soft_min_for_period",
@@ -8463,7 +8463,7 @@
                 }
               ],
               "title_text": {
-                "translation": "soft_validations.rent.max.title_text",
+                "translation": "soft_validations.rent.outside_range_title",
                 "arguments": [
                   {
                     "key": "brent",
@@ -8473,7 +8473,7 @@
                 ]
               },
               "informative_text": {
-                "translation": "soft_validations.rent.max.hint_text",
+                "translation": "soft_validations.rent.max_hint_text",
                 "arguments": [
                   {
                     "key": "soft_max_for_period",

config/forms/2022_2023.json

@@ -8376,7 +8376,7 @@
                 }
               ],
               "title_text": {
-                "translation": "soft_validations.rent.min.title_text",
+                "translation": "soft_validations.rent.outside_range_title",
                 "arguments": [
                   {
                     "key": "brent",
@@ -8385,16 +8385,7 @@
                   }
                 ]
               },
-              "informative_text": {
-                "translation": "soft_validations.rent.min.hint_text",
-                "arguments": [
-                  {
-                    "key": "soft_min_for_period",
-                    "label": false,
-                    "i18n_template": "soft_min_for_period"
-                  }
-                ]
-              },
+              "informative_text": {},
               "questions": {
                 "rent_value_check": {
                   "check_answer_label": "Total rent confirmation",
@@ -8408,7 +8399,8 @@
                       }
                     ]
                   },
-                  "header": "Are you sure this is correct?",
+                  "header": "This rent is lower than expected for this property type, in this area. Check:",
+                  "hint_text": "<ul><li>the decimal point is not missing (£X.XX)</li><li>the frequency is correct, for example weekly, monthly</li><li>the rent type is correct, for example affordable or social rent</li></ul><p>Are you sure this is correct?</p>",
                   "type": "interruption_screen",
                   "answer_options": {
                     "0": {
@@ -8428,7 +8420,7 @@
                 }
               ],
               "title_text": {
-                "translation": "soft_validations.rent.max.title_text",
+                "translation": "soft_validations.rent.outside_range_title",
                 "arguments": [
                   {
                     "key": "brent",
@@ -8437,16 +8429,7 @@
                   }
                 ]
               },
-              "informative_text": {
-                "translation": "soft_validations.rent.max.hint_text",
-                "arguments": [
-                  {
-                    "key": "soft_max_for_period",
-                    "label": false,
-                    "i18n_template": "soft_max_for_period"
-                  }
-                ]
-              },
+              "informative_text": {},
               "questions": {
                 "rent_value_check": {
                   "check_answer_label": "Total rent confirmation",
@@ -8460,7 +8443,8 @@
                       }
                     ]
                   },
-                  "header": "Are you sure this is correct?",
+                  "header": "This rent is higher than expected for this property type, in this area. Check:",
+                  "hint_text": "<ul><li>the decimal point is not missing (£X.XX)</li><li>the frequency is correct, for example weekly, monthly</li><li>the rent type is correct, for example affordable or social rent</li></ul><p>Are you sure this is correct?</p>",
                   "type": "interruption_screen",
                   "answer_options": {
                     "0": {

config/locales/en.yml

@@ -127,11 +127,13 @@ en:
         already_added: "You have already added this managing agent"
     not_answered: "You must answer %{question}"
     invalid_option: "Enter a valid value for %{question}"
+    invalid_number: "Enter a number for %{question}"
 
     other_field_missing: "If %{main_field_label} is other then %{other_field_label} must be provided"
     other_field_not_required: "%{other_field_label} must not be provided if %{main_field_label} was not other"
     numeric:
-      valid: "%{field} must be between %{min} and %{max}"
+      within_range: "%{field} must be between %{min} and %{max}"
+      above_min: "%{field} must be at least %{min}"
     date:
       invalid_date: "Enter a date in the correct format, for example 31 1 2022"
       outside_collection_window: "Enter a date within the current collection windows"
@@ -148,7 +150,7 @@ en:
       intermediate_rent_product_name:
         blank: "Enter name of other intermediate rent product"
       saledate:
-        financial_year: "Date must be from 22/23 financial year"
+        financial_year: "Date must be from 22/23 financial year, which is between 1st April 2022 and 31st March 2023"
       startdate:
         later_than_14_days_after: "The tenancy start date must not be later than 14 days from today’s date"
         before_scheme_end_date: "The tenancy start date must be before the end date for this supported housing scheme"
@@ -425,13 +427,18 @@ en:
         social_homebuy: "Social HomeBuy buyers should not have lived here before"
         rent_to_buy: "Rent to Buy buyers should not have lived here before"
       hodate:
-        must_be_before_exdate: "Practical completion or handover date must be before exchange date"
+        must_be_before_saledate: "Practical completion or handover date must be before exchange date"
+        must_be_less_than_3_years_from_saledate: "You told us practical completion or handover date is more than 3 years before completion date"
       exdate:
-        must_be_before_saledate: "Contract exchange date must be less than 1 year before completion date"
+        must_be_before_saledate: "Contract exchange date must be before completion date"
         must_be_less_than_1_year_from_saledate: "Contract exchange date must be less than 1 year before completion date"
       saledate:
-        must_be_after_exdate: "Completion date must be less than 1 year after contract exchange date"
+        must_be_after_exdate: "Completion date must be after contract exchange date"
         must_be_less_than_1_year_from_exdate: "Completion date must be less than 1 year after contract exchange date"
+        must_be_less_than_3_years_from_hodate: "You told us completion date is more than 3 years after practical completion or handover date"
+        must_be_after_hodate: "Completion date must be after practical completion or handover date"
+      previous_property_beds:
+        property_type_bedsit: "Bedsit bedroom maximum 1"
       previous_property_type:
         property_type_bedsit: "A bedsit can not have more than 1 bedroom"
       discounted_ownership_value: "Mortgage, deposit, and grant total must equal £%{value_with_discount}"
@@ -447,12 +454,9 @@ en:
       in_soft_max_range:
         message: "Net income is higher than expected based on the lead tenant’s working situation. Are you sure this is correct?"
     rent:
-      min:
-        title_text: "You told us the rent is %{brent}"
-        hint_text: "The minimum rent expected for this type of property in this local authority is £%{soft_min_for_period}"
-      max:
-        title_text: "You told us the rent is %{brent}"
-        hint_text: "The maximum rent expected for this type of property in this local authority is £%{soft_max_for_period}"
+      outside_range_title: "You told us the rent is %{brent}"
+      min_hint_text: "The minimum rent expected for this type of property in this local authority is £%{soft_min_for_period}."
+      max_hint_text: "The maximum rent expected for this type of property in this local authority is £%{soft_max_for_period}."
     purchase_price:
       title_text: "You told us the purchase price is %{value}"
       hint_text: "The %{min_or_max} purchase price expected for this type of property in this local authority is %{soft_min_or_soft_max}"
@@ -476,6 +480,7 @@ en:
     shared_ownership_deposit:
       title_text: "Mortgage, deposit and cash discount total should equal £%{expected_shared_ownership_deposit_value}"
     old_persons_shared_ownership: "At least one buyer should be aged over 64 for Older persons’ shared ownership scheme"
+    staircase_bought_seems_high: "You said %{percentage}% was bought in this staircasing transaction, which seems high. Are you sure?"
     monthly_charges_over_soft_max:
       title_text: "The amount of monthly charges is high for this type of property and sale type"
 

db/migrate/20230203104238_add_saledate_check_to_sales_log.rb

@@ -0,0 +1,5 @@
+class AddSaledateCheckToSalesLog < ActiveRecord::Migration[7.0]
+  def change
+    add_column :sales_logs, :saledate_check, :integer
+  end
+end

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: 2023_01_27_102334) do
+ActiveRecord::Schema[7.0].define(version: 2023_02_03_104238) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -70,17 +70,6 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_27_102334) do
     t.index ["start_year", "lettype", "beds", "la"], name: "index_la_rent_ranges_on_start_year_and_lettype_and_beds_and_la", unique: true
   end
 
-  create_table "la_sale_ranges", force: :cascade do |t|
-    t.string "la"
-    t.integer "bedrooms"
-    t.decimal "soft_min", precision: 10, scale: 2
-    t.decimal "soft_max", precision: 10, scale: 2
-    t.integer "start_year"
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
-    t.index ["start_year", "bedrooms", "la"], name: "index_la_sale_ranges_on_start_year_bedrooms_la", unique: true
-  end
-
   create_table "legacy_users", force: :cascade do |t|
     t.string "old_user_id"
     t.integer "user_id"
@@ -518,10 +507,10 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_27_102334) do
     t.integer "deposit_and_mortgage_value_check"
     t.integer "shared_ownership_deposit_value_check"
     t.integer "grant_value_check"
-    t.integer "value_value_check"
     t.integer "old_persons_shared_ownership_value_check"
     t.integer "staircase_bought_value_check"
     t.integer "monthly_charges_value_check"
+    t.integer "saledate_check"
     t.index ["bulk_upload_id"], name: "index_sales_logs_on_bulk_upload_id"
     t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id"
     t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id"

spec/helpers/collection_time_helper_spec.rb

@@ -0,0 +1,34 @@
+require "rails_helper"
+
+RSpec.describe CollectionTimeHelper do
+  let(:current_user) { create(:user, :data_coordinator) }
+  let(:user) { create(:user, :data_coordinator) }
+
+  around do |example|
+    Timecop.freeze(now) do
+      example.run
+    end
+  end
+
+  describe "Current collection start year" do
+    context "when the date is after 1st of April" do
+      let(:now) { Time.utc(2022, 8, 3) }
+
+      it "returns the same year as the current start year" do
+        expect(current_collection_start_year).to eq(2022)
+      end
+
+      it "returns the correct current start date" do
+        expect(current_collection_start_date).to eq(Time.zone.local(2022, 4, 1))
+      end
+    end
+
+    context "with the date before 1st of April" do
+      let(:now) { Time.utc(2022, 2, 3) }
+
+      it "returns the previous year as the current start year" do
+        expect(current_collection_start_year).to eq(2021)
+      end
+    end
+  end
+end

spec/mailers/bulk_upload_mailer_spec.rb

@@ -5,7 +5,7 @@ RSpec.describe BulkUploadMailer do
 
   let(:notify_client) { instance_double(Notifications::Client) }
   let(:user) { create(:user, email: "user@example.com") }
-  let(:bulk_upload) { build(:bulk_upload, :lettings) }
+  let(:bulk_upload) { build(:bulk_upload, :lettings, user:) }
 
   before do
     allow(Notifications::Client).to receive(:new).and_return(notify_client)
@@ -29,4 +29,50 @@ RSpec.describe BulkUploadMailer do
       mailer.send_bulk_upload_complete_mail(user:, bulk_upload:)
     end
   end
+
+  describe "#send_bulk_upload_failed_service_error_mail" do
+    it "sends correctly formed email" do
+      expect(notify_client).to receive(:send_email).with(
+        email_address: user.email,
+        template_id: described_class::BULK_UPLOAD_FAILED_SERVICE_ERROR_TEMPLATE_ID,
+        personalisation: {
+          filename: bulk_upload.filename,
+          upload_timestamp: bulk_upload.created_at,
+          lettings_or_sales: bulk_upload.log_type,
+          year_combo: bulk_upload.year_combo,
+          bulk_upload_link: start_bulk_upload_lettings_logs_url,
+        },
+      )
+
+      mailer.send_bulk_upload_failed_service_error_mail(bulk_upload:)
+    end
+  end
+
+  context "when bulk upload has log which is not completed" do
+    before do
+      create(:lettings_log, :in_progress, bulk_upload:)
+    end
+
+    describe "#send_bulk_upload_with_errors_mail" do
+      let(:error_description) do
+        "We created logs from your 2022/23 lettings data. There was a problem with 1 of the logs. Click the below link to fix these logs."
+      end
+
+      it "sends correctly formed email" do
+        expect(notify_client).to receive(:send_email).with(
+          email_address: bulk_upload.user.email,
+          template_id: described_class::BULK_UPLOAD_WITH_ERRORS_TEMPLATE_ID,
+          personalisation: {
+            title: "We found 1 log with errors",
+            filename: bulk_upload.filename,
+            upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time),
+            error_description:,
+            summary_report_link: "http://localhost:3000/lettings-logs/bulk-upload-results/#{bulk_upload.id}/resume",
+          },
+        )
+
+        mailer.send_bulk_upload_with_errors_mail(bulk_upload:)
+      end
+    end
+  end
 end

spec/models/form/lettings/questions/managing_organisation_spec.rb

@@ -199,4 +199,36 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do
       end
     end
   end
+
+  describe "#answer_label" do
+    context "when answered" do
+      let(:managing_organisation) { create(:organisation) }
+      let(:log) { create(:lettings_log, managing_organisation:) }
+
+      it "returns org name" do
+        expect(question.answer_label(log)).to eq(managing_organisation.name)
+      end
+    end
+
+    context "when unanswered" do
+      let(:log) { create(:lettings_log, managing_organisation: nil) }
+
+      it "returns nil" do
+        expect(question.answer_label(log)).to be_nil
+      end
+    end
+
+    context "when org does not exist" do
+      let(:managing_organisation) { create(:organisation) }
+      let(:log) { create(:lettings_log, managing_organisation:) }
+
+      before do
+        allow(Organisation).to receive(:find_by).and_return(nil)
+      end
+
+      it "returns nil" do
+        expect(question.answer_label(log)).to be_nil
+      end
+    end
+  end
 end

spec/models/form/sales/pages/handover_date_check_spec.rb

@@ -16,18 +16,28 @@ RSpec.describe Form::Sales::Pages::HandoverDateCheck, type: :model do
   end
 
   it "has the correct id" do
-    expect(page.id).to eq("")
+    expect(page.id).to eq("handover_date_check")
   end
 
   it "has the correct header" do
     expect(page.header).to be_nil
   end
 
+  it "has the correct title_text" do
+    expect(page.title_text).to eq({
+      "translation" => "validations.sale_information.hodate.must_be_less_than_3_years_from_saledate",
+      "arguments" => [],
+    })
+  end
+
+  it "has the correct informative_text" do
+    expect(page.informative_text).to eq({})
+  end
+
   it "has correct depends_on" do
     expect(page.depends_on).to eq([
-      {
-        "hodate_3_years_or_more_saledate?" => true,
-      },
+      { "hodate_3_years_or_more_saledate?" => true, "saledate_check" => nil },
+      { "hodate_3_years_or_more_saledate?" => true, "saledate_check" => 1 },
     ])
   end
 

spec/models/form/sales/pages/purchase_price_outright_ownership_spec.rb

@@ -0,0 +1,35 @@
+require "rails_helper"
+
+RSpec.describe Form::Sales::Pages::PurchasePriceOutrightOwnership, type: :model do
+  subject(:page) { described_class.new(page_id, page_definition, subsection) }
+
+  let(:page_id) { "purchase_price" }
+  let(:page_definition) { nil }
+  let(:subsection) { instance_double(Form::Subsection) }
+
+  it "has correct subsection" do
+    expect(page.subsection).to eq(subsection)
+  end
+
+  it "has correct questions" do
+    expect(page.questions.map(&:id)).to eq(%w[value])
+  end
+
+  it "has the correct id" do
+    expect(page.id).to eq("purchase_price")
+  end
+
+  it "has the correct header" do
+    expect(page.header).to be_nil
+  end
+
+  it "has the correct description" do
+    expect(page.description).to be_nil
+  end
+
+  it "has correct depends_on" do
+    expect(page.depends_on).to eq([
+      { "outright_sale_or_discounted_with_full_ownership?" => true },
+    ])
+  end
+end

spec/models/form/sales/pages/purchase_price_spec.rb

@@ -29,8 +29,7 @@ RSpec.describe Form::Sales::Pages::PurchasePrice, type: :model do
 
   it "has correct depends_on" do
     expect(page.depends_on).to eq([
-      { "ownershipsch" => 3 },
-      { "rent_to_buy_full_ownership?" => true },
+      { "ownershipsch" => 2, "rent_to_buy_full_ownership?" => false },
     ])
   end
 end

spec/models/form/sales/pages/sale_date_check_spec.rb

@@ -0,0 +1,47 @@
+require "rails_helper"
+
+RSpec.describe Form::Sales::Pages::SaleDateCheck, type: :model do
+  subject(:page) { described_class.new(page_id, page_definition, subsection) }
+
+  let(:page_id) { "" }
+  let(:page_definition) { nil }
+  let(:subsection) { instance_double(Form::Subsection) }
+
+  it "has correct subsection" do
+    expect(page.subsection).to eq(subsection)
+  end
+
+  it "has correct questions" do
+    expect(page.questions.map(&:id)).to eq(%w[saledate_check])
+  end
+
+  it "has the correct id" do
+    expect(page.id).to eq("completion_date_check")
+  end
+
+  it "has the correct header" do
+    expect(page.header).to be_nil
+  end
+
+  it "has the correct title_text" do
+    expect(page.title_text).to eq({
+      "translation" => "validations.sale_information.saledate.must_be_less_than_3_years_from_hodate",
+      "arguments" => [],
+    })
+  end
+
+  it "has the correct informative_text" do
+    expect(page.informative_text).to eq({})
+  end
+
+  it "has correct depends_on" do
+    expect(page.depends_on).to eq([
+      { "hodate_3_years_or_more_saledate?" => true, "hodate_check" => nil },
+      { "hodate_3_years_or_more_saledate?" => true, "hodate_check" => 1 },
+    ])
+  end
+
+  it "is interruption screen page" do
+    expect(page.interruption_screen?).to eq(true)
+  end
+end

spec/models/form/sales/questions/handover_date_check_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Form::Sales::Questions::HandoverDateCheck, type: :model do
   end
 
   it "has the correct header" do
-    expect(question.header).to eq("Are you sure practical completion or handover date is more than 3 years before exchange date?")
+    expect(question.header).to eq("Are you sure?")
   end
 
   it "has the correct check_answer_label" do
@@ -43,6 +43,6 @@ RSpec.describe Form::Sales::Questions::HandoverDateCheck, type: :model do
   end
 
   it "has the correct hidden_in_check_answers" do
-    expect(question.hidden_in_check_answers).to eq({ "depends_on" => [{ "hodate_check" => 0 }, { "hodate_check" => 1 }] })
+    expect(question.hidden_in_check_answers).to eq({ "depends_on" => [{ "hodate_check" => 0 }, { "hodate_check" => 1 }, { "saledate_check" => 0 }, { "saledate_check" => 1 }] })
   end
 end

spec/models/form/sales/questions/purchase_price_spec.rb

@@ -32,7 +32,9 @@ RSpec.describe Form::Sales::Questions::PurchasePrice, type: :model do
   end
 
   it "has the correct hint" do
-    expect(question.hint_text).to be_nil
+    expect(question.hint_text).to eq(
+      "For all schemes, including Right to Acquire (RTA), Right to Buy (RTB), Voluntary Right to Buy (VRTB) or Preserved Right to Buy (PRTB) sales, enter the full price of the property without any discount",
+    )
   end
 
   it "has correct width" do

spec/models/form/sales/questions/sale_date_check_spec.rb

@@ -0,0 +1,48 @@
+require "rails_helper"
+
+RSpec.describe Form::Sales::Questions::SaleDateCheck, type: :model do
+  subject(:question) { described_class.new(question_id, question_definition, page) }
+
+  let(:question_id) { nil }
+  let(:question_definition) { nil }
+  let(:page) { instance_double(Form::Page) }
+
+  it "has correct page" do
+    expect(question.page).to eq(page)
+  end
+
+  it "has the correct id" do
+    expect(question.id).to eq("saledate_check")
+  end
+
+  it "has the correct header" do
+    expect(question.header).to eq("Are you sure?")
+  end
+
+  it "has the correct check_answer_label" do
+    expect(question.check_answer_label).to eq("Sale completion date check")
+  end
+
+  it "has the correct type" do
+    expect(question.type).to eq("interruption_screen")
+  end
+
+  it "is not marked as derived" do
+    expect(question.derived?).to be false
+  end
+
+  it "has the correct hint" do
+    expect(question.hint_text).to be_nil
+  end
+
+  it "has the correct answer_options" do
+    expect(question.answer_options).to eq({
+      "0" => { "value" => "Yes" },
+      "1" => { "value" => "No" },
+    })
+  end
+
+  it "has the correct hidden_in_check_answers" do
+    expect(question.hidden_in_check_answers).to eq({ "depends_on" => [{ "hodate_check" => 0 }, { "hodate_check" => 1 }, { "saledate_check" => 0 }, { "saledate_check" => 1 }] })
+  end
+end

spec/models/form/sales/subsections/discounted_ownership_scheme_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Form::Sales::Subsections::DiscountedOwnershipScheme, type: :model
   end
 
   it "has correct pages" do
+    puts discounted_ownership_scheme.pages.map(&:id)
     expect(discounted_ownership_scheme.pages.map(&:id)).to eq(
       %w[
         living_before_purchase_discounted_ownership
@@ -20,6 +21,7 @@ RSpec.describe Form::Sales::Subsections::DiscountedOwnershipScheme, type: :model
         about_price_not_rtb
         grant_value_check
         purchase_price_discounted_ownership
+        purchase_price_outright_ownership
         discounted_ownership_deposit_and_mortgage_value_check_after_value_and_discount
         mortgage_used_discounted_ownership
         discounted_ownership_mortgage_used_mortgage_value_check

spec/models/form/sales/subsections/setup_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do
         organisation
         created_by
         completion_date
+        completion_date_check
         purchaser_code
         ownership_scheme
         shared_ownership_type

spec/models/form_handler_spec.rb

@@ -52,14 +52,14 @@ RSpec.describe FormHandler do
     it "is able to load a current sales form" do
       form = form_handler.get_form("current_sales")
       expect(form).to be_a(Form)
-      expect(form.pages.count).to eq(212)
+      expect(form.pages.count).to eq(214)
       expect(form.name).to eq("2022_2023_sales")
     end
 
     it "is able to load a previous sales form" do
       form = form_handler.get_form("previous_sales")
       expect(form).to be_a(Form)
-      expect(form.pages.count).to eq(212)
+      expect(form.pages.count).to eq(214)
       expect(form.name).to eq("2021_2022_sales")
     end
   end

spec/models/form_spec.rb

@@ -218,9 +218,9 @@ RSpec.describe Form, type: :model do
       expect(form.sections[0].class).to eq(Form::Sales::Sections::Setup)
       expect(form.subsections.count).to eq(1)
       expect(form.subsections.first.id).to eq("setup")
-      expect(form.pages.count).to eq(14)
+      expect(form.pages.count).to eq(15)
       expect(form.pages.first.id).to eq("organisation")
-      expect(form.questions.count).to eq(15)
+      expect(form.questions.count).to eq(16)
       expect(form.questions.first.id).to eq("owning_organisation_id")
       expect(form.start_date).to eq(Time.zone.parse("2022-04-01"))
       expect(form.end_date).to eq(Time.zone.parse("2023-07-01"))

spec/models/sales_log_spec.rb

@@ -47,7 +47,7 @@ RSpec.describe SalesLog, type: :model do
     let(:sales_log) { build(:sales_log) }
 
     it "returns optional fields" do
-      expect(sales_log.optional_fields).to eq(%w[purchid monthly_charges_value_check old_persons_shared_ownership_value_check])
+      expect(sales_log.optional_fields).to eq(%w[saledate_check purchid monthly_charges_value_check old_persons_shared_ownership_value_check])
     end
   end
 

spec/models/validations/household_validations_spec.rb

@@ -463,14 +463,14 @@ RSpec.describe Validations::HouseholdValidations do
         record.hhmemb = -1
         household_validator.validate_numeric_min_max(record)
         expect(record.errors["hhmemb"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Number of Household Members", min: 0, max: 8))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Number of Household Members", min: 0, max: 8))
       end
 
       it "validates that the number of household members cannot be more than 8" do
         record.hhmemb = 9
         household_validator.validate_numeric_min_max(record)
         expect(record.errors["hhmemb"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Number of Household Members", min: 0, max: 8))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Number of Household Members", min: 0, max: 8))
       end
 
       it "expects that the number of other household members is between the min and max" do

spec/models/validations/sales/household_validations_spec.rb

@@ -5,48 +5,6 @@ RSpec.describe Validations::Sales::HouseholdValidations do
 
   let(:validator_class) { Class.new { include Validations::Sales::HouseholdValidations } }
 
-  describe "#validate_number_of_other_people_living_in_the_property" do
-    context "when within permitted bounds" do
-      let(:record) { build(:sales_log, hholdcount: 2) }
-
-      it "does not add an error" do
-        household_validator.validate_number_of_other_people_living_in_the_property(record)
-
-        expect(record.errors[:hholdcount]).not_to be_present
-      end
-    end
-
-    context "when blank" do
-      let(:record) { build(:sales_log, hholdcount: nil) }
-
-      it "does not add an error" do
-        household_validator.validate_number_of_other_people_living_in_the_property(record)
-
-        expect(record.errors[:hholdcount]).not_to be_present
-      end
-    end
-
-    context "when below lower bound" do
-      let(:record) { build(:sales_log, hholdcount: -1) }
-
-      it "adds an error" do
-        household_validator.validate_number_of_other_people_living_in_the_property(record)
-
-        expect(record.errors[:hholdcount]).to be_present
-      end
-    end
-
-    context "when higher than upper bound" do
-      let(:record) { build(:sales_log, hholdcount: 5) }
-
-      it "adds an error" do
-        household_validator.validate_number_of_other_people_living_in_the_property(record)
-
-        expect(record.errors[:hholdcount]).to be_present
-      end
-    end
-  end
-
   describe "household member validations" do
     let(:record) { build(:sales_log) }
 

spec/models/validations/sales/sale_information_validations_spec.rb

@@ -62,7 +62,7 @@ RSpec.describe Validations::Sales::SaleInformationValidations do
       it "does not add an error" do
         sale_information_validator.validate_practical_completion_date_before_saledate(record)
 
-        expect(record.errors[:hodate]).to be_present
+        expect(record.errors[:hodate]).not_to be_present
       end
     end
   end
@@ -130,10 +130,10 @@ RSpec.describe Validations::Sales::SaleInformationValidations do
         sale_information_validator.validate_exchange_date(record)
 
         expect(record.errors[:exdate]).to eq(
-          ["Contract exchange date must be less than 1 year before completion date"],
+          ["Contract exchange date must be before completion date"],
         )
         expect(record.errors[:saledate]).to eq(
-          ["Completion date must be less than 1 year after contract exchange date"],
+          ["Completion date must be after contract exchange date"],
         )
       end
     end

spec/models/validations/shared_validations_spec.rb

@@ -18,42 +18,42 @@ RSpec.describe Validations::SharedValidations do
         record.age1 = "random"
         shared_validator.validate_numeric_min_max(record)
         expect(record.errors["age1"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120))
       end
 
       it "validates that other household member ages are a number" do
         record.age2 = "random"
         shared_validator.validate_numeric_min_max(record)
         expect(record.errors["age2"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Person 2’s age", min: 1, max: 120))
       end
 
       it "validates that person 1's age is greater than 16" do
         record.age1 = 15
         shared_validator.validate_numeric_min_max(record)
         expect(record.errors["age1"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120))
       end
 
       it "validates that other household member ages are greater than 1" do
         record.age2 = 0
         shared_validator.validate_numeric_min_max(record)
         expect(record.errors["age2"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Person 2’s age", min: 1, max: 120))
       end
 
       it "validates that person 1's age is less than 121" do
         record.age1 = 121
         shared_validator.validate_numeric_min_max(record)
         expect(record.errors["age1"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120))
       end
 
       it "validates that other household member ages are greater than 121" do
         record.age2 = 123
         shared_validator.validate_numeric_min_max(record)
         expect(record.errors["age2"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Person 2’s age", min: 1, max: 120))
       end
 
       it "validates that person 1's age is between 16 and 120" do
@@ -69,21 +69,27 @@ RSpec.describe Validations::SharedValidations do
       end
     end
 
+    it "adds the correct validation text when a question has a min but not a max" do
+      sales_record.savings = -10
+      shared_validator.validate_numeric_min_max(sales_record)
+      expect(sales_record.errors["savings"]).to include(match I18n.t("validations.numeric.above_min", field: "Buyer’s total savings (to nearest £10) before any deposit paid", min: "£0"))
+    end
+
     context "when validating percent" do
       it "validates that suffixes are added in the error message" do
         sales_record.stairbought = 150
         shared_validator.validate_numeric_min_max(sales_record)
         expect(sales_record.errors["stairbought"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Percentage bought in this staircasing transaction", min: "0%", max: "100%"))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Percentage bought in this staircasing transaction", min: "0%", max: "100%"))
       end
     end
 
     context "when validating price" do
       it "validates that £ prefix  and , is added in the error message" do
-        sales_record.income1 = "random"
+        sales_record.income1 = -5
         shared_validator.validate_numeric_min_max(sales_record)
         expect(sales_record.errors["income1"])
-          .to include(match I18n.t("validations.numeric.valid", field: "Buyer 1’s gross annual income", min: "£0", max: "£999,999"))
+          .to include(match I18n.t("validations.numeric.within_range", field: "Buyer 1’s gross annual income", min: "£0", max: "£999,999"))
       end
     end
   end

spec/requests/bulk_upload_controller_spec.rb

@@ -13,6 +13,14 @@ RSpec.describe BulkUploadController, type: :request do
   end
 
   context "when a user is not signed in" do
+    describe "GET #start" do
+      before { get start_bulk_upload_lettings_logs_path, headers:, params: {} }
+
+      it "does not let you see the bulk upload page" do
+        expect(response).to redirect_to("/account/sign-in")
+      end
+    end
+
     describe "GET #show" do
       before { get url, headers:, params: {} }
 
@@ -50,6 +58,40 @@ RSpec.describe BulkUploadController, type: :request do
       end
     end
 
+    describe "GET #start" do
+      before do
+        Timecop.freeze(time)
+        get start_bulk_upload_lettings_logs_path
+      end
+
+      after do
+        Timecop.unfreeze
+      end
+
+      context "when not crossover period" do
+        let(:time) { Time.utc(2022, 2, 8) }
+
+        it "redirects to bulk upload path" do
+          expect(request).to redirect_to(
+            bulk_upload_lettings_log_path(
+              id: "prepare-your-file",
+              form: { year: 2021 },
+            ),
+          )
+        end
+      end
+
+      context "when crossover period" do
+        let(:time) { Time.utc(2022, 6, 8) }
+
+        it "redirects to bulk upload path" do
+          expect(request).to redirect_to(
+            bulk_upload_lettings_log_path(id: "year"),
+          )
+        end
+      end
+    end
+
     describe "POST #bulk upload" do
       context "with a valid file based on the upload template" do
         let(:request) { post url, params: { bulk_upload: { lettings_log_bulk_upload: valid_file } } }

spec/requests/lettings_logs_controller_spec.rb

@@ -82,7 +82,7 @@ RSpec.describe LettingsLogsController, type: :request do
         it "validates lettings log parameters" do
           json_response = JSON.parse(response.body)
           expect(response).to have_http_status(:unprocessable_entity)
-          expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)]]])
+          expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)]]])
         end
       end
 

spec/services/bulk_upload/lettings/validator_spec.rb

@@ -42,15 +42,15 @@ RSpec.describe BulkUpload::Lettings::Validator do
       it "create validation error with correct values" do
         validator.call
 
-        error = BulkUploadError.first
+        error = BulkUploadError.order(:row, :field).first
 
-        expect(error.field).to eql("field_96")
-        expect(error.error).to eql("blank")
+        expect(error.field).to eql("field_11")
+        expect(error.error).to eql("You must only answer the length of the tenancy if it's fixed-term")
         expect(error.tenant_code).to eql("123")
         expect(error.property_ref).to be_nil
         expect(error.row).to eql("7")
-        expect(error.cell).to eql("CS7")
-        expect(error.col).to eql("CS")
+        expect(error.cell).to eql("L7")
+        expect(error.col).to eql("L")
       end
     end
 

spec/services/bulk_upload/processor_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe BulkUpload::Processor do
   let(:bulk_upload) { create(:bulk_upload, :lettings) }
 
   describe "#call" do
-    context "when processing a bulk upload with errors" do
+    context "when the bulk upload itself is not considered valid" do
       let(:mock_downloader) do
         instance_double(
           BulkUpload::Downloader,
@@ -16,12 +16,153 @@ RSpec.describe BulkUpload::Processor do
         )
       end
 
+      let(:mock_validator) do
+        instance_double(
+          BulkUpload::Lettings::Validator,
+          invalid?: true,
+          call: nil,
+        )
+      end
+
+      before do
+        allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader)
+        allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator)
+      end
+
+      it "sends failure email" do
+        mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil)
+
+        allow(BulkUploadMailer).to receive(:send_bulk_upload_failed_service_error_mail).and_return(mail_double)
+
+        processor.call
+
+        expect(BulkUploadMailer).to have_received(:send_bulk_upload_failed_service_error_mail)
+        expect(mail_double).to have_received(:deliver_later)
+      end
+
+      it "does not attempt to validate the contents of the file" do
+        processor.call
+
+        expect(mock_validator).not_to have_received(:call)
+      end
+    end
+
+    context "when the bulk upload processing throws an error" do
+      let(:mock_downloader) do
+        instance_double(
+          BulkUpload::Downloader,
+          call: nil,
+          path: file_fixture("2022_23_lettings_bulk_upload.csv"),
+          delete_local_file!: nil,
+        )
+      end
+
+      let(:mock_validator) do
+        instance_double(
+          BulkUpload::Lettings::Validator,
+          invalid?: false,
+        )
+      end
+
+      before do
+        allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader)
+        allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator)
+
+        allow(mock_validator).to receive(:call).and_raise(StandardError)
+      end
+
+      it "sends failure email" do
+        mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil)
+
+        allow(BulkUploadMailer).to receive(:send_bulk_upload_failed_service_error_mail).and_return(mail_double)
+
+        processor.call
+
+        expect(BulkUploadMailer).to have_received(:send_bulk_upload_failed_service_error_mail)
+        expect(mail_double).to have_received(:deliver_later)
+      end
+
+      it "we log the failure" do
+        allow(Sentry).to receive(:capture_exception)
+
+        processor.call
+
+        expect(Sentry).to have_received(:capture_exception)
+      end
+    end
+
+    context "when processing a bulk upload with errors but below threshold (therefore creates logs)" do
+      let(:mock_downloader) do
+        instance_double(
+          BulkUpload::Downloader,
+          call: nil,
+          path: file_fixture("2022_23_lettings_bulk_upload.csv"),
+          delete_local_file!: nil,
+        )
+      end
+
+      let(:mock_validator) do
+        instance_double(
+          BulkUpload::Lettings::Validator,
+          invalid?: false,
+          call: nil,
+          create_logs?: true,
+        )
+      end
+
       before do
         allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader)
+        allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator)
       end
 
-      it "persist the validation errors" do
-        expect { processor.call }.to change(BulkUploadError, :count)
+      it "deletes the local file afterwards" do
+        processor.call
+
+        expect(mock_downloader).to have_received(:delete_local_file!)
+      end
+
+      it "sends fix errors email" do
+        mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil)
+
+        allow(BulkUploadMailer).to receive(:send_bulk_upload_with_errors_mail).and_return(mail_double)
+
+        processor.call
+
+        expect(BulkUploadMailer).to have_received(:send_bulk_upload_with_errors_mail)
+        expect(mail_double).to have_received(:deliver_later)
+      end
+
+      it "does not send success email" do
+        allow(BulkUploadMailer).to receive(:send_bulk_upload_complete_mail).and_call_original
+
+        processor.call
+
+        expect(BulkUploadMailer).not_to have_received(:send_bulk_upload_complete_mail)
+      end
+    end
+
+    context "when processing a bulk upload with errors but above threshold (therefore does not create logs)" do
+      let(:mock_downloader) do
+        instance_double(
+          BulkUpload::Downloader,
+          call: nil,
+          path: file_fixture("2022_23_lettings_bulk_upload.csv"),
+          delete_local_file!: nil,
+        )
+      end
+
+      let(:mock_validator) do
+        instance_double(
+          BulkUpload::Lettings::Validator,
+          invalid?: false,
+          call: nil,
+          create_logs?: false,
+        )
+      end
+
+      before do
+        allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader)
+        allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator)
       end
 
       it "deletes the local file afterwards" do
@@ -30,6 +171,14 @@ RSpec.describe BulkUpload::Processor do
         expect(mock_downloader).to have_received(:delete_local_file!)
       end
 
+      it "does not send fix errors email" do
+        allow(BulkUploadMailer).to receive(:send_bulk_upload_with_errors_mail).and_call_original
+
+        processor.call
+
+        expect(BulkUploadMailer).not_to have_received(:send_bulk_upload_with_errors_mail)
+      end
+
       it "does not send success email" do
         allow(BulkUploadMailer).to receive(:send_bulk_upload_complete_mail).and_call_original
 
@@ -56,6 +205,7 @@ RSpec.describe BulkUpload::Processor do
           BulkUpload::Lettings::Validator,
           call: nil,
           create_logs?: true,
+          invalid?: false,
         )
       end
 
@@ -79,6 +229,14 @@ RSpec.describe BulkUpload::Processor do
         expect(mock_creator).to have_received(:call)
       end
 
+      it "does not send fix errors email" do
+        allow(BulkUploadMailer).to receive(:send_bulk_upload_with_errors_mail).and_call_original
+
+        processor.call
+
+        expect(BulkUploadMailer).not_to have_received(:send_bulk_upload_with_errors_mail)
+      end
+
       it "sends success email" do
         mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil)