From df8a76637cbe2ba575de0f58380ba2e9e715760a Mon Sep 17 00:00:00 2001 From: oleghasjanov Date: Mon, 11 Aug 2025 15:31:46 +0300 Subject: [PATCH] fixed duplicate contacts during update --- app/interactions/actions/domain_update.rb | 91 +++++++++++++++++-- .../repp/v1/domains/contacts_test.rb | 16 +++- 2 files changed, 94 insertions(+), 13 deletions(-) diff --git a/app/interactions/actions/domain_update.rb b/app/interactions/actions/domain_update.rb index 84035f75c..d035788d1 100644 --- a/app/interactions/actions/domain_update.rb +++ b/app/interactions/actions/domain_update.rb @@ -93,10 +93,81 @@ module Actions def duplicate_contact?(contact1, contact2) return false unless contact1 && contact2 - contact1.name == contact2.name && - contact1.ident == contact2.ident && - contact1.email == contact2.email && - contact1.phone == contact2.phone + contact1.code == contact2.code || + (contact1.name == contact2.name && + contact1.ident == contact2.ident && + contact1.email == contact2.email && + contact1.phone == contact2.phone) + end + + def filter_duplicate_contacts_before_assignment(props, role) + @removed_duplicates ||= [] + registrant = domain.registrant + + # Get existing contacts + existing_admin_contacts = domain.admin_domain_contacts.map(&:contact) + existing_tech_contacts = domain.tech_domain_contacts.map(&:contact) + + # Filter new contacts being added + filtered_props = props.select do |prop| + next true if prop[:_destroy] # Keep removal operations + + new_contact = Contact.find_by(id: prop[:contact_id]) + next false unless new_contact + + # Check against registrant + if registrant && duplicate_contact?(new_contact, registrant) + @removed_duplicates << { + role: role, + code: new_contact.code, + duplicate_of: registrant.code + } + next false + end + + # Check against existing admin contacts + is_duplicate = existing_admin_contacts.any? { |existing| duplicate_contact?(new_contact, existing) } + if is_duplicate && role == 'admin' + duplicate_of = existing_admin_contacts.find { |existing| duplicate_contact?(new_contact, existing) } + @removed_duplicates << { + role: role, + code: new_contact.code, + duplicate_of: duplicate_of.code + } + next false + end + + # Check against existing tech contacts + is_duplicate = existing_tech_contacts.any? { |existing| duplicate_contact?(new_contact, existing) } + if is_duplicate && role == 'tech' + duplicate_of = existing_tech_contacts.find { |existing| duplicate_contact?(new_contact, existing) } + @removed_duplicates << { + role: role, + code: new_contact.code, + duplicate_of: duplicate_of.code + } + next false + end + + # For tech contacts, also check against admin contacts + if role == 'tech' + is_duplicate = existing_admin_contacts.any? { |existing| duplicate_contact?(new_contact, existing) } + if is_duplicate + duplicate_of = existing_admin_contacts.find { |existing| duplicate_contact?(new_contact, existing) } + @removed_duplicates << { + role: role, + code: new_contact.code, + duplicate_of: duplicate_of.code + } + next false + end + end + + true + end + + notify_about_removed_duplicates unless @removed_duplicates.empty? + filtered_props end def notify_about_removed_duplicates @@ -235,8 +306,10 @@ module Actions domain.add_epp_error('2304', 'admin', DomainStatus::SERVER_ADMIN_CHANGE_PROHIBITED, I18n.t(:object_status_prohibits_operation)) elsif props.present? - domain.admin_domain_contacts_attributes = props - check_for_same_contacts(props, 'admin') + # Filter duplicates before assignment + props = filter_duplicate_contacts_before_assignment(props, 'admin') + domain.admin_domain_contacts_attributes = props if props.present? + check_for_same_contacts(props, 'admin') if props.present? end end @@ -250,8 +323,10 @@ module Actions domain.add_epp_error('2304', 'tech', DomainStatus::SERVER_TECH_CHANGE_PROHIBITED, I18n.t(:object_status_prohibits_operation)) elsif props.present? - domain.tech_domain_contacts_attributes = props - check_for_same_contacts(props, 'tech') + # Filter duplicates before assignment + props = filter_duplicate_contacts_before_assignment(props, 'tech') + domain.tech_domain_contacts_attributes = props if props.present? + check_for_same_contacts(props, 'tech') if props.present? end end diff --git a/test/integration/repp/v1/domains/contacts_test.rb b/test/integration/repp/v1/domains/contacts_test.rb index 905a68beb..28025f2f0 100644 --- a/test/integration/repp/v1/domains/contacts_test.rb +++ b/test/integration/repp/v1/domains/contacts_test.rb @@ -51,7 +51,10 @@ class ReppV1DomainsContactsTest < ActionDispatch::IntegrationTest assert_response :ok assert_equal 1000, json[:code] - assert @domain.admin_contacts.find_by(code: new_contact.code).present? + # Becase we implemented feature which allows us avoid duplicates of contacts, + # I changed this value from assert to assert_not. + # PS! Tech contact and registrant are same, that's why tech contact is not added. + assert_not @domain.admin_contacts.find_by(code: new_contact.code).present? end def test_can_add_new_tech_contacts @@ -66,13 +69,16 @@ class ReppV1DomainsContactsTest < ActionDispatch::IntegrationTest assert_equal 1000, json[:code] @domain.reload - assert @domain.tech_contacts.find_by(code: new_contact.code).present? + # Becase we implemented feature which allows us avoid duplicates of contacts, + # I changed this value from assert to assert_not. + # PS! Tech contact and registrant are same, that's why tech contact is not added. + assert_not @domain.tech_contacts.find_by(code: new_contact.code).present? end def test_can_remove_admin_contacts Spy.on_instance_method(Actions::DomainUpdate, :validate_email).and_return(true) - contact = contacts(:john) + contact = contacts(:william) payload = { contacts: [ { code: contact.code, type: 'admin' } ] } post "/repp/v1/domains/#{@domain.name}/contacts", headers: @auth_headers, params: payload assert @domain.admin_contacts.find_by(code: contact.code).present? @@ -90,7 +96,7 @@ class ReppV1DomainsContactsTest < ActionDispatch::IntegrationTest def test_can_remove_tech_contacts Spy.on_instance_method(Actions::DomainUpdate, :validate_email).and_return(true) - contact = contacts(:john) + contact = contacts(:william) payload = { contacts: [ { code: contact.code, type: 'tech' } ] } post "/repp/v1/domains/#{@domain.name}/contacts", headers: @auth_headers, params: payload assert @domain.tech_contacts.find_by(code: contact.code).present? @@ -281,6 +287,6 @@ class ReppV1DomainsContactsTest < ActionDispatch::IntegrationTest assert_equal 1000, json[:code] domain.reload - assert domain.admin_contacts.find_by(code: new_admin.code).present? + assert_not domain.admin_contacts.find_by(code: new_admin.code).present? end end