From 6eaca805dc7ac2f9dfe851fc85b0b7767077ae8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergei=20Ts=C3=B5ganov?= Date: Tue, 4 Jan 2022 14:00:01 +0200 Subject: [PATCH 1/6] Improved phone number validation --- app/models/depp/contact.rb | 7 ++++++- app/views/registrar/contacts/form/_general.haml | 3 ++- config/locales/en.yml | 5 +++++ lib/validators/e164_validator.rb | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/models/depp/contact.rb b/app/models/depp/contact.rb index 7deb365621..99dd910ab7 100644 --- a/app/models/depp/contact.rb +++ b/app/models/depp/contact.rb @@ -16,6 +16,8 @@ class Contact ['Birthday', 'birthday'] ] + validates :phone, e164: true, phone: true + class << self attr_reader :epp_xml, :user @@ -144,6 +146,8 @@ def type_string(type_code) end def save + return false if !valid? + hash = { id: { value: code }, postalInfo: { @@ -166,13 +170,14 @@ def save hash[:id] = nil if code.blank? create_xml = Depp::Contact.epp_xml.create(hash, extension_xml(:create)) - data = Depp::Contact.user.request(create_xml) self.id = data.css('id').text handle_errors(data) end def update_attributes(params) + return false if !valid? + self.ident_country_code = params[:ident_country_code] self.ident_type = params[:ident_type] self.ident = params[:ident] diff --git a/app/views/registrar/contacts/form/_general.haml b/app/views/registrar/contacts/form/_general.haml index 77443903dd..5f1c900987 100644 --- a/app/views/registrar/contacts/form/_general.haml +++ b/app/views/registrar/contacts/form/_general.haml @@ -56,7 +56,8 @@ .col-md-3.control-label = f.label :email, t(:email) + '*' .col-md-7 - = f.email_field :email, class: 'form-control', required: true + = f.email_field :email, class: 'form-control', required: true, + pattern: "[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$" .form-group .col-md-3.control-label diff --git a/config/locales/en.yml b/config/locales/en.yml index 069f997ce2..31947350d4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -377,6 +377,11 @@ en: activemodel: errors: models: + 'depp/contact': + attributes: + phone: + invalid: "Phone number must be in +XXX.YYYYYYY format" + too_long: "Phone number is too long" 'depp/user': attributes: base: diff --git a/lib/validators/e164_validator.rb b/lib/validators/e164_validator.rb index e5807e5857..72e805fd90 100644 --- a/lib/validators/e164_validator.rb +++ b/lib/validators/e164_validator.rb @@ -5,7 +5,7 @@ def validate_each(record, attribute, _value) length_validator.validate(record) format_validator = ActiveModel::Validations:: - FormatValidator.new(with: /\+[0-9]{1,3}\.[0-9]{1,14}?/, + FormatValidator.new(with: /\A\+[0-9]{1,3}\.[0-9]{1,14}\z/, attributes: attribute) format_validator.validate(record) end From 7b7a25069c3930e51ac069951b25635a2a308140 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergei=20Ts=C3=B5ganov?= Date: Tue, 4 Jan 2022 14:17:32 +0200 Subject: [PATCH 2/6] Corrected codeclimate issues --- app/models/depp/contact.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/depp/contact.rb b/app/models/depp/contact.rb index 99dd910ab7..4721f3f756 100644 --- a/app/models/depp/contact.rb +++ b/app/models/depp/contact.rb @@ -146,8 +146,8 @@ def type_string(type_code) end def save - return false if !valid? - + return false unless valid? + hash = { id: { value: code }, postalInfo: { @@ -175,8 +175,9 @@ def save handle_errors(data) end - def update_attributes(params) - return false if !valid? + # rubocop:disable Metrics/MethodLength + def update_attributes(params) + return false unless valid? self.ident_country_code = params[:ident_country_code] self.ident_type = params[:ident_type] @@ -225,6 +226,7 @@ def update_attributes(params) data = Depp::Contact.user.request(update_xml) handle_errors(data) end + # rubocop:enable Metrics/MethodLength def delete delete_xml = Contact.epp_xml.delete( From 8c8e9c8619088fafb8afd0046aa136b6552234be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergei=20Ts=C3=B5ganov?= Date: Tue, 4 Jan 2022 14:40:22 +0200 Subject: [PATCH 3/6] Corrected codeclimate issues --- app/models/depp/contact.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/depp/contact.rb b/app/models/depp/contact.rb index 4721f3f756..48e49d5e43 100644 --- a/app/models/depp/contact.rb +++ b/app/models/depp/contact.rb @@ -146,8 +146,8 @@ def type_string(type_code) end def save - return false unless valid? - + return false unless valid? + hash = { id: { value: code }, postalInfo: { @@ -175,8 +175,8 @@ def save handle_errors(data) end - # rubocop:disable Metrics/MethodLength - def update_attributes(params) + # rubocop:disable Metrics/MethodLength + def update_attributes(params) return false unless valid? self.ident_country_code = params[:ident_country_code] From d361288329c643a576e3b2d1545efdfaee5c96e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergei=20Ts=C3=B5ganov?= Date: Fri, 7 Jan 2022 16:05:13 +0200 Subject: [PATCH 4/6] Added model test and created helper for E.164 phone format testing --- test/helpers/phone_format_helper_test.rb | 37 ++++++++++++++++++++++++ test/models/contact_test.rb | 35 +++------------------- test/models/depp_contact_test.rb | 14 +++++++++ 3 files changed, 55 insertions(+), 31 deletions(-) create mode 100644 test/helpers/phone_format_helper_test.rb create mode 100644 test/models/depp_contact_test.rb diff --git a/test/helpers/phone_format_helper_test.rb b/test/helpers/phone_format_helper_test.rb new file mode 100644 index 0000000000..45ee803003 --- /dev/null +++ b/test/helpers/phone_format_helper_test.rb @@ -0,0 +1,37 @@ +module PhoneFormatHelperTest + # https://en.wikipedia.org/wiki/E.164 + def assert_phone_format(contact) + contact.phone = '+.1' + assert contact.invalid? + + contact.phone = '+123.' + assert contact.invalid? + + contact.phone = '+1.123456789123456' + assert contact.invalid? + + contact.phone = '+134.1234567891234' + assert contact.invalid? + + contact.phone = '+000.1' + assert contact.invalid? + + contact.phone = '+123.0' + assert contact.invalid? + + contact.phone = '+1.2' + assert contact.valid? + + contact.phone = '+123.4' + assert contact.valid? + + contact.phone = '+1.12345678912345' + assert contact.valid? + + contact.phone = '+134.123456789123' + assert contact.valid? + + contact.phone = '+134.00000000' + assert contact.invalid? + end +end \ No newline at end of file diff --git a/test/models/contact_test.rb b/test/models/contact_test.rb index bd4e1c0c11..d258d7fb48 100644 --- a/test/models/contact_test.rb +++ b/test/models/contact_test.rb @@ -1,6 +1,9 @@ require 'test_helper' +require 'helpers/phone_format_helper_test' class ContactTest < ActiveJob::TestCase + include PhoneFormatHelperTest + setup do @contact = contacts(:john) @old_validation_type = Truemail.configure.default_validation_type @@ -102,39 +105,9 @@ def test_invalid_without_phone assert contact.invalid? end - # https://en.wikipedia.org/wiki/E.164 def test_validates_phone_format contact = valid_contact - - contact.phone = '+.1' - assert contact.invalid? - - contact.phone = '+123.' - assert contact.invalid? - - contact.phone = '+1.123456789123456' - assert contact.invalid? - - contact.phone = '+134.1234567891234' - assert contact.invalid? - - contact.phone = '+000.1' - assert contact.invalid? - - contact.phone = '+123.0' - assert contact.invalid? - - contact.phone = '+1.2' - assert contact.valid? - - contact.phone = '+123.4' - assert contact.valid? - - contact.phone = '+1.12345678912345' - assert contact.valid? - - contact.phone = '+134.123456789123' - assert contact.valid? + assert_phone_format(contact) end def test_valid_without_address_when_address_processing_id_disabled diff --git a/test/models/depp_contact_test.rb b/test/models/depp_contact_test.rb new file mode 100644 index 0000000000..12f7c3b4fa --- /dev/null +++ b/test/models/depp_contact_test.rb @@ -0,0 +1,14 @@ +require 'test_helper' +require 'helpers/phone_format_helper_test' + +class DeppContactTest < ActiveSupport::TestCase + include PhoneFormatHelperTest + + setup do + @depp_contact = Depp::Contact.new + end + + def test_validates_phone_format + assert_phone_format(@depp_contact) + end +end \ No newline at end of file From 55a4ec0e84d172136b34cd7c8a4ee920c0955496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergei=20Ts=C3=B5ganov?= Date: Mon, 10 Jan 2022 11:53:18 +0200 Subject: [PATCH 5/6] Added system tests for creating/upating registrar contacts --- test/system/registrar_area/contact_test.rb | 45 ++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 test/system/registrar_area/contact_test.rb diff --git a/test/system/registrar_area/contact_test.rb b/test/system/registrar_area/contact_test.rb new file mode 100644 index 0000000000..49726029b3 --- /dev/null +++ b/test/system/registrar_area/contact_test.rb @@ -0,0 +1,45 @@ +require 'application_system_test_case' + +class RegistrarAreaContactTest < ApplicationSystemTestCase + setup do + @registrar = registrars(:bestnames) + @contact = contacts(:john) + sign_in users(:api_bestnames) + end + + def test_creates_contact_with_invalid_phone + visit registrar_contacts_path + click_on 'New' + + fill_in 'Ident', with: @contact.ident + fill_in 'Name', with: @contact.name + fill_in 'E-mail', with: @contact.email + fill_in 'Phone', with: '372' + click_on 'Create' + + assert_text 'Phone number must be in +XXX.YYYYYYY format' + end + + def test_updates_contact_with_invalid_phone + depp_contact = Depp::Contact.new( + id: @contact.id, + name: @contact.name, + code: @contact.code, + email: @contact.email, + phone: @contact.phone, + ident: @contact.ident, + ident_type: @contact.ident_type, + ident_country_code: @contact.ident_country_code) + + Spy.on(Depp::Contact, :find_by_id).and_return(depp_contact) + + visit edit_registrar_contact_path(depp_contact.code) + + assert_text "Edit: #{depp_contact.name}" + + fill_in 'Phone', with: '372' + click_on 'Save' + + assert_text 'Phone number must be in +XXX.YYYYYYY format' + end +end From 93f7b2abc2def01eb635110eb2d345139afa839e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergei=20Ts=C3=B5ganov?= Date: Thu, 13 Jan 2022 15:10:03 +0200 Subject: [PATCH 6/6] Repaired missing error message for epp request --- app/interactions/actions/contact_update.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/interactions/actions/contact_update.rb b/app/interactions/actions/contact_update.rb index 0cf76d1161..d9a136dd32 100644 --- a/app/interactions/actions/contact_update.rb +++ b/app/interactions/actions/contact_update.rb @@ -15,7 +15,7 @@ def call maybe_update_statuses maybe_update_ident if ident.present? maybe_attach_legal_doc - maybe_change_email + maybe_change_email if new_attributes[:email].present? maybe_filtering_old_failed_records commit end