Validate period for invalid values, refactor optional attribute validator, tests #2763

app/controllers/epp/domains_controller.rb

@@ -91,11 +91,17 @@ class Epp::DomainsController < EppController
     render_epp_response '/epp/domains/check'
   end
 
+  # rubocop: disable Metrics/MethodLength
   def renew
     authorize! :renew, @domain
 
-    period = params[:parsed_frame].css('period').text
+    period = params[:parsed_frame].css('period').text.presence || 1
-    period_unit = params[:parsed_frame].css('period').first['unit']
+    period_unit = 'y'
+    period_element = params[:parsed_frame].css('period').first
+
+    if period_element.present? && period_element['unit'].present?
+      period_unit = period_element['unit']
+    end
 
     ActiveRecord::Base.transaction do
       success = @domain.renew(
@@ -122,6 +128,7 @@ class Epp::DomainsController < EppController
       end
     end
   end
+  # rubocop: enable Metrics/MethodLength
 
   def transfer
     authorize! :transfer, @domain, @password
@@ -182,7 +189,9 @@ class Epp::DomainsController < EppController
 
   def validate_renew
     @prefix = 'renew > renew >'
-    requires 'name', 'curExpDate', 'period'
+    requires 'name', 'curExpDate'
+
+    optional_attribute 'period', 'unit', values: %w(d m y)
   end
 
   def validate_transfer

app/controllers/epp_controller.rb

@@ -217,11 +217,11 @@ class EppController < ApplicationController
   end
 
   def optional_attribute(element_selector, attribute_selector, options)
-    element = requires(element_selector, allow_blank: options[:allow_blank])
+    full_selector = [@prefix, element_selector].compact.join(' ')
+    element = params[:parsed_frame].css(full_selector).first
     return unless element
 
     attribute = element[attribute_selector]
-
     return if (attribute && options[:values].include?(attribute)) || !attribute
 
     epp_errors << {

doc/epp/domain.md

@@ -129,8 +129,9 @@ Domain name mapping protocol short version:
       <domain:renew>         1        Attribute: xmlns:domain="https://raw.githubusercontent.com/internetee/registry/alpha/doc/schemas/domain-eis-1.0.xsd"
         <domain:name>        1        Domain name. Can contain unicode characters. 
         <domain:curExpDate>  1        Current expiry date (ISO8601 format)
-        <domain:period>      1        Registration period for domain. 
+        <domain:period>      0-1      Registration period for domain. 
                                       Must add up to 1 / 2 / 3 years. Attribute: unit="y/m/d"
+                                      Default value is 1 year.
     <extension>              0-1     
       <eis:extdata>          0-1      Attribute: xmlns:eis="https://raw.githubusercontent.com/internetee/registry/alpha/doc/schemas/eis-1.0.xsd"
         <eis:legalDocument>  0-1      Base64 encoded document. 

spec/epp/domain_spec.rb

@@ -2243,6 +2243,73 @@ describe 'EPP Domain', epp: true do
       Setting.days_to_renew_domain_before_expire = 90
     end
 
+    it 'renews a domain with no period specified' do
+      Setting.days_to_renew_domain_before_expire = 0
+      old_balance = @registrar1.balance
+      old_activities = @registrar1.cash_account.account_activities.count
+
+      exp_date = domain.valid_to
+      xml = @epp_xml.domain.renew(
+        name: { value: domain.name },
+        curExpDate: { value: exp_date.to_date.to_s },
+        period: nil
+      )
+
+      response = epp_plain_request(xml)
+      response[:results][0][:msg].should == 'Command completed successfully'
+      response[:results][0][:result_code].should == '1000'
+
+      ex_date = response[:parsed].css('renData exDate').text
+      name = response[:parsed].css('renData name').text
+      ex_date.should == "#{(exp_date + 1.year).to_s(:iso8601)}"
+      name.should == domain.name
+
+      domain.reload
+      domain.valid_to.should be_within(1).of(exp_date + 1.year)
+      domain.outzone_at.should be_within(1).of(exp_date + 1.year + Setting.expire_warning_period.days)
+      domain.delete_at.should be_within(1).of(
+        exp_date + 1.year + Setting.expire_warning_period.days + Setting.redemption_grace_period.days
+      )
+
+      @registrar1.balance.should == old_balance - 15.0
+      @registrar1.cash_account.account_activities.count.should == old_activities + 1
+      a = @registrar1.cash_account.account_activities.last
+      a.description.should == "Renew #{Domain.last.name}"
+      a.sum.should == -BigDecimal.new('15.0')
+      a.activity_type = AccountActivity::RENEW
+      a.log_pricelist_id.should == @pricelist_renew_1_year.id
+      Setting.days_to_renew_domain_before_expire = 90
+    end
+
+    it 'does not renew domain with invalid period' do
+      Setting.days_to_renew_domain_before_expire = 0
+      old_balance = @registrar1.balance
+      old_activities = @registrar1.cash_account.account_activities.count
+
+      exp_date = domain.valid_to
+      xml = @epp_xml.domain.renew(
+        name: { value: domain.name },
+        curExpDate: { value: exp_date.to_date.to_s },
+        period: { value: '1', attrs: { unit: '' } }
+      )
+
+      response = epp_plain_request(xml, validate_input: false)
+      response[:results][0][:msg].should == 'Attribute is invalid: unit'
+      response[:results][0][:result_code].should == '2306'
+
+      xml = @epp_xml.domain.renew(
+        name: { value: domain.name },
+        curExpDate: { value: exp_date.to_date.to_s },
+        period: { value: '1', attrs: { unit: 'bla' } }
+      )
+
+      response = epp_plain_request(xml, validate_input: false)
+      response[:results][0][:msg].should == 'Attribute is invalid: unit'
+      response[:results][0][:result_code].should == '2306'
+
+      Setting.days_to_renew_domain_before_expire = 90
+    end
+
     it 'renews a domain with 2 year period' do
       old_balance = @registrar1.balance
       old_activities = @registrar1.cash_account.account_activities.count