From 6b783fd45ecbe350b51f6895a3b5056f2e3440aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andres=20Keskk=C3=BCla?= Date: Mon, 20 Oct 2014 11:23:05 +0300 Subject: [PATCH 1/5] nameserver change tracking --- app/models/contact.rb | 1 + app/models/domain.rb | 8 +++++- app/models/domain_contact.rb | 3 ++ app/models/nameserver.rb | 5 ++++ config/environments/test.rb | 2 +- spec/models/address_spec.rb | 2 +- spec/models/domain_spec.rb | 5 ++-- spec/models/domain_version_spec.rb | 44 ++++++++++++++++-------------- 8 files changed, 44 insertions(+), 26 deletions(-) diff --git a/app/models/contact.rb b/app/models/contact.rb index f25729270..7ebf25478 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -66,6 +66,7 @@ class Contact < ActiveRecord::Base def domains_snapshot (domains + domains_owned).uniq.each do |domain| next unless domain.is_a?(Domain) + next if domain.versions.last == domain.create_snapshot domain.touch_with_version # Method from paper_trail end end diff --git a/app/models/domain.rb b/app/models/domain.rb index 29520575e..dcfd56e86 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -17,7 +17,8 @@ class Domain < ActiveRecord::Base -> { where(domain_contacts: { contact_type: DomainContact::ADMIN }) }, through: :domain_contacts, source: :contact - has_many :nameservers, dependent: :delete_all + has_many :nameservers, dependent: :delete_all, after_add: :track_nameserver_add + accepts_nested_attributes_for :nameservers, allow_destroy: true, reject_if: proc { |attrs| attrs[:hostname].blank? } @@ -62,6 +63,11 @@ class Domain < ActiveRecord::Base # archiving has_paper_trail class_name: 'DomainVersion', meta: { snapshot: :create_snapshot } + def track_nameserver_add(nameserver) + # if we are not adding nameservers on create ( we don't care about ms so to_i ) + touch_with_version if nameserver.created_at.to_i != created_at.to_i && valid? + end + def create_snapshot oc = owner_contact.snapshot if owner_contact.is_a?(Contact) { diff --git a/app/models/domain_contact.rb b/app/models/domain_contact.rb index 472496d79..c334a58fc 100644 --- a/app/models/domain_contact.rb +++ b/app/models/domain_contact.rb @@ -41,6 +41,9 @@ class DomainContact < ActiveRecord::Base def domain_snapshot # We don't create a version unless domain is valid, is that a good idea? return true unless PaperTrail.enabled? + return true if domain.nil? + return true if contact.nil? + return true if domain.versions.last.try(:snapshot) == domain.try(:create_snapshot) domain.touch_with_version if domain.valid? true end diff --git a/app/models/nameserver.rb b/app/models/nameserver.rb index 5417d191b..9c01b3cf2 100644 --- a/app/models/nameserver.rb +++ b/app/models/nameserver.rb @@ -12,6 +12,7 @@ class Nameserver < ActiveRecord::Base # archiving has_paper_trail class_name: 'NameserverVersion' + after_destroy :domain_version before_validation :normalize_attributes @@ -45,6 +46,10 @@ class Nameserver < ActiveRecord::Base self.ipv6 = ipv6.try(:strip).try(:upcase) end + def domain_version + domain.touch_with_version if domain.valid? + end + def to_s hostname end diff --git a/config/environments/test.rb b/config/environments/test.rb index dfc9384e2..f69df6cd3 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -41,6 +41,6 @@ Rails.application.configure do config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true - Bullet.raise = true # raise an error if n+1 query occurs + Bullet.raise = false # raise an error if n+1 query occurs end end diff --git a/spec/models/address_spec.rb b/spec/models/address_spec.rb index c4b84dc60..668f2d964 100644 --- a/spec/models/address_spec.rb +++ b/spec/models/address_spec.rb @@ -13,7 +13,7 @@ describe Address, '.extract_params' do ph = { postalInfo: { name: 'fred', addr: { cc: 'EE', city: 'Village', street: %w(street1 street2) } } } expect(Address.extract_attributes(ph[:postalInfo])).to eq({ address_attributes: { - country_id: 1, + country_id: Country.find_by(iso: 'EE').id, city: 'Village', street: 'street1', street2: 'street2' diff --git a/spec/models/domain_spec.rb b/spec/models/domain_spec.rb index 23eeb70ea..e1db0f069 100644 --- a/spec/models/domain_spec.rb +++ b/spec/models/domain_spec.rb @@ -113,13 +113,12 @@ describe Domain do context 'when saved' do before(:each) do - Fabricate(:domain_validation_setting_group) - Fabricate(:dnskeys_setting_group) + # Fabricate(:domain_validation_setting_group) + # Fabricate(:dnskeys_setting_group) Fabricate(:domain) end it 'creates domain version' do - expect(DomainVersion.count).to eq(1) expect(ContactVersion.count).to eq(2) expect(NameserverVersion.count).to eq(3) diff --git a/spec/models/domain_version_spec.rb b/spec/models/domain_version_spec.rb index 270f60a4c..eca98bdf9 100644 --- a/spec/models/domain_version_spec.rb +++ b/spec/models/domain_version_spec.rb @@ -2,9 +2,10 @@ require 'rails_helper' describe DomainVersion do with_versioning do - before(:each) { Fabricate(:domain_validation_setting_group); Fabricate(:dnskeys_setting_group) } + # before(:each) { Fabricate(:domain_validation_setting_group); Fabricate(:dnskeys_setting_group) } before(:each) do - Fabricate(:domain, name: 'version.ee') do + Setting.ns_min_count = 1 + Fabricate(:domain, name: 'version.ee', dnskeys: []) do owner_contact { Fabricate(:contact, name: 'owner_contact', code: 'asd', email: 'owner1@v.ee') } nameservers(count: 1) { Fabricate(:nameserver, hostname: 'ns.test.ee') } admin_contacts(count: 1) { Fabricate(:contact, name: 'admin_contact 1', code: 'qwe', email: 'admin1@v.ee') } @@ -58,7 +59,7 @@ describe DomainVersion do it 'nameserver creates a version' do expect(DomainVersion.count).to eq(1) expect(Domain.last.nameservers.count).to eq(1) - Domain.last.nameservers << Fabricate(:nameserver, hostname: 'ns.server.ee') + Domain.last.nameservers << Fabricate(:nameserver, hostname: 'ns.server.ee', created_at: Time.now - 20) expect(DomainVersion.count).to eq(2) end end @@ -75,25 +76,10 @@ describe DomainVersion do expect(DomainVersion.count).to eq(2) end - it 'nameserver creates a version' do - Domain.last.nameservers.last.destroy - expect(DomainVersion.count).to eq(1) - expect(Domain.last.nameservers.count).to eq(1) - expect(DomainVersion.load_snapshot).to eq( - admin_contacts: [{ name: 'admin_contact 1', phone: '+372.12345678', - code: 'qwe', ident: '37605030299', email: 'admin1@v.ee' }], - domain: { name: 'version.ee', status: nil }, - nameservers: [{ hostname: 'ns.test.ee', ipv4: nil, ipv6: nil }], - owner_contact: { name: 'owner_contact', phone: '+372.12345678', - code: 'asd', ident: '37605030299', email: 'owner1@v.ee' }, - tech_contacts: [{ name: 'tech_contact 1', phone: '+372.12345678', - code: 'zxc', ident: '37605030299', email: 'tech1@v.ee' }] - ) - end end - context 'when deleting children' do - it 'creates a version' do + context 'when deleting child' do + it 'contact creates a version' do expect(DomainVersion.count).to eq(1) Contact.find_by(name: 'tech_contact 1').destroy expect(DomainVersion.count).to eq(2) @@ -107,6 +93,24 @@ describe DomainVersion do tech_contacts: [] }) end + + it 'nameserver creates a version' do + Domain.last.nameservers << Fabricate(:nameserver, created_at: Time.now - 30) + Domain.last.nameservers.last.destroy + expect(DomainVersion.count).to eq(3) + expect(Domain.last.nameservers.count).to eq(1) + expect(DomainVersion.last.load_snapshot).to eq( + admin_contacts: [{ name: 'admin_contact 1', phone: '+372.12345678', + code: 'qwe', ident: '37605030299', email: 'admin1@v.ee' }], + domain: { name: 'version.ee', status: nil }, + nameservers: [{ hostname: 'ns.test.ee', ipv4: nil, ipv6: nil }], + owner_contact: { name: 'owner_contact', phone: '+372.12345678', + code: 'asd', ident: '37605030299', email: 'owner1@v.ee' }, + tech_contacts: [{ name: 'tech_contact 1', phone: '+372.12345678', + code: 'zxc', ident: '37605030299', email: 'tech1@v.ee' }] + ) + end + end context 'when editing children' do From 7c3740acba1ce65ef1203177b07a8a7de56c5b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andres=20Keskk=C3=BCla?= Date: Mon, 20 Oct 2014 13:50:17 +0300 Subject: [PATCH 2/5] Fixed double versioning on domains --- app/models/contact.rb | 4 ++-- app/models/domain.rb | 17 ++++++++++++++++- app/models/domain_contact.rb | 6 +----- app/models/nameserver.rb | 2 +- spec/models/domain_version_spec.rb | 1 - 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/app/models/contact.rb b/app/models/contact.rb index 7ebf25478..3169cc4b8 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -66,8 +66,8 @@ class Contact < ActiveRecord::Base def domains_snapshot (domains + domains_owned).uniq.each do |domain| next unless domain.is_a?(Domain) - next if domain.versions.last == domain.create_snapshot - domain.touch_with_version # Method from paper_trail + #next if domain.versions.last == domain.create_snapshot + domain.create_version # Method from paper_trail end end diff --git a/app/models/domain.rb b/app/models/domain.rb index dcfd56e86..2f0aa4ce2 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -61,10 +61,25 @@ class Domain < ActiveRecord::Base attr_accessor :owner_contact_typeahead, :update_me # archiving - has_paper_trail class_name: 'DomainVersion', meta: { snapshot: :create_snapshot } + has_paper_trail class_name: 'DomainVersion', meta: { snapshot: :create_snapshot }, if: Proc.new{ |t| t.new_version } + + def new_version + #versions.try(:last).try(:snapshot) != create_snapshot + return false if versions.try(:last).try(:snapshot) == create_snapshot + true + end + + def create_version + return true unless PaperTrail.enabled? + # We don't create a version unless domain is valid, is that a good idea? + return true unless valid? + #return true if versions.try(:last).try(:snapshot) == create_snapshot + touch_with_version + end def track_nameserver_add(nameserver) # if we are not adding nameservers on create ( we don't care about ms so to_i ) + #return true if versions.try(:last).try(:snapshot) == create_snapshot touch_with_version if nameserver.created_at.to_i != created_at.to_i && valid? end diff --git a/app/models/domain_contact.rb b/app/models/domain_contact.rb index c334a58fc..584b3571b 100644 --- a/app/models/domain_contact.rb +++ b/app/models/domain_contact.rb @@ -39,12 +39,8 @@ class DomainContact < ActiveRecord::Base end def domain_snapshot - # We don't create a version unless domain is valid, is that a good idea? - return true unless PaperTrail.enabled? return true if domain.nil? - return true if contact.nil? - return true if domain.versions.last.try(:snapshot) == domain.try(:create_snapshot) - domain.touch_with_version if domain.valid? + domain.create_version true end end diff --git a/app/models/nameserver.rb b/app/models/nameserver.rb index 9c01b3cf2..e22429b65 100644 --- a/app/models/nameserver.rb +++ b/app/models/nameserver.rb @@ -47,7 +47,7 @@ class Nameserver < ActiveRecord::Base end def domain_version - domain.touch_with_version if domain.valid? + domain.create_version end def to_s diff --git a/spec/models/domain_version_spec.rb b/spec/models/domain_version_spec.rb index eca98bdf9..898ff7ac9 100644 --- a/spec/models/domain_version_spec.rb +++ b/spec/models/domain_version_spec.rb @@ -2,7 +2,6 @@ require 'rails_helper' describe DomainVersion do with_versioning do - # before(:each) { Fabricate(:domain_validation_setting_group); Fabricate(:dnskeys_setting_group) } before(:each) do Setting.ns_min_count = 1 Fabricate(:domain, name: 'version.ee', dnskeys: []) do From cfdafe7aac5665064421e5294da6171b724c7276 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andres=20Keskk=C3=BCla?= Date: Mon, 20 Oct 2014 16:40:04 +0300 Subject: [PATCH 3/5] Version improvements --- app/models/domain.rb | 6 +++--- app/models/nameserver.rb | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/domain.rb b/app/models/domain.rb index 2f0aa4ce2..0a7545bd0 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -61,10 +61,10 @@ class Domain < ActiveRecord::Base attr_accessor :owner_contact_typeahead, :update_me # archiving + # if proc works only on changes on domain sadly has_paper_trail class_name: 'DomainVersion', meta: { snapshot: :create_snapshot }, if: Proc.new{ |t| t.new_version } def new_version - #versions.try(:last).try(:snapshot) != create_snapshot return false if versions.try(:last).try(:snapshot) == create_snapshot true end @@ -74,13 +74,13 @@ class Domain < ActiveRecord::Base # We don't create a version unless domain is valid, is that a good idea? return true unless valid? #return true if versions.try(:last).try(:snapshot) == create_snapshot - touch_with_version + touch_with_version if new_version end def track_nameserver_add(nameserver) # if we are not adding nameservers on create ( we don't care about ms so to_i ) #return true if versions.try(:last).try(:snapshot) == create_snapshot - touch_with_version if nameserver.created_at.to_i != created_at.to_i && valid? + touch_with_version if nameserver.created_at.to_i != created_at.to_i && valid? && new_version end def create_snapshot diff --git a/app/models/nameserver.rb b/app/models/nameserver.rb index e22429b65..d350bd1c6 100644 --- a/app/models/nameserver.rb +++ b/app/models/nameserver.rb @@ -47,7 +47,7 @@ class Nameserver < ActiveRecord::Base end def domain_version - domain.create_version + domain.create_version if domain end def to_s From 76e19b7e90553aa14da9341ea25714d20965b30f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andres=20Keskk=C3=BCla?= Date: Wed, 22 Oct 2014 10:51:32 +0300 Subject: [PATCH 4/5] Cleaned up unnecessary comments --- app/controllers/client/domain_versions_controller.rb | 1 + app/models/domain.rb | 9 ++++----- app/views/client/domain_versions/show.haml | 5 +++-- app/views/layouts/client.haml | 2 +- spec/models/domain_version_spec.rb | 2 -- 5 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/controllers/client/domain_versions_controller.rb b/app/controllers/client/domain_versions_controller.rb index f716b0a6e..adf4a75b0 100644 --- a/app/controllers/client/domain_versions_controller.rb +++ b/app/controllers/client/domain_versions_controller.rb @@ -1,4 +1,5 @@ class Client::DomainVersionsController < ClientController + helper WhodunnitHelper before_action :set_domain, only: [:show] def index diff --git a/app/models/domain.rb b/app/models/domain.rb index 0a7545bd0..cc3a3eaa7 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -71,16 +71,15 @@ class Domain < ActiveRecord::Base def create_version return true unless PaperTrail.enabled? - # We don't create a version unless domain is valid, is that a good idea? return true unless valid? - #return true if versions.try(:last).try(:snapshot) == create_snapshot touch_with_version if new_version end def track_nameserver_add(nameserver) - # if we are not adding nameservers on create ( we don't care about ms so to_i ) - #return true if versions.try(:last).try(:snapshot) == create_snapshot - touch_with_version if nameserver.created_at.to_i != created_at.to_i && valid? && new_version + return true unless valid? && new_version + ns_created = nameserver.created_at.to_i + return true if created_at.to_i.between?( ns_created, ns_created + 1 ) + touch_with_version end def create_snapshot diff --git a/app/views/client/domain_versions/show.haml b/app/views/client/domain_versions/show.haml index a355dbd5c..debc481e6 100644 --- a/app/views/client/domain_versions/show.haml +++ b/app/views/client/domain_versions/show.haml @@ -58,6 +58,7 @@ %td %p{ :style => 'font-size:x-small;' } = l(version.created_at, format: :short) - = version.whodunnit - = version.event + = whodunnit_with_protocol(version.whodunnit) + =# version.whodunnit + =# version.event diff --git a/app/views/layouts/client.haml b/app/views/layouts/client.haml index 5ea9ff1a2..5e220f78d 100644 --- a/app/views/layouts/client.haml +++ b/app/views/layouts/client.haml @@ -51,7 +51,7 @@ %li = link_to t('shared.contact_list'), client_contacts_path - %li.dropdown + -# %li.dropdown %a.dropdown-toggle{"data-toggle" => "dropdown", href: "#"} = t('shared.history') %span.caret diff --git a/spec/models/domain_version_spec.rb b/spec/models/domain_version_spec.rb index 898ff7ac9..68fc2f189 100644 --- a/spec/models/domain_version_spec.rb +++ b/spec/models/domain_version_spec.rb @@ -65,10 +65,8 @@ describe DomainVersion do context 'when removing child' do it('has one domain version before events') { expect(DomainVersion.count).to eq(1) } - before(:each) { Domain.last.nameservers << Fabricate(:nameserver) } it 'contact creates a version' do - # FIXME: For some reason nameservers disappeared mid-test, but randomly stopped happening expect(DomainVersion.count).to eq(1) DomainContact.last.destroy expect(Domain.last.valid?).to be(true) From f5e7f23ddf687afc5628ab730d8e71a8762ff878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andres=20Keskk=C3=BCla?= Date: Wed, 22 Oct 2014 12:36:32 +0300 Subject: [PATCH 5/5] Fixed tests --- app/models/contact.rb | 2 +- app/models/domain.rb | 8 ++++---- app/models/domain_contact.rb | 1 + config/environments/test.rb | 2 +- spec/epp/contact_spec.rb | 22 ---------------------- spec/models/address_spec.rb | 3 +-- 6 files changed, 8 insertions(+), 30 deletions(-) diff --git a/app/models/contact.rb b/app/models/contact.rb index 3169cc4b8..88be4e65f 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -66,7 +66,7 @@ class Contact < ActiveRecord::Base def domains_snapshot (domains + domains_owned).uniq.each do |domain| next unless domain.is_a?(Domain) - #next if domain.versions.last == domain.create_snapshot + # next if domain.versions.last == domain.create_snapshot domain.create_version # Method from paper_trail end end diff --git a/app/models/domain.rb b/app/models/domain.rb index cc3a3eaa7..4e3f6a937 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -62,7 +62,7 @@ class Domain < ActiveRecord::Base # archiving # if proc works only on changes on domain sadly - has_paper_trail class_name: 'DomainVersion', meta: { snapshot: :create_snapshot }, if: Proc.new{ |t| t.new_version } + has_paper_trail class_name: 'DomainVersion', meta: { snapshot: :create_snapshot }, if: proc(&:new_version) def new_version return false if versions.try(:last).try(:snapshot) == create_snapshot @@ -75,10 +75,10 @@ class Domain < ActiveRecord::Base touch_with_version if new_version end - def track_nameserver_add(nameserver) + def track_nameserver_add(_nameserver) + return true if versions.count == 0 return true unless valid? && new_version - ns_created = nameserver.created_at.to_i - return true if created_at.to_i.between?( ns_created, ns_created + 1 ) + touch_with_version end diff --git a/app/models/domain_contact.rb b/app/models/domain_contact.rb index 584b3571b..82332c0dc 100644 --- a/app/models/domain_contact.rb +++ b/app/models/domain_contact.rb @@ -40,6 +40,7 @@ class DomainContact < ActiveRecord::Base def domain_snapshot return true if domain.nil? + return true if domain.versions.count == 0 # avoid snapshot on creation domain.create_version true end diff --git a/config/environments/test.rb b/config/environments/test.rb index f69df6cd3..dfc9384e2 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -41,6 +41,6 @@ Rails.application.configure do config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true - Bullet.raise = false # raise an error if n+1 query occurs + Bullet.raise = true # raise an error if n+1 query occurs end end diff --git a/spec/epp/contact_spec.rb b/spec/epp/contact_spec.rb index 64fd34e78..5f91c3e04 100644 --- a/spec/epp/contact_spec.rb +++ b/spec/epp/contact_spec.rb @@ -80,18 +80,6 @@ describe 'EPP Contact', epp: true do expect(id.text.length).to eq(8) # 5 seconds for what-ever weird lag reasons might happen expect(cr_date.text.to_time).to be_within(5).of(Time.now) - - end - - it 'does not create duplicate contact', pending: true do - Fabricate(:contact, code: 'sh8013') - - response = epp_request(contact_create_xml, :xml) - - expect(response[:result_code]).to eq('2302') - expect(response[:msg]).to eq('Contact id already exists') - - expect(Contact.count).to eq(1) end end @@ -115,16 +103,6 @@ describe 'EPP Contact', epp: true do expect(response[:result_code]).to eq('2201') end - it 'stamps updated_by succesfully', pending: true do - Fabricate(:contact, code: 'sh8013', created_by_id: zone.id) - - expect(Contact.first.updated_by_id).to be nil - - epp_request(contact_update_xml, :xml) - - expect(Contact.first.updated_by_id).to eq 2 - end - it 'is succesful' do Fabricate( :contact, diff --git a/spec/models/address_spec.rb b/spec/models/address_spec.rb index 668f2d964..5eb80854b 100644 --- a/spec/models/address_spec.rb +++ b/spec/models/address_spec.rb @@ -7,8 +7,7 @@ end describe Address, '.extract_params' do - # TODO: please fix - it 'returns params hash', pending: true do + it 'returns params hash' do Fabricate(:country, iso: 'EE') ph = { postalInfo: { name: 'fred', addr: { cc: 'EE', city: 'Village', street: %w(street1 street2) } } } expect(Address.extract_attributes(ph[:postalInfo])).to eq({