Merge branch 'pl-status-page-mvc-background-job' into 'master'

Add background worker to publish incident to status page

See merge request gitlab-org/gitlab!26717

app/workers/concerns/application_worker.rb

@@ -13,6 +13,17 @@ module ApplicationWorker
 
   included do
     set_queue
+
+    def structured_payload(payload = {})
+      context = Labkit::Context.current.to_h.merge(
+        'class' => self.class,
+        'job_status' => 'running',
+        'queue' => self.class.queue,
+        'jid' => jid
+      )
+
+      payload.stringify_keys.merge(context)
+    end
   end
 
   class_methods do

config/sidekiq_queues.yml

@@ -232,6 +232,8 @@
   - 2
 - - service_desk_email_receiver
   - 1
+- - status_page_publish_incident
+  - 1
 - - sync_seat_link_request
   - 1
 - - system_hook_push

ee/app/policies/ee/project_policy.rb

@@ -124,6 +124,11 @@ module EE
         @subject.feature_available?(:code_review_analytics, @user)
       end
 
+      condition(:status_page_available) do
+        @subject.feature_available?(:status_page, @user) &&
+          @subject.beta_feature_available?(:status_page)
+      end
+
       condition(:group_timelogs_available) do
         @subject.feature_available?(:group_timelogs)
       end
@@ -366,6 +371,8 @@ module EE
       end
 
       rule { requirements_available & owner }.enable :destroy_requirement
+
+      rule { status_page_available & can?(:developer_access) }.enable :publish_status_page
     end
 
     override :lookup_access_level!

ee/app/services/status_page/publish_incident_service.rb

 # frozen_string_literal: true
 
 module StatusPage
-  # Delegate work to more specific publishing services.
+  # Publishes content to status page by delegating to specific
+  # publishing services.
   #
-  # Use this service for publishing an incident to CDN which calls:
+  # Use this service for publishing an incident to CDN synchronously.
+  # To publish asynchronously use +StatusPage::TriggerPublishService+ instead.
+  #
+  # This services calls:
   # * StatusPage::PublishDetailsService
   # * StatusPage::PublishListService
   class PublishIncidentService
     include Gitlab::Utils::StrongMemoize
 
-    def initialize(project:, issue_id:)
+    def initialize(user:, project:, issue_id:)
+      @user = user
       @project = project
       @issue_id = issue_id
     end
 
     def execute
+      return error_permission_denied unless can_publish?
       return error_issue_not_found unless issue
 
       response = publish_details
@@ -25,7 +31,7 @@ module StatusPage
 
     private
 
-    attr_reader :project, :issue_id
+    attr_reader :user, :project, :issue_id
 
     def publish_details
       PublishDetailsService.new(project: project).execute(issue, user_notes)
@@ -55,8 +61,20 @@ module StatusPage
       end
     end
 
+    def can_publish?
+      user.can?(:publish_status_page, project)
+    end
+
+    def error_permission_denied
+      error('No publish permission')
+    end
+
     def error_issue_not_found
-      ServiceResponse.error(message: 'Issue not found')
+      error('Issue not found')
+    end
+
+    def error(message)
+      ServiceResponse.error(message: message)
     end
   end
 end

ee/app/services/status_page/trigger_publish_service.rb

+# frozen_string_literal: true
+
+module StatusPage
+  # Triggers a background job to publish of incidents to the status page.
+  #
+  # Use this service when issues/notes/emoji have changed to kickoff the
+  # publish process.
+  class TriggerPublishService
+    def initialize(user:, project:)
+      @user = user
+      @project = project
+    end
+
+    def execute(issue_id)
+      return unless can_publish?
+      return unless status_page_enabled?
+
+      StatusPage::PublishIncidentWorker
+        .perform_async(user.id, project.id, issue_id)
+    end
+
+    private
+
+    attr_reader :user, :project
+
+    def can_publish?
+      user&.can?(:publish_status_page, project)
+    end
+
+    def status_page_enabled?
+      project.status_page_setting&.enabled?
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -570,6 +570,13 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: 
+- :name: status_page_publish_incident
+  :feature_category: :status_page
+  :has_external_dependencies: true
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
 - :name: sync_seat_link_request
   :feature_category: :analysis
   :has_external_dependencies: true

ee/app/workers/status_page/publish_incident_worker.rb

