Merge branch 'issue_40374' into 'master'

Fix WIP system note not being created

Closes #40374

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

app/models/concerns/issuable.rb

@@ -345,4 +345,11 @@ module Issuable
   def first_contribution?
     false
   end
+
+  ##
+  # Overriden in MergeRequest
+  #
+  def wipless_title_changed(old_title)
+    old_title != title
+  end
 end

app/models/merge_request.rb

@@ -218,6 +218,12 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
+  # Verifies if title has changed not taking into account WIP prefix
+  # for merge requests.
+  def wipless_title_changed(old_title)
+    self.class.wipless_title(old_title) != self.wipless_title
+  end
+
   def hook_attrs
     Gitlab::HookData::MergeRequestBuilder.new(self).build
   end

app/services/issuable/common_system_notes_service.rb

@@ -41,6 +41,14 @@ module Issuable
       end
     end
 
+    def create_wip_note(old_title)
+      return unless issuable.is_a?(MergeRequest)
+
+      if MergeRequest.work_in_progress?(old_title) != issuable.work_in_progress?
+        SystemNoteService.handle_merge_request_wip(issuable, issuable.project, current_user)
+      end
+    end
+
     def create_labels_note(old_labels)
       added_labels = issuable.labels - old_labels
       removed_labels = old_labels - issuable.labels
@@ -49,7 +57,11 @@ module Issuable
     end
 
     def create_title_change_note(old_title)
-      SystemNoteService.change_title(issuable, issuable.project, current_user, old_title)
+      create_wip_note(old_title)
+
+      if issuable.wipless_title_changed(old_title)
+        SystemNoteService.change_title(issuable, issuable.project, current_user, old_title)
+      end
     end
 
     def create_description_change_note

app/services/merge_requests/base_service.rb

@@ -4,20 +4,6 @@ module MergeRequests
       SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, state, nil)
     end
 
-    def create_title_change_note(issuable, old_title)
-      removed_wip = MergeRequest.work_in_progress?(old_title) && !issuable.work_in_progress?
-      added_wip = !MergeRequest.work_in_progress?(old_title) && issuable.work_in_progress?
-      changed_title = MergeRequest.wipless_title(old_title) != issuable.wipless_title
-
-      if removed_wip
-        SystemNoteService.remove_merge_request_wip(issuable, issuable.project, current_user)
-      elsif added_wip
-        SystemNoteService.add_merge_request_wip(issuable, issuable.project, current_user)
-      end
-
-      super if changed_title
-    end
-
     def hook_data(merge_request, action, old_rev: nil, old_labels: [], old_assignees: [], old_total_time_spent: nil)
       hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees, old_total_time_spent: old_total_time_spent)
       hook_data[:object_attributes][:action] = action

app/services/system_note_service.rb

@@ -241,14 +241,10 @@ module SystemNoteService
     create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
   end
 
-  def remove_merge_request_wip(noteable, project, author)
-    body = 'unmarked as a **Work In Progress**'
+  def handle_merge_request_wip(noteable, project, author)
+    prefix = noteable.work_in_progress? ? "marked" : "unmarked"
 
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
-  end
-
-  def add_merge_request_wip(noteable, project, author)
-    body = 'marked as a **Work In Progress**'
+    body = "#{prefix} as a **Work In Progress**"
 
     create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
   end

changelogs/unreleased/issue_40374.yml

+---
+title: Fix WIP system note not being created
+merge_request:
+author:
+type: fixed

spec/services/issuable/common_system_notes_service_spec.rb

@@ -18,7 +18,18 @@ describe Issuable::CommonSystemNotesService do
 
       note = Note.last
       expect(note.note).to match(note_text)
-      expect(note.noteable_type).to eq('Issue')
+      expect(note.noteable_type).to eq(issuable.class.name)
+    end
+  end
+
+  shared_examples 'WIP notes creation' do |wip_action|
+    subject { described_class.new(project, user).execute(issuable, []) }
+
+    it 'creates WIP toggle and title change notes' do
+      expect { subject }.to change { Note.count }.from(0).to(2)
+
+      expect(Note.first.note).to match("#{wip_action} as a **Work In Progress**")
+      expect(Note.second.note).to match('changed title')
     end
   end
 
@@ -45,5 +56,35 @@ describe Issuable::CommonSystemNotesService do
 
       it_behaves_like 'system note creation', {}, 'changed milestone'
     end
+
+    context 'with merge requests WIP note' do
+      context 'adding WIP note' do
+        let(:issuable) { create(:merge_request, title: "merge request") }
+
+        it_behaves_like 'system note creation', { title: "WIP merge request" }, 'marked as a **Work In Progress**'
+
+        context 'and changing title' do
+          before do
+            issuable.update_attribute(:title, "WIP changed title")
+          end
+
+          it_behaves_like 'WIP notes creation', 'marked'
+        end
+      end
+
+      context 'removing WIP note' do
+        let(:issuable) { create(:merge_request, title: "WIP merge request") }
+
+        it_behaves_like 'system note creation', { title: "merge request" }, 'unmarked as a **Work In Progress**'
+
+        context 'and changing title' do
+          before do
+            issuable.update_attribute(:title, "changed title")
+          end
+
+          it_behaves_like 'WIP notes creation', 'unmarked'
+        end
+      end
+    end
   end
 end

spec/services/system_note_service_spec.rb

@@ -970,31 +970,33 @@ describe SystemNoteService do
     end
   end
 
-  describe '.remove_merge_request_wip' do
-    let(:noteable) { create(:issue, project: project, title: 'WIP: Lorem ipsum') }
+  describe '.handle_merge_request_wip' do
+    context 'adding wip note' do
+      let(:noteable) { create(:merge_request, source_project: project, title: 'WIP Lorem ipsum') }
 
-    subject { described_class.remove_merge_request_wip(noteable, project, author) }
+      subject { described_class.handle_merge_request_wip(noteable, project, author) }
 
-    it_behaves_like 'a system note' do
-      let(:action) { 'title' }
-    end
+      it_behaves_like 'a system note' do
+        let(:action) { 'title' }
+      end
 
-    it 'sets the note text' do
-      expect(subject.note).to eq 'unmarked as a **Work In Progress**'
+      it 'sets the note text' do
+        expect(subject.note).to eq 'marked as a **Work In Progress**'
+      end
     end
-  end
 
-  describe '.add_merge_request_wip' do
-    let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') }
+    context 'removing wip note' do
+      let(:noteable) { create(:merge_request, source_project: project, title: 'Lorem ipsum') }
 
-    subject { described_class.add_merge_request_wip(noteable, project, author) }
+      subject { described_class.handle_merge_request_wip(noteable, project, author) }
 
-    it_behaves_like 'a system note' do
-      let(:action) { 'title' }
-    end
+      it_behaves_like 'a system note' do
+        let(:action) { 'title' }
+      end
 
-    it 'sets the note text' do
-      expect(subject.note).to eq 'marked as a **Work In Progress**'
+      it 'sets the note text' do
+        expect(subject.note).to eq 'unmarked as a **Work In Progress**'
+      end
     end
   end