Merge branch 'pl-status-page-mvc-changes-notes' into 'master'

Publish status page on note changes

See merge request gitlab-org/gitlab!27950

app/services/notes/update_service.rb

@@ -81,3 +81,5 @@ module Notes
     end
   end
 end
+
+Notes::UpdateService.prepend_if_ee('EE::Notes::UpdateService')

ee/app/finders/status_page/incident_comments_finder.rb

 # frozen_string_literal: true
-#
+
+require_dependency 'status_page'
+
 # Retrieves Notes specifically for the Status Page
 # which are rendered as comments.
 #
@@ -15,11 +17,7 @@
 #
 module StatusPage
   class IncidentCommentsFinder
-    # Only comments with this emoji are visible.
-    # This filter will change once we have confidential notes.
-    # See https://gitlab.com/gitlab-org/gitlab/issues/207468
-    AWARD_EMOJI = 'microphone'
-
+    AWARD_EMOJI = StatusPage::AWARD_EMOJI
     MAX_LIMIT = StatusPage::Storage::MAX_COMMENTS
 
     def initialize(issue:)

ee/app/services/ee/notes/destroy_service.rb

@@ -10,6 +10,7 @@ module EE
         super
 
         Analytics::RefreshCommentsData.for_note(note)&.execute(force: true)
+        StatusPage.trigger_publish(project, current_user, note)
       end
     end
   end

ee/app/services/ee/notes/update_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Notes
+    module UpdateService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(note)
+        updated_note = super
+
+        if updated_note&.errors&.empty?
+          StatusPage.trigger_publish(project, current_user, updated_note)
+        end
+
+        updated_note
+      end
+    end
+  end
+end

ee/app/services/status_page/trigger_publish_service.rb

@@ -50,20 +50,44 @@ module StatusPage
     def eligable_issue_id
       case triggered_by
       when Issue then eligable_issue_id_from_issue
+      when Note then eligable_issue_id_from_note
       else
         raise ArgumentError, "unsupported trigger type #{triggered_by.class}"
       end
     end
 
+    # Trigger publish for public (non-confidential) issues
+    # - which were changed
+    # - just become confidential to unpublish
     def eligable_issue_id_from_issue
-      changes = triggered_by.previous_changes.keys & PUBLISH_WHEN_ISSUE_CHANGED
+      issue = triggered_by
+
+      changes = issue.previous_changes.keys & PUBLISH_WHEN_ISSUE_CHANGED
 
       return if changes.none?
-      # Ignore updates for already confidential issues
-      # Note: Issues becoming confidential _will_ be unpublished.
-      return if triggered_by.confidential? && changes.exclude?('confidential')
+      return if issue.confidential? && changes.exclude?('confidential')
+
+      issue.id
+    end
+
+    # Trigger publish for notes
+    # - on issues
+    # - which are user-generated (non-system)
+    # - which were changed or destroyed
+    # - had emoji `microphone` on it
+    def eligable_issue_id_from_note
+      note = triggered_by
+
+      return unless note.for_issue?
+      return if note.system?
+      # We can't know the emoji if the note was destroyed, so
+      # publish every time to make sure we remove the comment if needed
+      return note.noteable_id if note.destroyed?
+
+      return if note.previous_changes.none?
+      return if note.award_emoji.named(StatusPage::AWARD_EMOJI).none?
 
-      triggered_by.id
+      note.noteable_id
     end
   end
 end

ee/lib/status_page.rb

 # frozen_string_literal: true
 
 module StatusPage
+  # Only comments with this emoji are visible.
+  # This filter will change once we have confidential notes.
+  # See https://gitlab.com/gitlab-org/gitlab/issues/207468
+  AWARD_EMOJI = 'microphone'
+
   # Convenient method to trigger a status page update.
   def self.trigger_publish(project, user, triggered_by)
     TriggerPublishService.new(project, user, triggered_by).execute

ee/spec/services/ee/notes/destroy_service_spec.rb

@@ -3,11 +3,17 @@
 require 'spec_helper'
 
 describe Notes::DestroyService do
-  subject { described_class.new(note.project) }
+  let_it_be(:project, refind: true) { create(:project) }
+  let_it_be(:user) { create(:user) }
 
-  let(:note) { create(:note) }
+  let_it_be(:note, refind: true) do
+    create(:note_on_issue, project: project, author: user)
+  end
+
+  subject(:service) { described_class.new(project, user) }
 
   describe '#execute' do
+    describe 'refresh analytics comment data' do
       let(:analytics_mock) { instance_double('Analytics::RefreshCommentsData') }
 
       it 'invokes forced Analytics::RefreshCommentsData' do
@@ -15,7 +21,15 @@ describe Notes::DestroyService do
 
         expect(analytics_mock).to receive(:execute).with(force: true)
 
