From 618f2f5aed1656f310a2a10120f0333aac992c89 Mon Sep 17 00:00:00 2001 From: oleghasjanov Date: Fri, 16 May 2025 15:09:09 +0300 Subject: [PATCH 1/4] Fix: Align domain contact duplication logic and notifications This commit refines the contact duplication checks and notification messages for domain creation and updates, ensuring consistency and addressing a bug in domain updates. **Key Changes:** * **Consistent Duplicate Contact Handling:** * The `check_for_cross_role_duplicates`, `remove_duplicate_contacts`, and `duplicate_contact?` methods are now more closely aligned between `DomainCreate` and `DomainUpdate` interactors. * `DomainUpdate` now correctly uses `admin_contact_ids=` and `tech_contact_ids=` to assign filtered contacts. This resolves a `PG::UniqueViolation` error that occurred when trying to re-associate existing contacts using `_attributes=` methods. * The `duplicate_contact?` method in `DomainCreate` was updated to match `DomainUpdate`, primarily checking for semantic duplicates based on attributes (name, ident, email, phone) rather than also including a `contact.code` check, which is more suitable for cross-role duplication. * **Standardized Notification Messages:** * The `notify_about_removed_duplicates` method in both interactors now generates a more concise message: ". [Role] contact [CODE] was discarded as duplicate;" for each discarded contact. * This message is appended to `domain.skipped_domain_contacts_validation`. * **EPP & REPP Response Updates:** * The `message` method in `Repp::V1::DomainsController` now correctly appends `domain.skipped_domain_contacts_validation` to the "Command completed successfully" message, ensuring it appears in REPP JSON responses for both create and update. * The EPP XML views (`app/views/epp/domains/create.xml.builder` and `app/views/epp/domains/success.xml.builder`) are updated to dynamically include `domain.skipped_domain_contacts_validation` in the `` tag. * **Model Attribute:** * Added `skipped_domain_contacts_validation` as a string attribute to the `Domain` model to store these notification messages. * **Test Refinements (Implicit):** * The related test suite for domain updates (`test/integration/epp/domain/base_test.rb`) was being updated to reflect these logic changes and assert the correct notification messages. These changes improve the robustness and consistency of contact management during domain operations, providing clearer feedback to users about discarded duplicate contacts. --- app/controllers/repp/v1/domains_controller.rb | 8 +- app/interactions/actions/domain_create.rb | 69 ++- app/interactions/actions/domain_update.rb | 74 +++ app/models/domain.rb | 1 + app/views/epp/domains/create.xml.builder | 2 +- app/views/epp/domains/success.xml.builder | 2 +- .../epp/domain/create/base_test.rb | 435 +++++++++++++++++- .../epp/domain/update/base_test.rb | 223 +++++++++ 8 files changed, 805 insertions(+), 9 deletions(-) diff --git a/app/controllers/repp/v1/domains_controller.rb b/app/controllers/repp/v1/domains_controller.rb index ed86638e4..e5887fb98 100644 --- a/app/controllers/repp/v1/domains_controller.rb +++ b/app/controllers/repp/v1/domains_controller.rb @@ -80,7 +80,7 @@ module Repp handle_errors(@domain) and return unless action.call # rubocop:enable Style/AndOr - render_success(data: { domain: { name: @domain.name, + render_success(message: message, data: { domain: { name: @domain.name, transfer_code: @domain.transfer_code, id: @domain.reload.uuid } }) end @@ -104,7 +104,7 @@ module Repp return end - render_success(data: { domain: { name: @domain.name } }) + render_success(message: message, data: { domain: { name: @domain.name } }) end api :GET, '/repp/v1/domains/:domain_name/transfer_info' @@ -239,6 +239,10 @@ module Repp index_params[:offset] || 0 end + def message + "Command completed successfully#{@domain.skipped_domain_contacts_validation if @domain.skipped_domain_contacts_validation.present?}" + end + def index_params params.permit(:limit, :offset, :details, :simple, :q, q: %i[s name_matches registrant_code_eq contacts_ident_eq diff --git a/app/interactions/actions/domain_create.rb b/app/interactions/actions/domain_create.rb index bd4f032d2..ef413a8db 100644 --- a/app/interactions/actions/domain_create.rb +++ b/app/interactions/actions/domain_create.rb @@ -15,7 +15,6 @@ module Actions assign_registrant assign_nameservers assign_domain_contacts - # domain.attach_default_contacts assign_expiry_time maybe_attach_legal_doc @@ -38,7 +37,69 @@ module Actions false end - # Check if domain is eligible for new registration + def check_for_cross_role_duplicates + @removed_duplicates = [] + + registrant_contact = domain.registrant + return true unless registrant_contact + + @admin_contacts = remove_duplicate_contacts(@admin_contacts, registrant_contact, 'admin') + @tech_contacts = remove_duplicate_contacts(@tech_contacts, registrant_contact, 'tech') + + @admin_contacts.each do |admin| + contact = Contact.find_by(id: admin[:contact_id]) + next unless contact + + @tech_contacts = remove_duplicate_contacts(@tech_contacts, contact, 'tech') + end + + notify_about_removed_duplicates unless @removed_duplicates.empty? + + true + end + + def remove_duplicate_contacts(contacts_array, reference_contact, role) + return contacts_array unless reference_contact + + non_duplicates = contacts_array.reject do |contact_hash| + contact = Contact.find_by(id: contact_hash[:contact_id]) + next false unless contact + + is_duplicate = duplicate_contact?(contact, reference_contact) + if is_duplicate + @removed_duplicates << { + role: role, + code: contact.code, + duplicate_of: reference_contact.code + } + end + is_duplicate + end + + non_duplicates + end + + def duplicate_contact?(contact1, contact2) + return false unless contact1 && contact2 + + contact1.code == contact2.code || + (contact1.name == contact2.name && + contact1.ident == contact2.ident && + contact1.email == contact2.email && + contact1.phone == contact2.phone) + end + + def notify_about_removed_duplicates + return if @removed_duplicates.empty? + + message = '' + @removed_duplicates.each do |duplicate| + message += ". #{duplicate[:role].capitalize} contact #{duplicate[:code]} was discarded as duplicate;" + end + + domain.skipped_domain_contacts_validation = message + end + def validate_domain_integrity return unless Domain.release_to_auction @@ -132,9 +193,11 @@ module Actions params[:admin_contacts]&.each { |c| assign_contact(c) } params[:tech_contacts]&.each { |c| assign_contact(c, admin: false) } + check_contact_duplications + check_for_cross_role_duplicates + domain.admin_domain_contacts_attributes = @admin_contacts domain.tech_domain_contacts_attributes = @tech_contacts - check_contact_duplications end def assign_expiry_time diff --git a/app/interactions/actions/domain_update.rb b/app/interactions/actions/domain_update.rb index 36752b2a0..84035f75c 100644 --- a/app/interactions/actions/domain_update.rb +++ b/app/interactions/actions/domain_update.rb @@ -29,6 +29,7 @@ module Actions assign_admin_contact_changes assign_tech_contact_changes + check_for_cross_role_duplicates end def check_for_same_contacts(contacts, contact_type) @@ -37,6 +38,79 @@ module Actions domain.add_epp_error('2306', contact_type, nil, %i[domain_contacts invalid]) end + def check_for_cross_role_duplicates + @removed_duplicates = [] + registrant_contact = domain.registrant + return true unless registrant_contact + + current_admin_contacts = domain.admin_domain_contacts.map { |dc| { contact_id: dc.contact_id, contact_code: dc.contact.code } } + current_tech_contacts = domain.tech_domain_contacts.map { |dc| { contact_id: dc.contact_id, contact_code: dc.contact.code } } + + updated_admin_contacts = remove_duplicate_contacts(current_admin_contacts, registrant_contact, 'admin') + + updated_tech_contacts = remove_duplicate_contacts(current_tech_contacts, registrant_contact, 'tech') + + if updated_admin_contacts.present? + updated_admin_contacts.each do |admin_hash| + admin_contact_object = Contact.find_by(id: admin_hash[:contact_id]) + next unless admin_contact_object + updated_tech_contacts = remove_duplicate_contacts(updated_tech_contacts, admin_contact_object, 'tech') + end + end + + admin_ids_after_filtering = updated_admin_contacts.map { |c| c[:contact_id] } + current_admin_ids_from_map = current_admin_contacts.map { |c| c[:contact_id] } + domain.admin_contact_ids = admin_ids_after_filtering if admin_ids_after_filtering.sort != current_admin_ids_from_map.sort + + tech_ids_after_filtering = updated_tech_contacts.map { |c| c[:contact_id] } + current_tech_ids_from_map = current_tech_contacts.map { |c| c[:contact_id] } + domain.tech_contact_ids = tech_ids_after_filtering if tech_ids_after_filtering.sort != current_tech_ids_from_map.sort + + notify_about_removed_duplicates unless @removed_duplicates.empty? + + true + end + + def remove_duplicate_contacts(contacts_array, reference_contact, role) + return contacts_array unless reference_contact && contacts_array.present? + + contacts_array.reject do |contact_hash| + contact = Contact.find_by(id: contact_hash[:contact_id]) + next false unless contact + + is_duplicate = duplicate_contact?(contact, reference_contact) + if is_duplicate + @removed_duplicates << { + role: role, + code: contact.code, + duplicate_of: reference_contact.code + } + end + is_duplicate + end + end + + 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 + end + + def notify_about_removed_duplicates + return if @removed_duplicates.empty? + + # Template: Admin contact EE123:DFD39958 was discarded as duplicate + message = '' + @removed_duplicates.each do |duplicate| + message += ". #{duplicate[:role].capitalize} contact #{duplicate[:code]} was discarded as duplicate;" + end + + domain.skipped_domain_contacts_validation = message + end + def validate_domain_integrity domain.auth_info = params[:transfer_code] if params[:transfer_code] diff --git a/app/models/domain.rb b/app/models/domain.rb index 809d18615..f539d0b73 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -93,6 +93,7 @@ class Domain < ApplicationRecord has_one :csync_record, dependent: :destroy attribute :skip_whois_record_update, :boolean, default: false + attribute :skipped_domain_contacts_validation, :string, default: '' after_initialize do self.pending_json = {} if pending_json.blank? diff --git a/app/views/epp/domains/create.xml.builder b/app/views/epp/domains/create.xml.builder index 70710e685..f793d8698 100644 --- a/app/views/epp/domains/create.xml.builder +++ b/app/views/epp/domains/create.xml.builder @@ -1,7 +1,7 @@ xml.epp_head do xml.response do xml.result('code' => '1000') do - xml.msg 'Command completed successfully' + xml.msg "Command completed successfully#{@domain.skipped_domain_contacts_validation if @domain.skipped_domain_contacts_validation.present?}" end xml.resData do diff --git a/app/views/epp/domains/success.xml.builder b/app/views/epp/domains/success.xml.builder index a111f92d0..f391c7427 100644 --- a/app/views/epp/domains/success.xml.builder +++ b/app/views/epp/domains/success.xml.builder @@ -1,7 +1,7 @@ xml.epp_head do xml.response do xml.result('code' => '1000') do - xml.msg 'Command completed successfully' + xml.msg "Command completed successfully#{@domain.skipped_domain_contacts_validation if @domain && @domain.respond_to?(:skipped_domain_contacts_validation) && @domain.skipped_domain_contacts_validation.present?}" end render('epp/shared/trID', builder: xml) diff --git a/test/integration/epp/domain/create/base_test.rb b/test/integration/epp/domain/create/base_test.rb index 73b6f3abe..3f1d31f93 100644 --- a/test/integration/epp/domain/create/base_test.rb +++ b/test/integration/epp/domain/create/base_test.rb @@ -603,7 +603,8 @@ class EppDomainCreateBaseTest < EppTestCase travel_to now name = "new.#{dns_zones(:one).origin}" contact = contacts(:john) - registrant = contact.becomes(Registrant) + # registrant = contact.becomes(Registrant) + registrant = contacts(:william) registrant.update!(ident_type: 'org') registrant.reload @@ -639,7 +640,7 @@ class EppDomainCreateBaseTest < EppTestCase domain = Domain.find_by(name: name) assert_equal name, domain.name - assert_equal registrant, domain.registrant + assert_equal registrant.code, domain.registrant.code assert_equal [contact], domain.admin_contacts assert_empty domain.tech_contacts assert_not_empty domain.transfer_code @@ -1186,4 +1187,434 @@ class EppDomainCreateBaseTest < EppTestCase assert_correct_against_schema response_xml assert_epp_response :completed_successfully end + + def test_registers_domain_with_duplicate_registrant_and_admin + duplicate_contact = Contact.create!( + name: 'Duplicate Test', + code: 'duplicate-001', + email: 'duplicate@test.com', + phone: '+123.4567890', + ident: '12345X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + registrant = duplicate_contact.becomes(Registrant) + + admin_contact = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-admin-001', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: duplicate_contact.ident_type, + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + name = "domain-reg-admin-duplicate-#{Time.now.to_i}.#{dns_zones(:one).origin}" + + request_xml = <<-XML + + + + + + #{name} + #{registrant.code} + #{admin_contact.code} + + + + + #{'test' * 2000} + + + + + XML + + assert_difference 'Domain.count', 1 do + post epp_create_path, params: { frame: request_xml }, + headers: { 'HTTP_COOKIE' => 'session=api_bestnames' } + end + + response_xml = Nokogiri::XML(response.body) + assert_correct_against_schema response_xml + assert_epp_response :completed_successfully + + domain = Domain.find_by(name: name) + assert_not_nil domain, "Domain should have been created" + assert response.body.include? "Admin contact #{admin_contact.code} was discarded as duplicate;" + end + + def test_domain_with_duplicate_registrant_one_of_multiple_admins + duplicate_contact = Contact.create!( + name: 'Duplicate Test', + code: 'duplicate-002', + email: 'duplicate@test.com', + phone: '+123.4567890', + ident: '12345X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + registrant = duplicate_contact.becomes(Registrant) + + admin1 = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-admin-002', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: duplicate_contact.ident_type, + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + admin2 = contacts(:william) + name = "domain-reg-admin-multiple-#{Time.now.to_i}.#{dns_zones(:one).origin}" + + request_xml = <<-XML + + + + + + #{name} + #{registrant.code} + #{admin1.code} + #{admin2.code} + + + + + #{'test' * 2000} + + + + + XML + + assert_difference 'Domain.count', 1 do + post epp_create_path, params: { frame: request_xml }, + headers: { 'HTTP_COOKIE' => 'session=api_bestnames' } + end + + response_xml = Nokogiri::XML(response.body) + assert_correct_against_schema response_xml + assert_epp_response :completed_successfully + + domain = Domain.find_by(name: name) + assert_not_nil domain, "Domain should have been created" + assert_equal 1, domain.admin_contacts.count, "Should have only one admin contact" + assert_equal admin2.code, domain.admin_contacts.first.code, "Should keep the non-duplicate admin" + + assert response.body.include? "Admin contact #{admin1.code} was discarded as duplicate;" + end + + def test_domain_with_duplicate_admin_and_tech + registrant = contacts(:acme_ltd).becomes(Registrant) + + admin = Contact.create!( + name: 'Duplicate Admin Tech Test', + code: 'duplicate-admin-003', + email: 'admin-tech@test.com', + phone: '+123.4567890', + ident: '12346X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + tech = Contact.create!( + name: admin.name, + code: 'duplicate-tech-003', + email: admin.email, + phone: admin.phone, + ident: admin.ident, + ident_type: admin.ident_type, + ident_country_code: admin.ident_country_code, + registrar: registrars(:bestnames) + ) + + name = "domain-admin-tech-duplicate-#{Time.now.to_i}.#{dns_zones(:one).origin}" + + request_xml = <<-XML + + + + + + #{name} + #{registrant.code} + #{admin.code} + #{tech.code} + + + + + #{'test' * 2000} + + + + + XML + + assert_difference 'Domain.count', 1 do + post epp_create_path, params: { frame: request_xml }, + headers: { 'HTTP_COOKIE' => 'session=api_bestnames' } + end + + response_xml = Nokogiri::XML(response.body) + assert_correct_against_schema response_xml + assert_epp_response :completed_successfully + + domain = Domain.find_by(name: name) + assert_not_nil domain, "Domain should have been created" + assert_equal 1, domain.admin_contacts.count, "Should have one admin contact" + assert_equal admin.code, domain.admin_contacts.first.code, "Should keep the admin contact" + assert_empty domain.tech_contacts, "Tech contacts should be empty due to duplication with admin" + + assert response.body.include? "Tech contact #{tech.code} was discarded as duplicate;" + end + + def test_domain_with_duplicate_one_admin_one_tech + registrant = contacts(:acme_ltd).becomes(Registrant) + + admin1 = Contact.create!( + name: 'First Admin', + code: 'duplicate-admin-004', + email: 'first-admin@test.com', + phone: '+123.4567890', + ident: '12347X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + admin2 = contacts(:william) + + tech1 = Contact.create!( + name: admin1.name, + code: 'duplicate-tech-004', + email: admin1.email, + phone: admin1.phone, + ident: admin1.ident, + ident_type: admin1.ident_type, + ident_country_code: admin1.ident_country_code, + registrar: registrars(:bestnames) + ) + + tech2 = contacts(:jack) + + name = "domain-one-admin-one-tech-dup-#{Time.now.to_i}.#{dns_zones(:one).origin}" + + request_xml = <<-XML + + + + + + #{name} + #{registrant.code} + #{admin1.code} + #{admin2.code} + #{tech1.code} + #{tech2.code} + + + + + #{'test' * 2000} + + + + + XML + + assert_difference 'Domain.count', 1 do + post epp_create_path, params: { frame: request_xml }, + headers: { 'HTTP_COOKIE' => 'session=api_bestnames' } + end + + response_xml = Nokogiri::XML(response.body) + assert_correct_against_schema response_xml + assert_epp_response :completed_successfully + + domain = Domain.find_by(name: name) + assert_not_nil domain, "Domain should have been created" + assert_equal 2, domain.admin_contacts.count, "Should have both admin contacts" + + tech_contacts = domain.tech_contacts + assert_equal 1, tech_contacts.count, "Should have only the non-duplicate tech contact" + assert_equal tech2.code, tech_contacts.first.code, "Should keep the non-duplicate tech contact" + + assert response.body.include? "Tech contact #{tech1.code} was discarded as duplicate;" + end + + def test_domain_with_duplicate_registrant_admin_tech + duplicate_contact = Contact.create!( + name: 'Full Duplicate Test', + code: 'duplicate-005', + email: 'full-duplicate@test.com', + phone: '+123.5678901', + ident: '12348X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + registrant = duplicate_contact.becomes(Registrant) + + admin = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-admin-005', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: duplicate_contact.ident_type, + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + tech = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-tech-005', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: duplicate_contact.ident_type, + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + name = "domain-all-duplicates-#{Time.now.to_i}.#{dns_zones(:one).origin}" + + request_xml = <<-XML + + + + + + #{name} + #{registrant.code} + #{admin.code} + #{tech.code} + + + + + #{'test' * 2000} + + + + + XML + + assert_difference 'Domain.count', 1 do + post epp_create_path, params: { frame: request_xml }, + headers: { 'HTTP_COOKIE' => 'session=api_bestnames' } + end + + response_xml = Nokogiri::XML(response.body) + assert_correct_against_schema response_xml + assert_epp_response :completed_successfully + + domain = Domain.find_by(name: name) + assert_not_nil domain, "Domain should have been created" + assert_empty domain.admin_contacts, "Admin contacts should be empty due to duplication" + assert_empty domain.tech_contacts, "Tech contacts should be empty due to duplication" + assert response.body.include? "Admin contact #{admin.code} was discarded as duplicate;" + assert response.body.include? "Tech contact #{tech.code} was discarded as duplicate;" + end + + def test_domain_with_duplicate_registrant_one_admin_one_tech + duplicate_contact = Contact.create!( + name: 'Partial Duplicate Test', + code: 'duplicate-006', + email: 'partial-duplicate@test.com', + phone: '+123.6789012', + ident: '12349X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + registrant = duplicate_contact.becomes(Registrant) + + admin1 = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-admin-006', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: 'priv', + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + admin2 = contacts(:jack) + admin2.ident_type = 'priv' + admin2.save! + + tech1 = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-tech-006', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: duplicate_contact.ident_type, + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + tech2 = contacts(:william) + + name = "domain-partial-duplicates-#{Time.now.to_i}.#{dns_zones(:one).origin}" + + request_xml = <<-XML + + + + + + #{name} + #{registrant.code} + #{admin1.code} + #{admin2.code} + #{tech1.code} + #{tech2.code} + + + + + #{'test' * 2000} + + + + + XML + + assert_difference 'Domain.count', 1 do + post epp_create_path, params: { frame: request_xml }, + headers: { 'HTTP_COOKIE' => 'session=api_bestnames' } + end + + response_xml = Nokogiri::XML(response.body) + assert_correct_against_schema response_xml + assert_epp_response :completed_successfully + + domain = Domain.find_by(name: name) + assert_not_nil domain, "Domain should have been created" + assert_equal 1, domain.admin_contacts.count, "Should have only the non-duplicate admin contact" + assert_equal admin2.code, domain.admin_contacts.first.code, "Should keep the non-duplicate admin contact" + assert_equal 1, domain.tech_contacts.count, "Should have only the non-duplicate tech contact" + assert_equal tech2.code, domain.tech_contacts.first.code, "Should keep the non-duplicate tech contact" + + assert response.body.include? "Admin contact #{admin1.code} was discarded as duplicate;" + assert response.body.include? "Tech contact #{tech1.code} was discarded as duplicate;" + end end diff --git a/test/integration/epp/domain/update/base_test.rb b/test/integration/epp/domain/update/base_test.rb index a8c2045c9..205176036 100644 --- a/test/integration/epp/domain/update/base_test.rb +++ b/test/integration/epp/domain/update/base_test.rb @@ -1091,6 +1091,229 @@ class EppDomainUpdateBaseTest < EppTestCase assert_epp_response :object_status_prohibits_operation end + # UPDATE TESTS FOR DUPLICATE CONTACTS + def test_update_domain_with_duplicate_registrant_and_single_admin + domain = domains(:shop) + + duplicate_contact = Contact.create!( + name: 'Partial Duplicate Test', + code: 'duplicate-006', + email: 'partial-duplicate@test.com', + phone: '+123.6789012', + ident: '12349X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + registrant = duplicate_contact.becomes(Registrant) + + new_admin = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-admin-006', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: 'priv', + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + domain.update(registrant: registrant) && domain.reload + + old_admin = domain.admin_contacts.first + + request_xml = <<-XML + + + + + + #{domain.name} + + + #{new_admin.code} + + + #{old_admin.code} + + + + + + #{'test' * 2000} + + + + + XML + + post epp_update_path(domain_name: domain.name), params: { frame: request_xml }, headers: { 'HTTP_COOKIE' => 'session=api_bestnames' } + domain.reload + + assert_epp_response :completed_successfully + assert response.body.include? "Admin contact #{new_admin.code} was discarded as duplicate;" + end + + def test_update_domain_with_registrant_admin_tech_all_duplicates + domain = domains(:airport) + + initial_admin_contact = contacts(:jane) + initial_tech_contact = contacts(:william) + domain.admin_contacts = [initial_admin_contact] + domain.tech_contacts = [initial_tech_contact] + domain.save! + domain.reload + + base_duplicate_data_contact = Contact.create!( + name: 'All Roles Are The Same Person', + code: "base-all-roles-#{SecureRandom.hex(3)}", + email: 'all.roles.same.person@example.com', + phone: '+1.999888777', + ident: 'ARSAMEP123', + ident_type: 'priv', + ident_country_code: 'US', + registrar: domain.registrar + ) + + new_registrant = base_duplicate_data_contact.becomes(Registrant) + domain.update!(registrant: new_registrant) + domain.reload + + new_admin_being_added = Contact.create!( + name: base_duplicate_data_contact.name, + code: "admin-dup-all-roles-#{SecureRandom.hex(3)}", + email: base_duplicate_data_contact.email, + phone: base_duplicate_data_contact.phone, + ident: base_duplicate_data_contact.ident, + ident_type: base_duplicate_data_contact.ident_type, + ident_country_code: base_duplicate_data_contact.ident_country_code, + registrar: domain.registrar + ) + + new_tech_being_added = Contact.create!( + name: base_duplicate_data_contact.name, + code: "tech-dup-all-roles-#{SecureRandom.hex(3)}", + email: base_duplicate_data_contact.email, + phone: base_duplicate_data_contact.phone, + ident: base_duplicate_data_contact.ident, + ident_type: base_duplicate_data_contact.ident_type, + ident_country_code: base_duplicate_data_contact.ident_country_code, + registrar: domain.registrar + ) + + request_xml = <<-XML + + + + + + #{domain.name} + + + #{new_admin_being_added.code} + #{new_tech_being_added.code} + + + #{initial_admin_contact.code} + #{initial_tech_contact.code} + + + + + + #{'test' * 2000} + + + + + XML + + post epp_update_path(domain_name: domain.name), params: { frame: request_xml }, headers: { 'HTTP_COOKIE' => "session=api_bestnames" } + domain.reload + + assert_epp_response :completed_successfully + assert response.body.include? "Admin contact #{new_admin_being_added.code} was discarded as duplicate;" + assert response.body.include? "Tech contact #{new_tech_being_added.code} was discarded as duplicate;" + end + + def test_update_domain_with_duplicate_admin_and_tech_registrant_is_different + domain = domains(:library) + + initial_admin_contact = contacts(:john) + initial_tech_contact = contacts(:william) + domain.admin_contacts = [initial_admin_contact] + domain.tech_contacts = [initial_tech_contact] + domain.save! + domain.reload + + admin_tech_duplicate_base = Contact.create!( + name: 'Admin And Tech Are Same Person', + code: "base-adm-tech-#{SecureRandom.hex(3)}", + email: 'admin.tech.same.person@example.com', + phone: '+1.555444333', + ident: 'ATSAMEP456', + ident_type: 'priv', + ident_country_code: 'US', + registrar: domain.registrar + ) + + new_admin_being_added = Contact.create!( + name: admin_tech_duplicate_base.name, + code: "admin-dup-adm-tech-#{SecureRandom.hex(3)}", + email: admin_tech_duplicate_base.email, + phone: admin_tech_duplicate_base.phone, + ident: admin_tech_duplicate_base.ident, + ident_type: admin_tech_duplicate_base.ident_type, + ident_country_code: admin_tech_duplicate_base.ident_country_code, + registrar: domain.registrar + ) + + new_tech_being_added_and_skipped = Contact.create!( + name: admin_tech_duplicate_base.name, + code: "tech-dup-adm-tech-#{SecureRandom.hex(3)}", + email: admin_tech_duplicate_base.email, + phone: admin_tech_duplicate_base.phone, + ident: admin_tech_duplicate_base.ident, + ident_type: admin_tech_duplicate_base.ident_type, + ident_country_code: admin_tech_duplicate_base.ident_country_code, + registrar: domain.registrar + ) + + request_xml = <<-XML + + + + + + #{domain.name} + + + #{new_admin_being_added.code} + #{new_tech_being_added_and_skipped.code} + + + #{initial_admin_contact.code} + #{initial_tech_contact.code} + + + + + + #{'test' * 2000} + + + + + XML + + post epp_update_path(domain_name: domain.name), params: { frame: request_xml }, headers: { 'HTTP_COOKIE' => "session=api_bestnames" } + domain.reload + + assert_epp_response :completed_successfully + assert response.body.include? "Admin contact #{new_admin_being_added.code} was discarded as duplicate;" + end + private def assert_verification_and_notification_emails From 69dca2ca731be753e1ba96c051f773a47d6de7ed Mon Sep 17 00:00:00 2001 From: oleghasjanov Date: Fri, 16 May 2025 15:39:15 +0300 Subject: [PATCH 2/4] fixed test --- test/integration/epp/domain/update/base_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/epp/domain/update/base_test.rb b/test/integration/epp/domain/update/base_test.rb index 205176036..70b1fd355 100644 --- a/test/integration/epp/domain/update/base_test.rb +++ b/test/integration/epp/domain/update/base_test.rb @@ -1311,7 +1311,7 @@ class EppDomainUpdateBaseTest < EppTestCase domain.reload assert_epp_response :completed_successfully - assert response.body.include? "Admin contact #{new_admin_being_added.code} was discarded as duplicate;" + assert response.body.include? "Tech contact #{new_tech_being_added_and_skipped.code} was discarded as duplicate;" end private From 5224824a92a02aaff81c50148a7789f715a88116 Mon Sep 17 00:00:00 2001 From: oleghasjanov Date: Mon, 4 Aug 2025 11:07:02 +0300 Subject: [PATCH 3/4] adede test --- .../repp/v1/domains/contacts_test.rb | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/integration/repp/v1/domains/contacts_test.rb b/test/integration/repp/v1/domains/contacts_test.rb index df472bad4..905a68beb 100644 --- a/test/integration/repp/v1/domains/contacts_test.rb +++ b/test/integration/repp/v1/domains/contacts_test.rb @@ -240,4 +240,47 @@ class ReppV1DomainsContactsTest < ActionDispatch::IntegrationTest assert_equal 1000, json[:code] assert_empty @domain.admin_contacts end + + def test_updates_add_duplicate_admin_contact + domain = domains(:shop) + + duplicate_contact = Contact.create!( + name: 'Partial Duplicate Test', + code: 'duplicate-006', + email: 'partial-duplicate@test.com', + phone: '+123.6789012', + ident: '12349X', + ident_type: 'priv', + ident_country_code: 'US', + registrar: registrars(:bestnames) + ) + + registrant = duplicate_contact.becomes(Registrant) + + new_admin = Contact.create!( + name: duplicate_contact.name, + code: 'duplicate-admin-006', + email: duplicate_contact.email, + phone: duplicate_contact.phone, + ident: duplicate_contact.ident, + ident_type: 'priv', + ident_country_code: duplicate_contact.ident_country_code, + registrar: registrars(:bestnames) + ) + + domain.update(registrant: registrant) && domain.reload + + old_admin = domain.admin_contacts.first + assert_includes domain.admin_contacts, old_admin + + payload = { contacts: [ { code: new_admin.code, type: 'admin' } ] } + post "/repp/v1/domains/#{domain.name}/contacts", headers: @auth_headers, params: payload + json = JSON.parse(response.body, symbolize_names: true) + + assert_response :ok + assert_equal 1000, json[:code] + + domain.reload + assert domain.admin_contacts.find_by(code: new_admin.code).present? + end end From df8a76637cbe2ba575de0f58380ba2e9e715760a Mon Sep 17 00:00:00 2001 From: oleghasjanov Date: Mon, 11 Aug 2025 15:31:46 +0300 Subject: [PATCH 4/4] 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