diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-03-09 19:29:11 -0600 |
---|---|---|
committer | Luke "Jared" Bennett <lbennett@gitlab.com> | 2017-04-05 17:44:14 +0100 |
commit | 08bbb9fce66cb46d3262e6cd4c4379b59f065be0 (patch) | |
tree | 159eeb7ca43419f29926d6e77637db18bddd20a9 /spec | |
parent | 8bdfee8ba5fb0a8f48501e63274c8f9ce5708007 (diff) | |
download | gitlab-ce-08bbb9fce66cb46d3262e6cd4c4379b59f065be0.tar.gz |
Add option to start a new discussion on an MR
Diffstat (limited to 'spec')
32 files changed, 574 insertions, 496 deletions
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index b223a22ae60..9545b980fbf 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -266,7 +266,7 @@ describe Projects::CommitController do diff_for_path(id: commit2.id, old_path: existing_path, new_path: existing_path) expect(assigns(:diff_notes_disabled)).to be_falsey - expect(assigns(:comments_target)).to eq(noteable_type: 'Commit', + expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'Commit', commit_id: commit2.id) end diff --git a/spec/controllers/projects/discussions_controller_spec.rb b/spec/controllers/projects/discussions_controller_spec.rb index 79ab364a6f3..fe62898fa9b 100644 --- a/spec/controllers/projects/discussions_controller_spec.rb +++ b/spec/controllers/projects/discussions_controller_spec.rb @@ -4,7 +4,7 @@ describe Projects::DiscussionsController do let(:user) { create(:user) } let(:merge_request) { create(:merge_request) } let(:project) { merge_request.source_project } - let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) } + let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } let(:discussion) { note.discussion } let(:request_params) do diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 734966d50b2..731e511c270 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -508,7 +508,7 @@ describe Projects::IssuesController do end context 'resolving discussions in MergeRequest' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 72f41f7209a..6f8755645a1 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -574,7 +574,7 @@ describe Projects::MergeRequestsController do diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path) expect(assigns(:diff_notes_disabled)).to be_falsey - expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest', + expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'MergeRequest', noteable_id: merge_request.id) end diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index d80780b1d90..87084806568 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -14,7 +14,15 @@ describe Projects::NotesController do } end + describe 'GET index' do + # It renders the discussion partial for any threaded note + # TODO: Test + 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 @@ -49,7 +57,8 @@ describe Projects::NotesController do note: 'some note', noteable_id: merge_request.id.to_s, noteable_type: 'MergeRequest', - merge_request_diff_head_sha: 'sha' + merge_request_diff_head_sha: 'sha', + in_reply_to_discussion_id: nil } expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true)) diff --git a/spec/factories/discussions.rb b/spec/factories/discussions.rb new file mode 100644 index 00000000000..5e3a9b1e698 --- /dev/null +++ b/spec/factories/discussions.rb @@ -0,0 +1,5 @@ +FactoryGirl.define do + factory :discussion do + # TODO: Implement + end +end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index fe19a404e16..755c6d7e031 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -16,6 +16,15 @@ FactoryGirl.define do factory :note_on_personal_snippet, traits: [:on_personal_snippet] factory :system_note, traits: [:system] + factory :discussion_note_on_merge_request, traits: [:on_merge_request], class: DiscussionNote do + association :project, :repository + + trait :resolved do + resolved_at { Time.now } + resolved_by { create(:user) } + end + end + factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote do association :project, :repository end diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb index 572bca3de21..58f897cba3e 100644 --- a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb @@ -4,7 +4,7 @@ feature 'Resolving all open discussions in a merge request from an issue', featu let(:user) { create(:user) } let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project) } - let!(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: merge_request, project: project)]).first } + let!(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } describe 'as a user with access to the project' do before do diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb index fab2d532e06..783f2e93909 100644 --- a/spec/features/notes_on_merge_requests_spec.rb +++ b/spec/features/notes_on_merge_requests_spec.rb @@ -25,7 +25,7 @@ describe 'Comments', feature: true do describe 'the note form' do it 'is valid' do is_expected.to have_css('.js-main-target-form', visible: true, count: 1) - expect(find('.js-main-target-form input[type=submit]').value). + expect(find('.js-main-target-form .js-comment-button').value). to eq('Comment') page.within('.js-main-target-form') do expect(page).not_to have_link('Cancel') diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 77a04507be1..d2ec42ae70f 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -202,4 +202,8 @@ describe NotesFinder do end end end + + describe '#target' do + # TODO: Test + end end diff --git a/spec/models/commit_discussion_spec.rb b/spec/models/commit_discussion_spec.rb new file mode 100644 index 00000000000..6a5042d8827 --- /dev/null +++ b/spec/models/commit_discussion_spec.rb @@ -0,0 +1,5 @@ +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 new file mode 100644 index 00000000000..51855fb2f0d --- /dev/null +++ b/spec/models/concerns/noteable_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe Noteable, model: true do + # TODO: Test +end diff --git a/spec/models/concerns/resolvable_note_spec.rb b/spec/models/concerns/resolvable_note_spec.rb new file mode 100644 index 00000000000..3306b311e4a --- /dev/null +++ b/spec/models/concerns/resolvable_note_spec.rb @@ -0,0 +1,276 @@ +require 'spec_helper' + +describe Note, ResolvableNote, models: true do + subject { create(:discussion_note_on_merge_request) } + + describe '.resolvable' do + # TODO: Test + end + + describe '.resolved' do + # TODO: Test + end + + describe '.unresolved' do + # TODO: Test + end + + describe ".resolve!" do + let(:current_user) { create(:user) } + let!(:commit_note) { create(:diff_note_on_commit) } + let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) } + let!(:unresolved_note) { create(:discussion_note_on_merge_request) } + + before do + described_class.resolve!(current_user) + + commit_note.reload + resolved_note.reload + unresolved_note.reload + end + + it 'resolves only the resolvable, not yet resolved notes' do + expect(commit_note.resolved_at).to be_nil + expect(resolved_note.resolved_by).not_to eq(current_user) + expect(unresolved_note.resolved_at).not_to be_nil + expect(unresolved_note.resolved_by).to eq(current_user) + end + end + + describe ".unresolve!" do + let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) } + + before do + described_class.unresolve! + + resolved_note.reload + end + + it 'unresolves the resolved notes' do + expect(resolved_note.resolved_by).to be_nil + expect(resolved_note.resolved_at).to be_nil + end + end + + describe '#resolvable?' do + context "when potentially resolvable" do + before do + allow(subject).to receive(:discussion_resolvable?).and_return(true) + end + + context "when a system note" do + before do + subject.system = true + end + + it "returns false" do + expect(subject.resolvable?).to be false + end + end + + context "when a regular note" do + it "returns true" do + expect(subject.resolvable?).to be true + end + end + 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.resolvable?).to be false + end + end + end + + describe "#to_be_resolved?" do + context "when not resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(false) + end + + it "returns false" do + expect(subject.to_be_resolved?).to be false + end + 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 false" do + expect(subject.to_be_resolved?).to be false + end + end + + context "when not resolved" do + before do + allow(subject).to receive(:resolved?).and_return(false) + end + + it "returns true" do + expect(subject.to_be_resolved?).to be true + end + end + end + end + + describe "#resolved?" do + # TODO: Test + end + + describe "#resolve!" do + let(:current_user) { create(:user) } + + context "when not resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(false) + end + + it "returns nil" do + expect(subject.resolve!(current_user)).to be_nil + end + + it "doesn't set resolved_at" do + subject.resolve!(current_user) + + expect(subject.resolved_at).to be_nil + end + + it "doesn't set resolved_by" do + subject.resolve!(current_user) + + expect(subject.resolved_by).to be_nil + end + + it "doesn't mark as resolved" do + subject.resolve!(current_user) + + expect(subject.resolved?).to be false + end + end + + context "when resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(true) + end + + context "when already resolved" do + let(:user) { create(:user) } + + before do + subject.resolve!(user) + end + + it "returns nil" do + expect(subject.resolve!(current_user)).to be_nil + end + + it "doesn't change resolved_at" do + expect(subject.resolved_at).not_to be_nil + + expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at } + end + + it "doesn't change resolved_by" do + expect(subject.resolved_by).to eq(user) + + expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by } + end + + it "doesn't change resolved status" do + expect(subject.resolved?).to be true + + expect { subject.resolve!(current_user) }.not_to change { subject.resolved? } + end + end + + context "when not yet resolved" do + it "returns true" do + expect(subject.resolve!(current_user)).to be true + end + + it "sets resolved_at" do + subject.resolve!(current_user) + + expect(subject.resolved_at).not_to be_nil + end + + it "sets resolved_by" do + subject.resolve!(current_user) + + expect(subject.resolved_by).to eq(current_user) + end + + it "marks as resolved" do + subject.resolve!(current_user) + + expect(subject.resolved?).to be true + end + end + end + end + + describe "#unresolve!" do + context "when not resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(false) + end + + it "returns nil" do + expect(subject.unresolve!).to be_nil + end + end + + context "when resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(true) + end + + context "when resolved" do + let(:user) { create(:user) } + + before do + subject.resolve!(user) + end + + it "returns true" do + expect(subject.unresolve!).to be true + end + + it "unsets resolved_at" do + subject.unresolve! + + expect(subject.resolved_at).to be_nil + end + + it "unsets resolved_by" do + subject.unresolve! + + expect(subject.resolved_by).to be_nil + end + + it "unmarks as resolved" do + subject.unresolve! + + expect(subject.resolved?).to be false + end + end + + context "when not resolved" do + it "returns nil" do + expect(subject.unresolve!).to be_nil + end + end + end + end +end diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb new file mode 100644 index 00000000000..8f3a4a8cc49 --- /dev/null +++ b/spec/models/diff_discussion_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe DiffDiscussion, model: true do + # TODO: Test + + describe "#truncated_diff_lines" do + let(:truncated_lines) { subject.truncated_diff_lines } + + context "when diff is greater than allowed number of truncated diff lines " do + it "returns fewer lines" do + expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + + expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + context "when some diff lines are meta" do + it "returns no meta lines" do + expect(subject.diff_lines).to include(be_meta) + expect(truncated_lines).not_to include(be_meta) + end + end + end +end diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 9ea3a4b7020..92059936188 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -31,43 +31,6 @@ describe DiffNote, models: true do subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) } - describe ".resolve!" do - let(:current_user) { create(:user) } - let!(:commit_note) { create(:diff_note_on_commit) } - let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) } - let!(:unresolved_note) { create(:diff_note_on_merge_request) } - - before do - described_class.resolve!(current_user) - - commit_note.reload - resolved_note.reload - unresolved_note.reload - end - - it 'resolves only the resolvable, not yet resolved notes' do - expect(commit_note.resolved_at).to be_nil - expect(resolved_note.resolved_by).not_to eq(current_user) - expect(unresolved_note.resolved_at).not_to be_nil - expect(unresolved_note.resolved_by).to eq(current_user) - end - end - - describe ".unresolve!" do - let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) } - - before do - described_class.unresolve! - - resolved_note.reload - end - - it 'unresolves the resolved notes' do - expect(resolved_note.resolved_by).to be_nil - expect(resolved_note.resolved_at).to be_nil - end - end - describe "#position=" do context "when provided a string" do it "sets the position" do @@ -94,6 +57,10 @@ describe DiffNote, models: true do end end + describe "#original_position=" do + # TODO: Test + end + describe "#diff_file" do it "returns the correct diff file" do diff_file = subject.diff_file @@ -226,252 +193,6 @@ describe DiffNote, models: true do end end - describe "#resolvable?" do - context "when noteable is a commit" do - subject { create(:diff_note_on_commit, project: project, position: position) } - - it "returns false" do - expect(subject.resolvable?).to be false - end - end - - context "when noteable is a merge request" do - context "when a system note" do - before do - subject.system = true - end - - it "returns false" do - expect(subject.resolvable?).to be false - end - end - - context "when a regular note" do - it "returns true" do - expect(subject.resolvable?).to be true - end - end - end - end - - describe "#to_be_resolved?" do - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns false" do - expect(subject.to_be_resolved?).to be false - end - 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 false" do - expect(subject.to_be_resolved?).to be false - end - end - - context "when not resolved" do - before do - allow(subject).to receive(:resolved?).and_return(false) - end - - it "returns true" do - expect(subject.to_be_resolved?).to be true - end - end - end - end - - describe "#resolve!" do - let(:current_user) { create(:user) } - - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns nil" do - expect(subject.resolve!(current_user)).to be_nil - end - - it "doesn't set resolved_at" do - subject.resolve!(current_user) - - expect(subject.resolved_at).to be_nil - end - - it "doesn't set resolved_by" do - subject.resolve!(current_user) - - expect(subject.resolved_by).to be_nil - end - - it "doesn't mark as resolved" do - subject.resolve!(current_user) - - expect(subject.resolved?).to be false - end - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when already resolved" do - let(:user) { create(:user) } - - before do - subject.resolve!(user) - end - - it "returns nil" do - expect(subject.resolve!(current_user)).to be_nil - end - - it "doesn't change resolved_at" do - expect(subject.resolved_at).not_to be_nil - - expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at } - end - - it "doesn't change resolved_by" do - expect(subject.resolved_by).to eq(user) - - expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by } - end - - it "doesn't change resolved status" do - expect(subject.resolved?).to be true - - expect { subject.resolve!(current_user) }.not_to change { subject.resolved? } - end - end - - context "when not yet resolved" do - it "returns true" do - expect(subject.resolve!(current_user)).to be true - end - - it "sets resolved_at" do - subject.resolve!(current_user) - - expect(subject.resolved_at).not_to be_nil - end - - it "sets resolved_by" do - subject.resolve!(current_user) - - expect(subject.resolved_by).to eq(current_user) - end - - it "marks as resolved" do - subject.resolve!(current_user) - - expect(subject.resolved?).to be true - end - end - end - end - - describe "#unresolve!" do - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns nil" do - expect(subject.unresolve!).to be_nil - end - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when resolved" do - let(:user) { create(:user) } - - before do - subject.resolve!(user) - end - - it "returns true" do - expect(subject.unresolve!).to be true - end - - it "unsets resolved_at" do - subject.unresolve! - - expect(subject.resolved_at).to be_nil - end - - it "unsets resolved_by" do - subject.unresolve! - - expect(subject.resolved_by).to be_nil - end - - it "unmarks as resolved" do - subject.unresolve! - - expect(subject.resolved?).to be false - end - end - - context "when not resolved" do - it "returns nil" do - expect(subject.unresolve!).to be_nil - end - end - end - end - - describe "#discussion" do - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns nil" do - expect(subject.discussion).to be_nil - end - end - - context "when resolvable" do - let!(:diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: subject.position) } - let!(:diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: active_position2) } - - 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 - - it "returns the discussion this note is in" do - discussion = subject.discussion - - expect(discussion.id).to eq(subject.discussion_id) - expect(discussion.notes).to eq([subject, diff_note2]) - end - end - end - describe "#discussion_id" do let(:note) { create(:diff_note_on_merge_request) } diff --git a/spec/models/discussion_note_spec.rb b/spec/models/discussion_note_spec.rb new file mode 100644 index 00000000000..d61ea05e318 --- /dev/null +++ b/spec/models/discussion_note_spec.rb @@ -0,0 +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 bc32fadd391..f48450589d0 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -3,14 +3,34 @@ require 'spec_helper' describe Discussion, 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(:first_note) { create(:discussion_note_on_merge_request) } + let(:second_note) { create(:discussion_note_on_merge_request) } + let(:third_note) { create(:discussion_note_on_merge_request) } + + describe '.build' do + # TODO: Test + 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 + end describe "#resolvable?" do - context "when a diff discussion" do + context "when potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(true) + allow(subject).to receive(:discussion_resolvable?).and_return(true) end context "when all notes are unresolvable" do @@ -50,9 +70,9 @@ describe Discussion, model: true do end end - context "when not a diff discussion" do + context "when not potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(false) + allow(subject).to receive(:discussion_resolvable?).and_return(false) end it "returns false" do @@ -530,10 +550,14 @@ describe Discussion, model: true do end end + describe "#last_resolved_note" do + # TODO: Test + end + describe "#collapsed?" do - context "when a diff discussion" do + context "when potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(true) + allow(subject).to receive(:discussion_resolvable?).and_return(true) end context "when resolvable" do @@ -567,31 +591,43 @@ describe Discussion, model: true do allow(subject).to receive(:resolvable?).and_return(false) end - context "when active" do + context "when a diff discussion" do before do - allow(subject).to receive(:active?).and_return(true) + allow(subject).to receive(:diff_discussion?).and_return(true) end - it "returns false" do - expect(subject.collapsed?).to be false + 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 - end - context "when outdated" do - before do - allow(subject).to receive(:active?).and_return(false) + 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 - it "returns true" do - expect(subject.collapsed?).to be true + context "when not a diff discussion" do + it "returns false" do + expect(subject.collapsed?).to be false end end end end - context "when not a diff discussion" do + context "when not potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(false) + allow(subject).to receive(:discussion_resolvable?).and_return(false) end it "returns false" do @@ -599,23 +635,4 @@ describe Discussion, model: true do end end end - - describe "#truncated_diff_lines" do - let(:truncated_lines) { subject.truncated_diff_lines } - - context "when diff is greater than allowed number of truncated diff lines " do - it "returns fewer lines" do - expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES - - expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES - end - end - - context "when some diff lines are meta" do - it "returns no meta lines" do - expect(subject.diff_lines).to include(be_meta) - expect(truncated_lines).not_to include(be_meta) - end - end - end end diff --git a/spec/models/individual_note_discussion_spec.rb b/spec/models/individual_note_discussion_spec.rb new file mode 100644 index 00000000000..33de682aedc --- /dev/null +++ b/spec/models/individual_note_discussion_spec.rb @@ -0,0 +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 new file mode 100644 index 00000000000..72e33877b40 --- /dev/null +++ b/spec/models/legacy_diff_discussion_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe LegacyDiffDiscussion, models: true do + # TODO: Test +end diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb deleted file mode 100644 index 81517a18b74..00000000000 --- a/spec/models/legacy_diff_note_spec.rb +++ /dev/null @@ -1,101 +0,0 @@ -require 'spec_helper' - -describe LegacyDiffNote, models: true do - describe "Commit diff line notes" do - let!(:note) { create(:legacy_diff_note_on_commit, note: "+1 from me") } - let!(:commit) { note.noteable } - - it "saves a valid note" do - expect(note.commit_id).to eq(commit.id) - expect(note.noteable.id).to eq(commit.id) - end - - it "is recognized by #legacy_diff_note?" do - expect(note).to be_legacy_diff_note - end - end - - describe '#active?' do - it 'is always true when the note has no associated diff line' do - note = build(:legacy_diff_note_on_merge_request) - - expect(note).to receive(:diff_line).and_return(nil) - - expect(note).to be_active - end - - it 'is never true when the note has no noteable associated' do - note = build(:legacy_diff_note_on_merge_request) - - expect(note).to receive(:diff_line).and_return(double) - expect(note).to receive(:noteable).and_return(nil) - - expect(note).not_to be_active - end - - it 'returns the memoized value if defined' do - note = build(:legacy_diff_note_on_merge_request) - - note.instance_variable_set(:@active, 'foo') - expect(note).not_to receive(:find_noteable_diff) - - expect(note.active?).to eq 'foo' - end - - context 'for a merge request noteable' do - it 'is false when noteable has no matching diff' do - merge = build_stubbed(:merge_request, :simple) - note = build(:legacy_diff_note_on_merge_request, noteable: merge) - - allow(note).to receive(:diff_line).and_return(double) - expect(note).to receive(:find_noteable_diff).and_return(nil) - - expect(note).not_to be_active - end - - it 'is true when noteable has a matching diff' do - merge = create(:merge_request, :simple) - - # Generate a real line_code value so we know it will match. We use a - # random line from a random diff just for funsies. - diff = merge.raw_diffs.to_a.sample - line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample - code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) - - # We're persisting in order to trigger the set_diff callback - note = create(:legacy_diff_note_on_merge_request, noteable: merge, - line_code: code, - project: merge.source_project) - - # Make sure we don't get a false positive from a guard clause - expect(note).to receive(:find_noteable_diff).and_call_original - expect(note).to be_active - end - end - end - - describe "#discussion_id" do - let(:note) { create(:note) } - - context "when it is newly created" do - it "has a discussion id" do - expect(note.discussion_id).not_to be_nil - expect(note.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(:discussion_id, nil) - end - - it "has a discussion id" do - # The discussion_id is set in `after_initialize`, so `reload` won't work - reloaded_note = Note.find(note.id) - - expect(reloaded_note.discussion_id).not_to be_nil - expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/) - end - end - end -end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 24e7c1b17d9..faa89ff0507 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1225,12 +1225,12 @@ describe MergeRequest, models: true do end context "discussion status" do - let(:first_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) } - let(:second_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) } - let(:third_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) } + 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)]) } before do - allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion]) + allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion]) end describe '#resolvable_discussions' do @@ -1245,34 +1245,6 @@ describe MergeRequest, models: true do end end - describe '#discussions_can_be_resolved_by? user' do - let(:user) { build(:user) } - - context 'all discussions can be resolved by the user' do - before do - allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true) - end - - it 'allows a user to resolve the discussions' do - expect(subject.discussions_can_be_resolved_by?(user)).to be(true) - end - end - - context 'one discussion cannot be resolved by the user' do - before do - allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false) - end - - it 'allows a user to resolve the discussions' do - expect(subject.discussions_can_be_resolved_by?(user)).to be(false) - end - end - end - describe "#discussions_resolvable?" do context "when all discussions are unresolvable" do before do @@ -1398,6 +1370,38 @@ describe MergeRequest, models: true do end end end + + describe "#discussions_to_be_resolved" do + # TODO: Test + end + + describe '#discussions_can_be_resolved_by?' do + let(:user) { build(:user) } + + context 'all discussions can be resolved by the user' do + before do + allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true) + end + + it 'allows a user to resolve the discussions' do + expect(subject.discussions_can_be_resolved_by?(user)).to be(true) + end + end + + context 'one discussion cannot be resolved by the user' do + before do + allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false) + end + + it 'allows a user to resolve the discussions' do + expect(subject.discussions_can_be_resolved_by?(user)).to be(false) + end + end + end end describe '#conflicts_can_be_resolved_in_ui?' do diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 33536487c41..c5e4a639f06 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -245,6 +245,18 @@ describe Note, models: true do end end + describe '.discussions' do + # TODO: Test + end + + describe '.find_original_discussion' do + # TODO: Test + end + + describe '.find_discussion' do + # TODO: Test + end + describe ".grouped_diff_discussions" do let!(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } @@ -297,31 +309,6 @@ describe Note, models: true do end end - describe "#discussion_id" do - let(:note) { create(:note) } - - context "when it is newly created" do - it "has a discussion id" do - expect(note.discussion_id).not_to be_nil - expect(note.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(:discussion_id, nil) - end - - it "has a discussion id" do - # The discussion_id is set in `after_initialize`, so `reload` won't work - reloaded_note = Note.find(note.id) - - expect(reloaded_note.discussion_id).not_to be_nil - expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/) - end - end - end - describe '#for_personal_snippet?' do it 'returns false for a project snippet note' do expect(build(:note_on_project_snippet).for_personal_snippet?).to be_falsy @@ -388,6 +375,86 @@ describe Note, models: true do end end + describe '#can_be_discussion_note?' do + # TODO: Test + end + + describe '#discussion_class' do + # TODO: Test + end + + describe "#discussion_id" do + let(:note) { create(:note) } + + context "when it is newly created" do + it "has a discussion id" do + expect(note.discussion_id).not_to be_nil + expect(note.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(:discussion_id, nil) + end + + it "has a discussion id" do + # The discussion_id is set in `after_initialize`, so `reload` won't work + reloaded_note = Note.find(note.id) + + expect(reloaded_note.discussion_id).not_to be_nil + expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/) + end + end + end + + describe "#original_discussion_id" do + # TODO: Test + end + + describe '#to_discussion' do + subject { create(:discussion_note_on_merge_request) } + let!(:note2) { create(:discussion_note_on_merge_request, project: subject.project, noteable: subject.noteable, in_reply_to_discussion_id: subject.discussion_id) } + + it "returns a discussion with just this note" do + discussion = subject.to_discussion + + expect(discussion.id).to eq(subject.discussion_id) + expect(discussion.notes).to eq([subject]) + end + end + + describe "#discussion" do + let!(:note1) { create(:discussion_note_on_merge_request) } + let!(:note2) { create(:diff_note_on_merge_request, project: note1.project, noteable: note1.noteable) } + + context 'when the note is part of a discussion' do + subject { create(:discussion_note_on_merge_request, project: note1.project, noteable: note1.noteable, in_reply_to_discussion_id: note1.discussion_id) } + + it "returns the discussion this note is in" do + discussion = subject.discussion + + expect(discussion.id).to eq(subject.discussion_id) + expect(discussion.notes).to eq([note1, subject]) + end + end + + context 'when the note is not part of a discussion' do + subject { create(:note) } + + it "returns a discussion with just this note" do + discussion = subject.discussion + + expect(discussion.id).to eq(subject.discussion_id) + expect(discussion.notes).to eq([subject]) + end + end + end + + describe "#part_of_discussion?" do + # TODO: Test + end + describe 'expiring ETag cache' do let(:note) { build(:note_on_issue) } diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb new file mode 100644 index 00000000000..2fc6cce471f --- /dev/null +++ b/spec/models/sent_notification_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe SentNotification, model: true do + # TODO: Test +end diff --git a/spec/models/simple_discussion_spec.rb b/spec/models/simple_discussion_spec.rb new file mode 100644 index 00000000000..1cb149aa270 --- /dev/null +++ b/spec/models/simple_discussion_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe SimpleDiscussion, model: true do + # TODO: Test +end diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb index b1b398a897e..91d9057075f 100644 --- a/spec/requests/api/v3/issues_spec.rb +++ b/spec/requests/api/v3/issues_spec.rb @@ -824,7 +824,7 @@ describe API::V3::Issues, api: true do end context 'resolving issues in a merge request' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } before do diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 12c3cdf28c6..ab8df7b74cd 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Discussions::ResolveService do describe '#execute' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:project) { merge_request.project } let(:merge_request) { discussion.noteable } let(:user) { create(:user) } @@ -41,7 +41,7 @@ describe Discussions::ResolveService do end it 'can resolve multiple discussions at once' do - other_discussion = Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project)]).first + other_discussion = create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project).to_discussion service.execute([discussion, other_discussion]) diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 17990f41b3b..d166b9783ad 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -96,13 +96,13 @@ describe Issues::BuildService, services: true do end it 'mentions all the authors in the description' do - authors = merge_request.diff_discussions.map(&:author) + authors = merge_request.resolvable_discussions.map(&:author) expect(issue.description).to include(*authors.map(&:to_reference)) end it 'has a link for each unresolved discussion in the description' do - notes = merge_request.diff_discussions.map(&:first_note) + notes = merge_request.resolvable_discussions.map(&:first_note) links = notes.map { |note| Gitlab::UrlBuilder.build(note) } expect(issue.description).to include(*links) diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 776cbc4296b..80bfb731550 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -141,7 +141,7 @@ describe Issues::CreateService, services: true do it_behaves_like 'new issuable record that supports slash commands' context 'resolving discussions' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb new file mode 100644 index 00000000000..065f6eeeacb --- /dev/null +++ b/spec/services/notes/build_service_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe Notes::CreateService, services: true do + # TODO: Test +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 152c6d20daa..b406afeb34d 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -13,6 +13,9 @@ 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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e3146a56495..c4e00fcf080 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -439,7 +439,7 @@ describe NotificationService, services: true do notification.new_note(note) - expect(SentNotification.last.position).to eq(note.position) + expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.original_discussion_id) end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 5ec1ed8237b..42d63a9f9ba 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -796,7 +796,7 @@ describe SystemNoteService, services: true do end describe '.discussion_continued_in_issue' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } let(:issue) { create(:issue, project: project) } |