fixed duplicate contacts during update

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.code == contact2.code ||
-        contact1.ident == contact2.ident &&
+        (contact1.name == contact2.name &&
-        contact1.email == contact2.email &&
+         contact1.ident == contact2.ident &&
-        contact1.phone == contact2.phone
+         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
+        # Filter duplicates before assignment
-        check_for_same_contacts(props, 'admin')
+        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
+        # Filter duplicates before assignment
-        check_for_same_contacts(props, 'tech')
+        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
 

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