Merge branch 'next' into enable_dismiss_important_notices_for_archived_patients

Author: CodeCorvin

app/controllers/class_imports_controller.rb

@@ -28,7 +28,6 @@ def create
                   **class_import_params
                 )
 
-    @class_import.load_data!
     if @class_import.invalid?
       render :new, status: :unprocessable_content and return
     end

app/controllers/cohort_imports_controller.rb

@@ -26,7 +26,6 @@ def create
                   **cohort_import_params
                 )
 
-    @cohort_import.load_data!
     if @cohort_import.invalid?
       render :new, status: :unprocessable_content and return
     end

app/controllers/immunisation_imports_controller.rb

@@ -22,7 +22,6 @@ def create
                   **immunisation_import_params
                 )
 
-    @immunisation_import.load_data!
     if @immunisation_import.invalid?
       render :new, status: :unprocessable_content and return
     end

app/jobs/process_import_job.rb

@@ -6,11 +6,14 @@ class ProcessImportJob < ApplicationJob
   queue_as :imports
 
   def perform(import)
+    return if import.processed?
+
     SemanticLogger.tagged(import_id: import.id) do
       Sentry.set_tags(import_id: import.id)
 
       import.parse_rows!
 
+      return if import.invalid?
       return if import.rows_are_invalid?
 
       import.process!

app/models/concerns/csv_importable.rb

@@ -6,7 +6,7 @@ module CSVImportable
   MAX_CSV_ROWS = 20_000
 
   included do
-    attr_accessor :csv_is_malformed, :data, :rows
+    attr_accessor :rows
 
     encrypts :csv_data
 
@@ -83,45 +83,56 @@ module CSVImportable
     before_save :ensure_processed_with_count_statistics
   end
 
-  def csv=(file)
-    self.csv_data = remove_bom_if_present(file&.read)
-    self.csv_filename = file&.original_filename
-  end
+  # Assign an uploaded CSV file to this import.
+  #
+  # Reads the uploaded file into {Import::CSVData}, stores the original filename,
+  # and updates {#rows_count} based on the parsed CSV data.
+  #
+  # If the file contains a UTF byte-order mark (BOM) (common when exporting from
+  # Excel), the encoding is detected and handled before reading.
+  #
+  # Raises an error if called on a persisted record, as changing the CSV file for
+  # an existing import is not allowed.
+  #
+  # @param source [ActionDispatch::Http::UploadedFile] the uploaded CSV file
+  # @raise [RuntimeError] if called on a persisted record
+  # @raise [ArgumentError] if `source` is not an uploaded file
+  def csv=(source)
+    if persisted?
+      raise "Cannot change the CSV file for an existing import. " \
+              "Create a new import instead."
+    end
 
-  # CSV files exported from Excel may have a BOM.
-  # https://en.wikipedia.org/wiki/Byte_order_mark
-  # e.g. if you create a new class import from scratch in Excel on Mac v16,
-  # save the file as CSV, and upload it.
-  def remove_bom_if_present(data)
-    StringIO.new(data).tap(&:set_encoding_by_bom).read
+    if source.is_a?(ActionDispatch::Http::UploadedFile)
+      # CSV files exported from Excel may have a BOM.
+      # https://en.wikipedia.org/wiki/Byte_order_mark
+      # e.g. if you create a new class import from scratch in Excel on Mac v16,
+      # save the file as CSV, and upload it.
+      self.csv_data = source.to_io.tap(&:set_encoding_by_bom).read
+      self.csv_filename = source&.original_filename
+      self.rows_count = csv_data_object&.count
+    else
+      raise ArgumentError, "Expected an uploaded file, got #{source}"
+    end
   end
 
   # Needed so that validations match the form field name.
   def csv
     csv_data
   end
 
-  def csv_removed?
-    csv_removed_at != nil
+  def csv_data_object
+    @csv_data_object ||= Import::CSVData.new(csv_data)
   end
 
