Merge pull request #758 from internetee/registry-746

Registry 746

.reek

@@ -1066,7 +1066,6 @@ Attribute:
   - BankStatement#th6_file
   - Versions#version_loader
   - Contact#deliver_emails
-  - Contact#domains_present
   - Contact#legal_document_id
   - Counter#value
   - Deposit#amount

app/models/concerns/contact/identical.rb

@@ -0,0 +1,34 @@
+module Concerns::Contact::Identical
+  extend ActiveSupport::Concern
+
+  IDENTIFIABLE_ATTRIBUTES = %w[
+    name
+    email
+    phone
+    fax
+    ident
+    ident_type
+    ident_country_code
+    org_name
+  ]
+  private_constant :IDENTIFIABLE_ATTRIBUTES
+
+  def identical(registrar)
+    self.class.where(identifiable_hash)
+      .where(["statuses = ?::character varying[]", "{#{read_attribute(:statuses).join(',')}}"])
+      .where(registrar: registrar)
+      .where.not(id: id).take
+  end
+
+  private
+
+  def identifiable_hash
+    attributes = IDENTIFIABLE_ATTRIBUTES
+
+    if self.class.address_processing?
+      attributes += self.class.address_attribute_names
+    end
+
+    slice(*attributes)
+  end
+end

app/models/concerns/contact/transferable.rb

@@ -7,6 +7,8 @@ module Concerns::Contact::Transferable
   end
 
   def transfer(new_registrar)
+    return identical(new_registrar) if identical(new_registrar)
+
     new_contact = self.dup
     new_contact.registrar = new_registrar
     new_contact.original = self

app/models/concerns/domain/transferable.rb

@@ -52,7 +52,7 @@ module Concerns::Domain::Transferable
 
   def transfer_registrant(new_registrar)
     return if registrant.registrar == new_registrar
-    self.registrant = registrant.transfer(new_registrar)
+    self.registrant = registrant.transfer(new_registrar).becomes(Registrant)
   end
 
   def transfer_domain_contacts(new_registrar)

app/models/contact.rb

@@ -3,6 +3,7 @@ class Contact < ActiveRecord::Base
   include EppErrors
   include UserEvents
   include Concerns::Contact::Transferable
+  include Concerns::Contact::Identical
 
   belongs_to :original, class_name: self.name
   belongs_to :registrar, required: true
@@ -70,11 +71,6 @@ class Contact < ActiveRecord::Base
 
   after_save :update_related_whois_records
 
-  # for overwrite when doing children loop
-  attr_writer :domains_present
-
-  scope :current_registrars, ->(id) { where(registrar_id: id) }
-
   ORG = 'org'
   PRIV = 'priv'
   BIRTHDAY = 'birthday'.freeze
@@ -206,7 +202,7 @@ class Contact < ActiveRecord::Base
           ver_scope << "(children->'#{type}')::jsonb <@ json_build_array(#{contact.id})::jsonb"
         end
         next if DomainVersion.where("created_at > ?", Time.now - Setting.orphans_contacts_in_months.to_i.months).where(ver_scope.join(" OR ")).any?
-        next if contact.domains_present?
+        next if contact.in_use?
 
         contact.destroy
         counter.next
@@ -279,7 +275,7 @@ class Contact < ActiveRecord::Base
     calculated.delete(Contact::OK)
     calculated.delete(Contact::LINKED)
     calculated << Contact::OK     if calculated.empty?# && valid?
-    calculated << Contact::LINKED if domains_present?
+    calculated << Contact::LINKED if in_use?
 
     calculated.uniq
   end
@@ -347,7 +343,7 @@ class Contact < ActiveRecord::Base
   # no need separate method
   # should use only in transaction
   def destroy_and_clean frame
-    if domains_present?
+    if in_use?
       errors.add(:domains, :exist)
       return false
     end
@@ -405,14 +401,6 @@ class Contact < ActiveRecord::Base
     end
   end
 
-  # optimization under children loop,
-  # otherwise bullet will not be happy
-  def domains_present?
-    return @domains_present if @domains_present
-    domain_contacts.present? || registrant_domains.present?
-  end
-
-
   def search_name
     "#{code} #{name}"
   end
@@ -551,7 +539,7 @@ class Contact < ActiveRecord::Base
     Country.new(ident_country_code)
   end
 
-  def used?
+  def in_use?
     registrant_domains.any? || domain_contacts.any?
   end
 

app/presenters/registrant_presenter.rb

@@ -8,7 +8,7 @@ class RegistrantPresenter
            :reg_no,
            :street, :city, :state, :zip, :country,
            :ident_country,
