From 5107d2d3a9bbd361b7d78fe92de2d9a70f567c8f Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 22 Mar 2019 17:52:09 +0200 Subject: [PATCH 1/2] Refactor `force delete` procedure - Change `domains.force_delete_at` database column type to date, rename to `force_delete_date` - Save `force_delete_date` in application timezone - Improve tests Fixes #812, #253 --- app/models/concerns/domain/force_delete.rb | 5 ++-- app/models/domain.rb | 2 +- app/models/domain_cron.rb | 2 +- app/models/whois_record.rb | 3 ++- app/presenters/domain_presenter.rb | 2 +- .../admin/domains/partials/_general.html.erb | 4 +-- .../domains/partials/_general.html.erb | 4 +-- config/locales/admin/domains.en.yml | 1 - ..._change_domains_force_delete_at_to_date.rb | 5 ++++ ...ns_force_delete_at_to_force_delete_date.rb | 5 ++++ db/structure.sql | 6 ++++- doc/registrant-api/v1/domain.md | 6 ++--- doc/registrant-api/v1/registry_lock.md | 4 +-- doc/repp/v1/domain.md | 2 +- lib/serializers/registrant_api/domain.rb | 2 +- spec/presenters/domain_presenter_spec.rb | 6 ++--- test/fixtures/domains.yml | 2 +- .../serializers/registrant_api/domain_test.rb | 2 +- test/models/domain/force_delete_test.rb | 27 ++++++++++++++----- .../registrant_area/domains/details_test.rb | 5 +++- 20 files changed, 62 insertions(+), 33 deletions(-) create mode 100644 db/migrate/20190322152123_change_domains_force_delete_at_to_date.rb create mode 100644 db/migrate/20190322152529_rename_domains_force_delete_at_to_force_delete_date.rb diff --git a/app/models/concerns/domain/force_delete.rb b/app/models/concerns/domain/force_delete.rb index 9f69316d7..5247ce93d 100644 --- a/app/models/concerns/domain/force_delete.rb +++ b/app/models/concerns/domain/force_delete.rb @@ -12,8 +12,7 @@ module Concerns::Domain::ForceDelete preserve_current_statuses_for_force_delete add_force_delete_statuses - self.force_delete_at = (Time.zone.now + (Setting.redemption_grace_period.days + 1.day)).utc - .beginning_of_day + self.force_delete_date = Time.zone.today + Setting.redemption_grace_period.days + 1.day stop_all_pending_actions allow_deletion save(validate: false) @@ -22,7 +21,7 @@ module Concerns::Domain::ForceDelete def cancel_force_delete restore_statuses_before_force_delete remove_force_delete_statuses - self.force_delete_at = nil + self.force_delete_date = nil save(validate: false) end diff --git a/app/models/domain.rb b/app/models/domain.rb index c0e89d34f..fd5d920c2 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -501,7 +501,7 @@ class Domain < ActiveRecord::Base when DomainStatus::PENDING_DELETE self.delete_at = nil when DomainStatus::SERVER_MANUAL_INZONE # removal causes server hold to set - self.outzone_at = Time.zone.now if self.force_delete_at.present? + self.outzone_at = Time.zone.now if force_delete_scheduled? when DomainStatus::DomainStatus::EXPIRED # removal causes server hold to set self.outzone_at = self.expire_time + 15.day when DomainStatus::DomainStatus::SERVER_HOLD # removal causes server hold to set diff --git a/app/models/domain_cron.rb b/app/models/domain_cron.rb index 2d208d647..c05167a82 100644 --- a/app/models/domain_cron.rb +++ b/app/models/domain_cron.rb @@ -84,7 +84,7 @@ class DomainCron c = 0 - Domain.where('force_delete_at <= ?', Time.zone.now.end_of_day.utc).each do |x| + Domain.where('force_delete_date <= ?', Time.zone.now.end_of_day.utc).each do |x| DomainDeleteJob.enqueue(x.id, run_at: rand(((24*60) - (DateTime.now.hour * 60 + DateTime.now.minute))).minutes.from_now) STDOUT << "#{Time.zone.now.utc} DomainCron.destroy_delete_candidates: job added by force delete time ##{x.id} (#{x.name})\n" unless Rails.env.test? c += 1 diff --git a/app/models/whois_record.rb b/app/models/whois_record.rb index b2a0349aa..ad0036315 100644 --- a/app/models/whois_record.rb +++ b/app/models/whois_record.rb @@ -40,7 +40,8 @@ class WhoisRecord < ActiveRecord::Base h[:changed] = domain.updated_at.try(:to_s, :iso8601) h[:expire] = domain.valid_to.to_date.to_s h[:outzone] = domain.outzone_at.try(:to_date).try(:to_s) - h[:delete] = [domain.delete_at, domain.force_delete_at].compact.min.try(:to_date).try(:to_s) + h[:delete] = [domain.delete_at, domain.force_delete_date].compact.min.try(:to_date) + .try(:to_s) h[:registrant] = registrant.name h[:registrant_kind] = registrant.kind diff --git a/app/presenters/domain_presenter.rb b/app/presenters/domain_presenter.rb index 445e5b87d..d58163aae 100644 --- a/app/presenters/domain_presenter.rb +++ b/app/presenters/domain_presenter.rb @@ -39,7 +39,7 @@ class DomainPresenter end def force_delete_date - view.l(domain.force_delete_at, format: :date) if domain.force_delete_at + view.l(domain.force_delete_date) if domain.force_delete_scheduled? end def admin_contact_names diff --git a/app/views/admin/domains/partials/_general.html.erb b/app/views/admin/domains/partials/_general.html.erb index 3c6d3e986..feeaeb01f 100644 --- a/app/views/admin/domains/partials/_general.html.erb +++ b/app/views/admin/domains/partials/_general.html.erb @@ -31,8 +31,8 @@
<%= t('.delete_time') %>
<%= l(@domain.delete_at) %>
-
<%= Domain.human_attribute_name :force_delete_at %>
-
<%= l @domain.force_delete_at %>
+
<%= Domain.human_attribute_name :force_delete_date %>
+
<%= l @domain.force_delete_date %>
<%= t('.locked_by_registrant_at') %>
<%= l(@domain.locked_by_registrant_at) %>
diff --git a/app/views/registrant/domains/partials/_general.html.erb b/app/views/registrant/domains/partials/_general.html.erb index 46d55bd85..842d4e8e3 100644 --- a/app/views/registrant/domains/partials/_general.html.erb +++ b/app/views/registrant/domains/partials/_general.html.erb @@ -31,8 +31,8 @@
<%= Domain.human_attribute_name :delete_at %>
<%= l(@domain.delete_at) %>
-
<%= Domain.human_attribute_name :force_delete_at %>
-
<%= l(@domain.force_delete_at) %>
+
<%= Domain.human_attribute_name :force_delete_date %>
+
<%= l @domain.force_delete_date %>
diff --git a/config/locales/admin/domains.en.yml b/config/locales/admin/domains.en.yml index c4fd1ccb2..9e1b98781 100644 --- a/config/locales/admin/domains.en.yml +++ b/config/locales/admin/domains.en.yml @@ -52,7 +52,6 @@ en: general: outzone_time: Outzone time delete_time: Delete time - force_delete_time: Force delete time locked_by_registrant_at: Registry lock time admin_contacts: diff --git a/db/migrate/20190322152123_change_domains_force_delete_at_to_date.rb b/db/migrate/20190322152123_change_domains_force_delete_at_to_date.rb new file mode 100644 index 000000000..e0264681a --- /dev/null +++ b/db/migrate/20190322152123_change_domains_force_delete_at_to_date.rb @@ -0,0 +1,5 @@ +class ChangeDomainsForceDeleteAtToDate < ActiveRecord::Migration + def change + change_column :domains, :force_delete_at, :date + end +end diff --git a/db/migrate/20190322152529_rename_domains_force_delete_at_to_force_delete_date.rb b/db/migrate/20190322152529_rename_domains_force_delete_at_to_force_delete_date.rb new file mode 100644 index 000000000..c722bc257 --- /dev/null +++ b/db/migrate/20190322152529_rename_domains_force_delete_at_to_force_delete_date.rb @@ -0,0 +1,5 @@ +class RenameDomainsForceDeleteAtToForceDeleteDate < ActiveRecord::Migration + def change + rename_column :domains, :force_delete_at, :force_delete_date + end +end diff --git a/db/structure.sql b/db/structure.sql index 2417e89cd..88871b4ef 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -936,7 +936,7 @@ CREATE TABLE public.domains ( registrant_verification_asked_at timestamp without time zone, registrant_verification_token character varying, pending_json jsonb, - force_delete_at timestamp without time zone, + force_delete_date date, statuses character varying[], reserved boolean DEFAULT false, status_notes public.hstore, @@ -4935,3 +4935,7 @@ INSERT INTO schema_migrations (version) VALUES ('20190311111718'); INSERT INTO schema_migrations (version) VALUES ('20190312211614'); +INSERT INTO schema_migrations (version) VALUES ('20190322152123'); + +INSERT INTO schema_migrations (version) VALUES ('20190322152529'); + diff --git a/doc/registrant-api/v1/domain.md b/doc/registrant-api/v1/domain.md index eeb1bfccc..883745dff 100644 --- a/doc/registrant-api/v1/domain.md +++ b/doc/registrant-api/v1/domain.md @@ -71,7 +71,7 @@ Content-Type: application/json "pending_json":{ }, - "force_delete_at":null, + "force_delete_date":null, "statuses":[ "ok" ], @@ -187,7 +187,7 @@ Content-Type: application/json "pending_json":{ }, - "force_delete_at":null, + "force_delete_date":null, "statuses":[ "ok" ], @@ -295,7 +295,7 @@ Content-Type: application/json "pending_json":{ }, - "force_delete_at":null, + "force_delete_date":null, "statuses":[ "ok" ], diff --git a/doc/registrant-api/v1/registry_lock.md b/doc/registrant-api/v1/registry_lock.md index 553c7b28e..dea3ea5a3 100644 --- a/doc/registrant-api/v1/registry_lock.md +++ b/doc/registrant-api/v1/registry_lock.md @@ -71,7 +71,7 @@ Content-Type: application/json "pending_json":{ }, - "force_delete_at":null, + "force_delete_date":null, "statuses":[ "serverUpdateProhibited", "serverDeleteProhibited", @@ -216,7 +216,7 @@ Content-Type: application/json "pending_json":{ }, - "force_delete_at":null, + "force_delete_date":null, "statuses":[ "ok" ], diff --git a/doc/repp/v1/domain.md b/doc/repp/v1/domain.md index 4c9476125..8c46240a7 100644 --- a/doc/repp/v1/domain.md +++ b/doc/repp/v1/domain.md @@ -57,7 +57,7 @@ Content-Type: application/json "registrant_verification_token": null, "pending_json": { }, - "force_delete_at": null, + "force_delete_date": null, "statuses": [ "ok" ], diff --git a/lib/serializers/registrant_api/domain.rb b/lib/serializers/registrant_api/domain.rb index d6adf61ee..feec4bd15 100644 --- a/lib/serializers/registrant_api/domain.rb +++ b/lib/serializers/registrant_api/domain.rb @@ -40,7 +40,7 @@ module Serializers registrant_verification_asked_at: domain.registrant_verification_asked_at, registrant_verification_token: domain.registrant_verification_token, pending_json: domain.pending_json, - force_delete_at: domain.force_delete_at, + force_delete_date: domain.force_delete_date, statuses: domain.statuses, locked_by_registrant_at: domain.locked_by_registrant_at, reserved: domain.reserved, diff --git a/spec/presenters/domain_presenter_spec.rb b/spec/presenters/domain_presenter_spec.rb index ff4fc2841..0b475d6fd 100644 --- a/spec/presenters/domain_presenter_spec.rb +++ b/spec/presenters/domain_presenter_spec.rb @@ -63,16 +63,16 @@ RSpec.describe DomainPresenter do subject(:force_delete_date) { presenter.force_delete_date } context 'when present' do - let(:domain) { instance_double(Domain, force_delete_at: '05.07.2010') } + let(:domain) { instance_double(Domain, force_delete_date: '05.07.2010', force_delete_scheduled?: true) } it 'returns localized date' do - expect(view).to receive(:l).with('05.07.2010', format: :date).and_return('delete date') + expect(view).to receive(:l).with('05.07.2010').and_return('delete date') expect(force_delete_date).to eq('delete date') end end context 'when absent' do - let(:domain) { instance_double(Domain, force_delete_at: nil) } + let(:domain) { instance_double(Domain, force_delete_date: nil, force_delete_scheduled?: false) } specify { expect(force_delete_date).to be_nil } end diff --git a/test/fixtures/domains.yml b/test/fixtures/domains.yml index 792953bbd..cce7298cf 100644 --- a/test/fixtures/domains.yml +++ b/test/fixtures/domains.yml @@ -8,7 +8,7 @@ shop: valid_to: <%= Time.zone.parse('2010-07-05').to_s(:db) %> outzone_at: <%= Time.zone.parse('2010-07-06').to_s(:db) %> delete_at: <%= Time.zone.parse('2010-07-07').to_s(:db) %> - force_delete_at: <%= Time.zone.parse('2010-07-08').to_s(:db) %> + force_delete_date: 2010-07-08 period: 1 period_unit: m uuid: 1b3ee442-e8fe-4922-9492-8fcb9dccc69c diff --git a/test/lib/serializers/registrant_api/domain_test.rb b/test/lib/serializers/registrant_api/domain_test.rb index a592edf45..57b494ddb 100644 --- a/test/lib/serializers/registrant_api/domain_test.rb +++ b/test/lib/serializers/registrant_api/domain_test.rb @@ -73,7 +73,7 @@ class SerializersRegistrantApiDomainTest < ActiveSupport::TestCase registrant tech_contacts admin_contacts transfer_code name_dirty name_puny period period_unit creator_str updator_str legacy_id legacy_registrar_id legacy_registrant_id outzone_at delete_at registrant_verification_asked_at - registrant_verification_token pending_json force_delete_at statuses + registrant_verification_token pending_json force_delete_date statuses locked_by_registrant_at reserved status_notes nameservers] assert_equal(keys, @json.keys & keys) diff --git a/test/models/domain/force_delete_test.rb b/test/models/domain/force_delete_test.rb index 646a34015..9092fad86 100644 --- a/test/models/domain/force_delete_test.rb +++ b/test/models/domain/force_delete_test.rb @@ -3,21 +3,23 @@ require 'test_helper' class DomainForceDeleteTest < ActiveSupport::TestCase setup do @domain = domains(:shop) + @original_redemption_grace_period = Setting.redemption_grace_period end - def test_schedule_force_delete - @original_redemption_grace_period = Setting.redemption_grace_period + teardown do + Setting.redemption_grace_period = @original_redemption_grace_period + end + + def test_schedules_force_delete + assert_not @domain.force_delete_scheduled? Setting.redemption_grace_period = 30 - travel_to Time.zone.parse('2010-07-05 00:00') + travel_to Time.zone.parse('2010-07-05') @domain.schedule_force_delete @domain.reload assert @domain.force_delete_scheduled? - assert_equal Time.zone.parse('2010-08-04 03:00'), @domain.force_delete_at - - travel_back - Setting.redemption_grace_period = @original_redemption_grace_period + assert_equal Date.parse('2010-08-05'), @domain.force_delete_date end def test_scheduling_force_delete_adds_corresponding_statuses @@ -80,6 +82,17 @@ class DomainForceDeleteTest < ActiveSupport::TestCase end end + def test_cancels_force_delete + @domain.update_columns(statuses: [DomainStatus::FORCE_DELETE], force_delete_date: '2010-07-05') + assert @domain.force_delete_scheduled? + + @domain.cancel_force_delete + @domain.reload + + assert_not @domain.force_delete_scheduled? + assert_nil @domain.force_delete_date + end + def test_cancelling_force_delete_bypasses_validation @domain = domains(:invalid) @domain.schedule_force_delete diff --git a/test/system/registrant_area/domains/details_test.rb b/test/system/registrant_area/domains/details_test.rb index 875203c1b..b23686b97 100644 --- a/test/system/registrant_area/domains/details_test.rb +++ b/test/system/registrant_area/domains/details_test.rb @@ -7,7 +7,10 @@ class RegistrantAreaDomainDetailsTest < ApplicationSystemTestCase end def test_general_data + @domain.update_columns(force_delete_date: '2010-07-08', statuses: [DomainStatus::FORCE_DELETE]) + visit registrant_domain_url(@domain) + assert_text 'Name shop.test' assert_text "Registered at #{l Time.zone.parse('2010-07-04')}" assert_link 'Best Names', href: registrant_registrar_path(@domain.registrar) @@ -18,7 +21,7 @@ class RegistrantAreaDomainDetailsTest < ApplicationSystemTestCase assert_text "Valid to #{l Time.zone.parse('2010-07-05')}" assert_text "Outzone at #{l Time.zone.parse('2010-07-06')}" assert_text "Delete at #{l Time.zone.parse('2010-07-07')}" - assert_text "Force delete at #{l Time.zone.parse('2010-07-08')}" + assert_text "Force delete date #{l Date.parse('2010-07-08')}" end def test_registrant From 6d40e9ec6a20e7f79deb8c291a1868d234111740 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 22 Mar 2019 19:06:10 +0200 Subject: [PATCH 2/2] Support "force delete" procedure when releasing a domain - `DomainCron.destroy_delete_candidates` runner is removed from `config/schedule.rb` with `domains:release` rake task as a replacement Closes #1119 --- app/models/concerns/domain/releasable.rb | 8 ++++++-- app/models/domain_cron.rb | 14 -------------- config/schedule.rb | 4 ---- test/models/domain/releasable/auctionable_test.rb | 9 +++++++++ test/models/domain/releasable/discardable_test.rb | 10 ++++++++++ 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/app/models/concerns/domain/releasable.rb b/app/models/concerns/domain/releasable.rb index c25af44be..8a5e7b5a5 100644 --- a/app/models/concerns/domain/releasable.rb +++ b/app/models/concerns/domain/releasable.rb @@ -15,13 +15,17 @@ module Concerns def releasable_domains if release_to_auction - where('delete_at < ? AND ? != ALL(coalesce(statuses, array[]::varchar[]))', + where('(delete_at < ? OR force_delete_date <= ?)' \ + ' AND ? != ALL(coalesce(statuses, array[]::varchar[]))', Time.zone.now, + Time.zone.today, DomainStatus::SERVER_DELETE_PROHIBITED) else - where('delete_at < ? AND ? != ALL(coalesce(statuses, array[]::varchar[])) AND' \ + where('(delete_at < ? OR force_delete_date <= ?)' \ + ' AND ? != ALL(coalesce(statuses, array[]::varchar[])) AND' \ ' ? != ALL(COALESCE(statuses, array[]::varchar[]))', Time.zone.now, + Time.zone.today, DomainStatus::SERVER_DELETE_PROHIBITED, DomainStatus::DELETE_CANDIDATE) end diff --git a/app/models/domain_cron.rb b/app/models/domain_cron.rb index c05167a82..1414d4dc5 100644 --- a/app/models/domain_cron.rb +++ b/app/models/domain_cron.rb @@ -78,18 +78,4 @@ class DomainCron STDOUT << "#{Time.zone.now.utc} - Successfully set server_hold to #{marked} of #{real} domains\n" unless Rails.env.test? marked end - - def self.destroy_delete_candidates - STDOUT << "#{Time.zone.now.utc} - Destroying domains\n" unless Rails.env.test? - - c = 0 - - Domain.where('force_delete_date <= ?', Time.zone.now.end_of_day.utc).each do |x| - DomainDeleteJob.enqueue(x.id, run_at: rand(((24*60) - (DateTime.now.hour * 60 + DateTime.now.minute))).minutes.from_now) - STDOUT << "#{Time.zone.now.utc} DomainCron.destroy_delete_candidates: job added by force delete time ##{x.id} (#{x.name})\n" unless Rails.env.test? - c += 1 - end - - STDOUT << "#{Time.zone.now.utc} - Job destroy added for #{c} domains\n" unless Rails.env.test? - end end diff --git a/config/schedule.rb b/config/schedule.rb index f58e5b07b..d47b45ea9 100644 --- a/config/schedule.rb +++ b/config/schedule.rb @@ -38,10 +38,6 @@ if @cron_group == 'registry' runner 'Certificate.update_crl' end - every 42.minutes do - runner 'DomainCron.destroy_delete_candidates' - end - every 45.minutes do runner 'DomainCron.start_expire_period' end diff --git a/test/models/domain/releasable/auctionable_test.rb b/test/models/domain/releasable/auctionable_test.rb index 84f298b0a..b4cb9098b 100644 --- a/test/models/domain/releasable/auctionable_test.rb +++ b/test/models/domain/releasable/auctionable_test.rb @@ -40,6 +40,15 @@ class DomainReleasableAuctionableTest < ActiveSupport::TestCase assert_not @domain.domain_name.at_auction? end + def test_sells_domains_with_scheduled_force_delete_procedure_at_auction + @domain.update!(force_delete_date: '2010-07-05') + travel_to Time.zone.parse('2010-07-05') + + Domain.release_domains + + assert @domain.domain_name.at_auction? + end + def test_deletes_registered_domain @domain.update!(delete_at: Time.zone.parse('2010-07-05 07:59')) travel_to Time.zone.parse('2010-07-05 08:00') diff --git a/test/models/domain/releasable/discardable_test.rb b/test/models/domain/releasable/discardable_test.rb index b8b6aff59..b1603e7fa 100644 --- a/test/models/domain/releasable/discardable_test.rb +++ b/test/models/domain/releasable/discardable_test.rb @@ -15,6 +15,16 @@ class DomainReleasableDiscardableTest < ActiveSupport::TestCase assert @domain.discarded? end + def test_discards_domains_with_scheduled_force_delete_procedure + @domain.update!(force_delete_date: '2010-07-05') + travel_to Time.zone.parse('2010-07-05') + + Domain.release_domains + @domain.reload + + assert @domain.discarded? + end + def test_ignores_domains_with_delete_at_in_the_future_or_now @domain.update!(delete_at: Time.zone.parse('2010-07-05 08:00')) travel_to Time.zone.parse('2010-07-05 08:00')