diff options
44 files changed, 1077 insertions, 297 deletions
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 186098c67b7..8f82021a464 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -1,4 +1,5 @@ class Projects::NotesController < Projects::ApplicationController + include NotesHelper include ToggleAwardEmoji # Authorize @@ -12,7 +13,8 @@ class Projects::NotesController < Projects::ApplicationController notes_json = { notes: [], last_fetched_at: current_fetched_at } - @notes = notes_finder.execute.inc_author + @notes = notes_finder.execute.inc_relations_for_view + @notes = prepare_notes_for_rendering(@notes) @notes.each do |note| next if note.cross_reference_not_visible_for?(current_user) @@ -117,7 +119,7 @@ class Projects::NotesController < Projects::ApplicationController end def discussion_html(discussion) - return if discussion.render_as_individual_notes? + return if discussion.individual_note? render_to_string( "discussions/_discussion", @@ -153,21 +155,20 @@ class Projects::NotesController < Projects::ApplicationController def note_json(note) attrs = { - id: note.id + commands_changes: note.commands_changes } if note.persisted? - Banzai::NoteRenderer.render([note], @project, current_user) - attrs.merge!( valid: true, + id: note.id, discussion_id: note.discussion_id(noteable), html: note_html(note), note: note.note ) discussion = note.to_discussion(noteable) - unless discussion.render_as_individual_notes? + unless discussion.individual_note? attrs.merge!( discussion_resolvable: discussion.resolvable?, @@ -187,7 +188,6 @@ class Projects::NotesController < Projects::ApplicationController ) end - attrs[:commands_changes] = note.commands_changes attrs end diff --git a/app/models/commit_discussion.rb b/app/models/commit_discussion.rb deleted file mode 100644 index bcca3155335..00000000000 --- a/app/models/commit_discussion.rb +++ /dev/null @@ -1,9 +0,0 @@ -class CommitDiscussion < Discussion - def self.override_discussion_id(note) - discussion_id(note) - end - - def potentially_resolvable? - false - end -end diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb index eecb77ebf80..2aba979938b 100644 --- a/app/models/concerns/resolvable_note.rb +++ b/app/models/concerns/resolvable_note.rb @@ -1,13 +1,18 @@ module ResolvableNote extend ActiveSupport::Concern + RESOLVABLE_TYPES = %w(DiffNote DiscussionNote).freeze + included do belongs_to :resolved_by, class_name: "User" validates :resolved_by, presence: true, if: :resolved? - # Keep this scope in sync with the logic in `#resolvable?` in `Note` subclasses that are resolvable - scope :resolvable, -> { where(type: %w(DiffNote DiscussionNote)).user.where(noteable_type: 'MergeRequest') } + # Keep this scope in sync with the logic in `#potentially_resolvable?` in `Discussion` subclasses that are resolvable + scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: 'MergeRequest') } + # Keep this scope in sync with `#resolvable?` + scope :resolvable, -> { potentially_resolvable.user } + scope :resolved, -> { resolvable.where.not(resolved_at: nil) } scope :unresolved, -> { resolvable.where(resolved_at: nil) } end @@ -24,9 +29,11 @@ module ResolvableNote end end - # If you update this method remember to also update the scope `resolvable` + delegate :potentially_resolvable?, to: :to_discussion + + # Keep this method in sync with the `resolvable` scope def resolvable? - to_discussion.potentially_resolvable? && !system? + potentially_resolvable? && !system? end def resolved? diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index e5f437f8647..d24491b44e7 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -1,6 +1,8 @@ class DiffNote < Note include NoteOnDiff + NOTEABLE_TYPES = %w(MergeRequest Commit).freeze + serialize :original_position, Gitlab::Diff::Position serialize :position, Gitlab::Diff::Position @@ -8,7 +10,7 @@ class DiffNote < Note validates :position, presence: true validates :diff_line, presence: true validates :line_code, presence: true, line_code: true - validates :noteable_type, inclusion: { in: %w(Commit MergeRequest) } + validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } validate :positions_complete validate :verify_supported diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 8ab9031e42c..6e97a4862ed 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -1,7 +1,7 @@ class Discussion MEMOIZED_VALUES = [] # rubocop:disable Style/MutableConstant - attr_reader :notes + attr_reader :notes, :noteable delegate :created_at, :project, @@ -61,6 +61,13 @@ class Discussion @noteable = noteable end + def ==(other) + other.class == self.class && + other.noteable == self.noteable && + other.id == self.id && + other.notes == self.notes + end + def last_updated_at last_note.created_at end @@ -83,7 +90,7 @@ class Discussion false end - def render_as_individual_notes? + def individual_note? false end @@ -91,8 +98,9 @@ class Discussion notes.length == 1 end + # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote` def potentially_resolvable? - first_note.for_merge_request? + for_merge_request? end def resolvable? @@ -162,12 +170,7 @@ class Discussion end def collapsed? - if resolvable? - # New diff discussions only disappear once they are marked resolved - resolved? - else - false - end + resolved? end def expanded? diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb index 8c82d217126..585b8527883 100644 --- a/app/models/individual_note_discussion.rb +++ b/app/models/individual_note_discussion.rb @@ -3,11 +3,12 @@ class IndividualNoteDiscussion < Discussion [*super(note), SecureRandom.hex] end + # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote` def potentially_resolvable? false end - def render_as_individual_notes? + def individual_note? true end end diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb index e13f17abdbe..eb9766a9ffe 100644 --- a/app/models/legacy_diff_discussion.rb +++ b/app/models/legacy_diff_discussion.rb @@ -11,6 +11,7 @@ class LegacyDiffDiscussion < DiffDiscussion true end + # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote` def potentially_resolvable? false end diff --git a/app/models/note.rb b/app/models/note.rb index 06ceb60b982..00a58afd2b6 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -56,7 +56,7 @@ class Note < ActiveRecord::Base validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note| unless note.noteable.try(:project) == note.project - errors.add(:invalid_project, 'Note and noteable project mismatch') + errors.add(:project, 'does not match noteable project') end end @@ -236,14 +236,14 @@ class Note < ActiveRecord::Base end def can_be_discussion_note? - DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) + DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) && !part_of_discussion? end def discussion_class(noteable = nil) # When commit notes are rendered on an MR's Discussion page, they are # displayed in one discussion instead of individually - if noteable && noteable != self.noteable && for_commit? - CommitDiscussion + if noteable && noteable != self.noteable + OutOfContextDiscussion else IndividualNoteDiscussion end @@ -268,7 +268,24 @@ class Note < ActiveRecord::Base end def part_of_discussion? - !to_discussion.render_as_individual_notes? + !to_discussion.individual_note? + end + + def in_reply_to?(other) + case other + when Note + if part_of_discussion? + in_reply_to?(other.noteable) && in_reply_to?(other.to_discussion) + else + in_reply_to?(other.noteable) + end + when Discussion + self.discussion_id == other.id + when Noteable + self.noteable == other + else + false + end end private diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb new file mode 100644 index 00000000000..0019064e25c --- /dev/null +++ b/app/models/out_of_context_discussion.rb @@ -0,0 +1,12 @@ +class OutOfContextDiscussion < Discussion + # To make sure all out-of-context notes are displayed in one discussion, + # we override the discussion ID to be a newly generated but consistent ID. + def self.override_discussion_id(note) + discussion_id(note) + end + + # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote` + def potentially_resolvable? + false + end +end diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index 6770af6fffd..81fc2ddac77 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -23,9 +23,7 @@ class SentNotification < ActiveRecord::Base find_by(reply_key: reply_key) end - def record(noteable, recipient_id, reply_key, attrs = {}) - return unless reply_key - + def record(noteable, recipient_id, reply_key = self.reply_key, attrs = {}) noteable_id = nil commit_id = nil if noteable.is_a?(Commit) @@ -47,7 +45,7 @@ class SentNotification < ActiveRecord::Base create(attrs) end - def record_note(note, recipient_id, reply_key, attrs = {}) + def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {}) attrs[:in_reply_to_discussion_id] = note.original_discussion_id record(note.noteable, recipient_id, reply_key, attrs) @@ -87,7 +85,14 @@ class SentNotification < ActiveRecord::Base self.reply_key end - def note_params + def create_reply(message, dryrun: false) + klass = dryrun ? Notes::BuildService : Notes::CreateService + klass.new(self.project, self.recipient, reply_params.merge(note: message)).execute + end + + private + + def reply_params attrs = { noteable_type: self.noteable_type, noteable_id: self.noteable_id, @@ -111,10 +116,12 @@ class SentNotification < ActiveRecord::Base attrs end - private - def note_valid - Notes::BuildService.new(self.project, self.recipient, note_params.merge(note: 'Test')).execute.valid? + note = create_reply('Test', dryrun: true) + + unless note.valid? + self.errors.add(:base, "Note parameters are invalid: #{note.errors.full_messages.to_sentence}") + end end def keep_around_commit diff --git a/app/services/concerns/issues/resolve_discussions.rb b/app/services/concerns/issues/resolve_discussions.rb index 378967f0231..910a2a15e5d 100644 --- a/app/services/concerns/issues/resolve_discussions.rb +++ b/app/services/concerns/issues/resolve_discussions.rb @@ -21,7 +21,7 @@ module Issues @discussions_to_resolve ||= if discussion_to_resolve_id discussion_or_nil = merge_request_to_resolve_discussions_of - .find_diff_discussion(discussion_to_resolve_id) + .find_discussion(discussion_to_resolve_id) Array(discussion_or_nil) else merge_request_to_resolve_discussions_of diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb index 47ed3e3fc3d..8d5a022d319 100644 --- a/app/services/notes/build_service.rb +++ b/app/services/notes/build_service.rb @@ -1,9 +1,6 @@ module Notes class BuildService < BaseService def execute - # TODO: Remove when we use a selectbox instead of a submit button - params[:type] = DiscussionNote.name if params.delete(:new_discussion) - in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id) if project && in_reply_to_discussion_id.present? discussion = @@ -17,6 +14,9 @@ module Notes end params.merge!(discussion.reply_attributes) + elsif params.delete(:new_discussion) + # TODO: Remove when we use a selectbox instead of a submit button + params[:type] = DiscussionNote.name end note = Note.new(params) diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml index d1855568215..c762578971a 100644 --- a/app/views/notify/new_issue_email.html.haml +++ b/app/views/notify/new_issue_email.html.haml @@ -1,9 +1,11 @@ - if current_application_settings.email_author_in_body - %div - #{link_to @issue.author_name, user_url(@issue.author)} wrote: -- if @issue.description - = markdown(@issue.description, pipeline: :email, author: @issue.author) + %p.details + #{link_to @issue.author_name, user_url(@issue.author)} created an issue: - if @issue.assignee_id.present? %p Assignee: #{@issue.assignee_name} + +- if @issue.description + %div + = markdown(@issue.description, pipeline: :email, author: @issue.author) diff --git a/app/views/notify/new_mention_in_issue_email.html.haml b/app/views/notify/new_mention_in_issue_email.html.haml index 02f21baa368..d7c22219285 100644 --- a/app/views/notify/new_mention_in_issue_email.html.haml +++ b/app/views/notify/new_mention_in_issue_email.html.haml @@ -1,12 +1,4 @@ %p You have been mentioned in an issue. -- if current_application_settings.email_author_in_body - %div - #{link_to @issue.author_name, user_url(@issue.author)} wrote: -- if @issue.description - = markdown(@issue.description, pipeline: :email, author: @issue.author) - -- if @issue.assignee_id.present? - %p - Assignee: #{@issue.assignee_name} += render template: 'new_issue_email' diff --git a/app/views/notify/new_mention_in_merge_request_email.html.haml b/app/views/notify/new_mention_in_merge_request_email.html.haml index cbd434be02a..07d8c301988 100644 --- a/app/views/notify/new_mention_in_merge_request_email.html.haml +++ b/app/views/notify/new_mention_in_merge_request_email.html.haml @@ -1,15 +1,4 @@ %p You have been mentioned in Merge Request #{@merge_request.to_reference} -- if current_application_settings.email_author_in_body - %div - #{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote: -%p.details - != merge_path_description(@merge_request, '→') - -- if @merge_request.assignee_id.present? - %p - Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name} - -- if @merge_request.description - = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author) += render template: 'new_merge_request_email' diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml index 8890b300f7d..951c96bdb9c 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/app/views/notify/new_merge_request_email.html.haml @@ -1,12 +1,14 @@ - if current_application_settings.email_author_in_body - %div - #{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote: + %p.details + #{link_to @merge_request.author_name, user_url(@merge_request.author)} created a merge request: + %p.details != merge_path_description(@merge_request, '→') - if @merge_request.assignee_id.present? %p - Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name} + Assignee: #{@merge_request.assignee_name} - if @merge_request.description - = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author) + %div + = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author) diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml index d619f7ac262..2b2bab09c74 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -1,6 +1,6 @@ - if defined?(@discussions) - @discussions.each do |discussion| - - if discussion.render_as_individual_notes? + - if discussion.individual_note? = render partial: "projects/notes/note", collection: discussion.notes, as: :note - else = render 'discussions/discussion', discussion: discussion diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb index 0a9e928d13f..0e22f2189ee 100644 --- a/lib/gitlab/email/handler/create_note_handler.rb +++ b/lib/gitlab/email/handler/create_note_handler.rb @@ -1,4 +1,3 @@ - require 'gitlab/email/handler/base_handler' require 'gitlab/email/handler/reply_processing' @@ -42,11 +41,7 @@ module Gitlab end def create_note - Notes::CreateService.new( - project, - author, - sent_notification.note_params.merge(note: message) - ).execute + sent_notification.create_reply(message) end end end diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index a3b0e8a252e..b276f9321c7 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -15,14 +15,163 @@ describe Projects::NotesController do end describe 'GET index' do - # It renders the discussion partial for any threaded note - # TODO: Test + let(:last_fetched_at) { '1487756246' } + let(:request_params) do + { + namespace_id: project.namespace, + project_id: project, + target_type: 'issue', + target_id: issue.id, + format: 'json' + } + end + + let(:parsed_response) { JSON.parse(response.body).with_indifferent_access } + let(:note_json) { parsed_response[:notes].first } + + before do + sign_in(user) + project.team << [user, :developer] + end + + it 'passes last_fetched_at from headers to NotesFinder' do + request.headers['X-Last-Fetched-At'] = last_fetched_at + + expect(NotesFinder).to receive(:new) + .with(anything, anything, hash_including(last_fetched_at: last_fetched_at)) + .and_call_original + + get :index, request_params + end + + context 'for a discussion note' do + let!(:note) { create(:discussion_note_on_issue, noteable: issue, project: project) } + + it 'includes the ID' do + get :index, request_params + + expect(note_json[:id]).to eq(note.id) + end + + it 'includes discussion_html' do + get :index, request_params + + expect(note_json[:discussion_html]).not_to be_nil + end + + it "doesn't include diff_discussion_html" do + get :index, request_params + + expect(note_json[:diff_discussion_html]).to be_nil + end + end + + context 'for a diff discussion note' do + let(:project) { create(:project, :repository) } + let!(:note) { create(:diff_note_on_merge_request, project: project) } + + let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) } + + it 'includes the ID' do + get :index, params + + expect(note_json[:id]).to eq(note.id) + end + + it 'includes discussion_html' do + get :index, params + + expect(note_json[:discussion_html]).not_to be_nil + end + + it 'includes diff_discussion_html' do + get :index, params + + expect(note_json[:diff_discussion_html]).not_to be_nil + end + end + + context 'for a commit note' do + let(:project) { create(:project, :repository) } + let!(:note) { create(:note_on_commit, project: project) } + + context 'when displayed on a merge request' do + let(:merge_request) { create(:merge_request, source_project: project) } + + let(:params) { request_params.merge(target_type: 'merge_request', target_id: merge_request.id) } + + it 'includes the ID' do + get :index, params + + expect(note_json[:id]).to eq(note.id) + end + + it 'includes discussion_html' do + get :index, params + + expect(note_json[:discussion_html]).not_to be_nil + end + + it "doesn't include diff_discussion_html" do + get :index, params + + expect(note_json[:diff_discussion_html]).to be_nil + end + end + + context 'when displayed on the commit' do + let(:params) { request_params.merge(target_type: 'commit', target_id: note.commit_id) } + + it 'includes the ID' do + get :index, params + + expect(note_json[:id]).to eq(note.id) + end + + it "doesn't include discussion_html" do + get :index, params + + expect(note_json[:discussion_html]).to be_nil + end + + it "doesn't include diff_discussion_html" do + get :index, params + + expect(note_json[:diff_discussion_html]).to be_nil + end + end + end + + context 'for a regular note' do + let!(:note) { create(:note, noteable: issue, project: project) } + + it 'includes the ID' do + get :index, request_params + + expect(note_json[:id]).to eq(note.id) + end + + it 'includes html' do + get :index, request_params + + expect(note_json[:html]).not_to be_nil + end + + it "doesn't include discussion_html" do + get :index, request_params + + expect(note_json[:discussion_html]).to be_nil + end + + it "doesn't include diff_discussion_html" do + get :index, request_params + + expect(note_json[:diff_discussion_html]).to be_nil + end + end end describe 'POST create' do - # Test :type, :new_discussion, :in_reply_to_discussion_id (in_reply_to_id?) - # TODO: Test - let(:merge_request) { create(:merge_request) } let(:project) { merge_request.source_project } let(:request_params) do @@ -210,31 +359,4 @@ describe Projects::NotesController do end end end - - describe 'GET index' do - let(:last_fetched_at) { '1487756246' } - let(:request_params) do - { - namespace_id: project.namespace, - project_id: project, - target_type: 'issue', - target_id: issue.id - } - end - - before do - sign_in(user) - project.team << [user, :developer] - end - - it 'passes last_fetched_at from headers to NotesFinder' do - request.headers['X-Last-Fetched-At'] = last_fetched_at - - expect(NotesFinder).to receive(:new) - .with(anything, anything, hash_including(last_fetched_at: last_fetched_at)) - .and_call_original - - get :index, request_params - end - end end diff --git a/spec/factories/discussions.rb b/spec/factories/discussions.rb deleted file mode 100644 index 5e3a9b1e698..00000000000 --- a/spec/factories/discussions.rb +++ /dev/null @@ -1,5 +0,0 @@ -FactoryGirl.define do - factory :discussion do - # TODO: Implement - end -end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 0ad7034e96d..0f9056a4914 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -25,6 +25,14 @@ FactoryGirl.define do end end + factory :discussion_note_on_issue, traits: [:on_issue], class: DiscussionNote do + association :project, :repository + end + + factory :discussion_note_on_commit, traits: [:on_commit], class: DiscussionNote do + association :project, :repository + end + factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote do association :project, :repository end @@ -46,7 +54,7 @@ FactoryGirl.define do new_path: "files/ruby/popen.rb", old_line: nil, new_line: line_number, - diff_refs: noteable.diff_refs + diff_refs: noteable.try(:diff_refs) ) end @@ -127,8 +135,8 @@ FactoryGirl.define do next unless discussion discussion = discussion.to_discussion if discussion.is_a?(Note) next unless discussion - - note.assign_attributes(discussion.reply_attributes) + + note.assign_attributes(discussion.reply_attributes.merge(project: discussion.project)) end end end diff --git a/spec/factories/sent_notifications.rb b/spec/factories/sent_notifications.rb index 6287c40afe9..0590bf999c0 100644 --- a/spec/factories/sent_notifications.rb +++ b/spec/factories/sent_notifications.rb @@ -3,6 +3,6 @@ FactoryGirl.define do project factory: :empty_project recipient factory: :user noteable factory: :issue - reply_key "0123456789abcdef" * 2 + reply_key { SentNotification.reply_key } end end diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index d2ec42ae70f..765bf44d863 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -204,6 +204,43 @@ describe NotesFinder do end describe '#target' do - # TODO: Test + subject { described_class.new(project, user, params) } + + context 'for a issue target' do + let(:issue) { create(:issue, project: project) } + let(:params) { { target_type: 'issue', target_id: issue.id } } + + it 'returns the issue' do + expect(subject.target).to eq(issue) + end + end + + context 'for a merge request target' do + let(:merge_request) { create(:merge_request, source_project: project) } + let(:params) { { target_type: 'merge_request', target_id: merge_request.id } } + + it 'returns the merge_request' do + expect(subject.target).to eq(merge_request) + end + end + + context 'for a snippet target' do + let(:snippet) { create(:project_snippet, project: project) } + let(:params) { { target_type: 'snippet', target_id: snippet.id } } + + it 'returns the snippet' do + expect(subject.target).to eq(snippet) + end + end + + context 'for a commit target' do + let(:project) { create(:project, :repository) } + let(:commit) { project.commit } + let(:params) { { target_type: 'commit', target_id: commit.id } } + + it 'returns the commit' do + expect(subject.target).to eq(commit) + end + end end end diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb index f0554cc068d..540cb0ab1e0 100644 --- a/spec/helpers/issues_helper_spec.rb +++ b/spec/helpers/issues_helper_spec.rb @@ -150,7 +150,7 @@ describe IssuesHelper do describe "when passing a discussion" do let(:diff_note) { create(:diff_note_on_merge_request) } let(:merge_request) { diff_note.noteable } - let(:discussion) { Discussion.new([diff_note]) } + let(:discussion) { diff_note.to_discussion } it "links to the merge request with first note if a single discussion was passed" do expected_path = Gitlab::UrlBuilder.build(diff_note) diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index b300feaabe1..3f79eaf7afb 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -143,6 +143,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler, lib: true do expect(new_note.author).to eq(sent_notification.recipient) expect(new_note.position).to eq(note.position) expect(new_note.note).to include("I could not disagree more.") + expect(new_note.in_reply_to?(note)).to be_truthy end it "adds all attachments" do diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 01246f87dff..fcbbc3166ed 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -63,7 +63,7 @@ describe Notify do it 'contains a link to note author' do is_expected.to have_html_escaped_body_text(issue.author_name) - is_expected.to have_body_text 'wrote:' + is_expected.to have_body_text 'created an issue:' end end end @@ -215,7 +215,7 @@ describe Notify do it 'contains a link to note author' do is_expected.to have_html_escaped_body_text merge_request.author_name - is_expected.to have_body_text 'wrote:' + is_expected.to have_body_text 'created a merge request:' end end end @@ -536,8 +536,6 @@ describe Notify do allow(Note).to receive(:find).with(note.id).and_return(note) end - # TODO: Test discussions - shared_examples 'a note email' do it_behaves_like 'it should have Gmail Actions links' @@ -556,7 +554,7 @@ describe Notify do end it 'does not contain note author' do - is_expected.not_to have_body_text 'wrote:' + is_expected.not_to have_body_text note.author_name end context 'when enabled email_author_in_body' do @@ -566,7 +564,6 @@ describe Notify do it 'contains a link to note author' do is_expected.to have_html_escaped_body_text note.author_name - is_expected.to have_body_text 'wrote:' end end end @@ -639,7 +636,7 @@ describe Notify do end end - context 'items that are noteable, emails for a note on a diff' do + context 'items that are noteable, the email for a discussion note' do let(:project) { create(:project, :repository) } let(:note_author) { create(:user, name: 'author_name') } @@ -647,7 +644,117 @@ describe Notify do allow(Note).to receive(:find).with(note.id).and_return(note) end - shared_examples 'a note email on a diff' do |model| + shared_examples 'a discussion note email' do |model| + it_behaves_like 'it should have Gmail Actions links' + + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + expect(sender.display_name).to eq(note_author.name) + expect(sender.address).to eq(gitlab_sender) + end + + it 'is sent to the given recipient' do + is_expected.to deliver_to recipient.notification_email + end + + it 'contains the message from the note' do + is_expected.to have_body_text note.note + end + + it 'contains an introduction' do + is_expected.to have_body_text 'started a new discussion' + end + + context 'when a comment on an existing discussion' do + let!(:second_note) { create(model, author: note_author, noteable: nil, in_reply_to: note) } + + it 'contains an introduction' do + is_expected.to have_body_text 'commented on a' + end + end + end + + describe 'on a commit' do + let(:commit) { project.commit } + let(:note) { create(:discussion_note_on_commit, commit_id: commit.id, project: project, author: note_author) } + + before(:each) { allow(note).to receive(:noteable).and_return(commit) } + + subject { Notify.note_commit_email(recipient.id, note.id) } + + it_behaves_like 'a discussion note email', :discussion_note_on_commit + it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + let(:model) { commit } + end + it_behaves_like 'it should show Gmail Actions View Commit link' + it_behaves_like 'a user cannot unsubscribe through footer link' + + it 'has the correct subject' do + is_expected.to have_subject "Re: #{project.name} | #{commit.title.strip} (#{commit.short_id})" + end + + it 'contains a link to the commit' do + is_expected.to have_body_text commit.short_id + end + end + + describe 'on a merge request' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, author: note_author) } + let(:note_on_merge_request_path) { namespace_project_merge_request_path(project.namespace, project, merge_request, anchor: "note_#{note.id}") } + before(:each) { allow(note).to receive(:noteable).and_return(merge_request) } + + subject { Notify.note_merge_request_email(recipient.id, note.id) } + + it_behaves_like 'a discussion note email', :discussion_note_on_merge_request + it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + let(:model) { merge_request } + end + it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like 'an unsubscribeable thread' + + it 'has the correct subject' do + is_expected.to have_referable_subject(merge_request, reply: true) + end + + it 'contains a link to the merge request note' do + is_expected.to have_body_text note_on_merge_request_path + end + end + + describe 'on an issue' do + let(:issue) { create(:issue, project: project) } + let(:note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: note_author) } + let(:note_on_issue_path) { namespace_project_issue_path(project.namespace, project, issue, anchor: "note_#{note.id}") } + before(:each) { allow(note).to receive(:noteable).and_return(issue) } + + subject { Notify.note_issue_email(recipient.id, note.id) } + + it_behaves_like 'a discussion note email', :discussion_note_on_issue + it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + let(:model) { issue } + end + it_behaves_like 'it should show Gmail Actions View Issue link' + it_behaves_like 'an unsubscribeable thread' + + it 'has the correct subject' do + is_expected.to have_referable_subject(issue, reply: true) + end + + it 'contains a link to the issue note' do + is_expected.to have_body_text note_on_issue_path + end + end + end + + context 'items that are noteable, the email for a diff discussion note' do + let(:note_author) { create(:user, name: 'author_name') } + + before :each do + allow(Note).to receive(:find).with(note.id).and_return(note) + end + + shared_examples 'an email for a note on a diff discussion' do |model| let(:note) { create(model, project: project, author: note_author) } it "includes diffs with character-level highlighting" do @@ -674,18 +781,15 @@ describe Notify do is_expected.to have_html_escaped_body_text note.note end - it 'does not contain note author' do - is_expected.not_to have_body_text 'wrote:' + it 'contains an introduction' do + is_expected.to have_body_text 'started a new discussion on' end - context 'when enabled email_author_in_body' do - before do - stub_application_setting(email_author_in_body: true) - end + context 'when a comment on an existing discussion' do + let!(:second_note) { create(model, author: note_author, noteable: nil, in_reply_to: note) } - it 'contains a link to note author' do - is_expected.to have_html_escaped_body_text note.author_name - is_expected.to have_body_text 'wrote:' + it 'contains an introduction' do + is_expected.to have_body_text 'commented on a discussion on' end end end @@ -696,7 +800,7 @@ describe Notify do subject { Notify.note_commit_email(recipient.id, note.id) } - it_behaves_like 'a note email on a diff', :diff_note_on_commit + it_behaves_like 'an email for a note on a diff discussion', :diff_note_on_commit it_behaves_like 'it should show Gmail Actions View Commit link' it_behaves_like 'a user cannot unsubscribe through footer link' end @@ -707,7 +811,7 @@ describe Notify do subject { Notify.note_merge_request_email(recipient.id, note.id) } - it_behaves_like 'a note email on a diff', :diff_note_on_merge_request + it_behaves_like 'an email for a note on a diff discussion', :diff_note_on_merge_request it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' end diff --git a/spec/models/commit_discussion_spec.rb b/spec/models/commit_discussion_spec.rb deleted file mode 100644 index 6a5042d8827..00000000000 --- a/spec/models/commit_discussion_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'spec_helper' - -describe CommitDiscussion, model: true do - # TODO: Test -end diff --git a/spec/models/concerns/noteable_spec.rb b/spec/models/concerns/noteable_spec.rb index 51855fb2f0d..24962d3b074 100644 --- a/spec/models/concerns/noteable_spec.rb +++ b/spec/models/concerns/noteable_spec.rb @@ -1,5 +1,85 @@ require 'spec_helper' -describe Noteable, model: true do - # TODO: Test +describe MergeRequest, Noteable, model: true do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.project } + let!(:active_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) } + let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) } + let!(:active_diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: active_position2) } + let!(:outdated_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position) } + let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position) } + let!(:discussion_note1) { create(:discussion_note_on_merge_request, project: project, noteable: merge_request) } + let!(:discussion_note2) { create(:discussion_note_on_merge_request, in_reply_to: discussion_note1) } + let!(:commit_diff_note1) { create(:diff_note_on_commit, project: project) } + let!(:commit_diff_note2) { create(:diff_note_on_commit, project: project) } + let!(:commit_note1) { create(:note_on_commit, project: project) } + let!(:commit_note2) { create(:note_on_commit, project: project) } + let!(:commit_discussion_note1) { create(:discussion_note_on_commit, project: project) } + let!(:commit_discussion_note2) { create(:discussion_note_on_commit, in_reply_to: commit_discussion_note1) } + let!(:commit_discussion_note3) { create(:discussion_note_on_commit, project: project) } + let!(:note1) { create(:note, project: project, noteable: merge_request) } + let!(:note2) { create(:note, project: project, noteable: merge_request) } + + let(:active_position2) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: 16, + new_line: 22, + diff_refs: merge_request.diff_refs + ) + end + + let(:outdated_position) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 9, + diff_refs: project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e").diff_refs + ) + end + + describe '#discussions' do + subject { merge_request.discussions } + + it 'includes discussions for diff notes, commit diff notes, commit notes, and regular notes' do + expect(subject).to eq([ + DiffDiscussion.new([active_diff_note1, active_diff_note2], merge_request), + DiffDiscussion.new([active_diff_note3], merge_request), + DiffDiscussion.new([outdated_diff_note1, outdated_diff_note2], merge_request), + SimpleDiscussion.new([discussion_note1, discussion_note2], merge_request), + DiffDiscussion.new([commit_diff_note1, commit_diff_note2], merge_request), + OutOfContextDiscussion.new([commit_note1, commit_note2], merge_request), + SimpleDiscussion.new([commit_discussion_note1, commit_discussion_note2], merge_request), + SimpleDiscussion.new([commit_discussion_note3], merge_request), + IndividualNoteDiscussion.new([note1], merge_request), + IndividualNoteDiscussion.new([note2], merge_request) + ]) + end + end + + describe '#grouped_diff_discussions' do + subject { merge_request.grouped_diff_discussions } + + it "includes active discussions" do + discussions = subject.values + + expect(discussions.count).to eq(2) + expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id]) + expect(discussions.all?(&:active?)).to be true + + expect(discussions.first.notes).to eq([active_diff_note1, active_diff_note2]) + expect(discussions.last.notes).to eq([active_diff_note3]) + end + + it "doesn't include outdated discussions" do + expect(subject.values.map(&:id)).not_to include(outdated_diff_note1.discussion_id) + end + + it "groups the discussions by line code" do + expect(subject[active_diff_note1.line_code].id).to eq(active_diff_note1.discussion_id) + expect(subject[active_diff_note3.line_code].id).to eq(active_diff_note3.discussion_id) + end + end end diff --git a/spec/models/concerns/resolvable_note_spec.rb b/spec/models/concerns/resolvable_note_spec.rb index 3306b311e4a..a5a26958410 100644 --- a/spec/models/concerns/resolvable_note_spec.rb +++ b/spec/models/concerns/resolvable_note_spec.rb @@ -3,16 +3,37 @@ require 'spec_helper' describe Note, ResolvableNote, models: true do subject { create(:discussion_note_on_merge_request) } - describe '.resolvable' do - # TODO: Test - end + context 'resolvability scopes' do + let!(:note1) { create(:note) } + let!(:note2) { create(:diff_note_on_commit) } + let!(:note3) { create(:diff_note_on_merge_request, :resolved) } + let!(:note4) { create(:discussion_note_on_merge_request) } + let!(:note5) { create(:discussion_note_on_issue) } + let!(:note6) { create(:discussion_note_on_merge_request, :system) } + + describe '.potentially_resolvable' do + it 'includes diff and discussion notes on merge requests' do + expect(Note.potentially_resolvable).to match_array([note3, note4, note6]) + end + end - describe '.resolved' do - # TODO: Test - end + describe '.resolvable' do + it 'includes non-system diff and discussion notes on merge requests' do + expect(Note.resolvable).to match_array([note3, note4]) + end + end + + describe '.resolved' do + it 'includes resolved non-system diff and discussion notes on merge requests' do + expect(Note.resolved).to match_array([note3]) + end + end - describe '.unresolved' do - # TODO: Test + describe '.unresolved' do + it 'includes non-resolved non-system diff and discussion notes on merge requests' do + expect(Note.unresolved).to match_array([note4]) + end + end end describe ".resolve!" do @@ -55,7 +76,7 @@ describe Note, ResolvableNote, models: true do describe '#resolvable?' do context "when potentially resolvable" do before do - allow(subject).to receive(:discussion_resolvable?).and_return(true) + allow(subject).to receive(:potentially_resolvable?).and_return(true) end context "when a system note" do @@ -77,7 +98,7 @@ describe Note, ResolvableNote, models: true do context "when not potentially resolvable" do before do - allow(subject).to receive(:discussion_resolvable?).and_return(false) + allow(subject).to receive(:potentially_resolvable?).and_return(false) end it "returns false" do @@ -125,7 +146,37 @@ describe Note, ResolvableNote, models: true do end describe "#resolved?" do - # TODO: Test + let(:current_user) { create(:user) } + + context 'when not resolvable' do + before do + subject.resolve!(current_user) + + allow(subject).to receive(:resolvable?).and_return(false) + end + + it 'returns false' do + expect(subject.resolved?).to be_falsey + end + end + + context 'when resolvable' do + context 'when the note has been resolved' do + before do + subject.resolve!(current_user) + end + + it 'returns true' do + expect(subject.resolved?).to be_truthy + end + end + + context 'when the note has not been resolved' do + it 'returns false' do + expect(subject.resolved?).to be_falsey + end + end + end end describe "#resolve!" do diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb index 6109d77f27c..ba7dd5280bd 100644 --- a/spec/models/diff_discussion_spec.rb +++ b/spec/models/diff_discussion_spec.rb @@ -4,10 +4,16 @@ describe DiffDiscussion, model: true do subject { described_class.new([first_note, second_note, third_note]) } let(:first_note) { create(:diff_note_on_merge_request) } - let(:second_note) { create(:diff_note_on_merge_request) } - let(:third_note) { create(:diff_note_on_merge_request) } + let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) } + let(:third_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) } -# TODO: Test + describe '#reply_attributes' do + it 'includes position and original_position' do + attributes = subject.reply_attributes + expect(attributes[:position]).to eq(first_note.position.to_json) + expect(attributes[:original_position]).to eq(first_note.original_position.to_json) + end + end describe "#truncated_diff_lines" do let(:truncated_lines) { subject.truncated_diff_lines } diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 92059936188..533d38c0229 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -58,7 +58,29 @@ describe DiffNote, models: true do end describe "#original_position=" do - # TODO: Test + context "when provided a string" do + it "sets the original position" do + subject.original_position = new_position.to_json + + expect(subject.original_position).to eq(new_position) + end + end + + context "when provided a hash" do + it "sets the original position" do + subject.original_position = new_position.to_h + + expect(subject.original_position).to eq(new_position) + end + end + + context "when provided a position object" do + it "sets the original position" do + subject.original_position = new_position + + expect(subject.original_position).to eq(new_position) + end + end end describe "#diff_file" do diff --git a/spec/models/discussion_note_spec.rb b/spec/models/discussion_note_spec.rb index d61ea05e318..49084c854d8 100644 --- a/spec/models/discussion_note_spec.rb +++ b/spec/models/discussion_note_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' describe DiscussionNote, models: true do - # TODO: Test + end diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index f48450589d0..771ca37cb12 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -4,33 +4,34 @@ describe Discussion, model: true do subject { described_class.new([first_note, second_note, third_note]) } let(:first_note) { create(:discussion_note_on_merge_request) } - let(:second_note) { create(:discussion_note_on_merge_request) } + let(:merge_request) { first_note.noteable } + let(:second_note) { create(:discussion_note_on_merge_request, in_reply_to: first_note) } let(:third_note) { create(:discussion_note_on_merge_request) } describe '.build' do - # TODO: Test + it 'returns a discussion of the right type' do + discussion = described_class.build([first_note, second_note], merge_request) + expect(discussion).to be_a(SimpleDiscussion) + expect(discussion.notes.count).to be(2) + expect(discussion.first_note).to be(first_note) + expect(discussion.noteable).to be(merge_request) + end end describe '.build_collection' do - # TODO: Test - end - - describe '#id' do - # TODO: Test - end - - describe '#original_id' do - # TODO: Test - end - - describe '#potentially_resolvable?' do - # TODO: Test + it 'returns an array of discussions of the right type' do + discussions = described_class.build_collection([first_note, second_note, third_note], merge_request) + expect(discussions).to eq([ + SimpleDiscussion.new([first_note, second_note], merge_request), + SimpleDiscussion.new([third_note], merge_request) + ]) + end end describe "#resolvable?" do context "when potentially resolvable" do before do - allow(subject).to receive(:discussion_resolvable?).and_return(true) + allow(subject).to receive(:potentially_resolvable?).and_return(true) end context "when all notes are unresolvable" do @@ -72,7 +73,7 @@ describe Discussion, model: true do context "when not potentially resolvable" do before do - allow(subject).to receive(:discussion_resolvable?).and_return(false) + allow(subject).to receive(:potentially_resolvable?).and_return(false) end it "returns false" do @@ -542,7 +543,7 @@ describe Discussion, model: true do end describe "#first_note_to_resolve" do - it "returns the first not that still needs to be resolved" do + it "returns the first note that still needs to be resolved" do allow(first_note).to receive(:to_be_resolved?).and_return(false) allow(second_note).to receive(:to_be_resolved?).and_return(true) @@ -551,88 +552,16 @@ describe Discussion, model: true do end describe "#last_resolved_note" do - # TODO: Test - end - - describe "#collapsed?" do - context "when potentially resolvable" do - before do - allow(subject).to receive(:discussion_resolvable?).and_return(true) - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when resolved" do - before do - allow(subject).to receive(:resolved?).and_return(true) - end - - it "returns true" do - expect(subject.collapsed?).to be true - end - end - - context "when not resolved" do - before do - allow(subject).to receive(:resolved?).and_return(false) - end - - it "returns false" do - expect(subject.collapsed?).to be false - end - end - end - - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - context "when a diff discussion" do - before do - allow(subject).to receive(:diff_discussion?).and_return(true) - end - - context "when active" do - before do - allow(subject).to receive(:active?).and_return(true) - end - - it "returns false" do - expect(subject.collapsed?).to be false - end - end - - context "when outdated" do - before do - allow(subject).to receive(:active?).and_return(false) - end - - it "returns true" do - expect(subject.collapsed?).to be true - end - end - end + let(:current_user) { create(:user) } - context "when not a diff discussion" do - it "returns false" do - expect(subject.collapsed?).to be false - end - end - end + before do + first_note.resolve!(current_user) + third_note.resolve!(current_user) + second_note.resolve!(current_user) end - context "when not potentially resolvable" do - before do - allow(subject).to receive(:discussion_resolvable?).and_return(false) - end - - it "returns false" do - expect(subject.collapsed?).to be false - end + it "returns the last note that was resolved" do + expect(subject.last_resolved_note).to eq(second_note) end end end diff --git a/spec/models/individual_note_discussion_spec.rb b/spec/models/individual_note_discussion_spec.rb index 33de682aedc..3938dfc4bb1 100644 --- a/spec/models/individual_note_discussion_spec.rb +++ b/spec/models/individual_note_discussion_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' describe IndividualNoteDiscussion, models: true do - # TODO: Test + end diff --git a/spec/models/legacy_diff_discussion_spec.rb b/spec/models/legacy_diff_discussion_spec.rb index 72e33877b40..153e757a0ef 100644 --- a/spec/models/legacy_diff_discussion_spec.rb +++ b/spec/models/legacy_diff_discussion_spec.rb @@ -1,5 +1,11 @@ require 'spec_helper' describe LegacyDiffDiscussion, models: true do - # TODO: Test + subject { create(:legacy_diff_note_on_merge_request).to_discussion } + + describe '#reply_attributes' do + it 'includes line_code' do + expect(subject.reply_attributes[:line_code]).to eq(subject.line_code) + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3d26049b54a..5d0862abb92 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1225,18 +1225,14 @@ describe MergeRequest, models: true do end context "discussion status" do - let(:first_discussion) { Discussion.new([create(:discussion_note_on_merge_request)]) } - let(:second_discussion) { Discussion.new([create(:discussion_note_on_merge_request)]) } - let(:third_discussion) { Discussion.new([create(:discussion_note_on_merge_request)]) } + let(:first_discussion) { create(:discussion_note_on_merge_request).to_discussion } + let(:second_discussion) { create(:discussion_note_on_merge_request).to_discussion } + let(:third_discussion) { create(:discussion_note_on_merge_request).to_discussion } before do allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion]) end - describe '#resolvable_discussions' do - # TODO: Test - end - describe "#discussions_resolvable?" do context "when all discussions are unresolvable" do before do diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 7a85c5f22ff..67b4fed5f8f 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -245,16 +245,32 @@ describe Note, models: true do end end - describe '.discussions' do - # TODO: Test - end - describe '.find_original_discussion' do - # TODO: Test + let!(:note) { create(:discussion_note_on_merge_request) } + let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) } + let(:merge_request) { note.noteable } + + it 'returns a discussion with one note' do + discussion = merge_request.notes.find_original_discussion(note.original_discussion_id) + + expect(discussion).not_to be_nil + expect(discussion.notes.count).to be(1) + expect(discussion.first_note.original_discussion_id).to eq(note.original_discussion_id) + end end describe '.find_discussion' do - # TODO: Test + let!(:note) { create(:discussion_note_on_merge_request) } + let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) } + let(:merge_request) { note.noteable } + + it 'returns a discussion with multiple note' do + discussion = merge_request.notes.find_discussion(note.discussion_id) + + expect(discussion).not_to be_nil + expect(discussion.notes).to match_array([note, note2]) + expect(discussion.first_note.discussion_id).to eq(note.discussion_id) + end end describe ".grouped_diff_discussions" do @@ -376,15 +392,82 @@ describe Note, models: true do end describe '#can_be_discussion_note?' do - # TODO: Test + context 'for a note on a merge request' do + let(:note) { build(:note_on_merge_request) } + + it 'returns true' do + expect(note.can_be_discussion_note?).to be_truthy + end + end + + context 'for a note on an issue' do + let(:note) { build(:note_on_issue) } + + it 'returns true' do + expect(note.can_be_discussion_note?).to be_truthy + end + end + + context 'for a note on a commit' do + let(:note) { build(:note_on_commit) } + + it 'returns true' do + expect(note.can_be_discussion_note?).to be_truthy + end + end + + context 'for a note on a snippet' do + let(:note) { build(:note_on_project_snippet) } + + it 'returns true' do + expect(note.can_be_discussion_note?).to be_truthy + end + end + + context 'for a diff note on merge request' do + let(:note) { build(:diff_note_on_merge_request) } + + it 'returns false' do + expect(note.can_be_discussion_note?).to be_falsey + end + end + + context 'for a diff note on commit' do + let(:note) { build(:diff_note_on_commit) } + + it 'returns false' do + expect(note.can_be_discussion_note?).to be_falsey + end + end + + context 'for a discussion note' do + let(:note) { build(:discussion_note_on_merge_request) } + + it 'returns false' do + expect(note.can_be_discussion_note?).to be_falsey + end + end end describe '#discussion_class' do - # TODO: Test + let(:note) { build(:note_on_commit) } + let(:merge_request) { create(:merge_request) } + + context 'when the note is displayed out of context' do + it 'returns OutOfContextDiscussion' do + expect(note.discussion_class(merge_request)).to be(OutOfContextDiscussion) + end + end + + context 'when the note is displayed in the original context' do + it 'returns IndividualNoteDiscussion' do + expect(note.discussion_class(note.noteable)).to be(IndividualNoteDiscussion) + end + end end describe "#discussion_id" do - let(:note) { create(:note) } + let(:note) { create(:note_on_commit) } context "when it is newly created" do it "has a discussion id" do @@ -406,10 +489,39 @@ describe Note, models: true do expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/) end end + + context 'when the note is displayed out of context' do + let(:merge_request) { create(:merge_request) } + + it 'overrides the discussion id' do + expect(note.discussion_id(merge_request)).not_to eq(note.discussion_id) + end + end end describe "#original_discussion_id" do - # TODO: Test + let(:note) { create(:diff_note_on_merge_request) } + + context "when it is newly created" do + it "has a discussion id" do + expect(note.original_discussion_id).not_to be_nil + expect(note.original_discussion_id).to match(/\A\h{40}\z/) + end + end + + context "when it didn't store a discussion id before" do + before do + note.update_column(:original_discussion_id, nil) + end + + it "has a discussion id" do + # The original_discussion_id is set in `after_initialize`, so `reload` won't work + reloaded_note = Note.find(note.id) + + expect(reloaded_note.original_discussion_id).not_to be_nil + expect(reloaded_note.original_discussion_id).to match(/\A\h{40}\z/) + end + end end describe '#to_discussion' do @@ -452,7 +564,98 @@ describe Note, models: true do end describe "#part_of_discussion?" do - # TODO: Test + context 'for a regular note' do + let(:note) { build(:note) } + + it 'returns false' do + expect(note.part_of_discussion?).to be_falsey + end + end + + context 'for a diff note' do + let(:note) { build(:diff_note_on_commit) } + + it 'returns true' do + expect(note.part_of_discussion?).to be_truthy + end + end + + context 'for a discussion note' do + let(:note) { build(:discussion_note_on_merge_request) } + + it 'returns true' do + expect(note.part_of_discussion?).to be_truthy + end + end + end + + describe '#in_reply_to?' do + context 'for a note' do + context 'when part of a discussion' do + subject { create(:discussion_note_on_issue) } + let(:note) { create(:discussion_note_on_issue, in_reply_to: subject) } + + it 'checks if the note is in reply to the other discussion' do + expect(subject).to receive(:in_reply_to?).with(note).and_call_original + expect(subject).to receive(:in_reply_to?).with(note.noteable).and_call_original + expect(subject).to receive(:in_reply_to?).with(note.to_discussion).and_call_original + + subject.in_reply_to?(note) + end + end + + context 'when not part of a discussion' do + subject { create(:note) } + let(:note) { create(:note, in_reply_to: subject) } + + it 'checks if the note is in reply to the other noteable' do + expect(subject).to receive(:in_reply_to?).with(note).and_call_original + expect(subject).to receive(:in_reply_to?).with(note.noteable).and_call_original + + subject.in_reply_to?(note) + end + end + end + + context 'for a discussion' do + context 'when part of the same discussion' do + subject { create(:diff_note_on_merge_request) } + let(:note) { create(:diff_note_on_merge_request, in_reply_to: subject) } + + it 'returns true' do + expect(subject.in_reply_to?(note.to_discussion)).to be_truthy + end + end + + context 'when not part of the same discussion' do + subject { create(:diff_note_on_merge_request) } + let(:note) { create(:diff_note_on_merge_request) } + + it 'returns false' do + expect(subject.in_reply_to?(note.to_discussion)).to be_falsey + end + end + end + + context 'for a noteable' do + context 'when a comment on the same noteable' do + subject { create(:note) } + let(:note) { create(:note, in_reply_to: subject) } + + it 'returns true' do + expect(subject.in_reply_to?(note.noteable)).to be_truthy + end + end + + context 'when not a comment on the same noteable' do + subject { create(:note) } + let(:note) { create(:note) } + + it 'returns false' do + expect(subject.in_reply_to?(note.noteable)).to be_falsey + end + end + end end describe 'expiring ETag cache' do diff --git a/spec/models/out_of_context_discussion_spec.rb b/spec/models/out_of_context_discussion_spec.rb new file mode 100644 index 00000000000..d765e7052d1 --- /dev/null +++ b/spec/models/out_of_context_discussion_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe OutOfContextDiscussion, model: true do + +end diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb index 2fc6cce471f..7a7ece24fc9 100644 --- a/spec/models/sent_notification_spec.rb +++ b/spec/models/sent_notification_spec.rb @@ -1,5 +1,166 @@ require 'spec_helper' describe SentNotification, model: true do - # TODO: Test + describe 'validation' do + describe 'note validity' do + context "when the project doesn't match the noteable's project" do + subject { build(:sent_notification, project: create(:project)) } + + it "is invalid" do + expect(subject).not_to be_valid + end + end + + context "when the project doesn't match the discussion project" do + let(:discussion_id) { create(:note).original_discussion_id } + subject { build(:sent_notification, in_reply_to_discussion_id: discussion_id) } + + it "is invalid" do + expect(subject).not_to be_valid + end + end + + context "when the noteable project and discussion project match" do + let(:project) { create(:project) } + let(:issue) { create(:issue, project: project) } + let(:discussion_id) { create(:note, project: project, noteable: issue).original_discussion_id } + subject { build(:sent_notification, project: project, noteable: issue, in_reply_to_discussion_id: discussion_id) } + + it "is valid" do + expect(subject).to be_valid + end + end + end + end + + describe '.record' do + let(:user) { create(:user) } + let(:issue) { create(:issue) } + + it 'creates a new SentNotification' do + expect { described_class.record(issue, user.id) }.to change { SentNotification.count }.by(1) + end + end + + describe '.record_note' do + let(:user) { create(:user) } + let(:note) { create(:diff_note_on_merge_request) } + + it 'creates a new SentNotification' do + expect { described_class.record_note(note, user.id) }.to change { SentNotification.count }.by(1) + end + end + + describe '#create_reply' do + context 'for issue' do + let(:issue) { create(:issue) } + subject { described_class.record(issue, issue.author.id) } + + it 'creates a comment on the issue' do + note = subject.create_reply('Test') + expect(note.in_reply_to?(issue)).to be_truthy + end + end + + context 'for issue comment' do + let(:note) { create(:note_on_issue) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a comment on the issue' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'for issue discussion' do + let(:note) { create(:discussion_note_on_issue) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a reply on the discussion' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'for merge request' do + let(:merge_request) { create(:merge_request) } + subject { described_class.record(merge_request, merge_request.author.id) } + + it 'creates a comment on the merge_request' do + note = subject.create_reply('Test') + expect(note.in_reply_to?(merge_request)).to be_truthy + end + end + + context 'for merge request comment' do + let(:note) { create(:note_on_merge_request) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a comment on the merge request' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'for merge request diff discussion' do + let(:note) { create(:diff_note_on_merge_request) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a reply on the discussion' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'for merge request non-diff discussion' do + let(:note) { create(:discussion_note_on_merge_request) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a reply on the discussion' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'for commit' do + let(:project) { create(:project) } + let(:commit) { project.commit } + subject { described_class.record(commit, project.creator.id) } + + it 'creates a comment on the commit' do + note = subject.create_reply('Test') + expect(note.in_reply_to?(commit)).to be_truthy + end + end + + context 'for commit comment' do + let(:note) { create(:note_on_commit) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a comment on the commit' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'for commit diff discussion' do + let(:note) { create(:diff_note_on_commit) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a reply on the discussion' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'for commit non-diff discussion' do + let(:note) { create(:discussion_note_on_commit) } + subject { described_class.record_note(note, note.author.id) } + + it 'creates a reply on the discussion' do + new_note = subject.create_reply('Test') + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + end end diff --git a/spec/models/simple_discussion_spec.rb b/spec/models/simple_discussion_spec.rb index 1cb149aa270..1a342fbc740 100644 --- a/spec/models/simple_discussion_spec.rb +++ b/spec/models/simple_discussion_spec.rb @@ -1,5 +1,11 @@ require 'spec_helper' describe SimpleDiscussion, model: true do - # TODO: Test + subject { create(:discussion_note_on_issue).to_discussion } + + describe '#reply_attributes' do + it 'includes discussion_id' do + expect(subject.reply_attributes[:discussion_id]).to eq(subject.id) + end + end end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index d166b9783ad..6fa8806d270 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -11,7 +11,7 @@ describe Issues::BuildService, services: true do context 'for a single discussion' do describe '#execute' do let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) } - let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done")]) } + let(:discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done").to_discussion } let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) } it 'references the noteable title in the issue title' do @@ -47,7 +47,7 @@ describe Issues::BuildService, services: true do let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) } it 'mentions the author of the note' do - discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))]) + discussion = create(:diff_note_on_merge_request, author: create(:user, username: 'author')).to_discussion expect(service.item_for_discussion(discussion)).to include('@author') end @@ -60,7 +60,7 @@ describe Issues::BuildService, services: true do note_result = " > This is a string\n"\ " > > with a blockquote\n"\ " > > > That has a quote\n" - discussion = Discussion.new([create(:diff_note_on_merge_request, note: note_text)]) + discussion = create(:diff_note_on_merge_request, note: note_text).to_discussion expect(service.item_for_discussion(discussion)).to include(note_result) end end diff --git a/spec/services/issues/resolve_discussions_spec.rb b/spec/services/issues/resolve_discussions_spec.rb index 3a72f92383c..4a4929daefc 100644 --- a/spec/services/issues/resolve_discussions_spec.rb +++ b/spec/services/issues/resolve_discussions_spec.rb @@ -18,7 +18,7 @@ describe DummyService, services: true do end describe "for resolving discussions" do - let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, note: "Almost done")]) } + let(:discussion) { create(:diff_note_on_merge_request, project: project, note: "Almost done").to_discussion } let(:merge_request) { discussion.noteable } let(:other_merge_request) { create(:merge_request, source_project: project, source_branch: "other") } diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb index 065f6eeeacb..464b24cb447 100644 --- a/spec/services/notes/build_service_spec.rb +++ b/spec/services/notes/build_service_spec.rb @@ -1,5 +1,40 @@ require 'spec_helper' -describe Notes::CreateService, services: true do - # TODO: Test +describe Notes::BuildService, services: true do + let(:note) { create(:discussion_note_on_issue) } + let(:project) { note.project } + let(:author) { note.author } + + describe '#execute' do + context 'when in_reply_to_discussion_id is specified' do + context 'when a note with that original discussion ID exists' do + it 'sets the note up to be in reply to that note' do + new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.original_discussion_id).execute + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'when a note with that discussion ID exists' do + it 'sets the note up to be in reply to that note' do + new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute + expect(new_note).to be_valid + expect(new_note.in_reply_to?(note)).to be_truthy + end + end + + context 'when no note with that discussion ID exists' do + it 'sets an error' do + new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: 'foo').execute + expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found') + end + end + end + + it 'builds a note without saving it' do + new_note = described_class.new(project, author, noteable_type: note.noteable_type, noteable_id: note.noteable_id, note: 'Test').execute + expect(new_note).to be_valid + expect(new_note).not_to be_persisted + end + end end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index b406afeb34d..152c6d20daa 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -13,9 +13,6 @@ describe Notes::CreateService, services: true do project.team << [user, :master] end - # TODO: Test in_reply_to_discussion_id - # TODO: Test new_discussion - context "valid params" do it 'returns a valid note' do note = described_class.new(project, user, opts).execute |