diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 35090181bd91c264a02c757ee808138205bded95..e607707475f0f2072198c6bf90a8949dcaa93838 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -345,4 +345,11 @@ def record_metrics def first_contribution? false end + + ## + # Overriden in MergeRequest + # + def wipless_title_changed(old_title) + old_title != title + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f1a5cc73e83417c137b1f7283fd55287dd955448..09b1697ab6133b228078fefa021810e76c505650 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -181,6 +181,12 @@ def self.wip_title(title) 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 diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb index 92eaa5d5115ce3b8655558fc86a48963d120d0fa..3da21bd8b8f16a73b85b7ed35704998de5390088 100644 --- a/app/services/issuable/common_system_notes_service.rb +++ b/app/services/issuable/common_system_notes_service.rb @@ -41,6 +41,14 @@ def handle_description_change_note 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 @@ def create_labels_note(old_labels) 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 diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index d3938b065bc0edb3b20da7f4d886cf7daa7ad386..f6ffd48deae4803b0e16b3204821b5c5fbf845c7 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -4,20 +4,6 @@ def create_note(merge_request, state = merge_request.state) 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 diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index fe71a40556543fb67fa863812e55fdc8eedb1efe..30a5aab13bffa744978b867a5fd17a1e44b3a867 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -241,14 +241,10 @@ def cancel_merge_when_pipeline_succeeds(noteable, project, author) 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 diff --git a/changelogs/unreleased/issue_40374.yml b/changelogs/unreleased/issue_40374.yml new file mode 100644 index 0000000000000000000000000000000000000000..73b48b890fe2ab144db12bc74ec3f5d91bf5cef2 --- /dev/null +++ b/changelogs/unreleased/issue_40374.yml @@ -0,0 +1,5 @@ +--- +title: Fix WIP system note not being created +merge_request: +author: +type: fixed diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb index 9f92b662be1f8cf63961dc27b40459c0794fe5be..b8fa3e3d1241dc11ed66b6172852d1d4c55ba756 100644 --- a/spec/services/issuable/common_system_notes_service_spec.rb +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -18,7 +18,18 @@ 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 @@ 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 diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 0a6ab455abea729df0f4098ee18226ac8bcd66e1..a918383ecd2dad64c67cab39da917c85f0a1518b 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -970,31 +970,33 @@ def spend_time!(seconds) 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