Swap to db-tracking published issues

To have idempotent publishing jobs for publishing a project's
issues to the status page, issues are pushed based on the
confidential state of an issue. This commit swaps over to the
publishing an issue only if a StatusPage::PublishedIncident
exists for the issue. This allows finer control over to the user
on whether an issue should be published to the status page.

changelogs/unreleased/sy-publish-command.yml

+---
+title: Backfill StatusPage::Published incidents and enable a publish quick action
+  for EE
+merge_request: 30906
+author:
+type: added

db/post_migrate/20200421195234_backfill_status_page_published_incidents.rb

+# frozen_string_literal: true
+
+class BackfillStatusPagePublishedIncidents < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class Incident < ActiveRecord::Base
+    self.table_name = 'status_page_published_incidents'
+  end
+
+  class StatusPageIssue < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'issues'
+
+    scope :published_only, -> do
+      joins('INNER JOIN status_page_settings ON status_page_settings.project_id = issues.project_id')
+        .where('status_page_settings.enabled = true')
+        .where(confidential: false)
+    end
+  end
+
+  def up
+    current_time = Time.current
+
+    StatusPageIssue.published_only.each_batch do |batch|
+      incidents = batch.map do |status_page_issue|
+        {
+          issue_id: status_page_issue.id,
+          created_at: current_time,
+          updated_at: current_time
+        }
+      end
+
+      Incident.insert_all(incidents, unique_by: :issue_id)
+    end
+  end
+
+  def down
+    # no op
+
+    # While we expect this table to be empty at the point of
+    # the up migration, there is no reliable way to determine
+    # whether records were added as a part of the migration
+    # or after it has run.
+  end
+end

db/structure.sql

@@ -13877,6 +13877,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200421054948
 20200421092907
 20200421111005
+20200421195234
 20200421233150
 20200422091541
 20200422213749

ee/app/finders/status_page/incidents_finder.rb

@@ -10,9 +10,9 @@
 #
 #     finder = StatusPage::IncidentsFinder.new(project_id: project_id)
 #
-#     # A single issue which includes confidential issues by default)
+#     # A single issue which includes unpublished issues by default)
 #     issue = finder.find_by_id(issue_id)
-#     # Find a "public only" issue
+#     # Find a published issue
 #     issue = finder.find_by_id(issue_id, include_nonpublished: false)
 #
 #     # Most recent 20 non-confidential issues
@@ -53,7 +53,7 @@ module StatusPage
     end
 
     def published_only(issues)
-      issues.public_only
+      issues.on_status_page
     end
 
     def by_project(issues)

ee/app/models/ee/issue.rb

@@ -26,7 +26,7 @@ module EE
         issue_ids = EpicIssue.where(epic_id: epics).select(:issue_id)
         id_in(issue_ids)
       end
-      scope :on_status_page, -> { joins(project: :status_page_setting).where(status_page_settings: { enabled: true }).public_only }
+      scope :on_status_page, -> { joins(:status_page_published_incident).public_only }
       scope :counts_by_health_status, -> { reorder(nil).group(:health_status).count }
       scope :with_health_status, -> { where.not(health_status: nil) }
 

ee/app/services/ee/issues/create_service.rb

@@ -12,13 +12,6 @@ module EE
         super
       end
 
-      override :after_create
-      def after_create(issue)
-        super
-
-        StatusPage.trigger_publish(project, current_user, issue)
-      end
-
       def handle_issue_epic_link(issue)
         return unless params.key?(:epic)
 

ee/app/services/status_page/mark_for_publication_service.rb

@@ -3,6 +3,13 @@
 module StatusPage
   # Marks an issue as published.
   class MarkForPublicationService
+    def self.publishable?(project, user, issue)
+      project.status_page_setting&.enabled? &&
+        user&.can?(:mark_issue_for_publication, project) &&
+        !issue.confidential? &&
+        !issue.status_page_published_incident
+    end
+
     def initialize(project, user, issue)
       @project = project
       @user = user
@@ -10,29 +17,26 @@ module StatusPage
     end
 
     def execute
