From 1c3c7fb25d972fc19d5b4bb371cb21094d81e478 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Wed, 15 Mar 2017 14:18:44 +0100 Subject: [PATCH 1/2] Add system_note_metadata model --- app/models/note.rb | 5 +++- app/models/system_note_metadata.rb | 11 ++++++++ config/initializers/0_inflections.rb | 2 +- ...70314082049_create_system_note_metadata.rb | 19 +++++++++++++ db/schema.rb | 9 +++++++ spec/factories/system_note_metadata.rb | 6 +++++ spec/models/note_spec.rb | 1 + spec/models/system_note_metadata_spec.rb | 27 +++++++++++++++++++ 8 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 app/models/system_note_metadata.rb create mode 100644 db/migrate/20170314082049_create_system_note_metadata.rb create mode 100644 spec/factories/system_note_metadata.rb create mode 100644 spec/models/system_note_metadata_spec.rb diff --git a/app/models/note.rb b/app/models/note.rb index e22e96aec6f..ba5a552f469 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -37,6 +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 delegate :gfm_reference, :local_reference, to: :noteable delegate :name, to: :project, prefix: true @@ -70,7 +71,9 @@ class Note < ActiveRecord::Base scope :fresh, ->{ order(created_at: :asc, id: :asc) } scope :inc_author_project, ->{ includes(:project, :author) } scope :inc_author, ->{ includes(:author) } - scope :inc_relations_for_view, ->{ includes(:project, :author, :updated_by, :resolved_by, :award_emoji) } + scope :inc_relations_for_view, -> do + includes(:project, :author, :updated_by, :resolved_by, :award_emoji, :system_note_metadata) + end scope :diff_notes, ->{ where(type: %w(LegacyDiffNote DiffNote)) } scope :non_diff_notes, ->{ where(type: ['Note', nil]) } diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb new file mode 100644 index 00000000000..e8d3600d341 --- /dev/null +++ b/app/models/system_note_metadata.rb @@ -0,0 +1,11 @@ +class SystemNoteMetadata < ActiveRecord::Base + ICON_TYPES = %w[ + commit merge confidentiality status label assignee cross_reference + title time_tracking branch milestone discussion task moved + ].freeze + + validates :note, presence: true + validates :icon, inclusion: ICON_TYPES, allow_nil: true + + belongs_to :note +end diff --git a/config/initializers/0_inflections.rb b/config/initializers/0_inflections.rb index d4197da3fa9..f977104ff9d 100644 --- a/config/initializers/0_inflections.rb +++ b/config/initializers/0_inflections.rb @@ -10,5 +10,5 @@ # end # ActiveSupport::Inflector.inflections do |inflect| - inflect.uncountable %w(award_emoji project_statistics) + inflect.uncountable %w(award_emoji project_statistics system_note_metadata) end diff --git a/db/migrate/20170314082049_create_system_note_metadata.rb b/db/migrate/20170314082049_create_system_note_metadata.rb new file mode 100644 index 00000000000..c58da88631f --- /dev/null +++ b/db/migrate/20170314082049_create_system_note_metadata.rb @@ -0,0 +1,19 @@ +class CreateSystemNoteMetadata < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def change + create_table :system_note_metadata do |t| + t.references :note, null: false + t.integer :commit_count + t.string :icon + + t.timestamps null: false + end + + add_concurrent_foreign_key :system_note_metadata, :notes, column: :note_id, on_delete: :cascade + end +end diff --git a/db/schema.rb b/db/schema.rb index f476637ceb2..456dbd41230 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1075,6 +1075,14 @@ ActiveRecord::Schema.define(version: 20170317203554) do add_index "subscriptions", ["subscribable_id", "subscribable_type", "user_id", "project_id"], name: "index_subscriptions_on_subscribable_and_user_id_and_project_id", unique: true, using: :btree + create_table "system_note_metadata", force: :cascade do |t| + t.integer "note_id", null: false + t.integer "commit_count" + t.string "icon" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "taggings", force: :cascade do |t| t.integer "tag_id" t.integer "taggable_id" @@ -1308,6 +1316,7 @@ ActiveRecord::Schema.define(version: 20170317203554) do add_foreign_key "protected_branch_merge_access_levels", "protected_branches" add_foreign_key "protected_branch_push_access_levels", "protected_branches" add_foreign_key "subscriptions", "projects", on_delete: :cascade + add_foreign_key "system_note_metadata", "notes", name: "fk_d83a918cb1", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade add_foreign_key "trending_projects", "projects", on_delete: :cascade diff --git a/spec/factories/system_note_metadata.rb b/spec/factories/system_note_metadata.rb new file mode 100644 index 00000000000..5855f8d1051 --- /dev/null +++ b/spec/factories/system_note_metadata.rb @@ -0,0 +1,6 @@ +FactoryGirl.define do + factory :system_note_metadata do + note + icon 'merge' + end +end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 33536487c41..0ebdf46074e 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -9,6 +9,7 @@ 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 new file mode 100644 index 00000000000..ae5a43281ac --- /dev/null +++ b/spec/models/system_note_metadata_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe SystemNoteMetadata, models: true do + describe 'associations' do + it { is_expected.to belong_to(:note) } + end + + describe 'validation' do + it { is_expected.to validate_presence_of(:note) } + + context 'when icon type is invalid' do + subject do + build(:system_note_metadata, note: build(:note), icon: 'invalid_type' ) + end + + it { is_expected.to be_invalid } + end + + context 'when icon type is valid' do + subject do + build(:system_note_metadata, note: build(:note), icon: 'merge' ) + end + + it { is_expected.to be_valid } + end + end +end From c729d9dae7ad8c32cbe93b590baa24c8133364e5 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Wed, 15 Mar 2017 14:19:45 +0100 Subject: [PATCH 2/2] 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