Merge pull request #2137 from internetee/2135-fix-possible-transaction-retries

Fix possible transaction retries on domain renew

Gemfile

@@ -92,6 +92,7 @@ group :test do
   gem 'database_cleaner'
   gem 'minitest', '~> 5.14'
   gem 'simplecov', '0.17.1', require: false # CC last supported v0.17
+  gem 'spy'
   gem 'webdrivers'
   gem 'webmock'
 end

Gemfile.lock

@@ -456,6 +456,7 @@ GEM
       actionpack (>= 4.0)
       activesupport (>= 4.0)
       sprockets (>= 3.0.0)
+    spy (1.0.1)
     swd (1.2.0)
       activesupport (>= 3)
       attr_required (>= 0.0.5)
@@ -568,6 +569,7 @@ DEPENDENCIES
   sidekiq
   simplecov (= 0.17.1)
   simpleidn (= 0.2.1)
+  spy
   truemail (~> 2.4)
   uglifier
   validates_email_format_of (= 1.6.3)
@@ -577,4 +579,4 @@ DEPENDENCIES
   wkhtmltopdf-binary (~> 0.12.5.1)
 
 BUNDLED WITH
-   2.2.24
+   2.2.26

app/interactions/domains/bulk_renew/single_domain_renew.rb

@@ -8,9 +8,12 @@ module Domains
       object :registrar
 
       def execute
+        renewed_expire_time = prepare_renewed_expire_time
         in_transaction_with_retries do
           check_balance
-          success = domain.renew(domain.valid_to, period, unit)
+          success = domain.renew(renewed_expire_time: renewed_expire_time,
+                                 period: period,
+                                 unit: unit)
           if success
             check_balance
             reduce_balance
@@ -42,7 +45,8 @@ module Domains
         else
           transaction_wrapper { yield }
         end
-      rescue ActiveRecord::StatementInvalid
+      rescue ActiveRecord::StatementInvalid => e
+        log_error e
         sleep rand / 100
         retry
       end
@@ -59,6 +63,26 @@ module Domains
         domain.add_epp_error(2104, nil, nil, I18n.t(:domain_renew_error_for_domain))
         errors.add(:domain, I18n.t('domain_renew_error_for_domain', domain: domain.name))
       end
+
+      def prepare_renewed_expire_time
+        int_period = period.to_i
+        plural_period_unit_name = (unit == 'm' ? 'months' : 'years').to_sym
+        renewed_expire_time = domain.valid_to.advance(plural_period_unit_name => int_period.to_i)
+
+        max_reg_time = 11.years.from_now
+
+        if renewed_expire_time >= max_reg_time
+          domain.add_epp_error('2105', nil, nil, I18n.t('epp.domains.object_is_not_eligible_for_renewal',
+                                                        max_date: max_reg_time.to_date.to_s(:db)))
+        end
+        renewed_expire_time
+      end
+
+      def log_error(error)
+        message = (["#{self.class} - #{error.class}: #{error.message}"] + error.backtrace)
+                  .join("\n")
+        Rails.logger.error message
+      end
     end
   end
 end

app/models/epp/domain.rb

@@ -173,26 +173,13 @@ class Epp::Domain < Domain
 
   ### RENEW ###
 
-  def renew(cur_exp_date, period, unit = 'y')
+  def renew(renewed_expire_time:, period:, unit:)
     @is_renewal = true
-    validate_exp_dates(cur_exp_date)
 
     add_renew_epp_errors unless renewable?
 
     return false if errors.any?
 
-    period = period.to_i
-    plural_period_unit_name = (unit == 'm' ? 'months' : 'years').to_sym
-    renewed_expire_time = valid_to.advance(plural_period_unit_name => period.to_i)
-
-    max_reg_time = 11.years.from_now
-
-    if renewed_expire_time >= max_reg_time
-      add_epp_error('2105', nil, nil, I18n.t('epp.domains.object_is_not_eligible_for_renewal',
-                                             max_date: max_reg_time.to_date.to_s(:db)))
-      return false if errors.any?
-    end
-
     self.expire_time = renewed_expire_time
     self.outzone_at = nil
     self.delete_date = nil

test/integration/repp/v1/domains/renews_test.rb

@@ -13,6 +13,9 @@ class ReppV1DomainsRenewsTest < ActionDispatch::IntegrationTest
   def test_domain_can_be_renewed
     original_valid_to = @domain.valid_to
     travel_to Time.zone.parse('2010-07-05')
+    domain_spy = Spy.on_instance_method(Epp::Domain, :renew).and_call_through
+    renew_spy = Spy.on_instance_method(Domains::BulkRenew::SingleDomainRenew,
+                                       :prepare_renewed_expire_time).and_call_through
 
     @auth_headers['Content-Type'] = 'application/json'
     payload = { renew: { period: 1, period_unit: 'y', exp_date: original_valid_to } }
@@ -24,6 +27,8 @@ class ReppV1DomainsRenewsTest < ActionDispatch::IntegrationTest
     assert_equal 'Command completed successfully', json[:message]
 
     assert @domain.valid_to, original_valid_to + 1.year
+    assert domain_spy.has_been_called?
+    assert renew_spy.has_been_called?
   end
 
   def test_domain_renew_pricing_error
@@ -31,7 +36,7 @@ class ReppV1DomainsRenewsTest < ActionDispatch::IntegrationTest
     travel_to Time.zone.parse('2010-07-05')
 
     @auth_headers['Content-Type'] = 'application/json'
-    payload = { renew: { period: 100, period_unit: 'y', exp_date: original_valid_to } }
+    payload = { renew: { period: 10, period_unit: 'y', exp_date: original_valid_to } }
     post "/repp/v1/domains/#{@domain.name}/renew", headers: @auth_headers, params: payload.to_json
     json = JSON.parse(response.body, symbolize_names: true)
 

test/test_helper.rb

@@ -19,6 +19,7 @@ require 'capybara/minitest'
 require 'webmock/minitest'
 require 'support/assertions/epp_assertions'
 require 'sidekiq/testing'
+require 'spy/integration'
 
 Sidekiq::Testing.fake!