diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index 26b988bb2..d7cd5f948 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -1,5 +1,6 @@ class BulkUpload < ApplicationRecord enum log_type: { lettings: "lettings", sales: "sales" } + enum noint_fix_status: { not_applied: "not_applied", applied: "applied", not_needed: "not_needed" } belongs_to :user diff --git a/app/services/bulk_upload/sales/log_creator.rb b/app/services/bulk_upload/sales/log_creator.rb index 2d0888e4d..9f3a00fc9 100644 --- a/app/services/bulk_upload/sales/log_creator.rb +++ b/app/services/bulk_upload/sales/log_creator.rb @@ -7,6 +7,7 @@ class BulkUpload::Sales::LogCreator end def call + @bulk_upload.update!(noint_fix_status: BulkUpload.noint_fix_statuses[:not_needed]) row_parsers.each do |row_parser| row_parser.valid? diff --git a/app/services/bulk_upload/sales/year2023/row_parser.rb b/app/services/bulk_upload/sales/year2023/row_parser.rb index 0759d7845..a8f322828 100644 --- a/app/services/bulk_upload/sales/year2023/row_parser.rb +++ b/app/services/bulk_upload/sales/year2023/row_parser.rb @@ -791,7 +791,7 @@ private attributes["purchid"] = purchaser_code attributes["saledate"] = saledate - attributes["noint"] = 2 if field_28 == 1 + attributes["noint"] = field_28 attributes["age1_known"] = age1_known? attributes["age1"] = field_30 if attributes["age1_known"]&.zero? && field_30&.match(/\A\d{1,3}\z|\AR\z/) diff --git a/app/services/bulk_upload/sales/year2024/row_parser.rb b/app/services/bulk_upload/sales/year2024/row_parser.rb index 4e0f689a4..2c1b6089a 100644 --- a/app/services/bulk_upload/sales/year2024/row_parser.rb +++ b/app/services/bulk_upload/sales/year2024/row_parser.rb @@ -786,7 +786,7 @@ private attributes["purchid"] = purchaser_code attributes["saledate"] = saledate - attributes["noint"] = 2 if field_17 == 1 + attributes["noint"] = field_17 attributes["age1_known"] = age1_known? attributes["age1"] = field_31 if attributes["age1_known"]&.zero? && field_31&.match(/\A\d{1,3}\z|\AR\z/) diff --git a/config/environments/development.rb b/config/environments/development.rb index 6261c29c8..7e1890b02 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -82,7 +82,7 @@ Rails.application.configure do # config.action_cable.disable_request_forgery_protection = true # see https://discuss.rubyonrails.org/t/cve-2022-32224-possible-rce-escalation-bug-with-serialized-columns-in-active-record/81017 - config.active_record.yaml_column_permitted_classes = [Time] + config.active_record.yaml_column_permitted_classes = [Time, BigDecimal] config.active_job.queue_adapter = :inline end diff --git a/config/environments/production.rb b/config/environments/production.rb index 8e2a817ef..55f442b3d 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -130,7 +130,7 @@ Rails.application.configure do # config.active_record.shard_resolver = ->(request) { Tenant.find_by!(host: request.host).shard } # see https://discuss.rubyonrails.org/t/cve-2022-32224-possible-rce-escalation-bug-with-serialized-columns-in-active-record/81017 - config.active_record.yaml_column_permitted_classes = [Time] + config.active_record.yaml_column_permitted_classes = [Time, BigDecimal] # From https://github.com/paper-trail-gem/paper_trail/wiki/Setting-whodunnit-in-the-rails-console console do diff --git a/config/environments/review.rb b/config/environments/review.rb index ec4c67726..f7438fdb6 100644 --- a/config/environments/review.rb +++ b/config/environments/review.rb @@ -126,5 +126,5 @@ Rails.application.configure do # config.active_record.database_resolver_context = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session # see https://discuss.rubyonrails.org/t/cve-2022-32224-possible-rce-escalation-bug-with-serialized-columns-in-active-record/81017 - config.active_record.yaml_column_permitted_classes = [Time] + config.active_record.yaml_column_permitted_classes = [Time, BigDecimal] end diff --git a/config/environments/staging.rb b/config/environments/staging.rb index 27b4a5c50..56352d3bb 100644 --- a/config/environments/staging.rb +++ b/config/environments/staging.rb @@ -126,5 +126,5 @@ Rails.application.configure do # config.active_record.database_resolver_context = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session # see https://discuss.rubyonrails.org/t/cve-2022-32224-possible-rce-escalation-bug-with-serialized-columns-in-active-record/81017 - config.active_record.yaml_column_permitted_classes = [Time] + config.active_record.yaml_column_permitted_classes = [Time, BigDecimal] end diff --git a/config/environments/test.rb b/config/environments/test.rb index 1c399b19d..aa3fe7bba 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -64,7 +64,7 @@ Rails.application.configure do Faker::Config.locale = "en-GB" # see https://discuss.rubyonrails.org/t/cve-2022-32224-possible-rce-escalation-bug-with-serialized-columns-in-active-record/81017 - config.active_record.yaml_column_permitted_classes = [Time] + config.active_record.yaml_column_permitted_classes = [Time, BigDecimal] config.active_job.queue_adapter = :test end diff --git a/db/migrate/20240216163519_add_no_int_fix_marker_to_bulk_uploads.rb b/db/migrate/20240216163519_add_no_int_fix_marker_to_bulk_uploads.rb new file mode 100644 index 000000000..745cc1fc9 --- /dev/null +++ b/db/migrate/20240216163519_add_no_int_fix_marker_to_bulk_uploads.rb @@ -0,0 +1,5 @@ +class AddNoIntFixMarkerToBulkUploads < ActiveRecord::Migration[7.0] + def change + add_column :bulk_uploads, :noint_fix_status, :string, default: "not_applied" + end +end diff --git a/db/schema.rb b/db/schema.rb index 6959c4bf3..b5b9c9fc0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_02_09_153215) do +ActiveRecord::Schema[7.0].define(version: 2024_02_16_163519) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -41,6 +41,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_02_09_153215) do t.integer "needstype" t.text "choice" t.integer "total_logs_count" + t.string "noint_fix_status", default: "not_applied" t.index ["identifier"], name: "index_bulk_uploads_on_identifier", unique: true t.index ["user_id"], name: "index_bulk_uploads_on_user_id" end diff --git a/lib/tasks/correct_noint_value.rake b/lib/tasks/correct_noint_value.rake new file mode 100644 index 000000000..354daa8c1 --- /dev/null +++ b/lib/tasks/correct_noint_value.rake @@ -0,0 +1,24 @@ +desc "Alter noint values for bulk uploaded sales logs where these have not been set in the service" +task correct_noint_value: :environment do + update_counts = { + in_progress: 0, + completed: 0, + pending: 0, + deleted: 0, + } + affected_uploads = BulkUpload.where(log_type: "sales", noint_fix_status: BulkUpload.noint_fix_statuses[:not_applied]) + affected_uploads.each do |upload| + upload.logs.where(noint: 2).each do |log| + noint_at_upload = log.versions.length == 1 ? log.noint : log.versions.first.next.reify.noint + next unless noint_at_upload == 2 + + Rails.logger.info("Updating noint value on log #{log.id}, owning org #{log.owning_organisation_id}") + update_counts[log.status.to_sym] += 1 + log.noint = 1 + log.skip_update_status = true + log.save! + end + upload.update!(noint_fix_status: BulkUpload.noint_fix_statuses[:applied]) + end + Rails.logger.info("Logs updated; #{update_counts}") +end diff --git a/spec/factories/bulk_upload.rb b/spec/factories/bulk_upload.rb index 90a56a26b..6ac1a0c91 100644 --- a/spec/factories/bulk_upload.rb +++ b/spec/factories/bulk_upload.rb @@ -8,6 +8,7 @@ FactoryBot.define do identifier { SecureRandom.uuid } sequence(:filename) { |n| "bulk-upload-#{n}.csv" } needstype { 1 } + noint_fix_status { BulkUpload.noint_fix_statuses.values.sample } trait(:sales) do log_type { BulkUpload.log_types[:sales] } diff --git a/spec/lib/tasks/correct_noint_value_spec.rb b/spec/lib/tasks/correct_noint_value_spec.rb new file mode 100644 index 000000000..51dd7779c --- /dev/null +++ b/spec/lib/tasks/correct_noint_value_spec.rb @@ -0,0 +1,87 @@ +require "rails_helper" +require "rake" + +RSpec.describe "correct_noint_value" do + describe ":correct_noint_value", type: :task do + subject(:task) { Rake::Task["correct_noint_value"] } + + before do + Rake.application.rake_require("tasks/correct_noint_value") + Rake::Task.define_task(:environment) + task.reenable + end + + context "when the rake task is run" do + context "and there is a sales bulk upload with the fix needed" do + let(:bulk_upload) { create(:bulk_upload, :sales, noint_fix_status: BulkUpload.noint_fix_statuses[:not_applied]) } + + before do + bulk_upload.save! + end + + it "updates the noint value on a log with noint = 2 where it was set to 2 on create" do + log = create(:sales_log, :completed, noint: 2, bulk_upload:) + + task.invoke + log.reload + + expect(log.noint).to be(1) + end + + it "updates the noint value on a log with noint = 2 where it was set to 2 on create and other fields have since changed" do + log = create(:sales_log, :in_progress, noint: 2, bulk_upload:) + log.update!(status: Log.statuses[:completed]) + + task.invoke + log.reload + + expect(log.noint).to be(1) + end + + it "does not update the noint value on a log that has noint = 1" do + log = create(:sales_log, :completed, noint: 2, bulk_upload:) + log.update!(noint: 1) + + task.invoke + log.reload + + expect(log.noint).to be(1) + end + + it "does not update the noint value on a log with noint = 2 where noint was nil on create" do + log = create(:sales_log, :completed, noint: nil, bulk_upload:) + log.update!(noint: 2) + + task.invoke + log.reload + + expect(log.noint).to be(2) + end + + it "updates the noint_fix_status value on the bulk upload" do + task.invoke + bulk_upload.reload + + expect(bulk_upload.noint_fix_status).to eq(BulkUpload.noint_fix_statuses[:applied]) + end + end + + context "and there is a sales bulk upload with the fix marked as not needed" do + let(:bulk_upload) { create(:bulk_upload, :sales, noint_fix_status: BulkUpload.noint_fix_statuses[:not_needed]) } + + before do + bulk_upload.save! + end + + it "does not update the noint values on logs" do + log = create(:sales_log, :completed, noint: 2, bulk_upload:) + + task.invoke + log.reload + + expect(log.noint).to be(2) + end + end + end + end +end diff --git a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb index 1056d4402..c4f4a26a6 100644 --- a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb @@ -1001,6 +1001,24 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do end describe "#log" do + describe "#noint" do + context "when field is set to 1" do + let(:attributes) { valid_attributes.merge({ field_28: 1 }) } + + it "is correctly set" do + expect(parser.log.noint).to be(1) + end + end + + context "when field is set to 2" do + let(:attributes) { valid_attributes.merge({ field_28: 2 }) } + + it "is correctly set" do + expect(parser.log.noint).to be(2) + end + end + end + describe "#uprn" do let(:attributes) { setup_section_params.merge({ field_19: "12" }) } diff --git a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb index 8866d3de2..5a1a9e4df 100644 --- a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb @@ -350,7 +350,6 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do let(:attributes) do { bulk_upload:, - field_17: "test id", field_31: "2", field_46: "8", field_39: "1", @@ -1027,6 +1026,24 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do end describe "#log" do + describe "#noint" do + context "when field is set to 1" do + let(:attributes) { valid_attributes.merge({ field_17: 1 }) } + + it "is correctly set" do + expect(parser.log.noint).to be(1) + end + end + + context "when field is set to 2" do + let(:attributes) { valid_attributes.merge({ field_17: 2 }) } + + it "is correctly set" do + expect(parser.log.noint).to be(2) + end + end + end + describe "#uprn" do let(:attributes) { setup_section_params.merge({ field_22: "12" }) }