From b0bec41f56b73167ec3d267b8cf8778470205262 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Tue, 24 Apr 2018 11:53:27 +0300 Subject: [PATCH 1/5] Write a test that covers missing and non-existent values --- app/views/admin/domain_versions/archive.haml | 5 +- app/views/admin/domain_versions/show.haml | 5 +- .../integration/admin/domain_versions_test.rb | 64 +++++++++++++++++++ 3 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 test/integration/admin/domain_versions_test.rb diff --git a/app/views/admin/domain_versions/archive.haml b/app/views/admin/domain_versions/archive.haml index 5e6d8eaa9..4137064a2 100644 --- a/app/views/admin/domain_versions/archive.haml +++ b/app/views/admin/domain_versions/archive.haml @@ -56,7 +56,10 @@ - @versions.each do |version| - if version - domain = Domain.new(version.object.to_h) - - version.object_changes.to_h.each{|k,v| domain.public_send("#{k}=", v.last) } + - version.object_changes.to_h.each do |k, v| + - method_name = "#{k}=".to_sym + - if domain.respond_to?(method_name) + - domain.public_send("#{k}=", v.last) %tr %td= link_to(domain.name, admin_domain_version_path(version.id)) diff --git a/app/views/admin/domain_versions/show.haml b/app/views/admin/domain_versions/show.haml index 1a17ba3f9..9071feafb 100644 --- a/app/views/admin/domain_versions/show.haml +++ b/app/views/admin/domain_versions/show.haml @@ -1,5 +1,8 @@ - domain = Domain.new(@version.object.to_h) -- @version.object_changes.to_h.each{|k,v| domain.public_send("#{k}=", v.last) } +- @version.object_changes.to_h.each do |k, v| + - method_name = "#{k}=".to_sym + - if domain.respond_to?(method_name) + - domain.public_send("#{k}=", v.last) - if @version - children = HashWithIndifferentAccess.new(@version.children) diff --git a/test/integration/admin/domain_versions_test.rb b/test/integration/admin/domain_versions_test.rb new file mode 100644 index 000000000..8e69d5098 --- /dev/null +++ b/test/integration/admin/domain_versions_test.rb @@ -0,0 +1,64 @@ +require 'test_helper' + +class DomainVersionsTest < ActionDispatch::IntegrationTest + def setup + super + + create_domain_with_history + login_as users(:admin) + end + + def teardown + super + + delete_objects_once_done + end + + def create_domain_with_history + sql = <<-SQL.squish + INSERT INTO registrars (id, name, reg_no, email, country_code, code, + accounting_customer_code, language) + VALUES (54, 'test_registrar', 'test123', 'test@test.com', 'EE', 'TEST123', + 'test123', 'en'); + + INSERT INTO contacts (id, code, auth_info, registrar_id) + VALUES (54, 'test_code', '8b4d462aa04194ca78840a', 54); + + INSERT INTO domains (id, registrar_id, valid_to, registrant_id, + transfer_code) + VALUES (54, 54, '2018-06-23T12:14:02.732+03:00', 54, 'transfer_code'); + + INSERT INTO log_domains (item_type, item_id, event, whodunnit, object, + object_changes, created_at, nameserver_ids, tech_contact_ids, + admin_contact_ids, session, children) + VALUES ('Domain', 54, 'update', '1-AdminUser', + '{"id": 54, "registrar_id": 54, "valid_to": "2018-07-23T12:14:05.583+03:00", "registrant_id": 54, "transfer_code": "transfer_code"}', + '{"valid_from": "2017-07-23T12:14:05.583+03:00", "foo": "bar", "other_made_up_field": "value"}', + '2018-04-23 15:50:48.113491', '{}', '{}', '{}', '2018-04-23 12:44:56', + '{"null_fracdmin_contacts":[108],"tech_contacts":[109],"nameservers":[],"dnskeys":[],"legal_documents":[null],"registrant":[1]}' + ) + SQL + ActiveRecord::Base.connection.execute(sql) + end + + def delete_objects_once_done + ActiveRecord::Base.connection.execute('DELETE FROM log_domains where item_id = 54') + Domain.destroy_all + Contact.destroy_all + Registrar.destroy_all + end + + def test_removed_fields_are_not_causing_errors_in_index_view + visit admin_domain_versions_path + assert_text 'test_registrar' + assert_text 'test_registrar update 23.04.18, 18:50' + end + + def test_removed_fields_are_not_causing_errors_in_details_view + version_id = Domain.find(54).versions.last + visit admin_domain_version_path(version_id) + + assert_text 'test_registrar' + assert_text '23.04.18, 18:50 update 1-AdminUser' + end +end From a93228c579f417e84a619b8b72d5ba024d1d3b10 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Tue, 24 Apr 2018 13:19:11 +0300 Subject: [PATCH 2/5] Extract block to helper function --- app/controllers/admin/contact_versions_controller.rb | 2 ++ app/helpers/object_versions_helper.rb | 10 ++++++++++ app/views/admin/domain_versions/archive.haml | 5 +---- app/views/admin/domain_versions/show.haml | 5 +---- 4 files changed, 14 insertions(+), 8 deletions(-) create mode 100644 app/helpers/object_versions_helper.rb diff --git a/app/controllers/admin/contact_versions_controller.rb b/app/controllers/admin/contact_versions_controller.rb index d066f698e..e64a86774 100644 --- a/app/controllers/admin/contact_versions_controller.rb +++ b/app/controllers/admin/contact_versions_controller.rb @@ -1,5 +1,7 @@ module Admin class ContactVersionsController < BaseController + include ObjectVersionsHelper + load_and_authorize_resource def index diff --git a/app/helpers/object_versions_helper.rb b/app/helpers/object_versions_helper.rb new file mode 100644 index 000000000..c8b2d8de9 --- /dev/null +++ b/app/helpers/object_versions_helper.rb @@ -0,0 +1,10 @@ +module ObjectVersionsHelper + def attach_existing_fields(version, new_object) + version.object_changes.to_h.each do |k, v| + method_name = "#{k}=".to_sym + if new_object.respond_to?(method_name) + new_object.public_send(method_name, v.last) + end + end + end +end diff --git a/app/views/admin/domain_versions/archive.haml b/app/views/admin/domain_versions/archive.haml index 4137064a2..3f5258653 100644 --- a/app/views/admin/domain_versions/archive.haml +++ b/app/views/admin/domain_versions/archive.haml @@ -56,10 +56,7 @@ - @versions.each do |version| - if version - domain = Domain.new(version.object.to_h) - - version.object_changes.to_h.each do |k, v| - - method_name = "#{k}=".to_sym - - if domain.respond_to?(method_name) - - domain.public_send("#{k}=", v.last) + - attach_existing_fields(version, domain) %tr %td= link_to(domain.name, admin_domain_version_path(version.id)) diff --git a/app/views/admin/domain_versions/show.haml b/app/views/admin/domain_versions/show.haml index 9071feafb..3cce3e14c 100644 --- a/app/views/admin/domain_versions/show.haml +++ b/app/views/admin/domain_versions/show.haml @@ -1,8 +1,5 @@ - domain = Domain.new(@version.object.to_h) -- @version.object_changes.to_h.each do |k, v| - - method_name = "#{k}=".to_sym - - if domain.respond_to?(method_name) - - domain.public_send("#{k}=", v.last) +- attach_existing_fields(@version, domain) - if @version - children = HashWithIndifferentAccess.new(@version.children) From cec05c3943ed80c5192dd1b5dc5dbb3deb5b9b8f Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Tue, 24 Apr 2018 13:19:34 +0300 Subject: [PATCH 3/5] Test and fix contact_versions controller affected by the same bug --- .../admin/domain_versions_controller.rb | 2 + app/views/admin/contact_versions/index.haml | 2 +- app/views/admin/contact_versions/show.haml | 6 +- .../admin/contact_versions_test.rb | 60 +++++++++++++++++++ 4 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 test/integration/admin/contact_versions_test.rb diff --git a/app/controllers/admin/domain_versions_controller.rb b/app/controllers/admin/domain_versions_controller.rb index 283c84f97..2585ab894 100644 --- a/app/controllers/admin/domain_versions_controller.rb +++ b/app/controllers/admin/domain_versions_controller.rb @@ -1,5 +1,7 @@ module Admin class DomainVersionsController < BaseController + include ObjectVersionsHelper + load_and_authorize_resource def index diff --git a/app/views/admin/contact_versions/index.haml b/app/views/admin/contact_versions/index.haml index 0367db3be..519161a10 100644 --- a/app/views/admin/contact_versions/index.haml +++ b/app/views/admin/contact_versions/index.haml @@ -58,7 +58,7 @@ - @versions.each do |version| - if version - contact = Contact.new(version.object.to_h) - - version.object_changes.to_h.each { |k,v| contact.public_send("#{k}=", v.last) } + - attach_existing_fields(version, contact) %tr %td= link_to(contact.name, admin_contact_version_path(version.id)) diff --git a/app/views/admin/contact_versions/show.haml b/app/views/admin/contact_versions/show.haml index e19326fca..713f03046 100644 --- a/app/views/admin/contact_versions/show.haml +++ b/app/views/admin/contact_versions/show.haml @@ -1,5 +1,5 @@ - contact = Contact.new(@version.object.to_h) -- @version.object_changes.to_h.each { |k,v| contact.public_send("#{k}=", v.last ) } +- attach_existing_fields(@version, contact) = render 'shared/title', name: contact.name .row @@ -41,11 +41,11 @@ %br - %dt= t(:created) + %dt= t(:created_at) %dd{class: changing_css_class(@version,"created_at")} = l(contact.created_at, format: :short) - %dt= t(:updated) + %dt= t(:updated_at) %dd{class: changing_css_class(@version,"updated_at")} = l(contact.updated_at, format: :short) diff --git a/test/integration/admin/contact_versions_test.rb b/test/integration/admin/contact_versions_test.rb new file mode 100644 index 000000000..25e44f5a1 --- /dev/null +++ b/test/integration/admin/contact_versions_test.rb @@ -0,0 +1,60 @@ +require 'test_helper' + +class ContactVersionsTest < ActionDispatch::IntegrationTest + def setup + super + + create_contact_with_history + login_as users(:admin) + end + + def teardown + super + + delete_objects_once_done + end + + def create_contact_with_history + sql = <<-SQL.squish + INSERT INTO registrars (id, name, reg_no, email, country_code, code, + accounting_customer_code, language) + VALUES (75, 'test_registrar', 'test123', 'test@test.com', 'EE', 'TEST123', + 'test123', 'en'); + + INSERT INTO contacts (id, code, auth_info, registrar_id) + VALUES (75, 'test_code', '8b4d462aa04194ca78840a', 75); + + INSERT INTO log_contacts (item_type, item_id, event, whodunnit, object, + object_changes, created_at, session, children, ident_updated_at, uuid) + VALUES ('Contact', 75, 'update', '1-AdminUser', + '{"id": 75, "code": "test_code", "auth_info": "8b4d462aa04194ca78840a", "registrar_id": 75}', + '{"other_made_up_field": "value"}', + '2018-04-23 15:50:48.113491', '2018-04-23 12:44:56', + '{"legal_documents":[null]}', null, null + ) + SQL + + ActiveRecord::Base.connection.execute(sql) + end + + def delete_objects_once_done + ActiveRecord::Base.connection.execute('DELETE from log_contacts where item_id = 75') + Domain.destroy_all + Contact.destroy_all + Registrar.destroy_all + end + + def test_removed_fields_are_not_causing_errors_in_index_view + visit admin_contact_versions_path + assert_text 'test_registrar' + assert_text 'update 23.04.18, 18:50' + end + + def test_removed_fields_are_not_causing_errors_in_details_view + version_id = Contact.find(75).versions.last + visit admin_contact_version_path(version_id) + + assert_text 'test_registrar' + assert_text '23.04.18, 18:50 update 1-AdminUser' + end +end From a82db7a67a175ab83af16f1c3234020d6b7f1368 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Wed, 25 Apr 2018 16:03:13 +0300 Subject: [PATCH 4/5] Solve both sides of the missing fields bug, not only one --- app/helpers/object_versions_helper.rb | 11 ++++++++--- app/views/admin/contact_versions/index.haml | 5 +++-- app/views/admin/contact_versions/show.haml | 3 ++- app/views/admin/domain_versions/archive.haml | 3 ++- app/views/admin/domain_versions/show.haml | 3 ++- test/integration/admin/contact_versions_test.rb | 2 +- test/integration/admin/domain_versions_test.rb | 5 +++-- 7 files changed, 21 insertions(+), 11 deletions(-) diff --git a/app/helpers/object_versions_helper.rb b/app/helpers/object_versions_helper.rb index c8b2d8de9..d8e00abbe 100644 --- a/app/helpers/object_versions_helper.rb +++ b/app/helpers/object_versions_helper.rb @@ -1,10 +1,15 @@ module ObjectVersionsHelper def attach_existing_fields(version, new_object) - version.object_changes.to_h.each do |k, v| - method_name = "#{k}=".to_sym + version.object_changes.to_h.each do |key, value| + method_name = "#{key}=".to_sym if new_object.respond_to?(method_name) - new_object.public_send(method_name, v.last) + new_object.public_send(method_name, value.last) end end end + + def only_present_fields(version, model) + field_names = model.column_names + version.object.to_h.select { |key, _value| field_names.include?(key) } + end end diff --git a/app/views/admin/contact_versions/index.haml b/app/views/admin/contact_versions/index.haml index 519161a10..8f293ba5d 100644 --- a/app/views/admin/contact_versions/index.haml +++ b/app/views/admin/contact_versions/index.haml @@ -57,8 +57,9 @@ %tbody - @versions.each do |version| - if version - - contact = Contact.new(version.object.to_h) - - attach_existing_fields(version, contact) + - attributes = only_present_fields(version, Contact) + - contact = Contact.new(attributes) + - attach_existing_fields(version, contact) %tr %td= link_to(contact.name, admin_contact_version_path(version.id)) diff --git a/app/views/admin/contact_versions/show.haml b/app/views/admin/contact_versions/show.haml index 713f03046..6e5af9e14 100644 --- a/app/views/admin/contact_versions/show.haml +++ b/app/views/admin/contact_versions/show.haml @@ -1,4 +1,5 @@ -- contact = Contact.new(@version.object.to_h) +- attributes = only_present_fields(@version, Contact) +- contact = Contact.new(attributes) - attach_existing_fields(@version, contact) = render 'shared/title', name: contact.name diff --git a/app/views/admin/domain_versions/archive.haml b/app/views/admin/domain_versions/archive.haml index 3f5258653..9fdf8b000 100644 --- a/app/views/admin/domain_versions/archive.haml +++ b/app/views/admin/domain_versions/archive.haml @@ -55,7 +55,8 @@ %tbody - @versions.each do |version| - if version - - domain = Domain.new(version.object.to_h) + - attributes = only_present_fields(version, Domain) + - domain = Domain.new(attributes) - attach_existing_fields(version, domain) %tr diff --git a/app/views/admin/domain_versions/show.haml b/app/views/admin/domain_versions/show.haml index 3cce3e14c..9a38150be 100644 --- a/app/views/admin/domain_versions/show.haml +++ b/app/views/admin/domain_versions/show.haml @@ -1,4 +1,5 @@ -- domain = Domain.new(@version.object.to_h) +- present_fields = only_present_fields(@version, Domain) +- domain = Domain.new(present_fields) - attach_existing_fields(@version, domain) - if @version diff --git a/test/integration/admin/contact_versions_test.rb b/test/integration/admin/contact_versions_test.rb index 25e44f5a1..cd67eb964 100644 --- a/test/integration/admin/contact_versions_test.rb +++ b/test/integration/admin/contact_versions_test.rb @@ -27,7 +27,7 @@ class ContactVersionsTest < ActionDispatch::IntegrationTest INSERT INTO log_contacts (item_type, item_id, event, whodunnit, object, object_changes, created_at, session, children, ident_updated_at, uuid) VALUES ('Contact', 75, 'update', '1-AdminUser', - '{"id": 75, "code": "test_code", "auth_info": "8b4d462aa04194ca78840a", "registrar_id": 75}', + '{"id": 75, "code": "test_code", "auth_info": "8b4d462aa04194ca78840a", "registrar_id": 75, "old_field": "value"}', '{"other_made_up_field": "value"}', '2018-04-23 15:50:48.113491', '2018-04-23 12:44:56', '{"legal_documents":[null]}', null, null diff --git a/test/integration/admin/domain_versions_test.rb b/test/integration/admin/domain_versions_test.rb index 8e69d5098..195433948 100644 --- a/test/integration/admin/domain_versions_test.rb +++ b/test/integration/admin/domain_versions_test.rb @@ -32,8 +32,8 @@ class DomainVersionsTest < ActionDispatch::IntegrationTest object_changes, created_at, nameserver_ids, tech_contact_ids, admin_contact_ids, session, children) VALUES ('Domain', 54, 'update', '1-AdminUser', - '{"id": 54, "registrar_id": 54, "valid_to": "2018-07-23T12:14:05.583+03:00", "registrant_id": 54, "transfer_code": "transfer_code"}', - '{"valid_from": "2017-07-23T12:14:05.583+03:00", "foo": "bar", "other_made_up_field": "value"}', + '{"id": 54, "registrar_id": 54, "valid_to": "2018-07-23T12:14:05.583+03:00", "registrant_id": 54, "transfer_code": "transfer_code", "valid_from": "2017-07-23T12:14:05.583+03:00"}', + '{"foo": "bar", "other_made_up_field": "value"}', '2018-04-23 15:50:48.113491', '{}', '{}', '{}', '2018-04-23 12:44:56', '{"null_fracdmin_contacts":[108],"tech_contacts":[109],"nameservers":[],"dnskeys":[],"legal_documents":[null],"registrant":[1]}' ) @@ -50,6 +50,7 @@ class DomainVersionsTest < ActionDispatch::IntegrationTest def test_removed_fields_are_not_causing_errors_in_index_view visit admin_domain_versions_path + assert_text 'test_registrar' assert_text 'test_registrar update 23.04.18, 18:50' end From 2c494d93737649c81a44c615d73f93bc500ab4d1 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Thu, 26 Apr 2018 08:57:28 +0300 Subject: [PATCH 5/5] Fix brakeman issue by extracting gsub into a function --- app/helpers/contact_helper.rb | 5 +++++ app/views/admin/contact_versions/show.haml | 2 +- app/views/registrant/contacts/partials/_address.haml | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 app/helpers/contact_helper.rb diff --git a/app/helpers/contact_helper.rb b/app/helpers/contact_helper.rb new file mode 100644 index 000000000..6b15e3da4 --- /dev/null +++ b/app/helpers/contact_helper.rb @@ -0,0 +1,5 @@ +module ContactHelper + def printable_street(street) + street.to_s.gsub("\n", '
').html_safe + end +end diff --git a/app/views/admin/contact_versions/show.haml b/app/views/admin/contact_versions/show.haml index 6e5af9e14..38139b455 100644 --- a/app/views/admin/contact_versions/show.haml +++ b/app/views/admin/contact_versions/show.haml @@ -62,7 +62,7 @@ - if contact.street.present? %dt= t(:street) - %dd{class: changing_css_class(@version,"street")}= contact.street.to_s.gsub("\n", '
').html_safe + %dd{class: changing_css_class(@version,"street")}= printable_street(contact.street) - if contact.city.present? %dt= t(:city) diff --git a/app/views/registrant/contacts/partials/_address.haml b/app/views/registrant/contacts/partials/_address.haml index 9c0f548e3..fffef581f 100644 --- a/app/views/registrant/contacts/partials/_address.haml +++ b/app/views/registrant/contacts/partials/_address.haml @@ -8,7 +8,7 @@ %dd= @contact.org_name %dt= t(:street) - %dd= @contact.street.to_s.gsub("\n", '
').html_safe + %dd= printable_street(@contact.street) %dt= t(:city) %dd= @contact.city