diff --git a/.reek b/.reek index 6cdd58c75..0d5c66aa3 100644 --- a/.reek +++ b/.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 diff --git a/app/models/concerns/contact/identical.rb b/app/models/concerns/contact/identical.rb new file mode 100644 index 000000000..f529e09ac --- /dev/null +++ b/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 diff --git a/app/models/concerns/contact/transferable.rb b/app/models/concerns/contact/transferable.rb index 8da98fc57..14c1cac3c 100644 --- a/app/models/concerns/contact/transferable.rb +++ b/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 diff --git a/app/models/concerns/domain/transferable.rb b/app/models/concerns/domain/transferable.rb index f2e7736c2..56e77f34d 100644 --- a/app/models/concerns/domain/transferable.rb +++ b/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) diff --git a/app/models/contact.rb b/app/models/contact.rb index 10b71b55b..d54e7296d 100644 --- a/app/models/contact.rb +++ b/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 diff --git a/app/presenters/registrant_presenter.rb b/app/presenters/registrant_presenter.rb index 148c5d219..6a78f86c5 100644 --- a/app/presenters/registrant_presenter.rb +++ b/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:) diff --git a/app/views/mailers/contact_mailer/email_updated.html.erb b/app/views/mailers/contact_mailer/email_updated.html.erb index a4f6d8583..c3a22b063 100644 --- a/app/views/mailers/contact_mailer/email_updated.html.erb +++ b/app/views/mailers/contact_mailer/email_updated.html.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, line_break: '
') %> <% 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(line_break: '
') %> <% end %> diff --git a/app/views/mailers/contact_mailer/email_updated.text.erb b/app/views/mailers/contact_mailer/email_updated.text.erb index 97e46a5eb..d847f9f57 100644 --- a/app/views/mailers/contact_mailer/email_updated.text.erb +++ b/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 %> diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb index 1703bf90a..875042a27 100644 --- a/spec/models/contact_spec.rb +++ b/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 } diff --git a/spec/presenters/registrant_presenter_spec.rb b/spec/presenters/registrant_presenter_spec.rb index f9d112f1a..8c7c7b6d3 100644 --- a/spec/presenters/registrant_presenter_spec.rb +++ b/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| diff --git a/test/fixtures/contacts.yml b/test/fixtures/contacts.yml index fe11968b9..50befdcf8 100644 --- a/test/fixtures/contacts.yml +++ b/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 diff --git a/test/integration/api/domain_transfers_test.rb b/test/integration/api/domain_transfers_test.rb index b90b59be4..08ce2e34c 100644 --- a/test/integration/api/domain_transfers_test.rb +++ b/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 diff --git a/test/integration/epp/domain/transfer/request_test.rb b/test/integration/epp/domain/transfer/request_test.rb index 03c5e7daf..508ec4334 100644 --- a/test/integration/epp/domain/transfer/request_test.rb +++ b/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 diff --git a/test/models/contact/contact_test.rb b/test/models/contact/contact_test.rb index ef958e2a4..286ded534 100644 --- a/test/models/contact/contact_test.rb +++ b/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 diff --git a/test/models/contact/identical_test.rb b/test/models/contact/identical_test.rb new file mode 100644 index 000000000..61d0946bc --- /dev/null +++ b/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 diff --git a/test/models/contact/contact_transfer_test.rb b/test/models/contact/transfer_test.rb similarity index 81% rename from test/models/contact/contact_transfer_test.rb rename to test/models/contact/transfer_test.rb index 7346a6375..a695c112a 100644 --- a/test/models/contact/contact_transfer_test.rb +++ b/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 diff --git a/test/models/domain/transferable_test.rb b/test/models/domain/transferable_test.rb index 42613aa45..35bf5f96b 100644 --- a/test/models/domain/transferable_test.rb +++ b/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