diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-27 21:08:14 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-27 21:08:14 +0300 |
commit | ab801bd018f129b2c2f7ebe0de677728582c66f0 (patch) | |
tree | 017d87f33afadcbc15f6570abaa08c53ed4bdefd /spec/services | |
parent | 4d5ee2b81486df708e04a0e32ec2ea58b0e5ed5e (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'spec/services')
7 files changed, 522 insertions, 4 deletions
diff --git a/spec/services/draft_notes/create_service_spec.rb b/spec/services/draft_notes/create_service_spec.rb new file mode 100644 index 00000000000..8f244ed386b --- /dev/null +++ b/spec/services/draft_notes/create_service_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DraftNotes::CreateService do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.author } + + def create_draft(params) + described_class.new(merge_request, user, params).execute + end + + it 'creates a simple draft note' do + draft = create_draft(note: 'This is a test') + + expect(draft).to be_an_instance_of(DraftNote) + expect(draft.note).to eq('This is a test') + expect(draft.author).to eq(user) + expect(draft.project).to eq(merge_request.target_project) + expect(draft.discussion_id).to be_nil + end + + it 'cannot resolve when there is nothing to resolve' do + draft = create_draft(note: 'Not a reply!', resolve_discussion: true) + + expect(draft.errors[:base]).to include('User is not allowed to resolve thread') + expect(draft).not_to be_persisted + end + + context 'in a thread' do + it 'creates a draft note with discussion_id' do + discussion = create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion + + draft = create_draft(note: 'A reply!', in_reply_to_discussion_id: discussion.reply_id) + + expect(draft.note).to eq('A reply!') + expect(draft.discussion_id).to eq(discussion.reply_id) + expect(draft.resolve_discussion).to be_falsey + end + + it 'creates a draft that resolves the thread' do + discussion = create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion + + draft = create_draft(note: 'A reply!', in_reply_to_discussion_id: discussion.reply_id, resolve_discussion: true) + + expect(draft.note).to eq('A reply!') + expect(draft.discussion_id).to eq(discussion.reply_id) + expect(draft.resolve_discussion).to be true + end + end + + it 'creates a draft note with a position in a diff' do + diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs) + + position = Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: diff_refs + ) + + draft = create_draft(note: 'Comment on diff', position: position.to_json) + + expect(draft.note).to eq('Comment on diff') + expect(draft.original_position.to_json).to eq(position.to_json) + end + + context 'diff highlight cache clearing' do + context 'when diff file is unfolded and it is not a reply' do + it 'clears diff highlighting cache' do + expect_next_instance_of(DraftNote) do |draft| + allow(draft).to receive_message_chain(:diff_file, :unfolded?) { true } + end + + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + create_draft(note: 'This is a test') + end + end + + context 'when diff file is not unfolded and it is not a reply' do + it 'clears diff highlighting cache' do + expect_next_instance_of(DraftNote) do |draft| + allow(draft).to receive_message_chain(:diff_file, :unfolded?) { false } + end + + expect(merge_request).not_to receive(:diffs) + + create_draft(note: 'This is a test') + end + end + end +end diff --git a/spec/services/draft_notes/destroy_service_spec.rb b/spec/services/draft_notes/destroy_service_spec.rb new file mode 100644 index 00000000000..d0bf88dcdbe --- /dev/null +++ b/spec/services/draft_notes/destroy_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DraftNotes::DestroyService do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.author } + + def destroy(draft_note = nil) + DraftNotes::DestroyService.new(merge_request, user).execute(draft_note) + end + + it 'destroys a single draft note' do + drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user) + + expect { destroy(drafts.first) } + .to change { DraftNote.count }.by(-1) + + expect(DraftNote.count).to eq(1) + end + + it 'destroys all draft notes for a user in a merge request' do + create_list(:draft_note, 2, merge_request: merge_request, author: user) + + expect { destroy }.to change { DraftNote.count }.by(-2) + expect(DraftNote.count).to eq(0) + end + + context 'diff highlight cache clearing' do + context 'when destroying all draft notes of a user' do + it 'clears highlighting cache if unfold required for any' do + drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user) + + allow_any_instance_of(DraftNote).to receive_message_chain(:diff_file, :unfolded?) { true } + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + destroy(drafts.first) + end + end + + context 'when destroying one draft note' do + it 'clears highlighting cache if unfold required' do + create_list(:draft_note, 2, merge_request: merge_request, author: user) + + allow_any_instance_of(DraftNote).to receive_message_chain(:diff_file, :unfolded?) { true } + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + destroy + end + end + end +end diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb new file mode 100644 index 00000000000..4ebae2f9aa2 --- /dev/null +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -0,0 +1,261 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DraftNotes::PublishService do + include RepoHelpers + + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.author } + let(:commit) { project.commit(sample_commit.id) } + + let(:position) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: commit.diff_refs + ) + end + + def publish(draft: nil) + DraftNotes::PublishService.new(merge_request, user).execute(draft) + end + + context 'single draft note' do + let(:commit_id) { nil } + let!(:drafts) { create_list(:draft_note, 2, merge_request: merge_request, author: user, commit_id: commit_id, position: position) } + + it 'publishes' do + expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1) + expect(DraftNote.count).to eq(1) + end + + it 'does not skip notification', :sidekiq_might_not_need_inline do + expect(Notes::CreateService).to receive(:new).with(project, user, drafts.first.publish_params).and_call_original + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:new_note) + end + + result = publish(draft: drafts.first) + + expect(result[:status]).to eq(:success) + end + + context 'commit_id is set' do + let(:commit_id) { commit.id } + + it 'creates note from draft with commit_id' do + result = publish(draft: drafts.first) + + expect(result[:status]).to eq(:success) + expect(merge_request.notes.first.commit_id).to eq(commit_id) + end + end + end + + context 'multiple draft notes' do + let(:commit_id) { nil } + + before do + create(:draft_note, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position) + create(:draft_note, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position) + end + + context 'when review fails to create' do + before do + expect_next_instance_of(Review) do |review| + allow(review).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(review)) + end + end + + it 'does not publish any draft note' do + expect { publish }.not_to change { DraftNote.count } + end + + it 'returns an error' do + result = publish + + expect(result[:status]).to eq(:error) + expect(result[:message]).to match(/Unable to save Review/) + end + end + + it 'returns success' do + result = publish + + expect(result[:status]).to eq(:success) + end + + it 'publishes all draft notes for a user in a merge request' do + expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2).and change { Review.count }.by(1) + expect(DraftNote.count).to eq(0) + + notes = merge_request.notes.order(id: :asc) + expect(notes.first.note).to eq('first note') + expect(notes.last.note).to eq('second note') + end + + it 'sends batch notification' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive_message_chain(:async, :new_review).with(kind_of(Review)) + end + + publish + end + + context 'commit_id is set' do + let(:commit_id) { commit.id } + + it 'creates note from draft with commit_id' do + result = publish + + expect(result[:status]).to eq(:success) + + merge_request.notes.each do |note| + expect(note.commit_id).to eq(commit_id) + end + end + end + end + + context 'draft notes with suggestions' do + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + let(:suggestion_note) do + <<-MARKDOWN.strip_heredoc + ```suggestion + foo + ``` + MARKDOWN + end + + let!(:draft) { create(:draft_note_on_text_diff, note: suggestion_note, merge_request: merge_request, author: user) } + + it 'creates a suggestion with correct content' do + expect { publish(draft: draft) }.to change { Suggestion.count }.by(1) + .and change { DiffNote.count }.from(0).to(1) + + suggestion = Suggestion.last + + expect(suggestion.from_line).to eq(14) + expect(suggestion.to_line).to eq(14) + expect(suggestion.from_content).to eq(" vars = {\n") + expect(suggestion.to_content).to eq(" foo\n") + end + + context 'when the diff is changed' do + let(:file_path) { 'files/ruby/popen.rb' } + let(:branch_name) { project.default_branch } + let(:commit) { project.repository.commit } + + def update_file(file_path, new_content) + params = { + file_path: file_path, + commit_message: "Update File", + file_content: new_content, + start_project: project, + start_branch: project.default_branch, + branch_name: branch_name + } + + Files::UpdateService.new(project, user, params).execute + end + + before do + project.add_developer(user) + end + + it 'creates a suggestion based on the latest diff content and positions' do + diff_file = merge_request.diffs(paths: [file_path]).diff_files.first + raw_data = diff_file.new_blob.data + + # Add a line break to the beginning of the file + result = update_file(file_path, raw_data.prepend("\n")) + oldrev = merge_request.diff_head_sha + newrev = result[:result] + + expect(newrev).to be_present + + # Generates new MR revision at DB level + refresh = MergeRequests::RefreshService.new(project, user) + refresh.execute(oldrev, newrev, merge_request.source_branch_ref) + + expect { publish(draft: draft) }.to change { Suggestion.count }.by(1) + .and change { DiffNote.count }.from(0).to(1) + + suggestion = Suggestion.last + + expect(suggestion.from_line).to eq(15) + expect(suggestion.to_line).to eq(15) + expect(suggestion.from_content).to eq(" vars = {\n") + expect(suggestion.to_content).to eq(" foo\n") + end + end + end + + it 'only publishes the draft notes belonging to the current user' do + other_user = create(:user) + project.add_maintainer(other_user) + + create_list(:draft_note, 2, merge_request: merge_request, author: user) + create_list(:draft_note, 2, merge_request: merge_request, author: other_user) + + expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2) + expect(DraftNote.count).to eq(2) + end + + context 'with quick actions' do + it 'performs quick actions' do + other_user = create(:user) + project.add_developer(other_user) + + create(:draft_note, merge_request: merge_request, + author: user, + note: "thanks\n/assign #{other_user.to_reference}") + + expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(2) + expect(merge_request.reload.assignees).to match_array([other_user]) + expect(merge_request.notes.last).to be_system + end + + it 'does not create a note if it only contains quick actions' do + create(:draft_note, merge_request: merge_request, author: user, note: "/assign #{user.to_reference}") + + expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1) + expect(merge_request.reload.assignees).to eq([user]) + expect(merge_request.notes.last).to be_system + end + end + + context 'with drafts that resolve threads' do + let!(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } + let!(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) } + + it 'resolves the thread' do + publish(draft: draft_note) + + expect(note.discussion.resolved?).to be true + end + + it 'sends notifications if all threads are resolved' do + expect_next_instance_of(MergeRequests::ResolvedDiscussionNotificationService) do |instance| + expect(instance).to receive(:execute).with(merge_request) + end + + publish + end + end + + context 'user cannot create notes' do + before do + allow(Ability).to receive(:allowed?).with(user, :create_note, merge_request).and_return(false) + end + + it 'returns an error' do + expect(publish[:status]).to eq(:error) + end + end +end diff --git a/spec/services/notification_recipients/build_service_spec.rb b/spec/services/notification_recipients/build_service_spec.rb index 2e848c2f04d..e203093623d 100644 --- a/spec/services/notification_recipients/build_service_spec.rb +++ b/spec/services/notification_recipients/build_service_spec.rb @@ -58,4 +58,56 @@ describe NotificationRecipients::BuildService do end end end + + describe '#build_new_review_recipients' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:review) { create(:review, merge_request: merge_request, project: project, author: merge_request.author) } + let(:notes) { create_list(:note_on_merge_request, 3, review: review, noteable: review.merge_request, project: review.project) } + + shared_examples 'no N+1 queries' do + it 'avoids N+1 queries', :request_store do + create_user + + service.build_new_review_recipients(review) + + control_count = ActiveRecord::QueryRecorder.new do + service.build_new_review_recipients(review) + end + + create_user + + expect { service.build_new_review_recipients(review) }.not_to exceed_query_limit(control_count) + end + end + + context 'when there are multiple watchers' do + def create_user + watcher = create(:user) + create(:notification_setting, source: project, user: watcher, level: :watch) + + other_projects.each do |other_project| + create(:notification_setting, source: other_project, user: watcher, level: :watch) + end + end + + include_examples 'no N+1 queries' + end + + context 'when there are multiple subscribers' do + def create_user + subscriber = create(:user) + merge_request.subscriptions.create(user: subscriber, project: project, subscribed: true) + end + + include_examples 'no N+1 queries' + + context 'when the project is private' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + include_examples 'no N+1 queries' + end + end + end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 2a7166e3895..d3376ef0a04 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2863,6 +2863,57 @@ describe NotificationService, :mailer do end end + describe '#new_review' do + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:reviewer) { create(:user) } + let(:merge_request) { create(:merge_request, source_project: project, assignees: [user, user2], author: create(:user)) } + let(:review) { create(:review, merge_request: merge_request, project: project, author: reviewer) } + let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, author: reviewer, review: review) } + + before do + build_team(review.project) + add_users(review.project) + add_user_subscriptions(merge_request) + project.add_maintainer(merge_request.author) + project.add_maintainer(reviewer) + merge_request.assignees.each { |assignee| project.add_maintainer(assignee) } + + create(:diff_note_on_merge_request, + project: project, + noteable: merge_request, + author: reviewer, + review: review, + note: "cc @mention") + end + + it 'sends emails' do + expect(Notify).not_to receive(:new_review_email).with(review.author.id, review.id) + expect(Notify).not_to receive(:new_review_email).with(@unsubscriber.id, review.id) + merge_request.assignee_ids.each do |assignee_id| + expect(Notify).to receive(:new_review_email).with(assignee_id, review.id).and_call_original + end + expect(Notify).to receive(:new_review_email).with(merge_request.author.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@u_watcher.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@u_mentioned.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@subscriber.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@watcher_and_subscriber.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@subscribed_participant.id, review.id).and_call_original + + subject.new_review(review) + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { review } + let(:notification_trigger) { subject.new_review(review) } + + around do |example| + perform_enqueued_jobs { example.run } + end + end + end + def build_team(project) @u_watcher = create_global_setting_for(create(:user), :watch) @u_participating = create_global_setting_for(create(:user), :participating) diff --git a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb index 82ea356d599..5982dcbc404 100644 --- a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb +++ b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb @@ -64,7 +64,7 @@ describe Prometheus::ProxyVariableSubstitutionService do let(:params_keys) do { query: 'up{pod_name="{{pod_name}}"}', - variables: ['pod_name', pod_name] + variables: { 'pod_name' => pod_name } } end @@ -76,7 +76,7 @@ describe Prometheus::ProxyVariableSubstitutionService do let(:params_keys) do { query: 'up{pod_name="{{pod_name}}",env="{{ci_environment_slug}}"}', - variables: ['pod_name', pod_name, 'ci_environment_slug', 'custom_value'] + variables: { 'pod_name' => pod_name, 'ci_environment_slug' => 'custom_value' } } end @@ -95,8 +95,7 @@ describe Prometheus::ProxyVariableSubstitutionService do } end - it_behaves_like 'error', 'Optional parameter "variables" must be an ' \ - 'array of keys and values. Ex: [key1, value1, key2, value2]' + it_behaves_like 'error', 'Optional parameter "variables" must be a Hash. Ex: variables[key1]=value1' end context 'with nil variables' do diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb index a7d7c16a66f..c2a793b2368 100644 --- a/spec/services/users/migrate_to_ghost_user_service_spec.rb +++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb @@ -84,6 +84,15 @@ describe Users::MigrateToGhostUserService do end end + context 'reviews' do + let!(:user) { create(:user) } + let(:service) { described_class.new(user) } + + include_examples "migrating a deleted user's associated records to the ghost user", Review, [:author] do + let(:created_record) { create(:review, author: user) } + end + end + context "when record migration fails with a rollback exception" do before do expect_any_instance_of(ActiveRecord::Associations::CollectionProxy) |