CLDC-3224: Fix bulk upload noint bug (#2251)

* Empty commit

* CLDC-3224: Parse noint correctly in bulk upload row parsers

* CLDC-3224: Add task to fix previous noint data

* CLDC-3224: Remove nonsensical value for noint field in bu test

* CLDC-3224: Move where bulk upload noint fix status is set to ensure it matches processing version

* Put BU noint_fix_status setting before creating any logs

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
 

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?
 

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/)

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/)

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

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

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

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

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

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

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

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

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] }

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

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" }) }
 

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" }) }