summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/projects/notes_controller.rb14
-rw-r--r--app/models/commit_discussion.rb9
-rw-r--r--app/models/concerns/resolvable_note.rb15
-rw-r--r--app/models/diff_note.rb4
-rw-r--r--app/models/discussion.rb21
-rw-r--r--app/models/individual_note_discussion.rb3
-rw-r--r--app/models/legacy_diff_discussion.rb1
-rw-r--r--app/models/note.rb27
-rw-r--r--app/models/out_of_context_discussion.rb12
-rw-r--r--app/models/sent_notification.rb23
-rw-r--r--app/services/concerns/issues/resolve_discussions.rb2
-rw-r--r--app/services/notes/build_service.rb6
-rw-r--r--app/views/notify/new_issue_email.html.haml10
-rw-r--r--app/views/notify/new_mention_in_issue_email.html.haml10
-rw-r--r--app/views/notify/new_mention_in_merge_request_email.html.haml13
-rw-r--r--app/views/notify/new_merge_request_email.html.haml10
-rw-r--r--app/views/projects/notes/_notes.html.haml2
-rw-r--r--lib/gitlab/email/handler/create_note_handler.rb7
-rw-r--r--spec/controllers/projects/notes_controller_spec.rb186
-rw-r--r--spec/factories/discussions.rb5
-rw-r--r--spec/factories/notes.rb14
-rw-r--r--spec/factories/sent_notifications.rb2
-rw-r--r--spec/finders/notes_finder_spec.rb39
-rw-r--r--spec/helpers/issues_helper_spec.rb2
-rw-r--r--spec/lib/gitlab/email/handler/create_note_handler_spec.rb1
-rw-r--r--spec/mailers/notify_spec.rb142
-rw-r--r--spec/models/commit_discussion_spec.rb5
-rw-r--r--spec/models/concerns/noteable_spec.rb84
-rw-r--r--spec/models/concerns/resolvable_note_spec.rb73
-rw-r--r--spec/models/diff_discussion_spec.rb12
-rw-r--r--spec/models/diff_note_spec.rb24
-rw-r--r--spec/models/discussion_note_spec.rb2
-rw-r--r--spec/models/discussion_spec.rb123
-rw-r--r--spec/models/individual_note_discussion_spec.rb2
-rw-r--r--spec/models/legacy_diff_discussion_spec.rb8
-rw-r--r--spec/models/merge_request_spec.rb10
-rw-r--r--spec/models/note_spec.rb225
-rw-r--r--spec/models/out_of_context_discussion_spec.rb5
-rw-r--r--spec/models/sent_notification_spec.rb163
-rw-r--r--spec/models/simple_discussion_spec.rb8
-rw-r--r--spec/services/issues/build_service_spec.rb6
-rw-r--r--spec/services/issues/resolve_discussions_spec.rb2
-rw-r--r--spec/services/notes/build_service_spec.rb39
-rw-r--r--spec/services/notes/create_service_spec.rb3
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, '&rarr;')
-
-- if @merge_request.assignee_id.present?
- %p
- Assignee: #{@merge_request.author_name} &rarr; #{@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, '&rarr;')
- if @merge_request.assignee_id.present?
%p
- Assignee: #{@merge_request.author_name} &rarr; #{@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