From 6973ade8c42cd6ab98c53f1fcb492acc57b9eec3 Mon Sep 17 00:00:00 2001
From: natdeanlewissoftwire
<94526761+natdeanlewissoftwire@users.noreply.github.com>
Date: Fri, 12 May 2023 11:03:41 +0100
Subject: [PATCH] CLDC-2262 Bulk upload clear invalid data to create forms
(#1618)
* feat: wip blank fields and dependent fields on upload tos ee if valid and can upload with missing info - this is not the exact ac on the ticket yet
* feat: update seed
* feat: wip commit
* feat: add postcodenk error so can clear on validation
* feat: add postcode validation back
* feat: move la vals to shared and move and add tests
* feat: add correct pluralisation to warning message
* feat: add blank compound invalid fields methods
* feat: update validations
* feat: update pluralisation
* refactor: lint
* feat: clear errors associated with blanked values so log status is set correctly on creation
* feat: validate instead
* feat: avoid duplicated errors
* feat: dont auto-refuse income, different to imports
* feat: validate after every blank method
* feat: delete la validator spec
* tests: update
* refactor: erblinting
* refactor: cleanup
* refactor: move pluralizer to helper
* feat: copy update
* feat: rename
* feat: refactor to avoid redundant re-validations and test
* refactor: rubocop
* test: update
* test: update
* test: update
* update sidekiq
* feat: clear errors
* feat: run clearing twice in case first clear creates different errors
* feat: remove moved file
* feat: undo validation file tweaks as shared/specific overlap could do with a more general refactor
* feat: update tests
---
app/helpers/logs_helper.rb | 6 ++++
app/models/lettings_log.rb | 10 ++++++
app/models/log.rb | 28 +++++++++++++--
app/models/sales_log.rb | 7 +++-
.../validations/financial_validations.rb | 3 +-
.../bulk_upload/lettings/validator.rb | 5 +++
app/services/bulk_upload/processor.rb | 5 ++-
app/services/bulk_upload/sales/validator.rb | 12 ++++++-
.../confirm.html.erb | 2 +-
config/locales/en.yml | 3 +-
.../validations/financial_validations_spec.rb | 2 +-
.../bulk_upload/lettings/log_creator_spec.rb | 30 ++++++++++++++++
.../bulk_upload/sales/log_creator_spec.rb | 35 +++++++++++++++++++
13 files changed, 136 insertions(+), 12 deletions(-)
diff --git a/app/helpers/logs_helper.rb b/app/helpers/logs_helper.rb
index f525f4600..ca6fa838f 100644
--- a/app/helpers/logs_helper.rb
+++ b/app/helpers/logs_helper.rb
@@ -47,4 +47,10 @@ module LogsHelper
when :sales then sales_logs_csv_download_organisation_path(organisation, search:, codes_only:)
end
end
+
+ def pluralize_logs_and_errors_warning(log_count, error_count)
+ is_or_are = log_count == 1 ? "is" : "are"
+ need_or_needs = error_count == 1 ? "needs" : "need"
+ "There #{is_or_are} #{pluralize(log_count, 'log')} in this bulk upload with #{pluralize(error_count, 'error')} that still #{need_or_needs} to be fixed after upload."
+ end
end
diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb
index 922df5977..c1a3e4a16 100644
--- a/app/models/lettings_log.rb
+++ b/app/models/lettings_log.rb
@@ -529,6 +529,16 @@ class LettingsLog < Log
is_carehome == 1
end
+ def blank_compound_invalid_non_setup_fields!
+ super
+
+ self.postcode_known = nil if errors.attribute_names.include? :postcode_full
+
+ if errors.of_kind?(:earnings, :under_hard_min)
+ self.incfreq = nil
+ end
+ end
+
private
def reset_invalid_unresolved_log_fields!
diff --git a/app/models/log.rb b/app/models/log.rb
index 807b45c8b..2c6c0254a 100644
--- a/app/models/log.rb
+++ b/app/models/log.rb
@@ -98,10 +98,18 @@ class Log < ApplicationRecord
def blank_invalid_non_setup_fields!
setup_ids = form.setup_sections.flat_map(&:subsections).flat_map(&:questions).map(&:id)
- errors.each do |error|
- next if setup_ids.include?(error.attribute.to_s)
+ 2.times do
+ next if valid?
- public_send("#{error.attribute}=", nil)
+ errors.each do |error|
+ next if setup_ids.include?(error.attribute.to_s)
+
+ public_send("#{error.attribute}=", nil)
+ end
+
+ blank_compound_invalid_non_setup_fields!
+
+ errors.clear
end
end
@@ -136,6 +144,20 @@ class Log < ApplicationRecord
format_as_currency(field_value)
end
+ def blank_compound_invalid_non_setup_fields!
+ self.ppcodenk = nil if errors.attribute_names.include? :ppostcode_full
+
+ if errors.of_kind?(:uprn, :uprn_error)
+ self.uprn_known = nil
+ self.uprn_confirmed = nil
+ self.address_line1 = nil
+ self.address_line2 = nil
+ self.town_or_city = nil
+ self.postcode_full = nil
+ self.county = nil
+ end
+ end
+
def creation_method
bulk_upload_id ? "bulk upload" : "single log"
end
diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb
index 41b7bd5b1..8002ac815 100644
--- a/app/models/sales_log.rb
+++ b/app/models/sales_log.rb
@@ -2,7 +2,6 @@ class SalesLogValidator < ActiveModel::Validator
include Validations::Sales::SetupValidations
include Validations::Sales::HouseholdValidations
include Validations::Sales::PropertyValidations
- include Validations::SharedValidations
include Validations::Sales::FinancialValidations
include Validations::Sales::SaleInformationValidations
include Validations::SharedValidations
@@ -386,4 +385,10 @@ class SalesLog < Log
when 3 then "outright sale"
end
end
+
+ def blank_compound_invalid_non_setup_fields!
+ super
+
+ self.pcodenk = nil if errors.attribute_names.include? :postcode_full
+ end
end
diff --git a/app/models/validations/financial_validations.rb b/app/models/validations/financial_validations.rb
index 0b8950ce2..7ec730b97 100644
--- a/app/models/validations/financial_validations.rb
+++ b/app/models/validations/financial_validations.rb
@@ -5,7 +5,8 @@ module Validations::FinancialValidations
# or 'validate_' to run on submit as well
def validate_outstanding_rent_amount(record)
if !record.has_housing_benefit_rent_shortfall? && record.tshortfall.present?
- record.errors.add :tshortfall, :no_outstanding_charges, message: I18n.t("validations.financial.tshortfall.outstanding_amount_not_required")
+ record.errors.add :tshortfall, :no_outstanding_charges, message: I18n.t("validations.financial.tshortfall.outstanding_amount_not_expected")
+ record.errors.add :hbrentshortfall, :no_outstanding_charges, message: I18n.t("validations.financial.hbrentshortfall.outstanding_amount_not_expected")
end
end
diff --git a/app/services/bulk_upload/lettings/validator.rb b/app/services/bulk_upload/lettings/validator.rb
index ae272a0fe..d56919bb4 100644
--- a/app/services/bulk_upload/lettings/validator.rb
+++ b/app/services/bulk_upload/lettings/validator.rb
@@ -44,6 +44,11 @@ class BulkUpload::Lettings::Validator
return false if any_setup_errors?
return false if row_parsers.any?(&:block_log_creation?)
return false if any_logs_already_exist? && FeatureToggle.bulk_upload_duplicate_log_check_enabled?
+
+ row_parsers.each do |row_parser|
+ row_parser.log.blank_invalid_non_setup_fields!
+ end
+
return false if any_logs_invalid?
true
diff --git a/app/services/bulk_upload/processor.rb b/app/services/bulk_upload/processor.rb
index 49d44261f..66c6b9d10 100644
--- a/app/services/bulk_upload/processor.rb
+++ b/app/services/bulk_upload/processor.rb
@@ -14,14 +14,13 @@ class BulkUpload::Processor
if validator.any_setup_errors?
send_setup_errors_mail
+
elsif validator.create_logs?
create_logs
if created_logs_but_incompleted?
send_how_fix_upload_mail
- end
-
- if created_logs_and_all_completed?
+ elsif created_logs_and_all_completed?
bulk_upload.unpend
send_success_mail
end
diff --git a/app/services/bulk_upload/sales/validator.rb b/app/services/bulk_upload/sales/validator.rb
index ee87530ca..ede32266b 100644
--- a/app/services/bulk_upload/sales/validator.rb
+++ b/app/services/bulk_upload/sales/validator.rb
@@ -38,7 +38,13 @@ class BulkUpload::Sales::Validator
return false if any_setup_errors?
return false if row_parsers.any?(&:block_log_creation?)
- row_parsers.all? { |row_parser| row_parser.log.valid? }
+ row_parsers.each do |row_parser|
+ row_parser.log.blank_invalid_non_setup_fields!
+ end
+
+ return false if any_logs_invalid?
+
+ true
end
def any_setup_errors?
@@ -51,6 +57,10 @@ class BulkUpload::Sales::Validator
private
+ def any_logs_invalid?
+ row_parsers.any? { |row_parser| row_parser.log.invalid? }
+ end
+
def csv_parser
@csv_parser ||= case bulk_upload.year
when 2022
diff --git a/app/views/bulk_upload_lettings_resume/confirm.html.erb b/app/views/bulk_upload_lettings_resume/confirm.html.erb
index af73b33e9..a61a73651 100644
--- a/app/views/bulk_upload_lettings_resume/confirm.html.erb
+++ b/app/views/bulk_upload_lettings_resume/confirm.html.erb
@@ -7,7 +7,7 @@
Bulk upload for lettings (<%= @bulk_upload.year_combo %>)
Are you sure you want to upload all logs from this bulk upload?
- There are <%= pluralize(@bulk_upload.logs.count, "log") %> in this bulk upload with <%= pluralize(@bulk_upload.bulk_upload_errors.count, "error") %> that still need to be fixed after upload.
+ <%= pluralize_logs_and_errors_warning(@bulk_upload.logs.count, @bulk_upload.bulk_upload_errors.count) %>
<%= govuk_warning_text(icon_fallback_text: "Danger") do %>
You can not delete logs once you create them
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 1ac1fb6c3..8e7eb6e14 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -256,10 +256,11 @@ en:
financial:
tshortfall:
- outstanding_amount_not_required: "You cannot answer the outstanding amount question if you don’t have outstanding rent or charges"
+ outstanding_amount_not_expected: "You cannot answer the outstanding amount question if you don’t have outstanding rent or charges"
more_than_rent: "Enter a value less less than the basic rent amount"
must_be_positive: "Enter a value over £0.01 as you told us there is an outstanding amount"
hbrentshortfall:
+ outstanding_amount_not_expected: "Answer must be ‘yes’ as you have answered the outstanding amount question"
outstanding_no_benefits: "Answer cannot be ‘yes’ to outstanding amount for basic rent or charges if tenant does not receive housing benefit or Universal Credit or you‘re not sure"
benefits:
part_or_full_time: "Answer cannot be ‘all’ for income from Universal Credit, state pensions or benefits if the tenant or their partner works part-time or full-time"
diff --git a/spec/models/validations/financial_validations_spec.rb b/spec/models/validations/financial_validations_spec.rb
index 4bac948d0..02a2f752f 100644
--- a/spec/models/validations/financial_validations_spec.rb
+++ b/spec/models/validations/financial_validations_spec.rb
@@ -81,7 +81,7 @@ RSpec.describe Validations::FinancialValidations do
record.tshortfall = 99
financial_validator.validate_outstanding_rent_amount(record)
expect(record.errors["tshortfall"])
- .to include(match I18n.t("validations.financial.tshortfall.outstanding_amount_not_required"))
+ .to include(match I18n.t("validations.financial.tshortfall.outstanding_amount_not_expected"))
end
end
diff --git a/spec/services/bulk_upload/lettings/log_creator_spec.rb b/spec/services/bulk_upload/lettings/log_creator_spec.rb
index adf4d3845..9c28cc81e 100644
--- a/spec/services/bulk_upload/lettings/log_creator_spec.rb
+++ b/spec/services/bulk_upload/lettings/log_creator_spec.rb
@@ -82,6 +82,36 @@ RSpec.describe BulkUpload::Lettings::LogCreator do
end
end
+ context "when a valid csv with row with compound errors on non setup field" do
+ let(:file) { Tempfile.new }
+ let(:path) { file.path }
+ let(:log) do
+ build(
+ :lettings_log,
+ :completed,
+ earnings: 0,
+ incfreq: 1,
+ )
+ end
+
+ before do
+ file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2023_csv_row)
+ file.rewind
+ end
+
+ it "creates the log" do
+ expect { service.call }.to change(LettingsLog, :count).by(1)
+ end
+
+ it "blanks invalid fields" do
+ service.call
+
+ record = LettingsLog.last
+ expect(record.earnings).to be_blank
+ expect(record.incfreq).to be_blank
+ end
+ end
+
context "when pre-creating logs" do
subject(:service) { described_class.new(bulk_upload:, path:) }
diff --git a/spec/services/bulk_upload/sales/log_creator_spec.rb b/spec/services/bulk_upload/sales/log_creator_spec.rb
index 440fcca16..2abda527d 100644
--- a/spec/services/bulk_upload/sales/log_creator_spec.rb
+++ b/spec/services/bulk_upload/sales/log_creator_spec.rb
@@ -75,6 +75,41 @@ RSpec.describe BulkUpload::Sales::LogCreator do
end
end
+ context "when a valid csv with row with compound errors on non setup field" do
+ let(:file) { Tempfile.new }
+ let(:path) { file.path }
+ let(:log) do
+ build(
+ :sales_log,
+ :completed,
+ ownershipsch: 2,
+ pcodenk: 0,
+ ppcodenk: 0,
+ postcode_full: "AA11AA",
+ ppostcode_full: "BB22BB",
+ )
+ end
+
+ before do
+ file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row)
+ file.rewind
+ end
+
+ it "creates the log" do
+ expect { service.call }.to change(SalesLog, :count).by(1)
+ end
+
+ it "blanks invalid field" do
+ service.call
+
+ record = SalesLog.last
+ expect(record.pcodenk).to be_blank
+ expect(record.postcode_full).to be_blank
+ expect(record.ppcodenk).to be_blank
+ expect(record.ppostcode_full).to be_blank
+ end
+ end
+
context "when pre-creating logs" do
subject(:service) { described_class.new(bulk_upload:, path:) }