From c729d9dae7ad8c32cbe93b590baa24c8133364e5 Mon Sep 17 00:00:00 2001
From: Jarka Kadlecova <jarka@gitlab.com>
Date: Wed, 15 Mar 2017 14:19:45 +0100
Subject: [PATCH] Create metadata when creating system notes

---
 app/models/note.rb                            |   2 +-
 app/models/system_note_metadata.rb            |   2 +-
 app/services/note_summary.rb                  |  20 +++
 app/services/system_note_service.rb           |  79 ++++----
 .../24784-system-notes-meta-data.yml          |   4 +
 ...70314082049_create_system_note_metadata.rb |  10 +-
 db/schema.rb                                  |   2 +-
 spec/factories/system_note_metadata.rb        |   2 +-
 spec/lib/gitlab/import_export/all_models.yml  |   1 +
 spec/models/note_spec.rb                      |   1 -
 spec/models/system_note_metadata_spec.rb      |   8 +-
 spec/services/note_summary_spec.rb            |  44 +++++
 spec/services/system_note_service_spec.rb     | 169 +++++++++++++++---
 13 files changed, 269 insertions(+), 75 deletions(-)
 create mode 100644 app/services/note_summary.rb
 create mode 100644 changelogs/unreleased/24784-system-notes-meta-data.yml
 create mode 100644 spec/services/note_summary_spec.rb

diff --git a/app/models/note.rb b/app/models/note.rb
index ba5a552f469..16d66cb1427 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -37,7 +37,7 @@ class Note < ActiveRecord::Base
 
   has_many :todos, dependent: :destroy
   has_many :events, as: :target, dependent: :destroy
-  has_one :system_note_metadata, dependent: :destroy
+  has_one :system_note_metadata
 
   delegate :gfm_reference, :local_reference, to: :noteable
   delegate :name, to: :project, prefix: true
diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb
index e8d3600d341..5cc66574941 100644
--- a/app/models/system_note_metadata.rb
+++ b/app/models/system_note_metadata.rb
@@ -5,7 +5,7 @@ class SystemNoteMetadata < ActiveRecord::Base
   ].freeze
 
   validates :note, presence: true
-  validates :icon, inclusion: ICON_TYPES, allow_nil: true
+  validates :action, inclusion: ICON_TYPES, allow_nil: true
 
   belongs_to :note
 end
diff --git a/app/services/note_summary.rb b/app/services/note_summary.rb
new file mode 100644
index 00000000000..a6f6320d573
--- /dev/null
+++ b/app/services/note_summary.rb
@@ -0,0 +1,20 @@
+class NoteSummary
+  attr_reader :note
+  attr_reader :metadata
+
+  def initialize(noteable, project, author, body, action: nil, commit_count: nil)
+    @note = { noteable: noteable, project: project, author: author, note: body }
+    @metadata = { action: action, commit_count: commit_count }.compact
+
+    set_commit_params if note[:noteable].is_a?(Commit)
+  end
+
+  def metadata?
+    metadata.present?
+  end
+
+  def set_commit_params
+    note.merge!(noteable_type: 'Commit', commit_id: note[:noteable].id)
+    note[:noteable] = nil
+  end
+end
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 8e02fe3741a..d3e502b66dd 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -26,7 +26,7 @@ module SystemNoteService
     body << new_commit_summary(new_commits).join("\n")
     body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})"
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count))
   end
 
   # Called when the assignee of a Noteable is changed or removed
@@ -46,7 +46,7 @@ module SystemNoteService
   def change_assignee(noteable, project, author, assignee)
     body = assignee.nil? ? 'removed assignee' : "assigned to #{assignee.to_reference}"
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'assignee'))
   end
 
   # Called when one or more labels on a Noteable are added and/or removed
@@ -86,7 +86,7 @@ module SystemNoteService
 
     body << ' ' << 'label'.pluralize(labels_count)
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'label'))
   end
 
   # Called when the milestone of a Noteable is changed
@@ -106,7 +106,7 @@ module SystemNoteService
   def change_milestone(noteable, project, author, milestone)
     body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project)}"
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'milestone'))
   end
 
   # Called when the estimated time of a Noteable is changed