-      return unless status_page_enabled?
-      return unless can_publish?
-      return unless publishable_issue?
+      return error('Issue cannot be published') unless publishable?
 
+      PublishedIncident.transaction do
         track_incident
         add_system_note
       end
 
-    private
-
-    attr_reader :user, :project, :issue
+      ServiceResponse.success
+    rescue StandardError => e
+      Gitlab::ErrorTracking.track_exception(e)
 
-    def can_publish?
-      user&.can?(:mark_issue_for_publication, project)
+      error(e.message)
     end
 
-    def status_page_enabled?
-      project.status_page_setting&.enabled?
-    end
+    private
 
-    def publishable_issue?
-      !issue.confidential? &&
-        !issue.status_page_published_incident
+    attr_reader :user, :project, :issue
+
+    def publishable?
+      self.class.publishable?(project, user, issue)
     end
 
     def add_system_note
@@ -42,5 +46,9 @@ module StatusPage
     def track_incident
       PublishedIncident.track(issue)
     end
+
+    def error(message)
+      ServiceResponse.error(message: message)
+    end
   end
 end

ee/app/services/status_page/publish_service.rb

@@ -40,7 +40,7 @@ module StatusPage
     end
 
     def unpublish_details?
-      issue.confidential?
+      issue.confidential? || !issue.status_page_published_incident
     end
 
     def process_list

ee/app/services/status_page/trigger_publish_service.rb

@@ -67,6 +67,7 @@ module StatusPage
 
       return if changes.none?
       return if issue.confidential? && changes.exclude?('confidential')
+      return unless issue.status_page_published_incident
 
       issue.id
     end

ee/app/services/status_page/unpublish_details_service.rb

@@ -13,6 +13,8 @@ module StatusPage
     private
 
     def process(issue)
+      PublishedIncident.untrack(issue)
+
       # Delete the incident prior to deleting images to avoid broken links
       json_key = json_object_key(issue)
       delete_object(json_key)

ee/lib/ee/gitlab/quick_actions/issue_actions.rb

@@ -134,6 +134,24 @@ module EE
 
             ::IterationsFinder.new(params.merge(project_ids: [project.id], group_ids: group_ids)).execute
           end
+
+          desc _('Publish to status page')
+          explanation _('Publishes this issue to the associated status page.')
+          types Issue
+          condition do
+            StatusPage::MarkForPublicationService.publishable?(project, current_user, quick_action_target)
+          end
+          command :publish do
+            if StatusPage.mark_for_publication(project, current_user, quick_action_target).success?
+              # Ideally, we want to use `StatusPage.trigger_publish` instead of dispatching a worker.
+              # See https://gitlab.com/gitlab-org/gitlab/-/issues/219266
+              StatusPage::PublishWorker.perform_async(current_user.id, project.id, quick_action_target.id)
+
+              @execution_message[:publish] = _('Issue published on status page.')
+            else
+              @execution_message[:publish] = _('Failed to publish issue on status page.')
+            end
+          end
         end
       end
     end

ee/spec/factories/issues.rb

+# frozen_string_literal: true
+
+FactoryBot.modify do
+  factory :issue do
+    trait :published do
+      after(:create) do |issue|
+        issue.create_status_page_published_incident!
+      end
+    end
+  end
+end

ee/spec/features/issues/user_uses_quick_actions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Issues > User uses EE quick actions', :js do
+  include Spec::Support::Helpers::Features::NotesHelpers
+
+  describe 'issue-only commands' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let(:issue) { create(:issue, project: project) }
+
+    before do
+      project.add_developer(user)
+      sign_in(user)
+      visit project_issue_path(project, issue)
+      wait_for_all_requests
+    end
+
+    after do
+      wait_for_requests
+    end
+
+    it_behaves_like 'status page quick actions'
+  end
+end

ee/spec/finders/status_page/incidents_finder_spec.rb

