From 41f08ab6017b9570dd0581de0e83267004c0c91b Mon Sep 17 00:00:00 2001 From: Jerome Pratt Date: Tue, 22 Oct 2024 16:07:31 +0100 Subject: [PATCH] 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 --- .../can_copy_data_from_registration.rb | 1 - .../can_strip_whitespace.rb | 2 +- .../past_registration.rb | 3 +- .../renewing_registration.rb | 3 +- .../registration_completion_service.rb | 2 - .../renewal_completion_service.rb | 1 - .../safe_copy_attributes_service.rb | 89 +++++++++++--- .../past_registration_spec.rb | 3 +- .../registration_completion_service_spec.rb | 6 +- .../safe_copy_attributes_service_spec.rb | 110 +++++++++++++++--- 10 files changed, 181 insertions(+), 39 deletions(-) diff --git a/app/models/concerns/waste_carriers_engine/can_copy_data_from_registration.rb b/app/models/concerns/waste_carriers_engine/can_copy_data_from_registration.rb index 5eb534375..ea236b472 100644 --- a/app/models/concerns/waste_carriers_engine/can_copy_data_from_registration.rb +++ b/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)) diff --git a/app/models/concerns/waste_carriers_engine/can_strip_whitespace.rb b/app/models/concerns/waste_carriers_engine/can_strip_whitespace.rb index addb1f5f4..5d323ae31 100644 --- a/app/models/concerns/waste_carriers_engine/can_strip_whitespace.rb +++ b/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 diff --git a/app/models/waste_carriers_engine/past_registration.rb b/app/models/waste_carriers_engine/past_registration.rb index c07216db6..b965f75ec 100644 --- a/app/models/waste_carriers_engine/past_registration.rb +++ b/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 ) diff --git a/app/models/waste_carriers_engine/renewing_registration.rb b/app/models/waste_carriers_engine/renewing_registration.rb index 66d01b532..0fc7be8c7 100644 --- a/app/models/waste_carriers_engine/renewing_registration.rb +++ b/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 diff --git a/app/services/waste_carriers_engine/registration_completion_service.rb b/app/services/waste_carriers_engine/registration_completion_service.rb index 039d7d415..abd17b372 100644 --- a/app/services/waste_carriers_engine/registration_completion_service.rb +++ b/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 diff --git a/app/services/waste_carriers_engine/renewal_completion_service.rb b/app/services/waste_carriers_engine/renewal_completion_service.rb index 5bb6e2356..ae99171a9 100644 --- a/app/services/waste_carriers_engine/renewal_completion_service.rb +++ b/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 ) diff --git a/app/services/waste_carriers_engine/safe_copy_attributes_service.rb b/app/services/waste_carriers_engine/safe_copy_attributes_service.rb index bfa4cf1f2..c3905f99a 100644 --- a/app/services/waste_carriers_engine/safe_copy_attributes_service.rb +++ b/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 diff --git a/spec/models/waste_carriers_engine/past_registration_spec.rb b/spec/models/waste_carriers_engine/past_registration_spec.rb index 4b2e1b71a..5972c6b2b 100644 --- a/spec/models/waste_carriers_engine/past_registration_spec.rb +++ b/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 diff --git a/spec/services/waste_carriers_engine/registration_completion_service_spec.rb b/spec/services/waste_carriers_engine/registration_completion_service_spec.rb index 4a6814d36..63b29dec9 100644 --- a/spec/services/waste_carriers_engine/registration_completion_service_spec.rb +++ b/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) } diff --git a/spec/services/waste_carriers_engine/safe_copy_attributes_service_spec.rb b/spec/services/waste_carriers_engine/safe_copy_attributes_service_spec.rb index ba1bbed34..1c164e148 100644 --- a/spec/services/waste_carriers_engine/safe_copy_attributes_service_spec.rb +++ b/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" => "test@example.com", + "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" => "test@example.com", + "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