+# frozen_string_literal: true
+
+module StatusPage
+  class PublishIncidentWorker
+    include ApplicationWorker
+    include Gitlab::Utils::StrongMemoize
+
+    sidekiq_options retry: 5
+
+    feature_category :status_page
+    worker_has_external_dependencies!
+    idempotent!
+
+    def perform(user_id, project_id, issue_id)
+      @user_id = user_id
+      @project_id = project_id
+      @issue_id = issue_id
+
+      return unless user && project
+
+      publish
+    end
+
+    private
+
+    attr_reader :user_id, :project_id, :issue_id
+
+    def publish
+      result = PublishIncidentService
+        .new(user: user, project: project, issue_id: issue_id)
+        .execute
+
+      log_info(result.message) if result.error?
+    end
+
+    def user
+      strong_memoize(:user) { User.find_by_id(user_id) }
+    end
+
+    def project
+      strong_memoize(:project) { Project.find_by_id(project_id) }
+    end
+
+    def log_info(message)
+      logger.info(structured_payload(message: message))
+    end
+  end
+end

ee/spec/policies/project_policy_spec.rb

@@ -1022,6 +1022,55 @@ describe ProjectPolicy do
     end
   end
 
+  describe 'publish_status_page' do
+    let(:anonymous) { nil }
+    let(:feature) { :status_page }
+    let(:policy) { :publish_status_page }
+
+    context 'when feature is available' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:role, :allowed) do
+        :anonymous  | false
+        :guest      | false
+        :reporter   | false
+        :developer  | true
+        :maintainer | true
+        :owner      | true
+        :admin      | true
+      end
+
+      with_them do
+        let(:current_user) { public_send(role) if role }
+
+        before do
+          stub_feature_flags(feature => true)
+          stub_licensed_features(feature => true)
+        end
+
+        it do
+          is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy))
+        end
+
+        context 'when feature is not available' do
+          before do
+            stub_licensed_features(feature => false)
+          end
+
+          it { is_expected.to be_disallowed(policy) }
+        end
+
+        context 'when feature flag is disabled' do
+          before do
+            stub_feature_flags(feature => false)
+          end
+
+          it { is_expected.to be_disallowed(policy) }
+        end
+      end
+    end
+  end
+
   context 'support bot' do
     let(:current_user) { User.support_bot }
 

ee/spec/services/status_page/publish_incident_service_spec.rb

@@ -3,17 +3,24 @@
 require 'spec_helper'
 
 describe StatusPage::PublishIncidentService do
+  let_it_be(:user) { create(:user) }
   let_it_be(:project, refind: true) { create(:project) }
   let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:settings) { create(:status_page_setting, :enabled, project: project) }
+  let(:user_can_publish) { true }
 
-  let(:service) { described_class.new(project: project, issue_id: issue.id) }
+  let(:service) do
+    described_class.new(user: user, project: project, issue_id: issue.id)
+  end
 
   subject(:result) { service.execute }
 
   describe '#execute' do
     before do
       stub_licensed_features(status_page: true)
+
+      allow(user).to receive(:can?).with(:publish_status_page, project)
+        .and_return(user_can_publish)
     end
 
     context 'when publishing succeeds' do
@@ -50,6 +57,15 @@ describe StatusPage::PublishIncidentService do
         expect(result.message).to eq('Issue not found')
       end
     end
+
+    context 'when user cannot publish' do
+      let(:user_can_publish) { false }
+
+      it 'returns error missing publish permission' do
+        expect(result).to be_error
+        expect(result.message).to eq('No publish permission')
+      end
+    end
   end
 
   private

ee/spec/services/status_page/trigger_publish_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe StatusPage::TriggerPublishService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project, refind: true) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
+  let(:service) { described_class.new(user: user, project: project) }
+  let(:worker) { StatusPage::PublishIncidentWorker }
+
+  let_it_be(:status_page_setting) do
+    create(:status_page_setting, :enabled, project: project)
+  end
+
+  subject { service.execute(issue.id) }
+
+  shared_examples 'no job scheduled' do
+    it 'does not schedule a job' do
+      expect(worker).not_to receive(:perform_async)
+
+      subject
+    end
+  end
+
+  describe '#execute' do
+    before do
+      project.add_maintainer(user)
+      stub_feature_flags(status_page: true)
+      stub_licensed_features(status_page: true)
+
+      allow(worker).to receive(:perform_async)
+        .with(user.id, project.id, issue.id)
+    end
+
+    it 'schedules a job' do
+      expect(worker).to receive(:perform_async)
+        .with(user.id, project.id, issue.id)
+
+      subject
+    end
+
+    context 'when status page is missing' do
+      before do
+        status_page_setting.destroy
+      end
+
+      include_examples 'no job scheduled'
+    end
+
+    context 'when status page is not enabled' do
+      before do
+        status_page_setting.update!(enabled: false)
+      end
+
+      include_examples 'no job scheduled'
+    end
+
+    context 'when license is not available' do
+      before do
+        stub_licensed_features(status_page: false)
+      end
+
+      include_examples 'no job scheduled'
+    end
+
+    context 'when feature is disabled' do
+      before do
+        stub_feature_flags(status_page: false)
+      end
+
+      include_examples 'no job scheduled'
+    end
+
+    context 'when user cannot publish status page' do
+      before do
+        project.add_reporter(user)
+      end
+
+      include_examples 'no job scheduled'
+    end
+  end
+end