@@ -7,13 +7,14 @@ describe StatusPage::IncidentsFinder do
 
   let_it_be(:issues) do
     {
-      public: create_list(:issue, 2, project: project),
-      confidential: create(:issue, :confidential, project: project),
+      published: create_list(:issue, 2, :published, project: project),
+      nonpublished: create(:issue, project: project),
+      confidential: create(:issue, :confidential, :published, project: project),
       unrelated: create(:issue)
     }
   end
 
-  let(:public_issues) { issues.fetch(:public) }
+  let(:published_issues) { issues.fetch(:published) }
   let(:finder) { described_class.new(project_id: project.id) }
 
   describe '#find_by_id' do
@@ -22,8 +23,8 @@ describe StatusPage::IncidentsFinder do
     context 'without params' do
       let(:params) { {} }
 
-      context 'for public issue' do
-        let(:issue) { public_issues.first }
+      context 'for published issue' do
+        let(:issue) { published_issues.first }
 
         it { is_expected.to eq(issue) }
       end
@@ -34,6 +35,12 @@ describe StatusPage::IncidentsFinder do
         it { is_expected.to eq(issue) }
       end
 
+      context 'for nonpublished issue' do
+        let(:issue) { issues.fetch(:nonpublished) }
+
+        it { is_expected.to eq(issue) }
+      end
+
       context 'for unrelated issue' do
         let(:issue) { issues.fetch(:unrelated) }
 
@@ -44,6 +51,12 @@ describe StatusPage::IncidentsFinder do
     context 'with include_nonpublished' do
       let(:params) { { include_nonpublished: false } }
 
+      context 'for nonpublished issue' do
+        let(:issue) { issues.fetch(:nonpublished) }
+
+        it { is_expected.to be_nil }
+      end
+
       context 'for confidential issue' do
         let(:issue) { issues.fetch(:confidential) }
 
@@ -53,8 +66,8 @@ describe StatusPage::IncidentsFinder do
   end
 
   describe '#all' do
-    let(:sorted_issues) { public_issues.sort_by(&:created_at).reverse }
-    let(:limit) { public_issues.size }
+    let(:sorted_issues) { published_issues.sort_by(&:created_at).reverse }
+    let(:limit) { published_issues.size }
 
     subject { finder.all }
 
@@ -63,20 +76,20 @@ describe StatusPage::IncidentsFinder do
     end
 
     context 'when limit is higher than the colletion size' do
-      let(:limit) { public_issues.size + 1 }
+      let(:limit) { published_issues.size + 1 }
 
       it { is_expected.to eq(sorted_issues) }
     end
 
     context 'when limit is lower than the colletion size' do
-      let(:limit) { public_issues.size - 1 }
+      let(:limit) { published_issues.size - 1 }
 
       it { is_expected.to eq(sorted_issues.first(1)) }
     end
 
     context 'when combined with other finder methods' do
       before do
-        finder.find_by_id(public_issues.first.id)
+        finder.find_by_id(published_issues.first.id)
       end
 
       it { is_expected.to eq(sorted_issues) }

ee/spec/lib/ee/gitlab/usage_data_spec.rb

@@ -50,9 +50,9 @@ describe Gitlab::UsageData do
       # Status Page
       create(:status_page_setting, project: projects[0], enabled: true)
       create(:status_page_setting, project: projects[1], enabled: false)
-      # 1 public issue on 1 projects with status page enabled
+      # 1 published issue on 1 projects with status page enabled
       create(:issue, project: projects[0])
-      create(:issue, :confidential, project: projects[0])
+      create(:issue, :published, project: projects[0])
     end
 
     subject { described_class.data }

ee/spec/models/issue_spec.rb

@@ -90,24 +90,14 @@ describe Issue do
     end
 
     describe '.on_status_page' do
-      context 'with public issue and private issue' do
-        let_it_be(:status_page_setting) { create(:status_page_setting, enabled: true) }
-        let_it_be(:public_issue) { create(:issue, project: status_page_setting.project) }
-        let_it_be(:private_issue) { create(:issue, :confidential, project: status_page_setting.project) }
+      let_it_be(:status_page_setting) { create(:status_page_setting, :enabled) }
+      let_it_be(:project) { status_page_setting.project }
+      let_it_be(:published_issue) { create(:issue, :published, project: project) }
+      let_it_be(:confidential_issue) { create(:issue, :published, :confidential, project: project) }
+      let_it_be(:nonpublished_issue) { create(:issue, project: project) }
 
       it { expect(Issue.on_status_page.count).to eq(1) }
