summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorFelipe Artur <felipefac@gmail.com>2017-11-21 17:25:37 -0200
committerFelipe Artur <felipefac@gmail.com>2017-11-23 17:25:14 -0200
commit5c2c471a835f9588e39ec11fa79571c8b44979f8 (patch)
treea1e861518e7ee2e9199891fd75f02657c7f50ba9
parente72804ed0a73f8807ca58881645f0c95c70de9c9 (diff)
downloadgitlab-ce-5c2c471a835f9588e39ec11fa79571c8b44979f8.tar.gz
Fix WIP system note not being createdissue_40374
-rw-r--r--app/models/concerns/issuable.rb7
-rw-r--r--app/models/merge_request.rb6
-rw-r--r--app/services/issuable/common_system_notes_service.rb14
-rw-r--r--app/services/merge_requests/base_service.rb14
-rw-r--r--app/services/system_note_service.rb10
-rw-r--r--changelogs/unreleased/issue_40374.yml5
-rw-r--r--spec/services/issuable/common_system_notes_service_spec.rb43
-rw-r--r--spec/services/system_note_service_spec.rb36
8 files changed, 95 insertions, 40 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 35090181bd9..e607707475f 100644
--- a/app/models/concerns/issuable.rb
+++ b/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
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f1a5cc73e83..09b1697ab61 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -181,6 +181,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
diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb
index 92eaa5d5115..3da21bd8b8f 100644
--- a/app/services/issuable/common_system_notes_service.rb
+++ b/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
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index d3938b065bc..f6ffd48deae 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/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
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index fe71a405565..30a5aab13bf 100644
--- a/app/services/system_note_service.rb
+++ b/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
diff --git a/changelogs/unreleased/issue_40374.yml b/changelogs/unreleased/issue_40374.yml
new file mode 100644
index 00000000000..73b48b890fe
--- /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 9f92b662be1..b8fa3e3d124 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 @@ 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
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 0a6ab455abe..a918383ecd2 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/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