-           :used?,
+           :in_use?,
            to: :registrant
 
   def initialize(registrant:, view:)

app/views/mailers/contact_mailer/email_updated.html.erb

@@ -10,7 +10,7 @@ uus aadress: <%= contact.email %>
 <br><br>
 E-posti aadressile saadetakse domeeni toimingutega seotud infot, sealhulgas kinnitustaotlused omanikuvahetuse ja domeeni kustutamise korral.
 <br><br>
-<% if contact.used? %>
+<% if contact.in_use? %>
     Muudatusega seotud domeenid:<br>
     <%= contact.domain_names_with_roles(locale: :et, line_break: '<br>') %>
 <% end %>
@@ -34,7 +34,7 @@ new address: <%= contact.email %>
 <br><br>
 E-mail addresses are used to send important information regarding your registered domains including applications for approval of registrant change and domain deletion. Please make sure that the update and contact information are correct.
 <br><br>
-<% if contact.used? %>
+<% if contact.in_use? %>
     Domains affected by this update:<br>
     <%= contact.domain_names_with_roles(line_break: '<br>') %>
 <% end %>

app/views/mailers/contact_mailer/email_updated.text.erb

@@ -10,7 +10,7 @@ uus aadress: <%= contact.email %>
 
 E-posti aadressile saadetakse domeeni toimingutega seotud infot, sealhulgas kinnitustaotlused omanikuvahetuse ja domeeni kustutamise korral.
 
-<% if contact.used? %>
+<% if contact.in_use? %>
     Muudatusega seotud domeenid:
     <%= contact.domain_names_with_roles(locale: :et) %>
 <% end %>
@@ -34,7 +34,7 @@ new address: <%= contact.email %>
 
 E-mail addresses are used to send important information regarding your registered domains including applications for approval of registrant change and domain deletion. Please make sure that the update and contact information are correct.
 
-<% if contact.used? %>
+<% if contact.in_use? %>
     Domains affected by this update:
     <%= contact.domain_names_with_roles %>
 <% end %>

spec/models/contact_spec.rb

@@ -76,10 +76,6 @@ RSpec.describe Contact do
       end
     end
 
-    it 'should not have relation with domains' do
-      @contact.domains_present?.should == false
-    end
-
     it 'should not overwrite code' do
       old_code = @contact.code
       @contact.code = 'CID:REG1:should-not-overwrite-old-code-12345'
@@ -417,36 +413,6 @@ RSpec.describe Contact do
     end
   end
 
-  describe '#used?' do
-    context 'when used as registrant' do
-      let(:registrant) { create(:registrant) }
-
-      before :example do
-        create(:domain, registrant: registrant)
-        registrant.reload
-      end
-
-      specify { expect(registrant).to be_used }
-    end
-
-    context 'when used as contact' do
-      let(:contact) { create(:contact) }
-
-      before :example do
-        domain = create(:domain)
-        domain.admin_domain_contacts << create(:admin_domain_contact, contact: contact)
-        contact.reload
-      end
-
-      specify { expect(contact).to be_used }
-    end
-
-    context 'when not used' do
-      let(:contact) { create(:contact) }
-      specify { expect(contact).to_not be_used }
-    end
-  end
-
   describe '#domain_names_with_roles' do
     let(:contact) { create(:registrant) }
     subject(:domain_names) { contact.domain_names_with_roles }

spec/presenters/registrant_presenter_spec.rb

@@ -67,7 +67,7 @@ RSpec.describe RegistrantPresenter do
     zip
     id_code
     reg_no
-    used?
+    in_use?
   )
 
   registrant_delegatable_attributes.each do |attr_name|

test/fixtures/contacts.yml

@@ -9,16 +9,24 @@ john:
   code: john-001
   auth_info: cacb5b
 
-william:
+william: &william
   name: William
   email: william@inbox.test
   phone: '+555.555'
+  fax: +555.555
   ident: 1234
   ident_type: priv
   ident_country_code: US
   registrar: bestnames
   code: william-001
   auth_info: 6573d0
+  street: Main Street
+  zip: 12345
+  city: New York
+  state: New York
+  country_code: US
+  statuses:
+    - ok
 
 jane:
   name: Jane
@@ -53,6 +61,19 @@ jack:
   code: jack-001
   auth_info: e2c440
 
+identical_to_william:
+  <<: *william
+  registrar: goodnames
+  code: william-002
+  auth_info: 5ab865
+
+not_in_use:
+  name: Useless
+  email: useless@inbox.test
+  registrar: bestnames
+  code: useless-001
+  auth_info: e75a2a
+
 invalid:
   name: any
   code: any

