diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-04-01 00:52:38 +0300 |
---|---|---|
committer | Luke "Jared" Bennett <lbennett@gitlab.com> | 2017-04-05 19:44:14 +0300 |
commit | afa53810deab37c95da245510a7cf85e8846a162 (patch) | |
tree | 1e6a4e64584e64fabb8e7ea21a7e9e13624ea018 | |
parent | 21e10888c3fc0fe545c0443cf0e23f593df847a4 (diff) |
Fix specs
-rw-r--r-- | app/assets/javascripts/notes.js | 7 | ||||
-rw-r--r-- | app/finders/notes_finder.rb | 4 | ||||
-rw-r--r-- | app/views/notify/_note_email.html.haml | 2 | ||||
-rw-r--r-- | app/views/notify/_note_email.text.erb | 2 | ||||
-rw-r--r-- | lib/gitlab/diff/position.rb | 2 | ||||
-rw-r--r-- | spec/features/merge_requests/diff_notes_avatars_spec.rb | 6 | ||||
-rw-r--r-- | spec/features/merge_requests/diff_notes_resolve_spec.rb | 2 | ||||
-rw-r--r-- | spec/helpers/notes_helper_spec.rb | 17 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 | ||||
-rw-r--r-- | spec/mailers/notify_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/concerns/noteable_spec.rb | 14 | ||||
-rw-r--r-- | spec/models/concerns/resolvable_discussion_spec.rb | 547 | ||||
-rw-r--r-- | spec/models/discussion_spec.rb | 537 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 12 | ||||
-rw-r--r-- | spec/services/issues/build_service_spec.rb | 6 |
15 files changed, 576 insertions, 585 deletions
diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index edfcda7c214..63a5d25a1dc 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -769,6 +769,10 @@ require('./task_list'); form.find('.js-note-new-discussion').remove(); this.setupNoteForm(form); + form + .removeClass('js-main-target-form') + .addClass("discussion-form js-discussion-note-form"); + if (typeof gl.diffNotesCompileComponents !== 'undefined') { var $commentBtn = form.find('comment-and-resolve-btn'); $commentBtn.attr(':discussion-id', "'" + discussionID + "'"); @@ -780,9 +784,6 @@ require('./task_list'); form .find('.js-comment-resolve-button') .attr('data-discussion-id', discussionID); - form - .removeClass('js-main-target-form') - .addClass("discussion-form js-discussion-note-form"); }; /* diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index c20bfb20171..3c499184b41 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -98,9 +98,9 @@ class NotesFinder # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. # - def search(query, notes) + def search(notes) query = @params[:search] - return unless query + return notes unless query pattern = "%#{query}%" notes.where(Note.arel_table[:note].matches(pattern)) diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml index 9e5aaed6f49..a80518f7986 100644 --- a/app/views/notify/_note_email.html.haml +++ b/app/views/notify/_note_email.html.haml @@ -1,4 +1,4 @@ -- discussion = @note.discussion unless @discussion.individual_note? +- discussion = @note.discussion if @note.part_of_discussion? - if discussion %p.details = succeed ':' do diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb index b25b513f4d8..cb2e7fab6d5 100644 --- a/app/views/notify/_note_email.text.erb +++ b/app/views/notify/_note_email.text.erb @@ -1,4 +1,4 @@ -<% discussion = @note.discussion unless @discussion.individual_note? -%> +<% discussion = @note.discussion if @note.part_of_discussion? -%> <% if discussion && !discussion.individual_note? -%> <%= @note.author_name -%> <% if discussion.new_discussion? -%> diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index fa567dd9da4..fc728123c97 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -42,7 +42,7 @@ module Gitlab def encode_with(coder) coder['attributes'] = self.to_h end - + def key @key ||= [base_sha, start_sha, head_sha, Digest::SHA1.hexdigest(old_path || ""), Digest::SHA1.hexdigest(new_path || ""), old_line, new_line] end diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb index a6c72b0b3ac..5769769a5ba 100644 --- a/spec/features/merge_requests/diff_notes_avatars_spec.rb +++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb @@ -163,11 +163,9 @@ feature 'Diff note avatars', feature: true, js: true do end context 'multiple comments' do - before do - create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) - create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) - create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) + let!(:notes) { create_list(:diff_note_on_merge_request, 3, project: project, noteable: merge_request, in_reply_to: note) } + before do visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, view: view) wait_for_ajax diff --git a/spec/features/merge_requests/diff_notes_resolve_spec.rb b/spec/features/merge_requests/diff_notes_resolve_spec.rb index 69164aabdb2..88d28b649a4 100644 --- a/spec/features/merge_requests/diff_notes_resolve_spec.rb +++ b/spec/features/merge_requests/diff_notes_resolve_spec.rb @@ -191,7 +191,7 @@ feature 'Diff notes resolve', feature: true, js: true do context 'multiple notes' do before do - create(:diff_note_on_merge_request, project: project, noteable: merge_request) + create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: note) visit_merge_request end diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb index 9c577501f00..a427de32c4c 100644 --- a/spec/helpers/notes_helper_spec.rb +++ b/spec/helpers/notes_helper_spec.rb @@ -36,21 +36,4 @@ describe NotesHelper do expect(helper.note_max_access_for_user(other_note)).to eq('Reporter') end end - - describe '#preload_max_access_for_authors' do - before do - # This method reads cache from RequestStore, so make sure it's clean. - RequestStore.clear! - end - - it 'loads multiple users' do - expected_access = { - owner.id => Gitlab::Access::OWNER, - master.id => Gitlab::Access::MASTER, - reporter.id => Gitlab::Access::REPORTER - } - - expect(helper.preload_max_access_for_authors(notes, project)).to eq(expected_access) - end - end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 4c3c7d8cce2..1ad16a9b57d 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -55,6 +55,7 @@ Note: - resolved_at - resolved_by_id - discussion_id +- original_discussion_id LabelLink: - id - label_id diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index fcbbc3166ed..c107e4c4457 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -755,7 +755,7 @@ describe Notify do end shared_examples 'an email for a note on a diff discussion' do |model| - let(:note) { create(model, project: project, author: note_author) } + let(:note) { create(model, author: note_author) } it "includes diffs with character-level highlighting" do is_expected.to have_body_text '<span class="p">}</span></span>' diff --git a/spec/models/concerns/noteable_spec.rb b/spec/models/concerns/noteable_spec.rb index 24962d3b074..0a181c008f3 100644 --- a/spec/models/concerns/noteable_spec.rb +++ b/spec/models/concerns/noteable_spec.rb @@ -4,14 +4,14 @@ 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_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: active_diff_note1) } 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!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position, in_reply_to: outdated_diff_note1) } 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_diff_note2) { create(:diff_note_on_commit, project: project, in_reply_to: commit_diff_note1) } 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) } @@ -63,7 +63,7 @@ describe MergeRequest, Noteable, model: true do subject { merge_request.grouped_diff_discussions } it "includes active discussions" do - discussions = subject.values + discussions = subject.values.flatten expect(discussions.count).to eq(2) expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id]) @@ -74,12 +74,12 @@ describe MergeRequest, Noteable, model: true do end it "doesn't include outdated discussions" do - expect(subject.values.map(&:id)).not_to include(outdated_diff_note1.discussion_id) + expect(subject.values.flatten.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) + expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id) + expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id) end end end diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb new file mode 100644 index 00000000000..2d2182fbae9 --- /dev/null +++ b/spec/models/concerns/resolvable_discussion_spec.rb @@ -0,0 +1,547 @@ +require 'spec_helper' + +describe Discussion, ResolvableDiscussion, models: true do + subject { described_class.new([first_note, second_note, third_note]) } + + let(:first_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 "#resolvable?" do + context "when potentially resolvable" do + before do + allow(subject).to receive(:potentially_resolvable?).and_return(true) + end + + context "when all notes are unresolvable" do + before do + allow(first_note).to receive(:resolvable?).and_return(false) + allow(second_note).to receive(:resolvable?).and_return(false) + allow(third_note).to receive(:resolvable?).and_return(false) + end + + it "returns false" do + expect(subject.resolvable?).to be false + end + end + + context "when some notes are unresolvable and some notes are resolvable" do + before do + allow(first_note).to receive(:resolvable?).and_return(true) + allow(second_note).to receive(:resolvable?).and_return(false) + allow(third_note).to receive(:resolvable?).and_return(true) + end + + it "returns true" do + expect(subject.resolvable?).to be true + end + end + + context "when all notes are resolvable" do + before do + allow(first_note).to receive(:resolvable?).and_return(true) + allow(second_note).to receive(:resolvable?).and_return(true) + allow(third_note).to receive(:resolvable?).and_return(true) + end + + 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(:potentially_resolvable?).and_return(false) + end + + it "returns false" do + expect(subject.resolvable?).to be false + end + end + end + + describe "#resolved?" do + context "when not resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(false) + end + + it "returns false" do + expect(subject.resolved?).to be false + end + end + + context "when resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(true) + + allow(first_note).to receive(:resolvable?).and_return(true) + allow(second_note).to receive(:resolvable?).and_return(false) + allow(third_note).to receive(:resolvable?).and_return(true) + end + + context "when all resolvable notes are resolved" do + before do + allow(first_note).to receive(:resolved?).and_return(true) + allow(third_note).to receive(:resolved?).and_return(true) + end + + it "returns true" do + expect(subject.resolved?).to be true + end + end + + context "when some resolvable notes are not resolved" do + before do + allow(first_note).to receive(:resolved?).and_return(true) + allow(third_note).to receive(:resolved?).and_return(false) + end + + it "returns false" do + expect(subject.resolved?).to be false + 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) + + allow(first_note).to receive(:resolvable?).and_return(true) + allow(second_note).to receive(:resolvable?).and_return(false) + allow(third_note).to receive(:resolvable?).and_return(true) + end + + context "when all resolvable notes are resolved" do + before do + allow(first_note).to receive(:resolved?).and_return(true) + allow(third_note).to receive(:resolved?).and_return(true) + end + + it "returns false" do + expect(subject.to_be_resolved?).to be false + end + end + + context "when some resolvable notes are not resolved" do + before do + allow(first_note).to receive(:resolved?).and_return(true) + allow(third_note).to receive(:resolved?).and_return(false) + end + + it "returns true" do + expect(subject.to_be_resolved?).to be true + end + end + end + end + + describe "#can_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 false" do + expect(subject.can_resolve?(current_user)).to be false + end + end + + context "when resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(true) + end + + context "when not signed in" do + let(:current_user) { nil } + + it "returns false" do + expect(subject.can_resolve?(current_user)).to be false + end + end + + context "when signed in" do + context "when the signed in user is the noteable author" do + before do + subject.noteable.author = current_user + end + + it "returns true" do + expect(subject.can_resolve?(current_user)).to be true + end + end + + context "when the signed in user can push to the project" do + before do + subject.project.team << [current_user, :master] + end + + it "returns true" do + expect(subject.can_resolve?(current_user)).to be true + end + end + + context "when the signed in user is a random user" do + it "returns false" do + expect(subject.can_resolve?(current_user)).to be false + end + 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 + let(:user) { create(:user) } + let(:second_note) { create(:diff_note_on_commit) } # unresolvable + + before do + allow(subject).to receive(:resolvable?).and_return(true) + end + + context "when all resolvable notes are resolved" do + before do + first_note.resolve!(user) + third_note.resolve!(user) + + first_note.reload + third_note.reload + end + + it "doesn't change resolved_at on the resolved notes" do + expect(first_note.resolved_at).not_to be_nil + expect(third_note.resolved_at).not_to be_nil + + expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_at } + expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_at } + end + + it "doesn't change resolved_by on the resolved notes" do + expect(first_note.resolved_by).to eq(user) + expect(third_note.resolved_by).to eq(user) + + expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_by } + expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_by } + end + + it "doesn't change the resolved state on the resolved notes" do + expect(first_note.resolved?).to be true + expect(third_note.resolved?).to be true + + expect { subject.resolve!(current_user) }.not_to change { first_note.resolved? } + expect { subject.resolve!(current_user) }.not_to change { third_note.resolved? } + 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 state" do + expect(subject.resolved?).to be true + + expect { subject.resolve!(current_user) }.not_to change { subject.resolved? } + end + end + + context "when some resolvable notes are resolved" do + before do + first_note.resolve!(user) + end + + it "doesn't change resolved_at on the resolved note" do + expect(first_note.resolved_at).not_to be_nil + + expect { subject.resolve!(current_user) }. + not_to change { first_note.reload.resolved_at } + end + + it "doesn't change resolved_by on the resolved note" do + expect(first_note.resolved_by).to eq(user) + + expect { subject.resolve!(current_user) }. + not_to change { first_note.reload && first_note.resolved_by } + end + + it "doesn't change the resolved state on the resolved note" do + expect(first_note.resolved?).to be true + + expect { subject.resolve!(current_user) }. + not_to change { first_note.reload && first_note.resolved? } + end + + it "sets resolved_at on the unresolved note" do + subject.resolve!(current_user) + third_note.reload + + expect(third_note.resolved_at).not_to be_nil + end + + it "sets resolved_by on the unresolved note" do + subject.resolve!(current_user) + third_note.reload + + expect(third_note.resolved_by).to eq(current_user) + end + + it "marks the unresolved note as resolved" do + subject.resolve!(current_user) + third_note.reload + + expect(third_note.resolved?).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 + + context "when no resolvable notes are resolved" do + it "sets resolved_at on the unresolved notes" do + subject.resolve!(current_user) + first_note.reload + third_note.reload + + expect(first_note.resolved_at).not_to be_nil + expect(third_note.resolved_at).not_to be_nil + end + + it "sets resolved_by on the unresolved notes" do + subject.resolve!(current_user) + first_note.reload + third_note.reload + + expect(first_note.resolved_by).to eq(current_user) + expect(third_note.resolved_by).to eq(current_user) + end + + it "marks the unresolved notes as resolved" do + subject.resolve!(current_user) + first_note.reload + third_note.reload + + expect(first_note.resolved?).to be true + expect(third_note.resolved?).to be true + end + + it "sets resolved_at" do + subject.resolve!(current_user) + first_note.reload + third_note.reload + + expect(subject.resolved_at).not_to be_nil + end + + it "sets resolved_by" do + subject.resolve!(current_user) + first_note.reload + third_note.reload + + expect(subject.resolved_by).to eq(current_user) + end + + it "marks as resolved" do + subject.resolve!(current_user) + first_note.reload + third_note.reload + + 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 + let(:user) { create(:user) } + + before do + allow(subject).to receive(:resolvable?).and_return(true) + + allow(first_note).to receive(:resolvable?).and_return(true) + allow(second_note).to receive(:resolvable?).and_return(false) + allow(third_note).to receive(:resolvable?).and_return(true) + end + + context "when all resolvable notes are resolved" do + before do + first_note.resolve!(user) + third_note.resolve!(user) + end + + it "unsets resolved_at on the resolved notes" do + subject.unresolve! + first_note.reload + third_note.reload + + expect(first_note.resolved_at).to be_nil + expect(third_note.resolved_at).to be_nil + end + + it "unsets resolved_by on the resolved notes" do + subject.unresolve! + first_note.reload + third_note.reload + + expect(first_note.resolved_by).to be_nil + expect(third_note.resolved_by).to be_nil + end + + it "unmarks the resolved notes as resolved" do + subject.unresolve! + first_note.reload + third_note.reload + + expect(first_note.resolved?).to be false + expect(third_note.resolved?).to be false + end + + it "unsets resolved_at" do + subject.unresolve! + first_note.reload + third_note.reload + + expect(subject.resolved_at).to be_nil + end + + it "unsets resolved_by" do + subject.unresolve! + first_note.reload + third_note.reload + + 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 some resolvable notes are resolved" do + before do + first_note.resolve!(user) + end + + it "unsets resolved_at on the resolved note" do + subject.unresolve! + + expect(subject.first_note.resolved_at).to be_nil + end + + it "unsets resolved_by on the resolved note" do + subject.unresolve! + + expect(subject.first_note.resolved_by).to be_nil + end + + it "unmarks the resolved note as resolved" do + subject.unresolve! + + expect(subject.first_note.resolved?).to be false + end + end + end + end + + describe "#first_note_to_resolve" 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) + + expect(subject.first_note_to_resolve).to eq(second_note) + end + end + + describe "#last_resolved_note" do + let(:current_user) { create(:user) } + + before do + first_note.resolve!(current_user) + third_note.resolve!(current_user) + second_note.resolve!(current_user) + 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/discussion_spec.rb b/spec/models/discussion_spec.rb index 771ca37cb12..718b208e8f8 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -27,541 +27,4 @@ describe Discussion, model: true do ]) end end - - describe "#resolvable?" do - context "when potentially resolvable" do - before do - allow(subject).to receive(:potentially_resolvable?).and_return(true) - end - - context "when all notes are unresolvable" do - before do - allow(first_note).to receive(:resolvable?).and_return(false) - allow(second_note).to receive(:resolvable?).and_return(false) - allow(third_note).to receive(:resolvable?).and_return(false) - end - - it "returns false" do - expect(subject.resolvable?).to be false - end - end - - context "when some notes are unresolvable and some notes are resolvable" do - before do - allow(first_note).to receive(:resolvable?).and_return(true) - allow(second_note).to receive(:resolvable?).and_return(false) - allow(third_note).to receive(:resolvable?).and_return(true) - end - - it "returns true" do - expect(subject.resolvable?).to be true - end - end - - context "when all notes are resolvable" do - before do - allow(first_note).to receive(:resolvable?).and_return(true) - allow(second_note).to receive(:resolvable?).and_return(true) - allow(third_note).to receive(:resolvable?).and_return(true) - end - - 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(:potentially_resolvable?).and_return(false) - end - - it "returns false" do - expect(subject.resolvable?).to be false - end - end - end - - describe "#resolved?" do - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns false" do - expect(subject.resolved?).to be false - end - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - - allow(first_note).to receive(:resolvable?).and_return(true) - allow(second_note).to receive(:resolvable?).and_return(false) - allow(third_note).to receive(:resolvable?).and_return(true) - end - - context "when all resolvable notes are resolved" do - before do - allow(first_note).to receive(:resolved?).and_return(true) - allow(third_note).to receive(:resolved?).and_return(true) - end - - it "returns true" do - expect(subject.resolved?).to be true - end - end - - context "when some resolvable notes are not resolved" do - before do - allow(first_note).to receive(:resolved?).and_return(true) - allow(third_note).to receive(:resolved?).and_return(false) - end - - it "returns false" do - expect(subject.resolved?).to be false - 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) - - allow(first_note).to receive(:resolvable?).and_return(true) - allow(second_note).to receive(:resolvable?).and_return(false) - allow(third_note).to receive(:resolvable?).and_return(true) - end - - context "when all resolvable notes are resolved" do - before do - allow(first_note).to receive(:resolved?).and_return(true) - allow(third_note).to receive(:resolved?).and_return(true) - end - - it "returns false" do - expect(subject.to_be_resolved?).to be false - end - end - - context "when some resolvable notes are not resolved" do - before do - allow(first_note).to receive(:resolved?).and_return(true) - allow(third_note).to receive(:resolved?).and_return(false) - end - - it "returns true" do - expect(subject.to_be_resolved?).to be true - end - end - end - end - - describe "#can_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 false" do - expect(subject.can_resolve?(current_user)).to be false - end - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when not signed in" do - let(:current_user) { nil } - - it "returns false" do - expect(subject.can_resolve?(current_user)).to be false - end - end - - context "when signed in" do - context "when the signed in user is the noteable author" do - before do - subject.noteable.author = current_user - end - - it "returns true" do - expect(subject.can_resolve?(current_user)).to be true - end - end - - context "when the signed in user can push to the project" do - before do - subject.project.team << [current_user, :master] - end - - it "returns true" do - expect(subject.can_resolve?(current_user)).to be true - end - end - - context "when the signed in user is a random user" do - it "returns false" do - expect(subject.can_resolve?(current_user)).to be false - end - 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 - let(:user) { create(:user) } - let(:second_note) { create(:diff_note_on_commit) } # unresolvable - - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when all resolvable notes are resolved" do - before do - first_note.resolve!(user) - third_note.resolve!(user) - - first_note.reload - third_note.reload - end - - it "doesn't change resolved_at on the resolved notes" do - expect(first_note.resolved_at).not_to be_nil - expect(third_note.resolved_at).not_to be_nil - - expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_at } - expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_at } - end - - it "doesn't change resolved_by on the resolved notes" do - expect(first_note.resolved_by).to eq(user) - expect(third_note.resolved_by).to eq(user) - - expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_by } - expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_by } - end - - it "doesn't change the resolved state on the resolved notes" do - expect(first_note.resolved?).to be true - expect(third_note.resolved?).to be true - - expect { subject.resolve!(current_user) }.not_to change { first_note.resolved? } - expect { subject.resolve!(current_user) }.not_to change { third_note.resolved? } - 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 state" do - expect(subject.resolved?).to be true - - expect { subject.resolve!(current_user) }.not_to change { subject.resolved? } - end - end - - context "when some resolvable notes are resolved" do - before do - first_note.resolve!(user) - end - - it "doesn't change resolved_at on the resolved note" do - expect(first_note.resolved_at).not_to be_nil - - expect { subject.resolve!(current_user) }. - not_to change { first_note.reload.resolved_at } - end - - it "doesn't change resolved_by on the resolved note" do - expect(first_note.resolved_by).to eq(user) - - expect { subject.resolve!(current_user) }. - not_to change { first_note.reload && first_note.resolved_by } - end - - it "doesn't change the resolved state on the resolved note" do - expect(first_note.resolved?).to be true - - expect { subject.resolve!(current_user) }. - not_to change { first_note.reload && first_note.resolved? } - end - - it "sets resolved_at on the unresolved note" do - subject.resolve!(current_user) - third_note.reload - - expect(third_note.resolved_at).not_to be_nil - end - - it "sets resolved_by on the unresolved note" do - subject.resolve!(current_user) - third_note.reload - - expect(third_note.resolved_by).to eq(current_user) - end - - it "marks the unresolved note as resolved" do - subject.resolve!(current_user) - third_note.reload - - expect(third_note.resolved?).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 - - context "when no resolvable notes are resolved" do - it "sets resolved_at on the unresolved notes" do - subject.resolve!(current_user) - first_note.reload - third_note.reload - - expect(first_note.resolved_at).not_to be_nil - expect(third_note.resolved_at).not_to be_nil - end - - it "sets resolved_by on the unresolved notes" do - subject.resolve!(current_user) - first_note.reload - third_note.reload - - expect(first_note.resolved_by).to eq(current_user) - expect(third_note.resolved_by).to eq(current_user) - end - - it "marks the unresolved notes as resolved" do - subject.resolve!(current_user) - first_note.reload - third_note.reload - - expect(first_note.resolved?).to be true - expect(third_note.resolved?).to be true - end - - it "sets resolved_at" do - subject.resolve!(current_user) - first_note.reload - third_note.reload - - expect(subject.resolved_at).not_to be_nil - end - - it "sets resolved_by" do - subject.resolve!(current_user) - first_note.reload - third_note.reload - - expect(subject.resolved_by).to eq(current_user) - end - - it "marks as resolved" do - subject.resolve!(current_user) - first_note.reload - third_note.reload - - 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 - let(:user) { create(:user) } - - before do - allow(subject).to receive(:resolvable?).and_return(true) - - allow(first_note).to receive(:resolvable?).and_return(true) - allow(second_note).to receive(:resolvable?).and_return(false) - allow(third_note).to receive(:resolvable?).and_return(true) - end - - context "when all resolvable notes are resolved" do - before do - first_note.resolve!(user) - third_note.resolve!(user) - end - - it "unsets resolved_at on the resolved notes" do - subject.unresolve! - first_note.reload - third_note.reload - - expect(first_note.resolved_at).to be_nil - expect(third_note.resolved_at).to be_nil - end - - it "unsets resolved_by on the resolved notes" do - subject.unresolve! - first_note.reload - third_note.reload - - expect(first_note.resolved_by).to be_nil - expect(third_note.resolved_by).to be_nil - end - - it "unmarks the resolved notes as resolved" do - subject.unresolve! - first_note.reload - third_note.reload - - expect(first_note.resolved?).to be false - expect(third_note.resolved?).to be false - end - - it "unsets resolved_at" do - subject.unresolve! - first_note.reload - third_note.reload - - expect(subject.resolved_at).to be_nil - end - - it "unsets resolved_by" do - subject.unresolve! - first_note.reload - third_note.reload - - 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 some resolvable notes are resolved" do - before do - first_note.resolve!(user) - end - - it "unsets resolved_at on the resolved note" do - subject.unresolve! - - expect(subject.first_note.resolved_at).to be_nil - end - - it "unsets resolved_by on the resolved note" do - subject.unresolve! - - expect(subject.first_note.resolved_by).to be_nil - end - - it "unmarks the resolved note as resolved" do - subject.unresolve! - - expect(subject.first_note.resolved?).to be false - end - end - end - end - - describe "#first_note_to_resolve" 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) - - expect(subject.first_note_to_resolve).to eq(second_note) - end - end - - describe "#last_resolved_note" do - let(:current_user) { create(:user) } - - before do - first_note.resolve!(current_user) - third_note.resolve!(current_user) - second_note.resolve!(current_user) - 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/note_spec.rb b/spec/models/note_spec.rb index 3cdabb2875f..2c7f6e59b19 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -263,10 +263,10 @@ describe Note, models: 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_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: active_diff_note1) } 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!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: outdated_diff_note1) } let(:active_position2) do Gitlab::Diff::Position.new( @@ -291,7 +291,7 @@ describe Note, models: true do subject { merge_request.notes.grouped_diff_discussions } it "includes active discussions" do - discussions = subject.values + discussions = subject.values.flatten expect(discussions.count).to eq(2) expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id]) @@ -302,12 +302,12 @@ describe Note, models: true do end it "doesn't include outdated discussions" do - expect(subject.values.map(&:id)).not_to include(outdated_diff_note1.discussion_id) + expect(subject.values.flatten.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) + expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id) + expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id) end end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 6fa8806d270..55d635235b0 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -91,9 +91,7 @@ describe Issues::BuildService, services: true do end describe 'with multiple discussions' do - before do - create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, line_number: 15) - end + let!(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, line_number: 15) } it 'mentions all the authors in the description' do authors = merge_request.resolvable_discussions.map(&:author) @@ -109,7 +107,7 @@ describe Issues::BuildService, services: true do end it 'mentions additional notes' do - create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, line_number: 15) + create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, in_reply_to: diff_note) expect(issue.description).to include('(+2 comments)') end |