-        it { expect(Issue.on_status_page.first).to eq(public_issue) }
-      end
-
-      context 'with project status page settings enabled and disabled' do
-        let_it_be(:status_page_setting_enabled) { create(:status_page_setting, enabled: true) }
-        let_it_be(:status_page_setting_disabled) { create(:status_page_setting, enabled: false) }
-        let_it_be(:issue_with_enabled_project) { create(:issue, project: status_page_setting_enabled.project) }
-        let_it_be(:issue_with_disabled_project) { create(:issue, project: status_page_setting_disabled.project) }
-
-        it { expect(Issue.on_status_page.count).to eq(1) }
-        it { expect(Issue.on_status_page.first).to eq(issue_with_enabled_project) }
-      end
+      it { expect(Issue.on_status_page.first).to eq(published_issue) }
     end
 
     context 'epics' do

ee/spec/services/ee/issues/create_service_spec.rb

@@ -89,22 +89,5 @@ describe Issues::CreateService do
     it_behaves_like 'new issuable with scoped labels' do
       let(:parent) { project }
     end
-
-    describe 'publish to status page' do
-      let(:execute) { service.execute }
-      let(:issue_id) { execute&.id }
-
-      context 'when creation succeeds' do
-        let_it_be(:params) { { title: 'New title' } }
-
-        include_examples 'trigger status page publish'
-      end
-
-      context 'when creation fails' do
-        let_it_be(:params) { { title: nil } }
-
-        include_examples 'no trigger status page publish'
-      end
-    end
   end
 end

ee/spec/services/ee/issues/update_service_spec.rb

@@ -217,6 +217,10 @@ describe Issues::UpdateService do
       let(:execute) { update_issue(params) }
       let(:issue_id) { execute&.id }
 
+      before do
+        create(:status_page_published_incident, issue: issue)
+      end
+
       context 'when update succeeds' do
         let(:params) { { title: 'New title' } }
 

ee/spec/services/status_page/mark_for_publication_service_spec.rb

@@ -21,6 +21,7 @@ describe StatusPage::MarkForPublicationService do
     shared_examples 'does not track the incident' do
       specify { expect { subject }.not_to change { ::StatusPage::PublishedIncident.count } }
       specify { expect { subject }.not_to change { issue.notes.count } }
+      specify { expect(subject).to be_error }
     end
 
     context 'when license is not available' do
@@ -47,6 +48,7 @@ describe StatusPage::MarkForPublicationService do
       context 'when issue is publishable' do
         specify { expect { subject }.to change { ::StatusPage::PublishedIncident.count }.by(1) }
         specify { expect { subject }.to change { issue.notes.count }.by(1) }
+        specify { expect(subject).to be_success }
       end
 
       context 'when issue is confidential' do
@@ -66,6 +68,22 @@ describe StatusPage::MarkForPublicationService do
 
         it_behaves_like 'does not track the incident'
       end
+
+      context 'when an error occurs' do
+        let(:error) { RuntimeError.new('Error!') }
+
+        before do
+          allow(::SystemNoteService).to receive(:publish_issue_to_status_page).and_raise(error)
+        end
+
+        it_behaves_like 'does not track the incident'
+
+        it 'reports the error to sentry' do
+          expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error)
+
+          subject
+        end
+      end
     end
   end
 end

ee/spec/services/status_page/publish_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe StatusPage::PublishService 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(:issue) { create(:issue, :published, project: project) }
   let_it_be(:settings) { create(:status_page_setting, :enabled, project: project) }
   let(:user_can_publish) { true }
 

ee/spec/services/status_page/trigger_publish_service_spec.rb

@@ -19,7 +19,7 @@ describe StatusPage::TriggerPublishService do
     subject { service.execute }
 
     describe 'triggered by issue' do