@@ -132,7 +132,7 @@ module SystemNoteService
              "changed time estimate to #{parsed_time}"
            end
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'time_tracking'))
   end
 
   # Called when the spent time of a Noteable is changed
@@ -161,7 +161,7 @@ module SystemNoteService
       body = "#{action} #{parsed_time} of time spent"
     end
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'time_tracking'))
   end
 
   # Called when the status of a Noteable is changed
@@ -183,53 +183,57 @@ module SystemNoteService
     body = status.dup
     body << " via #{source.gfm_reference(project)}" if source
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'status'))
   end
 
   # Called when 'merge when pipeline succeeds' is executed
   def merge_when_pipeline_succeeds(noteable, project, author, last_commit)
     body = "enabled an automatic merge when the pipeline for #{last_commit.to_reference(project)} succeeds"
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
   end
 
   # Called when 'merge when pipeline succeeds' is canceled
   def cancel_merge_when_pipeline_succeeds(noteable, project, author)
     body = 'canceled the automatic merge'
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    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**'
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    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**'
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
   end
 
   def add_merge_request_wip_from_commit(noteable, project, author, commit)
     body = "marked as a **Work In Progress** from #{commit.to_reference(project)}"
 
-    create_note(noteable: noteable, project: project, author: author, note: body)
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
   end
 
   def self.resolve_all_discussions(merge_request, project, author)
     body = "resolved all discussions"
 
-    create_note(noteable: merge_request, project: project, author: author, note: body)
+    create_note(NoteSummary.new(merge_request, project, author, body, action: 'discussion'))
   end
 
   def discussion_continued_in_issue(discussion, project, author, issue)
     body = "created #{issue.to_reference} to continue this discussion"
-    note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
-    note_attributes[:type] = note_attributes.delete(:note_type)
 
-    create_note(note_attributes)
+    note_params = discussion.reply_attributes.merge(project: project, author: author, note: body)
+    note_params[:type] = note_params.delete(:note_type)
+
+    note = Note.create(note_params.merge(system: true))
+    note.system_note_metadata = SystemNoteMetadata.new({ action: 'discussion' })
+
+    note
   end
 
   # Called when the title of a Noteable is changed
@@ -253,7 +257,8 @@ module SystemNoteService
     marked_new_title = Gitlab::Diff::InlineDiffMarker.new(new_title).mark(new_diffs, mode: :addition, markdown: true)
 
     body = "changed title from **#{marked_old_title}** to **#{marked_new_title}**"
-    create_note(noteable: noteable, project: project, author: author, note: body)
+
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
   end
 
   # Called when the confidentiality changes
@@ -269,7 +274,8 @@ module SystemNoteService
   # Returns the created Note object
   def change_issue_confidentiality(issue, project, author)
     body = issue.confidential ? 'made the issue confidential' : 'made the issue visible to everyone'
-    create_note(noteable: issue, project: project, author: author, note: body)
+
+    create_note(NoteSummary.new(issue, project, author, body, action: 'confidentiality'))
   end
 
   # Called when a branch in Noteable is changed
@@ -288,7 +294,8 @@ module SystemNoteService
   # Returns the created Note object
   def change_branch(noteable, project, author, branch_type, old_branch, new_branch)
     body = "changed #{branch_type} branch from `#{old_branch}` to `#{new_branch}`"
-    create_note(noteable: noteable, project: project, author: author, note: body)
+
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'branch'))
   end
 
   # Called when a branch in Noteable is added or deleted
@@ -314,7 +321,8 @@ module SystemNoteService
       end
 
     body = "#{verb} #{branch_type} branch `#{branch}`"
-    create_note(noteable: noteable, project: project, author: author, note: body)
+
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'branch'))
   end
 
   # Called when a branch is created from the 'new branch' button on a issue
@@ -325,7 +333,8 @@ module SystemNoteService
     link = url_helpers.namespace_project_compare_url(project.namespace, project, from: project.default_branch, to: branch)
 
     body = "created branch [`#{branch}`](#{link})"
-    create_note(noteable: issue, project: project, author: author, note: body)
+
+    create_note(NoteSummary.new(issue, project, author, body, action: 'branch'))
   end
 
   # Called when a Mentionable references a Noteable
