Automatic merge of gitlab-org/gitlab-ce master

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

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

app/workers/pages_domain_removal_cron_worker.rb

+# 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

changelogs/unreleased/remove-disabled-pages-domains-part-2.yml

+---
+title: Remove pages domains if they weren't verified for 1 week
+merge_request: 26227
+author:
+type: added

config/initializers/1_settings.rb

@@ -422,6 +422,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'

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

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

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
+        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_within(1.second).of(1.week.from_now)
-          end
+
+          expect(domain.remove_at).to be_like_time(7.days.from_now)
         end
       end
 

spec/workers/pages_domain_removal_cron_worker_spec.rb

+# 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

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