Do not make a duplicate contact from registrant

app/models/epp/domain.rb

@@ -400,41 +400,54 @@ class Epp::Domain < Domain
     add_epp_error('2303', nil, nil, I18n.t('pending_transfer_was_not_found'))
   end
 
-  def transfer_contacts(current_user)
+  # TODO: Eager load problems here. Investigate how it's possible not to query contact again
+  # Check if versioning works with update_column
+  def transfer_contacts(registrar_id)
+    transfer_owner_contact(registrar_id)
+    transfer_domain_contacts(registrar_id)
+  end
+
+  def transfer_owner_contact(registrar_id)
     is_other_domains_contact = DomainContact.where('contact_id = ? AND domain_id != ?', owner_contact_id, id).count > 0
     if owner_contact.domains_owned.count > 1 || is_other_domains_contact
-      c = Contact.find(owner_contact_id)
+      # copy contact
+      c = Contact.find(owner_contact_id) # n+1 workaround
       oc = c.deep_clone include: [:statuses, :address]
       oc.code = nil
-      oc.registrar_id = current_user.registrar_id
+      oc.registrar_id = registrar_id
       oc.save!
-
       self.owner_contact_id = oc.id
     else
       # transfer contact
-      # TODO: This is a workaround so Bullet won't complain about n+1 query
-      # The problem appears in automatic status callback when doing normal save!
-      owner_contact.update_column(:registrar_id, current_user.registrar_id)
+      owner_contact.update_column(:registrar_id, registrar_id) # n+1 workaround
     end
+  end
 
+  def transfer_domain_contacts(registrar_id)
     copied_ids = []
     contacts.each do |c|
       next if copied_ids.include?(c.id)
 
       is_other_domains_contact = DomainContact.where('contact_id = ? AND domain_id != ?', c.id, id).count > 0
+      # if contact used to be owner contact but was copied, then contact must be transferred
+      # (owner_contact_id_was != c.id)
       if c.domains.count > 1 || is_other_domains_contact
-        # create contact
-        old_contact = Contact.find(c.id)
-        oc = old_contact.deep_clone include: [:statuses, :address]
-        oc.code = nil
-        oc.registrar_id = current_user.registrar_id
-        oc.save!
+        # copy contact
+        if owner_contact_id_was == c.id # owner contact was copied previously, do not copy it again
+          oc = OpenStruct.new(id: owner_contact_id)
+        else
+          old_contact = Contact.find(c.id) # n+1 workaround
+          oc = old_contact.deep_clone include: [:statuses, :address]
+          oc.code = nil
+          oc.registrar_id = registrar_id
+          oc.save!
+        end
 
-        domain_contacts.where(contact_id: c.id).update_all({ contact_id: oc.id })
+        domain_contacts.where(contact_id: c.id).update_all({ contact_id: oc.id }) # n+1 workaround
         copied_ids << c.id
       else
         # transfer contact
-        c.update_column(:registrar_id, current_user.registrar_id)
+        c.update_column(:registrar_id, registrar_id) # n+1 workaround
       end
     end
   end
@@ -462,7 +475,7 @@ class Epp::Domain < Domain
         end
 
         if dt.approved?
-          transfer_contacts(current_user)
+          transfer_contacts(current_user.registrar_id)
           generate_auth_info
           self.registrar = current_user.registrar
         end
@@ -493,8 +506,8 @@ class Epp::Domain < Domain
           transferred_at: Time.zone.now
         )
 
+        transfer_contacts(pt.transfer_to_id)
         generate_auth_info
-
         self.registrar = pt.transfer_to
 
         attach_legal_document(self.class.parse_legal_document_from_frame(frame))

spec/epp/domain_spec.rb

@@ -1059,6 +1059,33 @@ describe 'EPP Domain', epp: true do
       original_domain_contact_count.should == DomainContact.count
     end
 
+    it 'transfers domain and references exsisting owner contact to domain contacts' do
+      d = Fabricate(:domain)
+      d.tech_contacts << domain.owner_contact
+
+      domain.tech_contacts << domain.owner_contact
+      original_owner_contact_id = domain.owner_contact_id
+
+      pw = domain.auth_info
+      xml = domain_transfer_xml({
+        name: { value: domain.name },
+        authInfo: { pw: { value: pw } }
+      })
+
+      login_as :registrar2 do
+        response = epp_plain_request(xml, :xml)
+        response[:msg].should == 'Command completed successfully'
+        response[:result_code].should == '1000'
+      end
+
+      domain.reload
+      # owner contact must be an new record
+      domain.owner_contact_id.should_not == original_owner_contact_id
+
+      # new owner contact must be a tech contact
+      domain.domain_contacts.where(contact_id: domain.owner_contact_id).count.should == 1
+    end
+
     it 'should not creates transfer without password' do
       xml = domain_transfer_xml({
         name: { value: domain.name }

spec/fabricators/contact_fabricator.rb

@@ -11,6 +11,6 @@ Fabricator(:contact) do
   registrar { Fabricate(:registrar, name: Faker::Company.name, reg_no: Faker::Company.duns_number) }
   disclosure { Fabricate(:contact_disclosure) }
   # rubocop: disable Style/SymbolProc
-  after_validation { |c| c.disable_generate_auth_info! } 
+  after_validation { |c| c.disable_generate_auth_info! }
   # rubocop: enamble Style/SymbolProc
 end