-      subject.execute(note)
+        service.execute(note)
+      end
+    end
+
+    describe 'publish to status page' do
+      let(:execute) { service.execute(note) }
+      let(:issue_id) { note.noteable_id }
+
+      include_examples 'trigger status page publish'
     end
   end
 end

ee/spec/services/ee/notes/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Notes::UpdateService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  let_it_be(:note, refind: true) do
+    create(:note_on_issue, project: project, author: user)
+  end
+
+  subject(:service) { described_class.new(project, user, opts) }
+
+  describe '#execute' do
+    let(:opts) { { note: note_text } }
+
+    describe 'publish to status page' do
+      let(:execute) { service.execute(note) }
+      let(:issue_id) { note.noteable_id }
+      let(:emoji_name) { StatusPage::AWARD_EMOJI }
+
+      before do
+        create(:award_emoji, user: user, name: emoji_name, awardable: note)
+      end
+
+      context 'for text-only update' do
+        let(:note_text) { 'text' }
+
+        include_examples 'trigger status page publish'
+
+        context 'without recognized emoji' do
+          let(:emoji_name) { 'thumbsup' }
+
+          include_examples 'no trigger status page publish'
+        end
+      end
+
+      context 'for quick action only update' do
+        let(:note_text) { "/todo\n" }
+
+        include_examples 'trigger status page publish'
+      end
+
+      context 'when update fails' do
+        let(:note_text) { '' }
+
+        include_examples 'no trigger status page publish'
+      end
+    end
+  end
+end

ee/spec/services/status_page/trigger_publish_service_spec.rb

@@ -11,7 +11,6 @@ describe StatusPage::TriggerPublishService do
   describe '#execute' do
     # Variables used by shared examples
     let(:execute) { subject }
-    let(:issue_id) { triggered_by.id }
 
     let_it_be(:status_page_setting, reload: true) do
       create(:status_page_setting, :enabled, project: project)
@@ -21,6 +20,7 @@ describe StatusPage::TriggerPublishService do
 
     describe 'triggered by issue' do
       let_it_be(:triggered_by, reload: true) { create(:issue, project: project) }
+      let(:issue_id) { triggered_by.id }
 
       using RSpec::Parameterized::TableSyntax
 
@@ -74,6 +74,90 @@ describe StatusPage::TriggerPublishService do
       end
     end
 
+    describe 'triggered by note' do
+      let(:issue_id) { triggered_by.noteable_id }
+      let(:emoji_name) { StatusPage::AWARD_EMOJI }
+
+      before do
+        create(:award_emoji, user: user, name: emoji_name,
+               awardable: triggered_by)
+      end
+
+      context 'for issues' do
+        let_it_be(:triggered_by, refind: true) do
+          create(:note_on_issue, project: project)
+        end
+
+        context 'without changes' do
+          include_examples 'no trigger status page publish'
+        end
+
+        context 'when changed' do
+          include_examples 'trigger status page publish' do
+            before do
+              triggered_by.update!(note: 'changed')
+            end
+          end
+        end
+
+        context 'when destroyed' do
+          include_examples 'trigger status page publish' do
+            before do
+              triggered_by.destroy
+            end
+          end
+        end
+
+        context 'as system note' do
+          let_it_be(:triggered_by, reload: true) do
+            create(:note_on_issue, :system, project: project)
+          end
+
+          include_examples 'no trigger status page publish' do
+            before do
+              triggered_by.update!(note: 'changed')
+            end
+          end
+        end
+
+        context 'without recognized emoji' do
+          let(:emoji_name) { 'thumbsup' }
+
+          context 'when changed' do
+            include_examples 'no trigger status page publish' do
+              before do
+                triggered_by.update!(note: 'changed')
+              end
+            end
+          end
+
+          context 'when destroyed' do
+            include_examples 'trigger status page publish' do
+              before do
+                triggered_by.destroy
+              end
+            end
+          end
+        end
+      end
+
+      context 'for merge requests' do
+        let_it_be(:project) { create(:project, :repository) }
+
+        let_it_be(:triggered_by) do
+          create(:note_on_merge_request, project: project)
+        end
+
+        context 'when changed' do
+          include_examples 'no trigger status page publish' do
+            before do
+              triggered_by.update!(note: 'changed')
+            end
+          end
+        end
+      end
+    end
+
     describe 'triggered by unsupported type' do
       context 'for some abitary type' do
         let(:triggered_by) { Object.new }
@@ -89,6 +173,7 @@ describe StatusPage::TriggerPublishService do
 
     context 'with eligable triggered_by' do
       let_it_be(:triggered_by) { create(:issue, project: project) }
+      let(:issue_id) { triggered_by.id }
 
       context 'when eligable' do
         include_examples 'trigger status page publish'