Merge branch 'remove-disabled-pages-domains-part-2' into 'master'

Remove pages domain scheduled for removal

See merge request gitlab-org/gitlab-ce!26227

10 files changed, 112 insertions, 10 deletions

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