-      let_it_be(:triggered_by, reload: true) { create(:issue, project: project) }
+      let_it_be(:triggered_by, reload: true) { create(:issue, :published, project: project) }
       let(:issue_id) { triggered_by.id }
 
       using RSpec::Parameterized::TableSyntax
@@ -53,6 +53,16 @@ describe StatusPage::TriggerPublishService do
         end
       end
 
+      context 'when a non-published issue changes' do
+        let(:triggered_by) { create(:issue, project: project) }
+
+        include_examples 'no trigger status page publish' do
+          before do
+            triggered_by.update!(title: 'changed')
+          end
+        end
+      end
+
       context 'when closing an issue' do
         include_examples 'trigger status page publish' do
           before do
@@ -65,7 +75,7 @@ describe StatusPage::TriggerPublishService do
 
       context 'when reopening an issue' do
         include_examples 'trigger status page publish' do
-          let_it_be(:triggered_by) { create(:issue, :closed, project: project) }
+          let_it_be(:triggered_by) { create(:issue, :closed, :published, project: project) }
 
           before do
             triggered_by.reopen!
@@ -203,7 +213,7 @@ describe StatusPage::TriggerPublishService do
     end
 
     context 'with eligable triggered_by' do
-      let_it_be(:triggered_by) { create(:issue, project: project) }
+      let_it_be(:triggered_by) { create(:issue, :published, project: project) }
       let(:issue_id) { triggered_by.id }
 
       context 'when eligable' do

ee/spec/services/status_page/unpublish_details_service_spec.rb

@@ -27,6 +27,10 @@ describe StatusPage::UnpublishDetailsService do
 
       allow(project).to receive(:status_page_setting)
         .and_return(status_page_setting)
+
+      allow(StatusPage::PublishedIncident).to receive(:find_by)
+        .with(issue: issue)
+        .and_return(nil)
     end
 
     context 'when deletion succeeds' do
@@ -46,6 +50,12 @@ describe StatusPage::UnpublishDetailsService do
         expect(result).to be_success
         expect(result.payload).to eq(object_key: key)
       end
+
+      it 'untracks the issue' do
+        expect(StatusPage::PublishedIncident).to receive(:untrack).with(issue)
+
+        result
+      end
     end
 
     context 'when delete fails due to exception' do

ee/spec/support/shared_examples/quick_actions/issue/status_page_quick_actions_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'status page quick actions' do
+  describe '/publish' do
+    let_it_be(:status_page_setting) { create(:status_page_setting, :enabled, project: project) }
+    let(:user) { project.owner }
+
+    before do
+      stub_licensed_features(status_page: true)
+    end
+
+    shared_examples 'skip silently' do
+      it 'does not allow publishing' do
+        expect(StatusPage).not_to receive(:mark_for_publication).with(project, user, issue)
+        expect(StatusPage::PublishWorker).not_to receive(:perform_async).with(user.id, project.id, issue.id)
+
+        add_note('/publish')
+        wait_for_requests
+
+        expect(page).not_to have_content('Issue published on status page.')
+        expect(page).not_to have_content('Failed to publish issue on status page.')
+      end
+    end
+
+    it 'publishes the issue' do
+      expect(StatusPage::PublishWorker).to receive(:perform_async).with(user.id, project.id, issue.id)
+
+      add_note('/publish')
+      wait_for_requests
+
+      expect(page).to have_content('Issue published on status page.')
+    end
+
+    context 'during issue creation' do
+      it 'publishes the issue' do
+        visit new_project_issue_path(project)
+
+        fill_in('Title', with: 'Title')
+        fill_in('Description', with: "Published issue \n\n/publish")
+        click_button('Submit issue')
+
+        wait_for_requests
+
+        expect(page).to have_content('Published issue')
+        expect(page).to have_content("#{user.username} published this issue to the status page")
+      end
+    end
+
+    context 'publishing causes an error' do
+      it 'provides an error message' do
+        allow(StatusPage::PublishedIncident).to receive(:track).with(issue).and_raise('Error')
+
+        add_note('/publish')
+        wait_for_requests
+
+        expect(page).not_to have_content("#{user.username} published this issue to the status page")
+        expect(page).to have_content('Failed to publish issue on status page.')
+      end
+    end
+
+    context 'user does not have permissions' do
+      let(:user) { create(:user) }
+
+      it_behaves_like 'skip silently'
+    end
+
+    context 'status page is not configured' do
+      before do
+        status_page_setting.update!(enabled: false)
+      end
+
+      after do
+        status_page_setting.update!(enabled: true)
+      end
+
+      it_behaves_like 'skip silently'
+    end
+
+    context 'issue is already published' do
+      before do
+        create(:status_page_published_incident, issue: issue)
+      end
+
+      it_behaves_like 'skip silently'
+    end
+  end
+end