@@ -349,23 +358,12 @@ module SystemNoteService
     return if cross_reference_disallowed?(noteable, mentioner)
 
     gfm_reference = mentioner.gfm_reference(noteable.project)
-
-    note_options = {
-      project: noteable.project,
-      author:  author,
-      note:    cross_reference_note_content(gfm_reference)
-    }
-
-    if noteable.is_a?(Commit)
-      note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id)
-    else
-      note_options[:noteable] = noteable
-    end
+    body = cross_reference_note_content(gfm_reference)
 
     if noteable.is_a?(ExternalIssue)
       noteable.project.issues_tracker.create_cross_reference_note(noteable, mentioner, author)
     else
-      create_note(note_options)
+      create_note(NoteSummary.new(noteable, noteable.project, author, body, action: 'cross_reference'))
     end
   end
 
@@ -444,7 +442,8 @@ module SystemNoteService
   def change_task_status(noteable, project, author, new_task)
     status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE
     body = "marked the task **#{new_task.source}** as #{status_label}"
-    create_note(noteable: noteable, project: project, author: author, note: body)
+
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'task'))
   end
 
   # Called when noteable has been moved to another project
@@ -466,7 +465,8 @@ module SystemNoteService
 
     cross_reference = noteable_ref.to_reference(project)
     body = "moved #{direction} #{cross_reference}"
-    create_note(noteable: noteable, project: project, author: author, note: body)
+
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'moved'))
   end
 
   private
@@ -482,8 +482,11 @@ module SystemNoteService
     end
   end
 
-  def create_note(args = {})
-    Note.create(args.merge(system: true))
+  def create_note(note_summary)
+    note = Note.create(note_summary.note.merge(system: true))
+    note.system_note_metadata = SystemNoteMetadata.new(note_summary.metadata) if note_summary.metadata?
+
+    note
   end
 
   def cross_reference_note_prefix
diff --git a/changelogs/unreleased/24784-system-notes-meta-data.yml b/changelogs/unreleased/24784-system-notes-meta-data.yml
new file mode 100644
index 00000000000..757ae9e0527
--- /dev/null
+++ b/changelogs/unreleased/24784-system-notes-meta-data.yml
@@ -0,0 +1,4 @@
+---
+title: Add metadata to system notes
+merge_request: 9964
+author:
diff --git a/db/migrate/20170314082049_create_system_note_metadata.rb b/db/migrate/20170314082049_create_system_note_metadata.rb
index c58da88631f..dd1e6cf8172 100644
--- a/db/migrate/20170314082049_create_system_note_metadata.rb
+++ b/db/migrate/20170314082049_create_system_note_metadata.rb
@@ -5,15 +5,19 @@ class CreateSystemNoteMetadata < ActiveRecord::Migration
 
   disable_ddl_transaction!
 
-  def change
+  def up
     create_table :system_note_metadata do |t|
       t.references :note, null: false
       t.integer :commit_count
-      t.string :icon
+      t.string :action
 
       t.timestamps null: false
     end
 
-    add_concurrent_foreign_key :system_note_metadata, :notes, column: :note_id, on_delete: :cascade
+    add_concurrent_foreign_key :system_note_metadata, :notes, column: :note_id
+  end
+
+  def down
+    drop_table :system_note_metadata
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 456dbd41230..dba242548c1 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1078,7 +1078,7 @@ ActiveRecord::Schema.define(version: 20170317203554) do
   create_table "system_note_metadata", force: :cascade do |t|
     t.integer "note_id", null: false
     t.integer "commit_count"
-    t.string "icon"
+    t.string "action"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
   end
diff --git a/spec/factories/system_note_metadata.rb b/spec/factories/system_note_metadata.rb
index 5855f8d1051..f487a2d7e4a 100644
--- a/spec/factories/system_note_metadata.rb
+++ b/spec/factories/system_note_metadata.rb
@@ -1,6 +1,6 @@
 FactoryGirl.define do
   factory :system_note_metadata do
     note
-    icon 'merge'
+    action 'merge'
   end
 end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index ddeb71730e7..002cffd3062 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -29,6 +29,7 @@ notes:
 - resolved_by
 - todos
 - events