test/integration/api/domain_transfers_test.rb

@@ -3,6 +3,7 @@ require 'test_helper'
 class APIDomainTransfersTest < ActionDispatch::IntegrationTest
   def setup
     @domain = domains(:shop)
+    @new_registrar = registrars(:goodnames)
     Setting.transfer_wait_time = 0 # Auto-approval
   end
 
@@ -29,10 +30,10 @@ class APIDomainTransfersTest < ActionDispatch::IntegrationTest
     assert @domain.transfers.last.approved?
   end
 
-  def test_changes_registrar
+  def test_assigns_new_registrar
     post '/repp/v1/domain_transfers', request_params, { 'HTTP_AUTHORIZATION' => http_auth_key }
     @domain.reload
-    assert_equal registrars(:goodnames), @domain.registrar
+    assert_equal @new_registrar, @domain.registrar
   end
 
   def test_regenerates_transfer_code
@@ -52,11 +53,16 @@ class APIDomainTransfersTest < ActionDispatch::IntegrationTest
   end
 
   def test_duplicates_registrant_admin_and_tech_contacts
-    assert_difference 'Contact.count', 3 do
+    assert_difference -> { @new_registrar.contacts.size }, 2 do
       post '/repp/v1/domain_transfers', request_params, { 'HTTP_AUTHORIZATION' => http_auth_key }
     end
   end
 
+  def test_reuses_identical_contact
+    post '/repp/v1/domain_transfers', request_params, { 'HTTP_AUTHORIZATION' => http_auth_key }
+    assert_equal 1, @new_registrar.contacts.where(name: 'William').size
+  end
+
   def test_fails_if_domain_does_not_exist
     request_params = { format: :json,
                        data: { domainTransfers: [{ domainName: 'non-existent.test', transferCode: 'any' }] } }
@@ -71,7 +77,7 @@ class APIDomainTransfersTest < ActionDispatch::IntegrationTest
                        data: { domainTransfers: [{ domainName: 'shop.test', transferCode: 'wrong' }] } }
     post '/repp/v1/domain_transfers', request_params, { 'HTTP_AUTHORIZATION' => http_auth_key }
     assert_response 400
-    refute_equal registrars(:goodnames), @domain.registrar
+    refute_equal @new_registrar, @domain.registrar
     assert_equal ({ errors: [{ title: 'shop.test transfer code is wrong' }] }),
                  JSON.parse(response.body, symbolize_names: true)
   end

test/integration/epp/domain/transfer/request_test.rb

@@ -3,6 +3,7 @@ require 'test_helper'
 class EppDomainTransferRequestTest < ActionDispatch::IntegrationTest
   def setup
     @domain = domains(:shop)
+    @new_registrar = registrars(:goodnames)
     Setting.transfer_wait_time = 0
   end
 
@@ -24,10 +25,10 @@ class EppDomainTransferRequestTest < ActionDispatch::IntegrationTest
       'https://epp.tld.ee/schema/domain-eis-1.0.xsd').text
   end
 
-  def test_changes_registrar
+  def test_assigns_new_registrar
     post '/epp/command/transfer', { frame: request_xml }, { 'HTTP_COOKIE' => 'session=api_goodnames' }
     @domain.reload
-    assert_equal registrars(:goodnames), @domain.registrar
+    assert_equal @new_registrar, @domain.registrar
   end
 
   def test_regenerates_transfer_code
@@ -48,11 +49,16 @@ class EppDomainTransferRequestTest < ActionDispatch::IntegrationTest
   end
 
   def test_duplicates_registrant_admin_and_tech_contacts
-    assert_difference 'Contact.count', 3 do
+    assert_difference -> { @new_registrar.contacts.size }, 2 do
       post '/epp/command/transfer', { frame: request_xml }, { 'HTTP_COOKIE' => 'session=api_goodnames' }
     end
   end
 
+  def test_reuses_identical_contact
+    post '/epp/command/transfer', { frame: request_xml }, { 'HTTP_COOKIE' => 'session=api_goodnames' }
+    assert_equal 1, @new_registrar.contacts.where(name: 'William').size
+  end
+
   def test_saves_legal_document
     assert_difference -> { @domain.legal_documents(true).size } do
       post '/epp/command/transfer', { frame: request_xml }, { 'HTTP_COOKIE' => 'session=api_goodnames' }
@@ -106,7 +112,7 @@ class EppDomainTransferRequestTest < ActionDispatch::IntegrationTest
 
     post '/epp/command/transfer', { frame: request_xml }, { 'HTTP_COOKIE' => 'session=api_goodnames' }
     @domain.reload