-  def load_data!
-    return if invalid?
-
-    self.data ||= CSVParser.call(csv_data)
-    self.rows_count = data.count
-  rescue CSV::MalformedCSVError
-    self.csv_is_malformed = true
+  def csv_removed?
+    csv_removed_at != nil
   end
 
   def parse_rows!
-    load_data! if data.nil?
     return if invalid?
 
-    self.rows =
-      remove_trailing_blank_rows(data)
-        .then { |rows| has_instruction_row? ? rows.drop(1) : rows }
-        .map { |row_data| parse_row(row_data) }
+    self.rows = csv_data_object.records.map { |row_data| parse_row(row_data) }
 
     if invalid?
       self.serialized_errors = errors.to_hash
@@ -130,46 +141,10 @@ def parse_rows!
     end
   end
 
-  def remove_trailing_blank_rows(table)
-    found_values = false
-
-    # map(&:itself) because CSV::Table doesn't have a reverse method
-    rows_in_reverse_order = table.map(&:itself).reverse
-
-    filtered_rows =
-      rows_in_reverse_order.select do |row|
-        if found_values
-          true
-        elsif row.fields.all?(&:blank?)
-          false
-        else
-          found_values = true
-          true
-        end
-      end
-
-    filtered_rows.reverse
-  end
-
-  def has_instruction_row?
-    data&.first&.[](0)&.to_s&.match?(/\A(Required|Optional)([,.:]|$)/)
-  end
-
   def processed?
     processed_at != nil
   end
 
-  def process!
-    return if processed?
-
-    parse_rows! if rows.nil?
-    return if invalid?
-
-    process_import!
-
-    TeamCachedCounts.new(team).reset_import_issues!
-  end
-
   def remove!
     return if csv_removed?
     update!(csv_data: nil, csv_removed_at: Time.zone.now)
@@ -186,24 +161,23 @@ def load_serialized_errors!(limit: nil)
   end
 
   def csv_is_valid
-    return unless csv_is_malformed
-
-    errors.add(:csv, :invalid)
+    errors.add(:csv, :invalid) unless csv_data_object.well_formed?
   end
 
   def csv_is_not_too_large
-    return unless data
+    return unless csv_data
 
     if rows_count > MAX_CSV_ROWS
       errors.add(:csv, :too_many_rows, count: MAX_CSV_ROWS)
     end
   end
 
   def csv_has_records
-    return unless data
+    return unless csv_data
 
     csv_has_no_records =
-      data.empty? || (data.count == 1 && has_instruction_row?)
+      csv_data_object.empty? ||
+        (csv_data_object.count == 1 && csv_data_object.has_instruction_row?)
     errors.add(:csv, :empty) if csv_has_no_records
   end
 
@@ -214,7 +188,7 @@ def rows_are_valid
 
     check_rows_are_unique
 
-    row_offset = has_instruction_row? ? 3 : 2
+    row_offset = csv_data_object.has_instruction_row? ? 3 : 2
 
     rows.each.with_index do |row, index|
       next if row.errors.empty?

app/models/immunisation_import.rb

@@ -64,10 +64,41 @@ def records_count
     end
   end
 
+  def process!
+    raise "'rows' are empty. Call parse_rows! before processing." if rows.nil?
+
+    counts = count_columns.index_with(0)
+
+    @vaccination_records_batch = Set.new
+    @patients_batch = Set.new
+    @patient_locations_batch = Set.new
+    @archive_reasons_batch = Set.new
+
+    ActiveRecord::Base.transaction do
+      rows.each do |row|
+        count_column_to_increment = process_row(row)
+        counts[count_column_to_increment] += 1
+        bulk_import(rows: 100)
+      end
+
+      bulk_import(rows: :all)
+
+      postprocess_rows!
+
+      update_columns(processed_at: Time.zone.now, status: :processed, **counts)
+    end
+
+    post_commit!
+    UpdatePatientsFromPDS.call(patients, queue: :imports)
+
+    TeamCachedCounts.new(team).reset_import_issues!
+  end
+
   private
 