+- system_note_metadata
 label_links:
 - target
 - label
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 0ebdf46074e..33536487c41 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -9,7 +9,6 @@ describe Note, models: true do
     it { is_expected.to belong_to(:author).class_name('User') }
 
     it { is_expected.to have_many(:todos).dependent(:destroy) }
-    it { is_expected.to have_one(:system_note_metadata).dependent(:destroy) }
   end
 
   describe 'modules' do
diff --git a/spec/models/system_note_metadata_spec.rb b/spec/models/system_note_metadata_spec.rb
index ae5a43281ac..d238e28209a 100644
--- a/spec/models/system_note_metadata_spec.rb
+++ b/spec/models/system_note_metadata_spec.rb
@@ -8,17 +8,17 @@ describe SystemNoteMetadata, models: true do
   describe 'validation' do
     it { is_expected.to validate_presence_of(:note) }
 
-    context 'when icon type is invalid' do
+    context 'when action type is invalid' do
       subject do
-        build(:system_note_metadata, note: build(:note), icon: 'invalid_type' )
+        build(:system_note_metadata, note: build(:note), action: 'invalid_type' )
       end
 
       it { is_expected.to be_invalid }
     end
 
-    context 'when icon type is valid' do
+    context 'when action type is valid' do
       subject do
-        build(:system_note_metadata, note: build(:note), icon: 'merge' )
+        build(:system_note_metadata, note: build(:note), action: 'merge' )
       end
 
       it { is_expected.to be_valid }
diff --git a/spec/services/note_summary_spec.rb b/spec/services/note_summary_spec.rb
new file mode 100644
index 00000000000..39f06f8f8e7
--- /dev/null
+++ b/spec/services/note_summary_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+
+describe NoteSummary, services: true do
+  let(:project)  { build(:empty_project) }
+  let(:noteable) { build(:issue) }
+  let(:user)     { build(:user) }
+
+  def create_note_summary
+    described_class.new(noteable, project, user, 'note', action: 'icon', commit_count: 5)
+  end
+
+  describe '#metadata?' do
+    it 'returns true when metadata present' do
+      expect(create_note_summary.metadata?).to be_truthy
+    end
+
+    it 'returns false when metadata not present' do
+      expect(described_class.new(noteable, project, user, 'note').metadata?).to be_falsey
+    end
+  end
+
+  describe '#note' do
+    it 'returns note hash' do
+      expect(create_note_summary.note).to eq(noteable: noteable, project: project, author: user, note: 'note')
+    end
+
+    context 'when noteable is a commit' do
+      let(:noteable) { build(:commit) }
+
+      it 'returns note hash specific to commit' do
+        expect(create_note_summary.note).to eq(
+          noteable: nil, project: project, author: user, note: 'note',
+          noteable_type: 'Commit', commit_id: noteable.id
+        )
+      end
+    end
+  end
+
+  describe '#metadata' do
+    it 'returns metadata hash' do
+      expect(create_note_summary.metadata).to eq(action: 'icon', commit_count: 5)
+    end
+  end
+end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index d7bf4c39cc0..90cde705b85 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -8,12 +8,15 @@ describe SystemNoteService, services: true do
   let(:noteable) { create(:issue, project: project) }
 
   shared_examples_for 'a system note' do
+    let(:expected_noteable) { noteable }
+    let(:commit_count)      { nil }
+
     it 'is valid' do
       expect(subject).to be_valid
     end
 
     it 'sets the noteable model' do
-      expect(subject.noteable).to eq noteable
+      expect(subject.noteable).to eq expected_noteable
     end
 
     it 'sets the project' do
@@ -27,6 +30,19 @@ describe SystemNoteService, services: true do
     it 'is a system note' do
       expect(subject).to be_system
     end
+
+    context 'metadata' do
+      it 'creates a new system note metadata record' do
+        expect { subject }.to change{ SystemNoteMetadata.count }.from(0).to(1)
+      end
+
+      it 'creates a record correctly' do
+        metadata = subject.system_note_metadata
+
+        expect(metadata.commit_count).to eq(commit_count)
+        expect(metadata.action).to eq(action)
+      end
+    end
   end
 
   describe '.add_commits' do