-    refute_equal registrars(:goodnames), @domain.registrar
+    refute_equal @new_registrar, @domain.registrar
     assert_equal '2201', Nokogiri::XML(response.body).at_css('result')[:code]
   end
 

test/models/contact/contact_test.rb

@@ -12,4 +12,18 @@ class ContactTest < ActiveSupport::TestCase
   def test_invalid_fixture_is_invalid
     assert contacts(:invalid).invalid?
   end
+
+  def test_in_use_if_acts_as_a_registrant
+    DomainContact.delete_all
+    assert @contact.in_use?
+  end
+
+  def test_in_use_if_acts_as_a_domain_contact
+    Domain.update_all(registrant_id: contacts(:william))
+    assert @contact.in_use?
+  end
+
+  def test_not_in_use_if_acts_as_neither_registrant_nor_domain_contact
+    refute contacts(:not_in_use).in_use?
+  end
 end

test/models/contact/identical_test.rb

@@ -0,0 +1,63 @@
+require 'test_helper'
+
+class ContactIdenticalTest < ActiveSupport::TestCase
+  REGULAR_FILTER_ATTRIBUTES = %i[
+    name
+    email
+    phone
+    fax
+    ident
+    ident_type
+    ident_country_code
+    org_name
+  ]
+
+  def setup
+    @contact = contacts(:william)
+    @identical = contacts(:identical_to_william)
+  end
+
+  def test_returns_identical
+    assert_equal @identical, @contact.identical(@identical.registrar)
+  end
+
+  def test_does_not_return_non_identical
+    REGULAR_FILTER_ATTRIBUTES.each do |attribute|
+      previous_value = @identical.public_send(attribute)
+      @identical.update_attribute(attribute, 'other')
+      assert_nil @contact.identical(@identical.registrar)
+      @identical.update_attribute(attribute, previous_value)
+    end
+
+    @identical.update!({ statuses: %w[ok linked] })
+    assert_nil @contact.identical(@identical.registrar)
+  end
+
+  def test_takes_address_into_account_when_processing_enabled
+    Contact.address_attribute_names.each do |attribute|
+      previous_value = @identical.public_send(attribute)
+      @identical.update_attribute(attribute, 'other')
+
+      Contact.stub :address_processing?, true do
+        assert_nil @contact.identical(@identical.registrar)
+      end
+
+      @identical.update_attribute(attribute, previous_value)
+    end
+  end
+
+  def test_ignores_address_when_processing_disabled
+    Setting.address_processing = false
+
+    Contact.address_attribute_names.each do |attribute|
+      previous_value = @identical.public_send(attribute)
+      @identical.update_attribute(attribute, 'other')
+
+      Contact.stub :address_processing?, false do
+        assert_equal @identical, @contact.identical(@identical.registrar)
+      end
+
+      @identical.update_attribute(attribute, previous_value)
+    end
+  end
+end

test/models/contact/transfer_test.rb

@@ -35,18 +35,23 @@ class ContactTransferTest < ActiveSupport::TestCase
   end
 
   def test_keeps_original_contact_untouched
-    original_hash = @contact.to_json
+    original_hash = @contact.attributes
     @contact.transfer(@new_registrar)
     @contact.reload
-    assert_equal original_hash, @contact.to_json
+    assert_equal original_hash, @contact.attributes
   end
 
   def test_creates_new_contact
-    assert_difference 'Contact.count' do
+    assert_difference -> { @new_registrar.contacts.count } do
       @contact.transfer(@new_registrar)
     end
   end
 
+  def test_reuses_identical_contact
+    identical = contacts(:identical_to_william)
+    assert_equal identical, contacts(:william).transfer(@new_registrar)
+  end
+
   def test_bypasses_validation
     @contact = contacts(:invalid)
 
@@ -55,12 +60,12 @@ class ContactTransferTest < ActiveSupport::TestCase
     end
   end
 
-  def test_changes_registrar
+  def test_assigns_new_registrar
     new_contact = @contact.transfer(@new_registrar)
     assert_equal @new_registrar, new_contact.registrar
   end
 
-  def test_links_to_original
+  def test_links_to_original_contact
     new_contact = @contact.transfer(@new_registrar)
     assert_equal @contact, new_contact.original
   end

test/models/domain/transferable_test.rb

@@ -34,7 +34,7 @@ class DomainTransferableTest < ActiveSupport::TestCase
     assert_equal '1bad4f', domain.transfer_code
   end
 
-  def test_changes_registrar
+  def test_assigns_new_registrar
     @domain.transfer(@new_registrar)
     assert_equal @new_registrar, @domain.registrar
   end