From 8d24fe7b00189fe25b4d5eee0b5b907eeb3a100a Mon Sep 17 00:00:00 2001 From: Martin Lensment Date: Wed, 17 Sep 2014 14:20:23 +0300 Subject: [PATCH] EPP error refactor (once again) --- app/models/concerns/epp_errors.rb | 48 ++++++++++++------------ app/models/contact.rb | 2 - app/models/domain_contact.rb | 2 +- app/models/domain_status.rb | 6 +-- app/models/epp/epp_domain.rb | 62 ++++++++++++++----------------- app/models/nameserver.rb | 12 ++---- config/locales/en.yml | 2 +- spec/epp/domain_spec.rb | 13 ++++--- 8 files changed, 67 insertions(+), 80 deletions(-) diff --git a/app/models/concerns/epp_errors.rb b/app/models/concerns/epp_errors.rb index c83899769..8e637fd74 100644 --- a/app/models/concerns/epp_errors.rb +++ b/app/models/concerns/epp_errors.rb @@ -11,35 +11,21 @@ module EppErrors epp_errors << collect_child_errors(key) end - epp_errors << collect_parent_errors(key, values) + epp_errors << collect_parent_errors(values) end errors[:epp_errors] = epp_errors errors[:epp_errors].flatten! end - def collect_parent_errors(key, values) + def collect_parent_errors(values) epp_errors = [] values = [values] if values.is_a?(String) values.each do |err| - if err.is_a?(Hash) - code = err[:code] || find_epp_code(err[:msg]) - next unless code - err_msg = { code: code, msg: err[:msg] } - err_msg[:value] = { val: err[:val], obj: err[:obj] } if err[:val] - epp_errors << err_msg - else - next unless code = find_epp_code(err) - err = { code: code, msg: err } - - # if the key represents relations, skip value - unless self.class.reflect_on_association(key) - err[:value] = { val: send(key), obj: self.class::EPP_ATTR_MAP[key] } - end - - epp_errors << err - end + code, value = find_epp_code_and_value(err) + next unless code + epp_errors << { code: code, msg: err, value: value} end epp_errors end @@ -52,24 +38,38 @@ module EppErrors epp_errors = [] send(key).each do |x| x.errors.messages.each do |key, values| - epp_errors << x.collect_parent_errors(key, values) + epp_errors << x.collect_parent_errors(values) end end if multi.include?(macro) epp_errors end - def find_epp_code(msg) + def find_epp_code_and_value(msg) epp_code_map.each do |code, values| values.each do |x| - t = errors.generate_message(*x) if x.is_a?(Array) - t = x if x.is_a?(String) - return code if t == msg + msg_args, value = construct_msg_args_and_value(x) + t = errors.generate_message(*msg_args) + return [code, value] if t == msg end end nil end + def construct_msg_args_and_value(epp_error_args) + args = {} + args = epp_error_args.delete_at(-1) if epp_error_args.last.is_a?(Hash) + msg_args = epp_error_args + + value = args.delete(:value) if args.key?(:value) + + interpolation = args[:interpolation] || args + + msg_args << interpolation + + [msg_args, value] + end + def add_epp_error(code, obj, val, msg) errors[:epp_errors] ||= [] t = errors.generate_message(*msg) if msg.is_a?(Array) diff --git a/app/models/contact.rb b/app/models/contact.rb index 8be915b5a..73b51b96f 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -101,8 +101,6 @@ class Contact < ActiveRecord::Base '2302' => [ # Object exists [:code, :epp_id_taken] ], - '2303' => # Object does not exist - [:not_found, :epp_obj_does_not_exist], '2305' => [ # Association exists [:domains, :exist] ], diff --git a/app/models/domain_contact.rb b/app/models/domain_contact.rb index 57f673785..46d961799 100644 --- a/app/models/domain_contact.rb +++ b/app/models/domain_contact.rb @@ -10,7 +10,7 @@ class DomainContact < ActiveRecord::Base def epp_code_map { '2302' => [ - [:contact, :taken] + [:contact, :taken, {value: {obj: 'contact', val: contact.code}}] ] } end diff --git a/app/models/domain_status.rb b/app/models/domain_status.rb index c995be32a..f026a6ae7 100644 --- a/app/models/domain_status.rb +++ b/app/models/domain_status.rb @@ -23,16 +23,12 @@ class DomainStatus < ActiveRecord::Base STATUSES = [CLIENT_DELETE_PROHIBITED, SERVER_DELETE_PROHIBITED, CLIENT_HOLD, SERVER_HOLD, CLIENT_RENEW_PROHIBITED, SERVER_RENEW_PROHIBITED, CLIENT_TRANSFER_PROHIBITED, SERVER_TRANSFER_PROHIBITED, CLIENT_UPDATE_PROHIBITED, SERVER_UPDATE_PROHIBITED, INACTIVE, OK, PENDING_CREATE, PENDING_DELETE, PENDING_RENEW, PENDING_TRANSFER, PENDING_UPDATE] - EPP_ATTR_MAP = { - value: 'status' - } - validates :value, uniqueness: { scope: :domain_id } def epp_code_map { '2302' => [ # Object exists - [:value, :taken] + [:value, :taken, { value: { obj: 'status', val: value } }] ] } end diff --git a/app/models/epp/epp_domain.rb b/app/models/epp/epp_domain.rb index af2cdf34b..033d8cd21 100644 --- a/app/models/epp/epp_domain.rb +++ b/app/models/epp/epp_domain.rb @@ -1,15 +1,36 @@ class Epp::EppDomain < Domain include EppErrors - EPP_ATTR_MAP = { - owner_contact: 'registrant', - name_dirty: 'name', - period: 'period' - } - validate :validate_nameservers_count validate :validate_admin_contacts_count + def epp_code_map # rubocop:disable Metrics/MethodLength + domain_validation_sg = SettingGroup.domain_validation + + { + '2302' => [ # Object exists + [:name_dirty, :taken, { value: { obj: 'name', val: name_dirty } }], + [:name_dirty, :reserved, { value: { obj: 'name', val: name_dirty } }] + ], + '2306' => [ # Parameter policy error + [:owner_contact, :blank], + [:admin_contacts, :out_of_range] + ], + '2004' => [ # Parameter value range error + [:nameservers, :out_of_range, + { + min: domain_validation_sg.setting(:ns_min_count).value, + max: domain_validation_sg.setting(:ns_max_count).value + } + ], + [:period, :out_of_range, { value: { obj: 'period', val: period } }] + ], + '2200' => [ + [:auth_info, :wrong_pw] + ] + } + end + def parse_and_attach_domain_dependencies(parsed_frame) attach_owner_contact(self.class.parse_owner_contact_from_frame(parsed_frame)) attach_contacts(self.class.parse_contacts_from_frame(parsed_frame)) @@ -216,38 +237,11 @@ class Epp::EppDomain < Domain add_epp_error('2306', 'curExpDate', cur_exp_date, I18n.t('errors.messages.epp_exp_dates_do_not_match')) end - def epp_code_map # rubocop:disable Metrics/MethodLength - domain_validation_sg = SettingGroup.domain_validation - - { - '2302' => [ # Object exists - [:name_dirty, :taken], - [:name_dirty, :reserved] - ], - '2306' => [ # Parameter policy error - [:owner_contact, :blank], - [:admin_contacts, :out_of_range] - ], - '2004' => [ # Parameter value range error - [:nameservers, :out_of_range, - { - min: domain_validation_sg.setting(:ns_min_count).value, - max: domain_validation_sg.setting(:ns_max_count).value - } - ], - [:period, :out_of_range] - ], - '2200' => [ - [:auth_info, :wrong_pw] - ] - } - end - ## SHARED # For domain transfer def authenticate(pw) - errors.add(:auth_info, { msg: errors.generate_message(:auth_info, :wrong_pw) }) if pw != auth_info + errors.add(:auth_info, :wrong_pw) if pw != auth_info errors.empty? end diff --git a/app/models/nameserver.rb b/app/models/nameserver.rb index b08702a7c..d661b849c 100644 --- a/app/models/nameserver.rb +++ b/app/models/nameserver.rb @@ -1,10 +1,6 @@ class Nameserver < ActiveRecord::Base include EppErrors - EPP_ATTR_MAP = { - hostname: 'hostObj' - } - belongs_to :registrar belongs_to :domain @@ -17,12 +13,12 @@ class Nameserver < ActiveRecord::Base def epp_code_map { '2302' => [ - [:hostname, :taken] + [:hostname, :taken, { value: { obj: 'hostObj', val: hostname } }] ], '2005' => [ - [:hostname, :invalid], - [:ipv4, :invalid], - [:ipv6, :invalid] + [:hostname, :invalid, { value: { obj: 'hostObj', val: hostname } }], + [:ipv4, :invalid, { value: { obj: 'hostAddr', val: ipv4 } }], + [:ipv6, :invalid, { value: { obj: 'hostAddr', val: ipv6 } }] ] } end diff --git a/config/locales/en.yml b/config/locales/en.yml index c2b04d2ba..622982e3b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -142,7 +142,7 @@ en: errors: messages: - #taken: 'Status already exists on this domain' + taken: 'Status already exists on this domain' blank: 'is missing' epp_domain_reserved: 'Domain name is reserved or restricted' epp_obj_does_not_exist: 'Object does not exist' diff --git a/spec/epp/domain_spec.rb b/spec/epp/domain_spec.rb index a6601cfbe..bbee0eac1 100644 --- a/spec/epp/domain_spec.rb +++ b/spec/epp/domain_spec.rb @@ -437,13 +437,16 @@ describe 'EPP Domain', epp: true do response = epp_request(xml, :xml) expect(response[:results][0][:result_code]).to eq('2302') - expect(response[:results][0][:msg]).to eq('Nameserver already exists on this domain') - expect(response[:results][0][:value]).to eq('ns1.example.com') + expect(response[:results][0][:msg]).to eq('Contact already exists on this domain!') + expect(response[:results][0][:value]).to eq('mak21') + expect(response[:results][1][:result_code]).to eq('2302') expect(response[:results][1][:msg]).to eq('Nameserver already exists on this domain') - expect(response[:results][2][:msg]).to eq('Status already exists on this domain') - expect(response[:results][2][:value]).to eq('clientHold') + expect(response[:results][1][:value]).to eq('ns1.example.com') + expect(response[:results][2][:msg]).to eq('Nameserver already exists on this domain') expect(response[:results][3][:msg]).to eq('Status already exists on this domain') - expect(response[:results][3][:value]).to eq('clientUpdateProhibited') + expect(response[:results][3][:value]).to eq('clientHold') + expect(response[:results][4][:msg]).to eq('Status already exists on this domain') + expect(response[:results][4][:value]).to eq('clientUpdateProhibited') expect(d.domain_statuses.count).to eq(2) end