From 5c8e507b934f6dedda6c75e6314d1386d487daa1 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 1 Mar 2019 19:33:31 +0200 Subject: [PATCH 1/4] Update `paper_trail` gem to 4.2.0 Closes #426 --- Gemfile | 7 +------ Gemfile.lock | 19 +++++++------------ 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/Gemfile b/Gemfile index c4b195731..f50f6f06a 100644 --- a/Gemfile +++ b/Gemfile @@ -20,12 +20,7 @@ gem 'figaro', '1.1.1' gem 'pg', '0.19.0' gem 'ransack', '1.5.1' # for searching gem 'validates_email_format_of', '1.6.3' # validates email against RFC 2822 and RFC 3696 - -# with polymorphic fix -gem 'paper_trail', - github: 'airblade/paper_trail', - ref: 'a453811226ec4ea59753ba6b827e390ced2fc140' -# NB! if this gets upgraded, ensure Setting.reload_settings! still works correctly +gem 'paper_trail', '~> 4.0' gem 'rails-settings-cached', '0.4.1' # for settings # html-xml diff --git a/Gemfile.lock b/Gemfile.lock index 4a2cea8e5..7a3cec0f0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,13 +1,3 @@ -GIT - remote: https://github.com/airblade/paper_trail.git - revision: a453811226ec4ea59753ba6b827e390ced2fc140 - ref: a453811226ec4ea59753ba6b827e390ced2fc140 - specs: - paper_trail (4.0.0.beta3) - activerecord (>= 3.0, < 6.0) - activesupport (>= 3.0, < 6.0) - request_store (~> 1.1.0) - GIT remote: https://github.com/internetee/company_register.git revision: da7130542304fc543c90d54cd037d019a777c526 @@ -266,6 +256,10 @@ GEM nori (2.6.0) open4 (1.3.4) orm_adapter (0.5.0) + paper_trail (4.2.0) + activerecord (>= 3.0, < 6.0) + activesupport (>= 3.0, < 6.0) + request_store (~> 1.1) pdfkit (0.6.2) pg (0.19.0) polyamorous (1.3.1) @@ -323,7 +317,8 @@ GEM i18n polyamorous (~> 1.1) rdoc (4.3.0) - request_store (1.1.0) + request_store (1.4.1) + rack (>= 1.4) responders (2.4.1) actionpack (>= 4.2.0, < 6.0) railties (>= 4.2.0, < 6.0) @@ -476,7 +471,7 @@ DEPENDENCIES mina (= 0.3.1) money-rails nokogiri - paper_trail! + paper_trail (~> 4.0) pdfkit (= 0.6.2) pg (= 0.19.0) pry (= 0.10.1) From 647bf4c3976aa98b8a66a83d07a774bdf0a0c1d5 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 1 Mar 2019 19:53:37 +0200 Subject: [PATCH 2/4] Restore `versions` DB table and add learning tests --- db/migrate/20190302091059_restore_versions.rb | 15 ++++++ ...02111152_add_object_changes_to_versions.rb | 5 ++ db/structure.sql | 19 +++++++- lib/gem_ext/paper_trail.rb | 7 --- test/learning/paper_trail_test.rb | 46 +++++++++++++++++++ 5 files changed, 84 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20190302091059_restore_versions.rb create mode 100644 db/migrate/20190302111152_add_object_changes_to_versions.rb create mode 100644 test/learning/paper_trail_test.rb diff --git a/db/migrate/20190302091059_restore_versions.rb b/db/migrate/20190302091059_restore_versions.rb new file mode 100644 index 000000000..7426efe5b --- /dev/null +++ b/db/migrate/20190302091059_restore_versions.rb @@ -0,0 +1,15 @@ +class RestoreVersions < ActiveRecord::Migration + def change + drop_table :versions + + create_table :versions do |t| + t.string :item_type, :null => false + t.integer :item_id, :null => false + t.string :event, :null => false + t.string :whodunnit + t.text :object + t.datetime :created_at + end + add_index :versions, [:item_type, :item_id] + end +end diff --git a/db/migrate/20190302111152_add_object_changes_to_versions.rb b/db/migrate/20190302111152_add_object_changes_to_versions.rb new file mode 100644 index 000000000..add80bead --- /dev/null +++ b/db/migrate/20190302111152_add_object_changes_to_versions.rb @@ -0,0 +1,5 @@ +class AddObjectChangesToVersions < ActiveRecord::Migration + def change + add_column :versions, :object_changes, :jsonb + end +end diff --git a/db/structure.sql b/db/structure.sql index fdd8b19ce..0e71153c8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2363,7 +2363,13 @@ ALTER SEQUENCE public.users_id_seq OWNED BY public.users.id; CREATE TABLE public.versions ( id integer NOT NULL, - depricated_table_but_somehow_paper_trail_tests_fails_without_it text + item_type character varying NOT NULL, + item_id integer NOT NULL, + event character varying NOT NULL, + whodunnit character varying, + object text, + created_at timestamp without time zone, + object_changes jsonb ); @@ -3980,6 +3986,13 @@ CREATE INDEX index_users_on_identity_code ON public.users USING btree (identity_ CREATE INDEX index_users_on_registrar_id ON public.users USING btree (registrar_id); +-- +-- Name: index_versions_on_item_type_and_item_id; Type: INDEX; Schema: public; Owner: -; Tablespace: +-- + +CREATE INDEX index_versions_on_item_type_and_item_id ON public.versions USING btree (item_type, item_id); + + -- -- Name: index_whois_records_on_domain_id; Type: INDEX; Schema: public; Owner: -; Tablespace: -- @@ -4927,6 +4940,10 @@ INSERT INTO schema_migrations (version) VALUES ('20190102144032'); INSERT INTO schema_migrations (version) VALUES ('20190209150026'); +INSERT INTO schema_migrations (version) VALUES ('20190302091059'); + +INSERT INTO schema_migrations (version) VALUES ('20190302111152'); + INSERT INTO schema_migrations (version) VALUES ('20190311111718'); INSERT INTO schema_migrations (version) VALUES ('20190312211614'); diff --git a/lib/gem_ext/paper_trail.rb b/lib/gem_ext/paper_trail.rb index c8454e550..12a8f0c3b 100644 --- a/lib/gem_ext/paper_trail.rb +++ b/lib/gem_ext/paper_trail.rb @@ -1,10 +1,3 @@ -# the following line is required for PaperTrail >= 4.0.0 with Rails -PaperTrail::Rails::Engine.eager_load! - -PaperTrail::Version.module_eval do - self.abstract_class = true -end - # Store console and rake changes in versions if defined?(::Rails::Console) PaperTrail.whodunnit = "console-#{`whoami`.strip}" diff --git a/test/learning/paper_trail_test.rb b/test/learning/paper_trail_test.rb new file mode 100644 index 000000000..d4c76b026 --- /dev/null +++ b/test/learning/paper_trail_test.rb @@ -0,0 +1,46 @@ +require 'test_helper' + +class Post < ActiveRecord::Base + has_paper_trail +end + +class PaperTrailLearningTest < ActiveSupport::TestCase + setup do + ActiveRecord::Base.connection.create_table :posts do |t| + t.string :title + + # Otherwise `touch_with_version` fails silently + t.datetime :updated_at + end + end + + def test_returns_version_list + @record = Post.create!(title: 'any') + + assert_equal 1, @record.versions.count + assert_respond_to @record.versions.first, :item_id + end + + def test_creates_new_version_upon_update + @record = Post.create!(title: 'old title') + original_record = @record.clone + + assert_difference -> { @record.versions.size } do + @record.update!(title: 'new title') + end + version = @record.versions.last + assert_equal @record.id, version.item_id + assert_equal @record.class.name, version.item_type + assert_equal version.reify, original_record + assert_equal ['old title', 'new title'], version.object_changes['title'] + assert_equal 'update', version.event + end + + def test_touch_with_version + @record = Post.create!(title: 'any') + + assert_difference -> { @record.versions.size } do + @record.touch_with_version + end + end +end \ No newline at end of file From 6bab335f5ac84d6eab9f9244d81d0a344f9746bb Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 4 Mar 2019 15:26:25 +0200 Subject: [PATCH 3/4] Add tests --- test/fixtures/log_contacts.yml | 5 +++++ test/fixtures/log_domains.yml | 8 +++++++ test/integration/contact/audit_log_test.rb | 16 +++++++++++++ test/integration/domain/audit_log_test.rb | 26 ++++++++++++++++++++++ 4 files changed, 55 insertions(+) create mode 100644 test/fixtures/log_contacts.yml create mode 100644 test/fixtures/log_domains.yml create mode 100644 test/integration/contact/audit_log_test.rb create mode 100644 test/integration/domain/audit_log_test.rb diff --git a/test/fixtures/log_contacts.yml b/test/fixtures/log_contacts.yml new file mode 100644 index 000000000..8bcf8fa51 --- /dev/null +++ b/test/fixtures/log_contacts.yml @@ -0,0 +1,5 @@ +one: + item_id: <%= ActiveRecord::FixtureSet.identify(:john) %> + item_type: Contact + event: update + created_at: <%= Time.zone.parse('2010-07-05') %> \ No newline at end of file diff --git a/test/fixtures/log_domains.yml b/test/fixtures/log_domains.yml new file mode 100644 index 000000000..f328f902a --- /dev/null +++ b/test/fixtures/log_domains.yml @@ -0,0 +1,8 @@ +one: + item_id: <%= ActiveRecord::FixtureSet.identify(:shop) %> + item_type: Domain + event: update + object: + registrant_id: <%= ActiveRecord::FixtureSet.identify(:john) %> + updated_at: <%= Time.zone.parse('2010-07-05') %> + created_at: <%= Time.zone.parse('2010-07-05') %> \ No newline at end of file diff --git a/test/integration/contact/audit_log_test.rb b/test/integration/contact/audit_log_test.rb new file mode 100644 index 000000000..f0f6a4bf2 --- /dev/null +++ b/test/integration/contact/audit_log_test.rb @@ -0,0 +1,16 @@ +require 'test_helper' + +class ContactAuditLogTest < ActionDispatch::IntegrationTest + def test_stores_metadata + contact = contacts(:john) + + contact.legal_document_id = 1 + assert_difference 'contact.versions.count' do + contact.save! + end + + contact_version = contact.versions.last + assert_equal ({ legal_documents: [1] }).with_indifferent_access, + contact_version.children.with_indifferent_access + end +end \ No newline at end of file diff --git a/test/integration/domain/audit_log_test.rb b/test/integration/domain/audit_log_test.rb new file mode 100644 index 000000000..03446c2ef --- /dev/null +++ b/test/integration/domain/audit_log_test.rb @@ -0,0 +1,26 @@ +require 'test_helper' + +class DomainAuditLogTest < ActionDispatch::IntegrationTest + def test_stores_metadata + domain = domains(:shop) + assert_equal [contacts(:jane).id], domain.admin_contacts.ids + assert_equal [contacts(:william).id, contacts(:acme_ltd).id].sort, domain.tech_contacts.ids.sort + assert_equal [nameservers(:shop_ns1).id, nameservers(:shop_ns2).id].sort, domain.nameservers.ids + .sort + assert_equal contacts(:john).id, domain.registrant_id + + domain.legal_document_id = 1 + assert_difference 'domain.versions.count' do + domain.save! + end + + domain_version = domain.versions.last + assert_equal ({ admin_contacts: [contacts(:jane).id], + tech_contacts: [contacts(:william).id, contacts(:acme_ltd).id], + nameservers: [nameservers(:shop_ns1).id, nameservers(:shop_ns2).id], + dnskeys: [], + legal_documents: [1], + registrant: [contacts(:john).id] }).with_indifferent_access, + domain_version.children.with_indifferent_access + end +end \ No newline at end of file From 65a50a8db095412fef5a01522f17f498d6794f97 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 3 Apr 2019 20:25:40 +0300 Subject: [PATCH 4/4] Remove useless specs --- spec/models/account_spec.rb | 8 ------- spec/models/admin_user_spec.rb | 10 --------- spec/models/api_user_spec.rb | 14 ------------ spec/models/contact_spec.rb | 24 --------------------- spec/models/dnskey_spec.rb | 13 ------------ spec/models/domain_contact_spec.rb | 34 ------------------------------ spec/models/domain_spec.rb | 19 ----------------- spec/models/keyrelay_spec.rb | 14 ------------ spec/models/white_ip_spec.rb | 16 -------------- 9 files changed, 152 deletions(-) diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 83e40b86e..46cef0b09 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -1,14 +1,6 @@ require 'rails_helper' RSpec.describe Account do - it 'has versions' do - with_versioning do - price = build(:account) - price.save! - expect(price.versions.size).to be(1) - end - end - describe 'registrar validation', db: false do subject(:account) { described_class.new } diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index f25ea77bd..1cb8b5b12 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -1,16 +1,6 @@ require 'rails_helper' RSpec.describe AdminUser do - context 'with invalid attribute' do - before do - @admin_user = described_class.new - end - - it 'should not have any versions' do - @admin_user.versions.should == [] - end - end - context 'with valid attributes' do before do @admin_user = create(:admin_user) diff --git a/spec/models/api_user_spec.rb b/spec/models/api_user_spec.rb index 89feeac6d..e27cc8771 100644 --- a/spec/models/api_user_spec.rb +++ b/spec/models/api_user_spec.rb @@ -17,10 +17,6 @@ RSpec.describe ApiUser do ]) end - it 'should not have any versions' do - @api_user.versions.should == [] - end - it 'should be active by default' do @api_user.active.should == true end @@ -41,16 +37,6 @@ RSpec.describe ApiUser do @api_user.valid? @api_user.errors.full_messages.should match_array([]) end - - it 'should have one version' do - with_versioning do - @api_user.versions.should == [] - @api_user.username = 'newusername' - @api_user.save - @api_user.errors.full_messages.should match_array([]) - @api_user.versions.size.should == 1 - end - end end describe '::min_password_length', db: false do diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb index 5913083c9..52469b28f 100644 --- a/spec/models/contact_spec.rb +++ b/spec/models/contact_spec.rb @@ -5,16 +5,6 @@ RSpec.describe Contact do create(:zone, origin: 'ee') end - context 'about class' do - it 'should have versioning enabled?' do - Contact.paper_trail_enabled_for_model?.should == true - end - - it 'should have custom log prexied table name for versions table' do - ContactVersion.table_name.should == 'log_contacts' - end - end - context 'with invalid attribute' do before :example do @contact = Contact.new @@ -28,10 +18,6 @@ RSpec.describe Contact do @contact.updator.should == nil end - it 'should not have any versions' do - @contact.versions.should == [] - end - it 'should not accept long code' do @contact.code = 'verylongcode' * 100 @contact.valid? @@ -66,16 +52,6 @@ RSpec.describe Contact do @contact = create(:contact) end - it 'should have one version' do - with_versioning do - @contact.versions.reload.should == [] - @contact.name = 'New name' - @contact.save - @contact.errors.full_messages.should match_array([]) - @contact.versions.size.should == 1 - end - end - it 'should not overwrite code' do old_code = @contact.code @contact.code = 'CID:REG1:should-not-overwrite-old-code-12345' diff --git a/spec/models/dnskey_spec.rb b/spec/models/dnskey_spec.rb index 5c0f7859f..d21968214 100644 --- a/spec/models/dnskey_spec.rb +++ b/spec/models/dnskey_spec.rb @@ -34,10 +34,6 @@ describe Dnskey do @dnskey.errors.full_messages.should match_array([ ]) end - - it 'should not have any versions' do - @dnskey.versions.should == [] - end end context 'with valid attributes' do @@ -66,15 +62,6 @@ describe Dnskey do @dnskey.errors.full_messages.should match_array([]) end - # TODO: figure out why not working - # it 'should have one version' do - # with_versioning do - # @dnskey.versions.should == [] - # @dnskey.touch_with_version - # @dnskey.versions.size.should == 1 - # end - # end - it 'generates correct DS digest and DS key tag for ria.ee' do d = create(:domain, name: 'ria.ee', dnskeys: [@dnskey]) dk = d.dnskeys.last diff --git a/spec/models/domain_contact_spec.rb b/spec/models/domain_contact_spec.rb index fc7ec010e..f5476c49b 100644 --- a/spec/models/domain_contact_spec.rb +++ b/spec/models/domain_contact_spec.rb @@ -49,18 +49,6 @@ describe DomainContact do it 'should have Tech name' do @domain_contact.name.should == 'Tech' end - - it 'should have one version' do - @domain_contact = create(:domain_contact) - - with_versioning do - @domain_contact.versions.reload.should == [] - @domain_contact.contact = create(:contact) - @domain_contact.save! - @domain_contact.errors.full_messages.should match_array([]) - @domain_contact.versions.size.should == 1 - end - end end context 'with valid attributes with tech domain contact' do @@ -82,18 +70,6 @@ describe DomainContact do it 'should have Tech name' do @domain_contact.name.should == 'Tech' end - - it 'should have one version' do - @domain_contact = create(:domain_contact) - - with_versioning do - @domain_contact.versions.reload.should == [] - @domain_contact.contact = create(:contact) - @domain_contact.save! - @domain_contact.errors.full_messages.should match_array([]) - @domain_contact.versions.size.should == 1 - end - end end context 'with valid attributes with admin domain contact' do @@ -115,15 +91,5 @@ describe DomainContact do it 'should have Tech name' do @domain_contact.name.should == 'Admin' end - - it 'should have one version' do - with_versioning do - @domain_contact.versions.reload.should == [] - @domain_contact.contact = create(:contact) - @domain_contact.save - @domain_contact.errors.full_messages.should match_array([]) - @domain_contact.versions.size.should == 1 - end - end end end diff --git a/spec/models/domain_spec.rb b/spec/models/domain_spec.rb index 259f2d307..ac424bcbe 100644 --- a/spec/models/domain_spec.rb +++ b/spec/models/domain_spec.rb @@ -33,10 +33,6 @@ RSpec.describe Domain do @domain = Domain.new end - it 'should not have any versions' do - @domain.versions.should == [] - end - it 'should not have whois body' do @domain.whois_record.should == nil end @@ -489,21 +485,6 @@ RSpec.describe Domain do expect(d.statuses.count).to eq(1) expect(d.statuses.first).to eq(DomainStatus::CLIENT_DELETE_PROHIBITED) end - - with_versioning do - context 'when saved' do - before(:each) do - domain = create(:domain, nameservers_attributes: [attributes_for(:nameserver), - attributes_for(:nameserver)]) - end - - it 'creates domain version' do - expect(DomainVersion.count).to eq(1) - expect(ContactVersion.count).to eq(3) - expect(NameserverVersion.count).to eq(2) - end - end - end end RSpec.describe Domain do diff --git a/spec/models/keyrelay_spec.rb b/spec/models/keyrelay_spec.rb index 5c335941b..fc388dfed 100644 --- a/spec/models/keyrelay_spec.rb +++ b/spec/models/keyrelay_spec.rb @@ -41,10 +41,6 @@ describe Keyrelay do "Only one parameter allowed: relative or absolute" ]) end - - it 'should not have any versions' do - @keyrelay.versions.should == [] - end end context 'with valid attributes' do @@ -63,16 +59,6 @@ describe Keyrelay do @keyrelay.errors.full_messages.should match_array([]) end - it 'should have one version' do - with_versioning do - @keyrelay.versions.should == [] - @keyrelay.auth_info_pw = 'newpw' - @keyrelay.save - @keyrelay.errors.full_messages.should match_array([]) - @keyrelay.versions.size.should == 1 - end - end - it 'is in pending status' do @keyrelay.status.should == 'pending' end diff --git a/spec/models/white_ip_spec.rb b/spec/models/white_ip_spec.rb index bb286c0ca..fa28991f1 100644 --- a/spec/models/white_ip_spec.rb +++ b/spec/models/white_ip_spec.rb @@ -23,22 +23,6 @@ describe WhiteIp do end end - context 'with valid attributes' do - before :all do - @white_ip = create(:white_ip) - end - - it 'should have one version' do - with_versioning do - @white_ip.versions.should == [] - @white_ip.ipv4 = '192.168.1.2' - @white_ip.save - @white_ip.errors.full_messages.should match_array([]) - @white_ip.versions.size.should == 1 - end - end - end - describe '#include_ip?' do context 'when given ip v4 exists' do before do