RUBY 3387 implement solution to govpay status being stored twice (#1581)

Fix for ticket [RUBY-3387]

This pull request addresses an error that occurred when copying attributes between models using the SafeCopyAttributesService. https://errbit-prd.aws-int.defra.cloud/apps/63b80e796b9969055b550d32/problems/6703a63c6b9969041a285ed7

The error occurred because the previous implementation of SafeCopyAttributesService was copying all attributes from the source instance, including deprecated or removed attributes that still existed in the database but were no longer defined in the model. When these attributes were assigned to a new instance of the class, it resulted in an error due to the absence of corresponding fields in the model.

To resolve this issue, the SafeCopyAttributesService has been changed to:

- Safely copy only the attributes defined in the target model, excluding any attributes that are not explicitly defined.
- Handle embedded associations recursively, ensuring that nested attributes are also copied correctly without including deprecated fields.
- Continue allowing specified attributes, as well as embedded associations to be omitted from the copying process.
- Changed tests in safe_copy_attributes_service_spec.rb to cover the new functionality and ensure deprecated attributes aren't copied.
- Updated tests in past_registration_spec.rb and registration_completion_service_spec.rb to compare attributes excluding the _id field, reflecting the changes in how embedded documents are copied.

[RUBY-3387]: https://eaflood.atlassian.net/browse/RUBY-3387?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ




* [RUBY-3387] Fix duplicate storage of govpay status by refactoring `SafeCopyAttributesService`

- Implement `safe_copy_attributes` method to handle attribute copying more efficiently.
- Introduce private helper methods `get_attributes`, `filter_attributes`, and `find_embedded_relation` to modularize logic.
- Update specs to cover new functionality, including handling of Hash and BSON::Document sources.
- Ensure only valid attributes are copied, and embedded documents are processed correctly.

* [RUBY-3387] Refactor `SafeCopyAttributesService` to streamline attribute copying

- Removed unused parameters `embedded_documents` and `attributes_to_exclude` from the `SafeCopyAttributesService`.
- Simplified the logic for copying attributes, focusing on filtering and processing embedded documents directly within the `copy_attributes` method.
- Updated related specs to reflect changes in the service, ensuring correct attributes are copied and non-existent attributes are excluded.

* [RUBY-3387] Implement exclusion of attributes in SafeCopyAttributesService

- Updated `SafeCopyAttributesService` to allow exclusion of specified attributes during the copy process.
- Modified `can_copy_data_from_registration` to pass `attributes_to_exclude` option.
- Enhanced `filter_attributes` method to exclude specified attributes.
- Adjusted tests in `safe_copy_attributes_service_spec` to cover new exclusion functionality.
- Removed redundant embedded document copying logic from `past_registration` and `registration_completion_service`.

* [RUBY-3387] Fix duplicate storage of govpay status in `SafeCopyAttributesService`

- Updated the service to correctly handle source relation data with both camelCase and snake_case keys.
- Adjusted the logic to prevent storing attributes not present on the model, specifically `govpayStatus`.
- Enhanced tests to verify that `govpayStatus` is not included in the result while valid attributes like `currency` are correctly processed.

* [RUBY-3387] Rubocop changes

- Corrected the logic to prevent `govpayStatus` from being stored twice by refining the attribute matching process.
- Improved code readability by adding parentheses for variable assignments within conditional statements.
- Removed redundant code and fixed indentation issues.
- Updated tests to reflect changes in attribute presence checks, ensuring `govpayStatus` is not included if not present in the order model.

* [RUBY-3387] Refactor attribute copying in `SafeCopyAttributesService`

- Simplified the `copy_attributes` method to improve readability and maintainability.
- Introduced `extract_attributes`, `filter_attributes`, and `process_embedded_relations` helper methods for better structure.
- Enhanced handling of embedded relations by adding `find_relation_data` to support different naming conventions.
- Improved error handling for unsupported source types.

* [RUBY-3387] Fix duplicate storage of `govpayStatus` in SafeCopyAttributesService

- Updated `SafeCopyAttributesService` to exclude relations specified in `attributes_to_exclude` when copying attributes.
- Refactored method to find matching attribute keys based on naming conventions.
- Added tests to ensure excluded relations are not copied and that copyable attributes are correctly included.

* [RUBY-3387] Fix attribute comparison in specs to ignore `_id` field

Updated tests in `past_registration_spec.rb` and `registration_completion_service_spec.rb` to compare attributes excluding the `_id` field as new implementation of SafeCopyAttributes does not copy the id field of the embedded documents like the old one did

* [RUBY-3387] Update spec to reference registered address as company address was removed

* [RUBY-3387] Add documentation to `SafeCopyAttributesService` in `safe_copy_attributes_service.rb`

Enhanced the `SafeCopyAttributesService` with detailed comments explaining its functionality, including how it safely copies attributes between instances and handles embedded relations.

* [RUBY-3387] Remove ids from do not copy attributes as id is automatically ignored

* [RUBY-3387] Rephrase comment to specify that all attributes and embedded
relations will be copied by default

app/models/concerns/waste_carriers_engine/can_copy_data_from_registration.rb

@@ -21,7 +21,6 @@ def copy_attributes_from_registration
       attributes = SafeCopyAttributesService.run(
         source_instance: registration,
         target_class: self.class,
-        embedded_documents: %w[metaData],
         attributes_to_exclude: options[:ignorable_attributes]
       )
       assign_attributes(strip_whitespace(attributes))

app/models/concerns/waste_carriers_engine/can_strip_whitespace.rb

@@ -34,7 +34,7 @@ def strip_hash(hash)
 
     def strip_array(array)
       array.each do |nested_object|
-        return nested_object if nested_object.is_a? BSON::Document
+        return nested_object if nested_object.is_a?(BSON::Document) || nested_object.is_a?(Hash)
 
         strip_whitespace(nested_object.attributes)
       end

app/models/waste_carriers_engine/past_registration.rb

@@ -12,7 +12,6 @@ class PastRegistration
     embedded_in :registration, class_name: "WasteCarriersEngine::Registration"
 
     NON_COPYABLE_ATTRIBUTES = %w[
-      _id
       accountEmail
       past_registrations
       locking_name
@@ -22,6 +21,7 @@ class PastRegistration
       deregistration_token_created_at
       view_certificate_token
       view_certificate_token_created_at
+      conviction_sign_offs
     ].freeze
 
     def self.build_past_registration(registration, cause = nil)
@@ -35,7 +35,6 @@ def self.build_past_registration(registration, cause = nil)
       attributes = SafeCopyAttributesService.run(
         source_instance: registration,
         target_class: self,
-        embedded_documents: %w[addresses metaData financeDetails key_people conviction_search_result],
         attributes_to_exclude: NON_COPYABLE_ATTRIBUTES
       )
 

app/models/waste_carriers_engine/renewing_registration.rb

@@ -14,8 +14,7 @@ class RenewingRegistration < TransientRegistration
     field :from_magic_link, type: Boolean
 
     COPY_DATA_OPTIONS = {
-      ignorable_attributes: %w[_id
-                               accountEmail
+      ignorable_attributes: %w[accountEmail
                                constructionWaste
                                contactEmail
                                conviction_search_result

app/services/waste_carriers_engine/registration_completion_service.rb

@@ -127,7 +127,6 @@ def copy_data_from_transient_registration
       transient_registration.reload
 
       do_not_copy_attributes = %w[
-        _id
         conviction_search_result
         conviction_sign_offs
         created_at
@@ -148,7 +147,6 @@ def copyable_attributes(do_not_copy_attributes)
       SafeCopyAttributesService.run(
         source_instance: transient_registration,
         target_class: Registration,
-        embedded_documents: %w[addresses metaData financeDetails],
         attributes_to_exclude: do_not_copy_attributes
       )
     end

app/services/waste_carriers_engine/renewal_completion_service.rb

@@ -154,7 +154,6 @@ def copy_data_from_transient_registration
       renewal_attributes = SafeCopyAttributesService.run(
         source_instance: transient_registration,
         target_class: Registration,
-        embedded_documents: %w[addresses metaData financeDetails key_people conviction_search_result],
         attributes_to_exclude: do_not_copy_attributes
       )
 

app/services/waste_carriers_engine/safe_copy_attributes_service.rb

@@ -1,32 +1,95 @@
 # frozen_string_literal: true
 
 module WasteCarriersEngine
-  class SafeCopyAttributesService < BaseService
+  # This responsible for safely copying attributes and embedded relations from a source instance of
+  # one class to a new instance of the targeted class. By default it will copy every attribute except _id
+  # and every embedded relation that are defined in the target class. However, both attributes and embedded
+  # relations can be excluded by passing them into the attributes_to_exclude argument
+  # Embedded relations are processed recursively, and attributes to exclude
+  # will be applied to the embedded relations as well.
 
-    attr_accessor :source_instance, :target_class, :embedded_documents, :attributes_to_exclude
+  class SafeCopyAttributesService
+    def self.run(source_instance:, target_class:, attributes_to_exclude: [])
+      new(source_instance, target_class, attributes_to_exclude).run
+    end
 
-    def run(source_instance:, target_class:, embedded_documents: [], attributes_to_exclude: [])
+    def initialize(source_instance, target_class, attributes_to_exclude = [])
       @source_instance = source_instance
       @target_class = target_class
-      @embedded_documents = embedded_documents
       @attributes_to_exclude = attributes_to_exclude
+    end
 
-      source_attributes.except(*unsupported_attribute_keys)
+    def run
+      copy_attributes(@source_instance, @target_class)
     end
 
-    def source_attributes
-      attributes = source_instance.is_a?(BSON::Document) ? source_instance : source_instance.attributes
+    private
+
+    # Recursively copies attributes from the source to match the target class
+    def copy_attributes(source, target_class)
+      attributes = extract_attributes(source)
+      valid_attributes = filter_attributes(attributes, target_class)
+      embedded_attributes = process_embedded_relations(attributes, target_class)
+      valid_attributes.merge(embedded_attributes)
+    end
 
-      attributes.except(*attributes_to_exclude)
+    # Extracts attributes from the source instance based on its type
+    def extract_attributes(source)
+      case source
+      when Hash, BSON::Document
+        source.to_h.stringify_keys
+      when ->(obj) { obj.respond_to?(:attributes) }
+        source.attributes
+      else
+        raise ArgumentError, "Unsupported source_instance type: #{source.class}"
+      end
     end
 
-    def target_fields
-      # Include both camelCase (DB) and snake_case (model) attribute names:
-      (target_class.fields.keys + target_class.fields.keys.map(&:underscore)).uniq
+    # Filters attributes to include only those defined in the target class, excluding specified attributes
+    def filter_attributes(attributes, target_class)
+      target_fields = target_class.fields.keys.map(&:to_s)
+      attributes.slice(*target_fields).except("_id", *@attributes_to_exclude)
+    end
+
+    # Processes embedded relations defined in the target class
+    def process_embedded_relations(attributes, target_class)
+      embedded_attributes = {}
+
+      target_class.embedded_relations.each do |relation_name, relation_metadata|
+        # Skip if the relation is in the attributes_to_exclude list
+        next if @attributes_to_exclude.map(&:underscore).include?(relation_name.underscore)
+
+        # Find the corresponding key in attributes (handles snake_case and camelCase)
+        key = matching_attribute_key(attributes, relation_name)
+        next unless key
+
+        source_data = attributes[key]
+        embedded_class = relation_metadata.class_name.constantize
+        embedded_attributes[key] = process_embedded_data(source_data, embedded_class)
+      end
+
+      embedded_attributes
+    end
+
+    # Finds the attribute key in attributes that corresponds to the relation name
+    def matching_attribute_key(attributes, relation_name)
+      snake_case_name = relation_name.underscore
+      camel_case_name = relation_name.camelize(:lower)
+
+      if attributes.key?(snake_case_name)
+        snake_case_name
+      elsif attributes.key?(camel_case_name)
+        camel_case_name
+      end
     end
 
-    def unsupported_attribute_keys
-      source_attributes.except(*target_fields).excluding(embedded_documents).keys
+    # Recursively processes embedded data
+    def process_embedded_data(data, embedded_class)
+      if data.is_a?(Array)
+        data.map { |item| copy_attributes(item, embedded_class) }
+      elsif data.is_a?(Hash) || data.is_a?(BSON::Document)
+        copy_attributes(data, embedded_class)
+      end
     end
   end
 end

spec/models/waste_carriers_engine/past_registration_spec.rb

@@ -24,7 +24,8 @@ module WasteCarriersEngine
       end
 
       it "copies nested objects from the registration" do
-        expect(past_registration.registered_address).to eq(registration.registered_address)
+        expect(past_registration.registered_address.attributes.except("_id"))
+          .to eq(registration.registered_address.attributes.except("_id"))
       end
 
       context "with non-copyable attribute values" do

spec/services/waste_carriers_engine/registration_completion_service_spec.rb

@@ -81,7 +81,11 @@ module WasteCarriersEngine
           expect { complete_registration }.to change(WasteCarriersEngine::Registration, :count).by(1)
         end
 
-        it { expect(complete_registration.registered_address).to eq(transient_registration.registered_address) }
+        it do
+          expect(complete_registration.registered_address.attributes.except("_id"))
+            .to eq(transient_registration.registered_address.attributes.except("_id"))
+        end
+
         it { expect(complete_registration.expires_on).to be_present }
         it { expect(complete_registration.finance_details.orders.count).to eq(1) }
         it { expect(complete_registration.finance_details.payments.count).to eq(1) }

spec/services/waste_carriers_engine/safe_copy_attributes_service_spec.rb

@@ -5,26 +5,27 @@
 module WasteCarriersEngine
   RSpec.describe SafeCopyAttributesService do
     describe "#run" do
+      subject(:run_service) do
+        described_class.run(
+          source_instance: source_instance,
+          target_class: target_class,
+          attributes_to_exclude: exclusion_list
+        )
+      end
+      let(:exclusion_list) { [] }
 
-      subject(:run_service) { described_class.run(source_instance:, target_class:, embedded_documents:, attributes_to_exclude:) }
-
-      let(:embedded_documents) { nil }
-      let(:exclusion_list) { nil }
-      let(:attributes_to_exclude) { exclusion_list }
-
-      # ensure all available attributes are populated on the source
+      # Ensure all available attributes are populated on the source
       before do
         unless source_instance.is_a?(BSON::Document)
           source_instance.class.fields.keys.excluding("_id").each do |attr|
             next unless source_instance.send(attr).blank? && source_instance.respond_to?("#{attr}=")
 
-            source_instance.send "#{attr}=", 0
+            source_instance.send("#{attr}=", 0)
           end
         end
       end
 
       shared_examples "returns the correct attributes" do
-
         it { expect { run_service }.not_to raise_error }
 
         it "returns copyable attributes" do
@@ -48,11 +49,9 @@ module WasteCarriersEngine
 
       context "when the target is a Registration" do
         let(:target_class) { Registration }
-        # include all embeds_many relationships
-        let(:embedded_documents) { %w[addresses financeDetails metaData] }
         let(:copyable_attributes) { %w[location contactEmail] }
-        let(:non_copyable_attributes) { %w[workflow_state temp_contact_postcode not_even_an_attribute] }
-        let(:exclusion_list) { %w[_id email_history] }
+        let(:non_copyable_attributes) { %w[workflow_state temp_contact_postcode not_even_an_attribute _id email_history] }
+        let(:exclusion_list) { %w[email_history] }
 
         context "when the source is a NewRegistration" do
           let(:source_instance) { build(:new_registration, :has_required_data) }
@@ -73,7 +72,14 @@ module WasteCarriersEngine
         end
 
         context "when the source is a BSON::Document" do
-          let(:source_instance) { BSON::Document.new(build(:new_registration, :has_required_data).attributes) }
+          let(:source_instance) do
+            attributes = build(:new_registration, :has_required_data).attributes
+            attributes["non_existent_attribute"] = "some value"
+            BSON::Document.new(attributes)
+          end
+
+          # Include the non-existent attribute in non_copyable_attributes
+          let(:non_copyable_attributes) { super() + ["non_existent_attribute"] }
 
           it_behaves_like "returns the correct attributes"
         end
@@ -93,11 +99,85 @@ module WasteCarriersEngine
         let(:source_instance) { build(:key_person, :has_required_data) }
         let(:target_class) { KeyPerson }
         let(:copyable_attributes) { %w[first_name dob] }
-        let(:non_copyable_attributes) { %w[not_an_attribute neitherIsThis] }
+        let(:non_copyable_attributes) { %w[not_an_attribute neitherIsThis _id position] }
         let(:exclusion_list) { %w[_id position] }
 
         it_behaves_like "returns the correct attributes"
       end
+
+      context "when the source is a BSON::Document with a nested attribute not present on the model" do
+        let(:target_class) { Registration }
+        let(:source_instance) do
+          attributes = {
+            "location" => "uk",
+            "contactEmail" => "[email protected]",
+            "non_existent_attribute" => "some value",
+            "financeDetails" => {
+              "orders" => [
+                {
+                  "govpayStatus" => "success",
+                  "currency" => "GBP"
+                }
+              ]
+
+            }
+          }
+          BSON::Document.new(attributes)
+        end
+
+        let(:copyable_attributes) { %w[location contactEmail] }
+        let(:non_copyable_attributes) { %w[non_existent_attribute _id] }
+        let(:result) { run_service }
+        let(:order_attributes) { result["financeDetails"]["orders"].first }
+
+        it "does not have the govpayStatus attribute as it is not present on the order model" do
+          expect(order_attributes.keys).not_to include("govpayStatus")
+        end
+
+        it "has the currency attribute as it is present on the order model" do
+          expect(order_attributes.keys).to include("currency")
+        end
+      end
+
+      context "when an embedded relation is included in attributes_to_exclude" do
+        let(:target_class) { Registration }
+        let(:source_instance) do
+          attributes = {
+            "location" => "uk",
+            "contactEmail" => "[email protected]",
+            "financeDetails" => {
+              "orders" => [
+                {
+                  "govpayStatus" => "success",
+                  "currency" => "GBP"
+                }
+              ]
+            }
+          }
+          BSON::Document.new(attributes)
+        end
+
+        let(:copyable_attributes) { %w[location contactEmail] }
+        let(:non_copyable_attributes) { %w[financeDetails _id] }
+        let(:exclusion_list) { %w[financeDetails] }
+        let(:result) { run_service }
+
+        it "does not include the excluded embedded relation" do
+          expect(result.keys).not_to include("financeDetails")
+        end
+
+        it "includes other copyable attributes" do
+          copyable_attributes.each do |attr|
+            expect(result[attr]).not_to be_nil
+          end
+        end
+
+        it "does not include non-copyable attributes" do
+          non_copyable_attributes.each do |attr|
+            expect(result[attr]).to be_nil
+          end
+        end
+      end
     end
   end
 end