diff options
Diffstat (limited to 'spec')
26 files changed, 972 insertions, 213 deletions
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 |