ee/spec/workers/status_page/publish_incident_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe StatusPage::PublishIncidentWorker do
+  include ExclusiveLeaseHelpers
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
+
+  let(:worker) { described_class.new }
+  let(:logger) { worker.send(:logger) }
+  let(:service) { instance_double(StatusPage::PublishIncidentService) }
+  let(:service_result) { ServiceResponse.success }
+
+  before do
+    allow(StatusPage::PublishIncidentService)
+      .to receive(:new).with(user: user, project: project, issue_id: issue.id)
+      .and_return(service)
+    allow(service).to receive(:execute)
+      .and_return(service_result)
+  end
+
+  describe '#perform' do
+    subject { worker.perform(user.id, project.id, issue.id) }
+
+    it_behaves_like 'an idempotent worker' do
+      let(:job_args) { [user.id, project.id, issue.id] }
+
+      context 'when service succeeds' do
+        it 'execute the service' do
+          expect(service).to receive(:execute)
+
+          subject
+        end
+      end
+
+      context 'with unknown project' do
+        let(:project) { build(:project) }
+
+        it 'does not execute the service' do
+          expect(StatusPage::PublishIncidentService).not_to receive(:execute)
+
+          subject
+        end
+      end
+
+      context 'when service returns an error' do
+        let(:error_message) { 'some message' }
+        let(:service_result) { ServiceResponse.error(message: error_message) }
+
+        it 'succeeds and logs the error' do
+          expect(logger)
+            .to receive(:info)
+            .with(a_hash_including('message' => error_message))
+            .exactly(worker_exec_times).times
+
+          subject
+        end
+      end
+    end
+
+    context 'when service raises an exception' do
+      let(:error_message) { 'some exception' }
+      let(:exception) { StandardError.new(error_message) }
+
+      it 're-raises exception' do
+        allow(service).to receive(:execute).and_raise(exception)
+
+        expect { subject }.to raise_error(exception)
+      end
+    end
+  end
+end

spec/support/shared_examples/workers/idempotency_shared_examples.rb

@@ -16,6 +16,8 @@
 #   end
 #
 RSpec.shared_examples 'an idempotent worker' do
+  let(:worker_exec_times) { IdempotentWorkerHelper::WORKER_EXEC_TIMES }
+
   # Avoid stubbing calls for a more accurate run.
   subject do
     defined?(job_args) ? perform_multiple(job_args) : perform_multiple

spec/workers/concerns/application_worker_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe ApplicationWorker do
-  let(:worker) do
+  let_it_be(:worker) do
     Class.new do
       def self.name
         'Gitlab::Foo::Bar::DummyWorker'
@@ -13,12 +13,51 @@ describe ApplicationWorker do
     end
   end
 
+  let(:instance) { worker.new }
+
   describe 'Sidekiq options' do
     it 'sets the queue name based on the class name' do
       expect(worker.sidekiq_options['queue']).to eq('foo_bar_dummy')
     end
   end
 
+  describe '#structured_payload' do
+    let(:payload) { {} }
+
+    subject(:result) { instance.structured_payload(payload) }
+
+    it 'adds worker related payload' do
+      instance.jid = 'a jid'
+
+      expect(result).to include(
+        'class' => worker.class,
+        'job_status' => 'running',
+        'queue' => worker.queue,
+        'jid' => instance.jid
+      )
+    end
+
+    it 'adds labkit context' do
+      user = build_stubbed(:user, username: 'jane-doe')
+
+      instance.with_context(user: user) do
+        expect(result).to include('meta.user' => user.username)
+      end
+    end
+
+    it 'adds custom payload converting stringified keys' do
+      payload[:message] = 'some message'
+
+      expect(result).to include('message' => payload[:message])
+    end
+
+    it 'does not override predefined context keys with custom payload' do
+      payload['class'] = 'custom value'
+
+      expect(result).to include('class' => worker.class)
+    end
+  end
+
   describe '.queue_namespace' do
     it 'sets the queue name based on the class name' do
       worker.queue_namespace :some_namespace

spec/workers/expire_job_cache_worker_spec.rb

@@ -21,12 +21,12 @@ describe ExpireJobCacheWorker do
           allow(Gitlab::EtagCaching::Store).to receive(:new) { spy_store }
 
           expect(spy_store).to receive(:touch)
-            .exactly(IdempotentWorkerHelper::WORKER_EXEC_TIMES).times
+            .exactly(worker_exec_times).times
             .with(pipeline_path)
             .and_call_original
 
           expect(spy_store).to receive(:touch)
-            .exactly(IdempotentWorkerHelper::WORKER_EXEC_TIMES).times
+            .exactly(worker_exec_times).times
             .with(job_path)
             .and_call_original