@@ -38,7 +54,10 @@ describe SystemNoteService, services: true do
     let(:old_commits) { [] }
     let(:oldrev)      { nil }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:commit_count) { new_commits.size }
+      let(:action)       { 'commit' }
+    end
 
     describe 'note body' do
       let(:note_lines) { subject.note.split("\n").reject(&:blank?) }
@@ -117,7 +136,9 @@ describe SystemNoteService, services: true do
 
     let(:assignee) { create(:user) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'assignee' }
+    end
 
     context 'when assignee added' do
       it 'sets the note text' do
@@ -141,7 +162,9 @@ describe SystemNoteService, services: true do
     let(:added)   { [] }
     let(:removed) { [] }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'label' }
+    end
 
     context 'with added labels' do
       let(:added)   { labels }
@@ -176,7 +199,9 @@ describe SystemNoteService, services: true do
 
     let(:milestone) { create(:milestone, project: project) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'milestone' }
+    end
 
     context 'when milestone added' do
       it 'sets the note text' do
@@ -199,7 +224,9 @@ describe SystemNoteService, services: true do
     let(:status) { 'new_status' }
     let(:source) { nil }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'status' }
+    end
 
     context 'with a source' do
       let(:source) { double('commit', gfm_reference: 'commit 123456') }
@@ -225,7 +252,9 @@ describe SystemNoteService, services: true do
 
     subject { described_class.merge_when_pipeline_succeeds(noteable, project, author, noteable.diff_head_commit) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
 
     it "posts the 'merge when pipeline succeeds' system note" do
       expect(subject.note).to match(/enabled an automatic merge when the pipeline for (\w+\/\w+@)?\h{40} succeeds/)
@@ -240,7 +269,9 @@ describe SystemNoteService, services: true do
 
     subject { described_class.cancel_merge_when_pipeline_succeeds(noteable, project, author) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
 
     it "posts the 'merge when pipeline succeeds' system note" do
       expect(subject.note).to eq  "canceled the automatic merge"
@@ -253,7 +284,9 @@ describe SystemNoteService, services: true do
     subject { described_class.change_title(noteable, project, author, 'Old title') }
 
     context 'when noteable responds to `title`' do
-      it_behaves_like 'a system note'
+      it_behaves_like 'a system note' do
+        let(:action) { 'title' }
+      end
 
       it 'sets the note text' do
         expect(subject.note).
@@ -266,7 +299,9 @@ describe SystemNoteService, services: true do
     subject { described_class.change_issue_confidentiality(noteable, project, author) }
 
     context 'when noteable responds to `confidential`' do
-      it_behaves_like 'a system note'
+      it_behaves_like 'a system note' do
+        let(:action) { 'confidentiality' }
+      end
 
       it 'sets the note text' do
         expect(subject.note).to eq 'made the issue visible to everyone'
@@ -281,7 +316,9 @@ describe SystemNoteService, services: true do
     let(:old_branch) { 'old_branch'}
     let(:new_branch) { 'new_branch'}
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'branch' }
+    end
 
     context 'when target branch name changed' do
       it 'sets the note text' do
@@ -295,7 +332,9 @@ describe SystemNoteService, services: true do
 
     let(:project) { create(:project, :repository) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'branch' }
+    end
 
     context 'when source branch deleted' do
       it 'sets the note text' do
@@ -309,7 +348,9 @@ describe SystemNoteService, services: true do
 
     let(:project) { create(:project, :repository) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'branch' }
+    end
 
     context 'when a branch is created from the new branch button' do
       it 'sets the note text' do
@@ -323,7 +364,9 @@ describe SystemNoteService, services: true do
 
     let(:mentioner) { create(:issue, project: project) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'cross_reference' }
+    end
 
     context 'when cross-reference disallowed' do
       before do
@@ -333,6 +376,10 @@ describe SystemNoteService, services: true do
       it 'returns nil' do
         expect(subject).to be_nil
       end
+
+      it 'does not create a system note metadata record' do
+        expect { subject }.not_to change{ SystemNoteMetadata.count }
+      end
     end
 
     context 'when cross-reference allowed' do
@@ -340,6 +387,10 @@ describe SystemNoteService, services: true do
         expect(described_class).to receive(:cross_reference_disallowed?).and_return(false)
       end
 
+      it_behaves_like 'a system note' do
+        let(:action) { 'cross_reference' }
+      end
+
       describe 'note_body' do
         context 'cross-project' do
           let(:project2)  { create(:project, :repository) }
@@ -552,6 +603,9 @@ describe SystemNoteService, services: true do
       let(:direction) { :to }
 
       it_behaves_like 'cross project mentionable'
+      it_behaves_like 'a system note' do
+        let(:action) { 'moved' }
+      end
 
       it 'notifies about noteable being moved to' do
         expect(subject.note).to match('moved to')
@@ -562,6 +616,9 @@ describe SystemNoteService, services: true do
       let(:direction) { :from }
 
       it_behaves_like 'cross project mentionable'
+      it_behaves_like 'a system note' do
+        let(:action) { 'moved' }
+      end
 
       it 'notifies about noteable being moved from' do
         expect(subject.note).to match('moved from')
@@ -732,33 +789,34 @@ describe SystemNoteService, services: true do
     let(:merge_request) { discussion.noteable }
     let(:project) { merge_request.source_project }
     let(:issue) { create(:issue, project: project) }
-    let(:user) { create(:user) }
 
     def reloaded_merge_request
       MergeRequest.find(merge_request.id)
     end
 
-    before do
-      project.team << [user, :developer]
+    subject { described_class.discussion_continued_in_issue(discussion, project, author, issue) }
+
+    it_behaves_like 'a system note' do
+      let(:expected_noteable) { discussion.first_note.noteable }
+      let(:action)              { 'discussion' }
     end
 
     it 'creates a new note in the discussion' do
       # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
-      expect { SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue) }.
-        to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+      expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
     end
 
     it 'mentions the created issue in the system note' do