locale/gitlab.pot

@@ -9241,6 +9241,9 @@ msgstr ""
 msgid "Failed to protect the environment"
 msgstr ""
 
+msgid "Failed to publish issue on status page."
+msgstr ""
+
 msgid "Failed to remove a Zoom meeting"
 msgstr ""
 
@@ -12085,6 +12088,9 @@ msgstr ""
 msgid "Issue or Merge Request ID is required"
 msgstr ""
 
+msgid "Issue published on status page."
+msgstr ""
+
 msgid "Issue template (optional)"
 msgstr ""
 
@@ -17550,6 +17556,12 @@ msgstr ""
 msgid "Public projects Minutes cost factor"
 msgstr ""
 
+msgid "Publish to status page"
+msgstr ""
+
+msgid "Publishes this issue to the associated status page."
+msgstr ""
+
 msgid "Pull"
 msgstr ""
 

spec/migrations/backfill_status_page_published_incidents_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200421195234_backfill_status_page_published_incidents.rb')
+
+describe BackfillStatusPagePublishedIncidents, :migration do
+  subject(:migration) { described_class.new }
+
+  describe '#up' do
+    let(:projects) { table(:projects) }
+    let(:status_page_settings) { table(:status_page_settings) }
+    let(:issues) { table(:issues) }
+    let(:incidents) { table(:status_page_published_incidents) }
+
+    let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab') }
+    let(:project_without_status_page) { projects.create!(namespace_id: namespace.id) }
+    let(:enabled_project) { projects.create!(namespace_id: namespace.id) }
+    let(:disabled_project) { projects.create!(namespace_id: namespace.id) }
+
+    let!(:enabled_setting) { status_page_settings.create!(enabled: true, project_id: enabled_project.id, **status_page_setting_attrs) }
+    let!(:disabled_setting) { status_page_settings.create!(enabled: false, project_id: disabled_project.id, **status_page_setting_attrs) }
+
+    let!(:published_issue) { issues.create!(confidential: false, project_id: enabled_project.id) }
+    let!(:nonpublished_issue_1) { issues.create!(confidential: true, project_id: enabled_project.id) }
+    let!(:nonpublished_issue_2) { issues.create!(confidential: false, project_id: disabled_project.id) }
+    let!(:nonpublished_issue_3) { issues.create!(confidential: false, project_id: project_without_status_page.id) }
+
+    let(:current_time) { Time.current.change(usec: 0) }
+    let(:status_page_setting_attrs) do
+      {
+        aws_s3_bucket_name: 'bucket',
+        aws_region: 'region',
+        aws_access_key: 'key',
+        encrypted_aws_secret_key: 'abc123',
+        encrypted_aws_secret_key_iv: 'abc123'
+      }
+    end
+
+    it 'creates a StatusPage::PublishedIncident record for each published issue' do
+      Timecop.freeze(current_time) do
+        expect(incidents.all).to be_empty
+
+        migrate!
+
+        incident = incidents.first
+
+        expect(incidents.count).to eq(1)
+        expect(incident.issue_id).to eq(published_issue.id)
+        expect(incident.created_at).to eq(current_time)
+        expect(incident.updated_at).to eq(current_time)
+      end
+    end
+  end
+end