diff options
Diffstat (limited to 'spec')
| -rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 60 | ||||
| -rw-r--r-- | spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb | 76 | ||||
| -rw-r--r-- | spec/models/discussion_spec.rb | 9 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 40 | ||||
| -rw-r--r-- | spec/requests/api/issues_spec.rb | 26 | ||||
| -rw-r--r-- | spec/services/discussions/resolve_service_spec.rb | 52 | ||||
| -rw-r--r-- | spec/services/issues/build_service_spec.rb | 130 | ||||
| -rw-r--r-- | spec/services/issues/create_service_spec.rb | 43 | ||||
| -rw-r--r-- | spec/services/system_note_service_spec.rb | 28 |
9 files changed, 464 insertions, 0 deletions
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 90419368f22..dbe5ddccbcf 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -55,6 +55,30 @@ describe Projects::IssuesController do end describe 'GET #new' do + context 'internal issue tracker' do + before do + sign_in(user) + project.team << [user, :developer] + end + + it 'builds a new issue' do + get :new, namespace_id: project.namespace.path, project_id: project + + expect(assigns(:issue)).to be_a_new(Issue) + end + + it 'fills in an issue for a merge request' do + project_with_repository = create(:project) + project_with_repository.team << [user, :developer] + mr = create(:merge_request_with_diff_notes, source_project: project_with_repository) + + get :new, namespace_id: project_with_repository.namespace.path, project_id: project_with_repository, merge_request_for_resolving_discussions: mr.iid + + expect(assigns(:issue).title).not_to be_empty + expect(assigns(:issue).description).not_to be_empty + end + end + context 'external issue tracker' do it 'redirects to the external issue tracker' do external = double(new_issue_path: 'https://example.com/issues/new') @@ -272,6 +296,42 @@ describe Projects::IssuesController do end describe 'POST #create' do + context 'resolving discussions in MergeRequest' do + let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:merge_request) { discussion.noteable } + let(:project) { merge_request.source_project } + + before do + project.team << [user, :master] + sign_in user + end + + let(:merge_request_params) do + { merge_request_for_resolving_discussions: merge_request.iid } + end + + def post_issue(issue_params) + post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, issue: issue_params, merge_request_for_resolving_discussions: merge_request.iid + end + + it 'creates an issue for the project' do + expect { post_issue({ title: 'Hello' }) }.to change { project.issues.reload.size }.by(1) + end + + it "doesn't overwrite given params" do + post_issue(description: 'Manually entered description') + + expect(assigns(:issue).description).to eq('Manually entered description') + end + + it 'resolves the discussion in the merge_request' do + post_issue(title: 'Hello') + discussion.first_note.reload + + expect(discussion.resolved?).to eq(true) + end + end + context 'Akismet is enabled' do before do allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) 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 new file mode 100644 index 00000000000..762cab0c0e1 --- /dev/null +++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb @@ -0,0 +1,76 @@ +require 'rails_helper' + +feature 'Resolving all open discussions in a merge request from an issue', feature: true do + let(:user) { create(:user) } + let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) } + 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 } + + before do + project.team << [user, :master] + login_as user + end + + context 'with the internal tracker disabled' do + before do + project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED) + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + it 'does not show a link to create a new issue' do + expect(page).not_to have_link 'open an issue to resolve them later' + end + end + + context 'merge request has discussions that need to be resolved' do + before do + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + it 'shows a warning that the merge request contains unresolved discussions' do + expect(page).to have_content 'This merge request has unresolved discussions' + end + + it 'has a link to resolve all discussions by creating an issue' do + page.within '.mr-widget-body' do + expect(page).to have_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_for_resolving_discussions: merge_request.iid) + end + end + + context 'creating an issue for discussions' do + before do + page.click_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_for_resolving_discussions: merge_request.iid) + end + + it 'shows an issue with the title filled in' do + title_field = page.find_field('issue[title]') + + expect(title_field.value).to include(merge_request.title) + end + + it 'has a mention of the discussion in the description' do + description_field = page.find_field('issue[description]') + + expect(description_field.value).to include(discussion.first_note.note) + end + + it 'has a hidden field for the merge request' do + merge_request_field = find('#merge_request_for_resolving_discussions', visible: false) + + expect(merge_request_field.value).to eq(merge_request.iid.to_s) + end + + it 'can create a new issue for the project' do + expect { click_button 'Submit issue' }.to change { project.issues.reload.size }.by(1) + end + + it 'resolves the discussion in the merge request' do + click_button 'Submit issue' + + discussion.first_note.reload + + expect(discussion.resolved?).to eq(true) + end + end + end +end diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index 2a67c60b978..bc32fadd391 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -521,6 +521,15 @@ describe Discussion, model: true do end end + describe "#first_note_to_resolve" do + it "returns the first not 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 "#collapsed?" do context "when a diff discussion" do before do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 2cc818af6c7..925232169f1 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1124,6 +1124,46 @@ describe MergeRequest, models: true do allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion]) end + describe '#resolvable_discussions' do + before do + allow(first_discussion).to receive(:to_be_resolved?).and_return(true) + allow(second_discussion).to receive(:to_be_resolved?).and_return(false) + allow(third_discussion).to receive(:to_be_resolved?).and_return(false) + end + + it 'includes only discussions that need to be resolved' do + expect(subject.resolvable_discussions).to eq([first_discussion]) + 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 diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 5700f800c2e..553983575c4 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -692,6 +692,32 @@ describe API::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(:merge_request) { discussion.noteable } + let(:project) { merge_request.source_project } + before do + project.team << [user, :master] + post api("/projects/#{project.id}/issues", user), + title: 'New Issue', + merge_request_for_resolving_discussions: merge_request.iid + end + + it 'creates a new project issue' do + expect(response).to have_http_status(:created) + end + + it 'resolves the discussions in a merge request' do + discussion.first_note.reload + + expect(discussion.resolved?).to be(true) + end + + it 'assigns a description to the issue mentioning the merge request' do + expect(json_response['description']).to include(merge_request.to_reference) + end + end + context 'with due date' do it 'creates a new project issue' do due_date = 2.weeks.from_now.strftime('%Y-%m-%d') diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb new file mode 100644 index 00000000000..12c3cdf28c6 --- /dev/null +++ b/spec/services/discussions/resolve_service_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe Discussions::ResolveService do + describe '#execute' do + let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:project) { merge_request.project } + let(:merge_request) { discussion.noteable } + let(:user) { create(:user) } + let(:service) { described_class.new(discussion.noteable.project, user, merge_request: merge_request) } + + before do + project.team << [user, :master] + end + + it "doesn't resolve discussions the user can't resolve" do + expect(discussion).to receive(:can_resolve?).with(user).and_return(false) + + service.execute(discussion) + + expect(discussion.resolved?).to be(false) + end + + it 'resolves the discussion' do + service.execute(discussion) + + expect(discussion.resolved?).to be(true) + end + + it 'executes the notification service' do + expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(discussion.noteable) + + service.execute(discussion) + end + + it 'adds a system note to the discussion' do + issue = create(:issue, project: project) + + expect(SystemNoteService).to receive(:discussion_continued_in_issue).with(discussion, project, user, issue) + service = described_class.new(project, user, merge_request: merge_request, follow_up_issue: issue) + service.execute(discussion) + 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 + + service.execute([discussion, other_discussion]) + + expect(discussion.resolved?).to be(true) + expect(other_discussion.resolved?).to be(true) + end + end +end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb new file mode 100644 index 00000000000..4cfba35c830 --- /dev/null +++ b/spec/services/issues/build_service_spec.rb @@ -0,0 +1,130 @@ +require 'spec_helper.rb' + +describe Issues::BuildService, services: true do + let(:project) { create(:project) } + let(:user) { create(:user) } + + before do + project.team << [user, :developer] + end + + context 'for discussions in a merge request' do + let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) } + let(:issue) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request).execute } + + def position_on_line(line_number) + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: line_number, + diff_refs: merge_request.diff_refs + ) + end + + describe '#items_for_discussions' do + it 'has an item for each discussion' do + create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.source_project, position: position_on_line(13)) + service = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) + + service.execute + + expect(service.items_for_discussions.size).to eq(2) + end + end + + describe '#item_for_discussion' do + let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) } + + it 'mentions the author of the note' do + discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))]) + expect(service.item_for_discussion(discussion)).to include('@author') + end + + it 'wraps the note in a blockquote' do + note_text = "This is a string\n"\ + ">>>\n"\ + "with a blockquote\n"\ + "> That has a quote\n"\ + ">>>\n" + 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)]) + expect(service.item_for_discussion(discussion)).to include(">>>\n#{note_result}\n>>>") + end + end + + describe '#execute' do + it 'has the merge request reference in the title' do + expect(issue.title).to include(merge_request.title) + end + + it 'has the reference of the merge request in the description' do + expect(issue.description).to include(merge_request.to_reference) + end + + it 'does not assign title when a title was given' do + issue = described_class.new(project, user, + merge_request_for_resolving_discussions: merge_request, + title: 'What an issue').execute + + expect(issue.title).to eq('What an issue') + end + + it 'does not assign description when a description was given' do + issue = described_class.new(project, user, + merge_request_for_resolving_discussions: merge_request, + description: 'Fix at your earliest conveignance').execute + + expect(issue.description).to eq('Fix at your earliest conveignance') + end + + describe 'with multiple discussions' do + before do + create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, position: position_on_line(15)) + end + + it 'mentions all the authors in the description' do + authors = merge_request.diff_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) + links = notes.map { |note| Gitlab::UrlBuilder.build(note) } + + expect(issue.description).to include(*links) + end + + it 'mentions additional notes' do + create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, position: position_on_line(15)) + + expect(issue.description).to include('(+2 comments)') + end + end + end + end + + context 'For a merge request without discussions' do + let(:merge_request) { create(:merge_request, source_project: project) } + + describe '#execute' do + it 'mentions the merge request in the description' do + issue = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request).execute + + expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}") + end + end + end + + describe '#execute' do + it 'builds a new issues with given params' do + issue = described_class.new(project, user, title: 'Issue #1', description: 'Issue description').execute + + expect(issue.title).to eq('Issue #1') + expect(issue.description).to eq('Issue description') + end + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 5c0331ebe66..8bde61ee336 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -136,5 +136,48 @@ describe Issues::CreateService, services: true do end it_behaves_like 'new issuable record that supports slash commands' + + context 'for a merge request' do + let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:merge_request) { discussion.noteable } + let(:project) { merge_request.source_project } + let(:opts) { { merge_request_for_resolving_discussions: merge_request } } + + before do + project.team << [user, :master] + end + + it 'resolves the discussion for the merge request' do + described_class.new(project, user, opts).execute + discussion.first_note.reload + + expect(discussion.resolved?).to be(true) + end + + it 'added a system note to the discussion' do + described_class.new(project, user, opts).execute + + reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first + + expect(reloaded_discussion.last_note.system).to eq(true) + end + + it 'assigns the title and description for the issue' do + issue = described_class.new(project, user, opts).execute + + expect(issue.title).not_to be_nil + expect(issue.description).not_to be_nil + end + + it 'can set nil explicityly to the title and description' do + issue = described_class.new(project, user, + merge_request_for_resolving_discussions: merge_request, + description: nil, + title: nil).execute + + expect(issue.description).to be_nil + expect(issue.title).to be_nil + end + end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 435cfb07292..07a9d8e1997 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -712,4 +712,32 @@ describe SystemNoteService, services: true do end end end + + describe '.discussion_continued_in_issue' do + let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:merge_request) { discussion.noteable } + let(:project) { merge_request.source_project } + let(:issue) { create(:issue, project: project) } + let(:user) { create(:user) } + + def reloaded_merge_request + MergeRequest.find(merge_request.id) + end + + before do + project.team << [user, :developer] + end + + it 'creates a new note in the discussion' do + # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded. + expect { SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue) }. + to change { reloaded_merge_request.discussions.first.notes.size }.by(1) + end + + it 'mentions the created issue in the system note' do + note = SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue) + + expect(note.note).to include(issue.to_reference) + end + end end |