-      note = SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue)
-
-      expect(note.note).to include(issue.to_reference)
+      expect(subject.note).to include(issue.to_reference)
     end
   end
 
   describe '.change_time_estimate' do
     subject { described_class.change_time_estimate(noteable, project, author) }
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'time_tracking' }
+    end
 
     context 'with a time estimate' do
       it 'sets the note text' do
@@ -788,7 +846,9 @@ describe SystemNoteService, services: true do
       described_class.change_time_spent(noteable, project, author)
     end
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'time_tracking' }
+    end
 
     context 'when time was added' do
       it 'sets the note text' do
@@ -820,6 +880,34 @@ describe SystemNoteService, services: true do
     end
   end
 
+  describe '.remove_merge_request_wip' do
+    let(:noteable) { create(:issue, project: project, title: 'WIP: Lorem ipsum') }
+
+    subject { described_class.remove_merge_request_wip(noteable, project, author) }
+
+    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**'
+    end
+  end
+
+  describe '.add_merge_request_wip' do
+    let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') }
+
+    subject { described_class.add_merge_request_wip(noteable, project, author) }
+
+    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**'
+    end
+  end
+
   describe '.add_merge_request_wip_from_commit' do
     let(:project) { create(:project, :repository) }
     let(:noteable) do
@@ -835,7 +923,9 @@ describe SystemNoteService, services: true do
       )
     end
 
-    it_behaves_like 'a system note'
+    it_behaves_like 'a system note' do
+      let(:action) { 'title' }
+    end
 
     it "posts the 'marked as a Work In Progress from commit' system note" do
       expect(subject.note).to match(
@@ -843,4 +933,33 @@ describe SystemNoteService, services: true do
       )
     end
   end
+
+  describe '.change_task_status' do
+    let(:noteable) { create(:issue, project: project) }
+    let(:task)     { double(:task, complete?: true, source: 'task') }
+
+    subject { described_class.change_task_status(noteable, project, author, task) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'task' }
+    end
+
+    it "posts the 'marked as a Work In Progress from commit' system note" do
+      expect(subject.note).to eq("marked the task **task** as completed")
+    end
+  end
+
+  describe '.resolve_all_discussions' do
+    let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
+
+    subject { described_class.resolve_all_discussions(noteable, project, author) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'discussion' }
+    end
+
+    it 'sets the note text' do
+      expect(subject.note).to eq 'resolved all discussions'
+    end
+  end
 end