+  # TODO: This is called by the `rows_are_valid` validation. Move it to it's own validation.
   def check_rows_are_unique
-    row_offset = has_instruction_row? ? 3 : 2
+    row_offset = csv_data_object.has_instruction_row? ? 3 : 2
 
     rows
       .map(&:full_row_deduplication_attributes)
@@ -124,32 +155,6 @@ def process_row(row)
     count_column_to_increment
   end
 
-  def process_import!
-    counts = count_columns.index_with(0)
-
-    @vaccination_records_batch = Set.new
-    @patients_batch = Set.new
-    @patient_locations_batch = Set.new
-    @archive_reasons_batch = Set.new
-
-    ActiveRecord::Base.transaction do
-      rows.each do |row|
-        count_column_to_increment = process_row(row)
-        counts[count_column_to_increment] += 1
-        bulk_import(rows: 100)
-      end
-
-      bulk_import(rows: :all)
-
-      postprocess_rows!
-
-      update_columns(processed_at: Time.zone.now, status: :processed, **counts)
-    end
-
-    post_commit!
-    UpdatePatientsFromPDS.call(patients, queue: :imports)
-  end
-
   def bulk_import(rows: 100)
     return if rows != :all && @vaccination_records_batch.size < rows
 

app/models/import/csv_data.rb

@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+class Import::CSVData
+  attr_accessor :data, :malformed
+
+  # Use with serialization in the Import model
+  def initialize(data)
+    @data = data
+  end
+
+  def well_formed?
+    csv_table
+    !malformed
+  end
+
+  def empty? = csv_table.blank?
+
+  def csv_table
+    @csv_table ||=
+      begin
+        CSVParser.call(data) if data.present?
+      rescue CSV::MalformedCSVError
+        @malformed = true
+        nil
+      end
+  end
+
+  def count = csv_table&.count || 0
+
+  def records(&block)
+    remove_trailing_blank_rows
+      .then { |rows| has_instruction_row? ? rows.drop(1) : rows }
+      .each(&block)
+  end
+
+  def has_instruction_row?
+    csv_table&.first&.[](0)&.to_s&.match?(/\A(Required|Optional)([,.:]|$)/)
+  end
+
+  private
+
+  def remove_trailing_blank_rows
+    found_values = false
+
+    # map(&:itself) because CSV::Table doesn't have a reverse method
+    rows_in_reverse_order = csv_table.map(&:itself).reverse
+
+    filtered_rows =
+      rows_in_reverse_order.select do |row|
+        if found_values
+          true
+        elsif row.fields.all?(&:blank?)
+          false
+        else
+          found_values = true
+          true
+        end
+      end
+
+    filtered_rows.reverse
+  end
+end

app/models/patient_import.rb

@@ -34,7 +34,9 @@ def records_count
     changesets.from_file.count
   end
 
-  def process_import!
+  def process!
+    raise "'rows' are empty. Call parse_rows! before processing." if rows.nil?
+
     changesets =
       rows.each_with_index.map do |row, row_number|
         PatientChangeset.from_import_row(row:, import: self, row_number:)
@@ -54,6 +56,8 @@ def process_import!
     return if changesets_are_invalid?
 
     enqueue_review_jobs(self.changesets)
+
+    TeamCachedCounts.new(team).reset_import_issues!
   end
 
   def validate_pds_match_rate!
@@ -179,6 +183,8 @@ def enqueue_pds_cascading_searches(changesets)
     end
   end
 
+  # TODO: This is called by the `rows_are_valid` validation. Move it to it's own validation.
+  # TODO: Currently entested, unlike the equivalent in ImmunisationImport. Add tests.
   def check_rows_are_unique
     rows
       .map(&:nhs_number_value)