From ac744fd4fceb996dcafe7958dd35b081331f46fe Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Tue, 30 Apr 2019 12:05:54 +0000 Subject: [PATCH] Remove disabled pages domains Domain will be removed by verification worker after 1 week of being disabled --- app/models/pages_domain.rb | 2 + app/workers/all_queues.yml | 1 + .../pages_domain_removal_cron_worker.rb | 16 +++++++ .../remove-disabled-pages-domains-part-2.yml | 5 +++ config/initializers/1_settings.rb | 4 ++ spec/factories/pages_domains.rb | 4 ++ spec/models/pages_domain_spec.rb | 28 +++++++++++++ .../verify_pages_domain_service_spec.rb | 12 +++--- .../pages_domain_removal_cron_worker_spec.rb | 42 +++++++++++++++++++ ...es_domain_verification_cron_worker_spec.rb | 8 ++-- 10 files changed, 112 insertions(+), 10 deletions(-) create mode 100644 app/workers/pages_domain_removal_cron_worker.rb create mode 100644 changelogs/unreleased/remove-disabled-pages-domains-part-2.yml create mode 100644 spec/workers/pages_domain_removal_cron_worker_spec.rb diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index d73b2889f30..9e806b2e232 100644 --- a/app/models/pages_domain.rb +++ b/app/models/pages_domain.rb @@ -38,6 +38,8 @@ class PagesDomain < ApplicationRecord where(verified_at.eq(nil).or(enabled_until.eq(nil).or(enabled_until.lt(threshold)))) end + scope :for_removal, -> { where("remove_at < ?", Time.now) } + def verified? !!verified_at end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 7cf2e7100d5..e4e85de93da 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -6,6 +6,7 @@ - cronjob:gitlab_usage_ping - cronjob:import_export_project_cleanup - cronjob:pages_domain_verification_cron +- cronjob:pages_domain_removal_cron - cronjob:pipeline_schedule - cronjob:prune_old_events - cronjob:remove_expired_group_links diff --git a/app/workers/pages_domain_removal_cron_worker.rb b/app/workers/pages_domain_removal_cron_worker.rb new file mode 100644 index 00000000000..3aca123e5ac --- /dev/null +++ b/app/workers/pages_domain_removal_cron_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class PagesDomainRemovalCronWorker + include ApplicationWorker + include CronjobQueue + + def perform + return unless Feature.enabled?(:remove_disabled_domains) + + PagesDomain.for_removal.find_each do |domain| + domain.destroy! + rescue => e + Raven.capture_exception(e) + end + end +end diff --git a/changelogs/unreleased/remove-disabled-pages-domains-part-2.yml b/changelogs/unreleased/remove-disabled-pages-domains-part-2.yml new file mode 100644 index 00000000000..9b208cbaa0e --- /dev/null +++ b/changelogs/unreleased/remove-disabled-pages-domains-part-2.yml @@ -0,0 +1,5 @@ +--- +title: Remove pages domains if they weren't verified for 1 week +merge_request: 26227 +author: +type: added diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 3c426cdb969..e9b36873d75 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -338,6 +338,10 @@ Settings.cron_jobs['pages_domain_verification_cron_worker'] ||= Settingslogic.ne Settings.cron_jobs['pages_domain_verification_cron_worker']['cron'] ||= '*/15 * * * *' Settings.cron_jobs['pages_domain_verification_cron_worker']['job_class'] = 'PagesDomainVerificationCronWorker' +Settings.cron_jobs['pages_domain_removal_cron_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['pages_domain_removal_cron_worker']['cron'] ||= '47 0 * * *' +Settings.cron_jobs['pages_domain_removal_cron_worker']['job_class'] = 'PagesDomainRemovalCronWorker' + Settings.cron_jobs['issue_due_scheduler_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['issue_due_scheduler_worker']['cron'] ||= '50 00 * * *' Settings.cron_jobs['issue_due_scheduler_worker']['job_class'] = 'IssueDueSchedulerWorker' diff --git a/spec/factories/pages_domains.rb b/spec/factories/pages_domains.rb index b74f72f2bd3..db8384877b0 100644 --- a/spec/factories/pages_domains.rb +++ b/spec/factories/pages_domains.rb @@ -45,6 +45,10 @@ nNp/xedE1YxutQ== remove_at { 1.day.from_now } end + trait :should_be_removed do + remove_at { 1.day.ago } + end + trait :unverified do verified_at nil end diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 142ddebbbf8..ec4d4517f82 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -344,4 +344,32 @@ describe PagesDomain do end end end + + describe '.for_removal' do + subject { described_class.for_removal } + + context 'when domain is not schedule for removal' do + let!(:domain) { create :pages_domain } + + it 'does not return domain' do + is_expected.to be_empty + end + end + + context 'when domain is scheduled for removal yesterday' do + let!(:domain) { create :pages_domain, remove_at: 1.day.ago } + + it 'returns domain' do + is_expected.to eq([domain]) + end + end + + context 'when domain is scheduled for removal tomorrow' do + let!(:domain) { create :pages_domain, remove_at: 1.day.from_now } + + it 'does not return domain' do + is_expected.to be_empty + end + end + end end diff --git a/spec/services/verify_pages_domain_service_spec.rb b/spec/services/verify_pages_domain_service_spec.rb index e5c7b5bb9a7..f2b3b44d223 100644 --- a/spec/services/verify_pages_domain_service_spec.rb +++ b/spec/services/verify_pages_domain_service_spec.rb @@ -57,12 +57,12 @@ describe VerifyPagesDomainService do expect(domain).not_to be_verified end - it 'disables domain and shedules it for removal' do - Timecop.freeze do - service.execute - expect(domain).not_to be_enabled - expect(domain.remove_at).to be_within(1.second).of(1.week.from_now) - end + it 'disables domain and shedules it for removal in 1 week' do + service.execute + + expect(domain).not_to be_enabled + + expect(domain.remove_at).to be_like_time(7.days.from_now) end end diff --git a/spec/workers/pages_domain_removal_cron_worker_spec.rb b/spec/workers/pages_domain_removal_cron_worker_spec.rb new file mode 100644 index 00000000000..0e1171e8491 --- /dev/null +++ b/spec/workers/pages_domain_removal_cron_worker_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PagesDomainRemovalCronWorker do + subject(:worker) { described_class.new } + + describe '#perform' do + context 'when there is domain which should be removed' do + let!(:domain_for_removal) { create(:pages_domain, :should_be_removed) } + + before do + stub_feature_flags(remove_disabled_domains: true) + end + + it 'removes domain' do + expect { worker.perform }.to change { PagesDomain.count }.by(-1) + expect(PagesDomain.exists?).to eq(false) + end + + context 'when domain removal is disabled' do + before do + stub_feature_flags(remove_disabled_domains: false) + end + + it 'does not remove pages domain' do + expect { worker.perform }.not_to change { PagesDomain.count } + expect(PagesDomain.find_by(domain: domain_for_removal.domain)).to be_present + end + end + end + + context 'where there is a domain which scheduled for removal in the future' do + let!(:domain_for_removal) { create(:pages_domain, :scheduled_for_removal) } + + it 'does not remove pages domain' do + expect { worker.perform }.not_to change { PagesDomain.count } + expect(PagesDomain.find_by(domain: domain_for_removal.domain)).to be_present + end + end + end +end diff --git a/spec/workers/pages_domain_verification_cron_worker_spec.rb b/spec/workers/pages_domain_verification_cron_worker_spec.rb index 9b479da1cb6..186824a444f 100644 --- a/spec/workers/pages_domain_verification_cron_worker_spec.rb +++ b/spec/workers/pages_domain_verification_cron_worker_spec.rb @@ -6,11 +6,11 @@ describe PagesDomainVerificationCronWorker do subject(:worker) { described_class.new } describe '#perform' do - it 'enqueues a PagesDomainVerificationWorker for domains needing verification' do - verified = create(:pages_domain) - reverify = create(:pages_domain, :reverify) - disabled = create(:pages_domain, :disabled) + let!(:verified) { create(:pages_domain) } + let!(:reverify) { create(:pages_domain, :reverify) } + let!(:disabled) { create(:pages_domain, :disabled) } + it 'enqueues a PagesDomainVerificationWorker for domains needing verification' do [reverify, disabled].each do |domain| expect(PagesDomainVerificationWorker).to receive(:perform_async).with(domain.id) end