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 | |
parent | 4d5ee2b81486df708e04a0e32ec2ea58b0e5ed5e (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'spec')
20 files changed, 763 insertions, 132 deletions
diff --git a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb index 64f90e44bb6..fb8da52930c 100644 --- a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb +++ b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb @@ -84,12 +84,12 @@ describe Projects::Environments::PrometheusApiController do before do expected_params[:query] = %{up{pod_name="#{pod_name}"}} - expected_params[:variables] = ['pod_name', pod_name] + expected_params[:variables] = { 'pod_name' => pod_name } end it 'replaces variables with values' do get :proxy, params: environment_params.merge( - query: 'up{pod_name="{{pod_name}}"}', variables: ['pod_name', pod_name] + query: 'up{pod_name="{{pod_name}}"}', variables: { 'pod_name' => pod_name } ) expect(response).to have_gitlab_http_status(:success) diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 01a9647a763..3a6ddfb1783 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -445,4 +445,27 @@ describe RegistrationsController do end end end + + describe '#experience_level' do + subject { get :experience_level } + + let_it_be(:user) { create(:user) } + + let(:part_of_onboarding_issues_experiment) { false } + + before do + stub_experiment_for_user(onboarding_issues: part_of_onboarding_issues_experiment) + sign_in(user) + end + + context 'when not part of the onboarding issues experiment' do + it { is_expected.to have_gitlab_http_status(:not_found) } + end + + context 'when part of the onboarding issues experiment' do + let(:part_of_onboarding_issues_experiment) { true } + + it { is_expected.to render_template(:experience_level) } + end + end end diff --git a/spec/frontend/ide/components/repo_commit_section_spec.js b/spec/frontend/ide/components/repo_commit_section_spec.js index 237be018807..d4e4e064a52 100644 --- a/spec/frontend/ide/components/repo_commit_section_spec.js +++ b/spec/frontend/ide/components/repo_commit_section_spec.js @@ -2,6 +2,7 @@ import { mount } from '@vue/test-utils'; import { createStore } from '~/ide/stores'; import router from '~/ide/ide_router'; import RepoCommitSection from '~/ide/components/repo_commit_section.vue'; +import EmptyState from '~/ide/components/commit_sidebar/empty_state.vue'; import { stageKeys } from '~/ide/constants'; import { file } from '../helpers'; @@ -63,7 +64,7 @@ describe('RepoCommitSection', () => { wrapper.destroy(); }); - describe('empty Stage', () => { + describe('empty state', () => { beforeEach(() => { store.state.noChangesStateSvgPath = TEST_NO_CHANGES_SVG; store.state.committedStateSvgPath = 'svg'; @@ -74,11 +75,16 @@ describe('RepoCommitSection', () => { it('renders no changes text', () => { expect( wrapper - .find('.js-empty-state') + .find(EmptyState) .text() .trim(), ).toContain('No changes'); - expect(wrapper.find('.js-empty-state img').attributes('src')).toBe(TEST_NO_CHANGES_SVG); + expect( + wrapper + .find(EmptyState) + .find('img') + .attributes('src'), + ).toBe(TEST_NO_CHANGES_SVG); }); }); @@ -109,6 +115,10 @@ describe('RepoCommitSection', () => { expect(changedFileNames).toEqual(allFiles.map(x => x.path)); }); + + it('does not show empty state', () => { + expect(wrapper.find(EmptyState).exists()).toBe(false); + }); }); describe('with unstaged file', () => { @@ -129,5 +139,9 @@ describe('RepoCommitSection', () => { keyPrefix: stageKeys.unstaged, }); }); + + it('does not show empty state', () => { + expect(wrapper.find(EmptyState).exists()).toBe(false); + }); }); }); diff --git a/spec/frontend/ide/stores/actions/file_spec.js b/spec/frontend/ide/stores/actions/file_spec.js index 43cb06f5d92..e50697af5eb 100644 --- a/spec/frontend/ide/stores/actions/file_spec.js +++ b/spec/frontend/ide/stores/actions/file_spec.js @@ -587,20 +587,6 @@ describe('IDE store file actions', () => { }) .catch(done.fail); }); - - it('bursts unused seal', done => { - store - .dispatch('changeFileContent', { - path: tmpFile.path, - content: 'content', - }) - .then(() => { - expect(store.state.unusedSeal).toBe(false); - - done(); - }) - .catch(done.fail); - }); }); describe('with changed file', () => { diff --git a/spec/frontend/ide/stores/actions_spec.js b/spec/frontend/ide/stores/actions_spec.js index d52b0435906..666ed8a24aa 100644 --- a/spec/frontend/ide/stores/actions_spec.js +++ b/spec/frontend/ide/stores/actions_spec.js @@ -292,21 +292,6 @@ describe('Multi-file store actions', () => { }) .catch(done.fail); }); - - it('bursts unused seal', done => { - store - .dispatch('createTempEntry', { - name: 'test', - branchId: 'mybranch', - type: 'blob', - }) - .then(() => { - expect(store.state.unusedSeal).toBe(false); - - done(); - }) - .catch(done.fail); - }); }); }); @@ -682,19 +667,6 @@ describe('Multi-file store actions', () => { }); }); }); - - it('bursts unused seal', done => { - store.state.entries.test = file('test'); - - store - .dispatch('deleteEntry', 'test') - .then(() => { - expect(store.state.unusedSeal).toBe(false); - - done(); - }) - .catch(done.fail); - }); }); describe('renameEntry', () => { @@ -839,20 +811,6 @@ describe('Multi-file store actions', () => { .then(done) .catch(done.fail); }); - - it('bursts unused seal', done => { - store - .dispatch('renameEntry', { - path: 'orig', - name: 'renamed', - }) - .then(() => { - expect(store.state.unusedSeal).toBe(false); - - done(); - }) - .catch(done.fail); - }); }); describe('folder', () => { diff --git a/spec/frontend/ide/stores/mutations/file_spec.js b/spec/frontend/ide/stores/mutations/file_spec.js index 9b96b910fcb..cd308ee9991 100644 --- a/spec/frontend/ide/stores/mutations/file_spec.js +++ b/spec/frontend/ide/stores/mutations/file_spec.js @@ -356,14 +356,6 @@ describe('IDE store file mutations', () => { expect(localState.changedFiles.length).toBe(1); }); - - it('bursts unused seal', () => { - expect(localState.unusedSeal).toBe(true); - - mutations.ADD_FILE_TO_CHANGED(localState, localFile.path); - - expect(localState.unusedSeal).toBe(false); - }); }); describe('REMOVE_FILE_FROM_CHANGED', () => { @@ -374,14 +366,6 @@ describe('IDE store file mutations', () => { expect(localState.changedFiles.length).toBe(0); }); - - it('bursts unused seal', () => { - expect(localState.unusedSeal).toBe(true); - - mutations.REMOVE_FILE_FROM_CHANGED(localState, localFile.path); - - expect(localState.unusedSeal).toBe(false); - }); }); describe.each` @@ -533,19 +517,6 @@ describe('IDE store file mutations', () => { }, ); - describe('STAGE_CHANGE', () => { - it('bursts unused seal', () => { - expect(localState.unusedSeal).toBe(true); - - mutations.STAGE_CHANGE(localState, { - path: localFile.path, - diffInfo: localStore.getters.getDiffInfo(localFile.path), - }); - - expect(localState.unusedSeal).toBe(false); - }); - }); - describe('TOGGLE_FILE_CHANGED', () => { it('updates file changed status', () => { mutations.TOGGLE_FILE_CHANGED(localState, { diff --git a/spec/frontend/ide/stores/mutations_spec.js b/spec/frontend/ide/stores/mutations_spec.js index 2eca9acb8d8..55cc6eb66ab 100644 --- a/spec/frontend/ide/stores/mutations_spec.js +++ b/spec/frontend/ide/stores/mutations_spec.js @@ -265,16 +265,6 @@ describe('Multi-file store mutations', () => { expect(localState.changedFiles).toEqual([]); expect(localState.stagedFiles).toEqual([]); }); - - it('bursts unused seal', () => { - localState.entries.test = file('test'); - - expect(localState.unusedSeal).toBe(true); - - mutations.DELETE_ENTRY(localState, 'test'); - - expect(localState.unusedSeal).toBe(false); - }); }); describe('UPDATE_FILE_AFTER_COMMIT', () => { diff --git a/spec/frontend/monitoring/store/getters_spec.js b/spec/frontend/monitoring/store/getters_spec.js index 9f17dda3b9f..c7d0fb119de 100644 --- a/spec/frontend/monitoring/store/getters_spec.js +++ b/spec/frontend/monitoring/store/getters_spec.js @@ -329,7 +329,7 @@ describe('Monitoring store Getters', () => { }); }); - describe('getCustomVariablesArray', () => { + describe('getCustomVariablesParams', () => { let state; beforeEach(() => { @@ -340,25 +340,21 @@ describe('Monitoring store Getters', () => { it('transforms the variables object to an array in the [variable, variable_value] format for all variable types', () => { mutations[types.SET_VARIABLES](state, mockTemplatingDataResponses.allVariableTypes); - const variablesArray = getters.getCustomVariablesArray(state); - - expect(variablesArray).toEqual([ - 'simpleText', - 'Simple text', - 'advText', - 'default', - 'simpleCustom', - 'value1', - 'advCustomNormal', - 'value2', - ]); + const variablesArray = getters.getCustomVariablesParams(state); + + expect(variablesArray).toEqual({ + 'variables[advCustomNormal]': 'value2', + 'variables[advText]': 'default', + 'variables[simpleCustom]': 'value1', + 'variables[simpleText]': 'Simple text', + }); }); it('transforms the variables object to an empty array when no keys are present', () => { mutations[types.SET_VARIABLES](state, {}); - const variablesArray = getters.getCustomVariablesArray(state); + const variablesArray = getters.getCustomVariablesParams(state); - expect(variablesArray).toEqual([]); + expect(variablesArray).toEqual({}); }); }); diff --git a/spec/helpers/milestones_helper_spec.rb b/spec/helpers/timeboxes_helper_spec.rb index 4ce7143bdf0..6fe738914ce 100644 --- a/spec/helpers/milestones_helper_spec.rb +++ b/spec/helpers/timeboxes_helper_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe MilestonesHelper do +describe TimeboxesHelper do describe '#milestones_filter_dropdown_path' do let(:project) { create(:project) } let(:project2) { create(:project) } @@ -39,23 +39,34 @@ describe MilestonesHelper do end end - describe "#milestone_date_range" do - def result_for(*args) - milestone_date_range(build(:milestone, *args)) - end - + describe "#timebox_date_range" do let(:yesterday) { Date.yesterday } let(:tomorrow) { yesterday + 2 } let(:format) { '%b %-d, %Y' } let(:yesterday_formatted) { yesterday.strftime(format) } let(:tomorrow_formatted) { tomorrow.strftime(format) } - it { expect(result_for(due_date: nil, start_date: nil)).to be_nil } - it { expect(result_for(due_date: tomorrow)).to eq("expires on #{tomorrow_formatted}") } - it { expect(result_for(due_date: yesterday)).to eq("expired on #{yesterday_formatted}") } - it { expect(result_for(start_date: tomorrow)).to eq("starts on #{tomorrow_formatted}") } - it { expect(result_for(start_date: yesterday)).to eq("started on #{yesterday_formatted}") } - it { expect(result_for(start_date: yesterday, due_date: tomorrow)).to eq("#{yesterday_formatted}–#{tomorrow_formatted}") } + context 'milestone' do + def result_for(*args) + timebox_date_range(build(:milestone, *args)) + end + + it { expect(result_for(due_date: nil, start_date: nil)).to be_nil } + it { expect(result_for(due_date: tomorrow)).to eq("expires on #{tomorrow_formatted}") } + it { expect(result_for(due_date: yesterday)).to eq("expired on #{yesterday_formatted}") } + it { expect(result_for(start_date: tomorrow)).to eq("starts on #{tomorrow_formatted}") } + it { expect(result_for(start_date: yesterday)).to eq("started on #{yesterday_formatted}") } + it { expect(result_for(start_date: yesterday, due_date: tomorrow)).to eq("#{yesterday_formatted}–#{tomorrow_formatted}") } + end + + context 'iteration' do + # Iterations always have start and due dates, so only A-B format is expected + it 'formats properly' do + iteration = build(:iteration, start_date: yesterday, due_date: tomorrow) + + expect(timebox_date_range(iteration)).to eq("#{yesterday_formatted}–#{tomorrow_formatted}") + end + end end describe '#milestone_counts' do diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 1a4f1123c73..8b99cc41a53 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -1726,4 +1726,59 @@ describe Notify do is_expected.to have_body_text target_url end end + + describe 'merge request reviews' do + let!(:review) { create(:review, project: project, merge_request: merge_request) } + let!(:notes) { create_list(:note, 3, review: review, project: project, author: review.author, noteable: merge_request) } + + subject { described_class.new_review_email(recipient.id, review.id) } + + it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + let(:model) { review.merge_request } + end + + it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like 'an unsubscribeable thread' + + it 'is sent to the given recipient as the author' do + sender = subject.header[:from].addrs[0] + + aggregate_failures do + expect(sender.display_name).to eq(review.author_name) + expect(sender.address).to eq(gitlab_sender) + expect(subject).to deliver_to(recipient.notification_email) + end + end + + it 'contains the message from the notes of the review' do + review.notes.each do |note| + is_expected.to have_body_text note.note + end + end + + context 'when diff note' do + let!(:notes) { create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request) } + + it 'links to notes' do + review.notes.each do |note| + # Text part + expect(subject.text_part.body.raw_source).to include( + project_merge_request_url(project, merge_request, anchor: "note_#{note.id}") + ) + end + end + end + + it 'contains review author name' do + is_expected.to have_body_text review.author_name + end + + it 'has the correct subject and body' do + aggregate_failures do + is_expected.to have_subject "Re: #{project.name} | #{merge_request.title} (#{merge_request.to_reference})" + + is_expected.to have_body_text project_merge_request_path(project, merge_request) + end + end + end end diff --git a/spec/migrations/backfill_status_page_published_incidents_spec.rb b/spec/migrations/backfill_status_page_published_incidents_spec.rb new file mode 100644 index 00000000000..ccdc8be4168 --- /dev/null +++ b/spec/migrations/backfill_status_page_published_incidents_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20200421195234_backfill_status_page_published_incidents.rb') + +describe BackfillStatusPagePublishedIncidents, :migration do + subject(:migration) { described_class.new } + + describe '#up' do + let(:projects) { table(:projects) } + let(:status_page_settings) { table(:status_page_settings) } + let(:issues) { table(:issues) } + let(:incidents) { table(:status_page_published_incidents) } + + let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab') } + let(:project_without_status_page) { projects.create!(namespace_id: namespace.id) } + let(:enabled_project) { projects.create!(namespace_id: namespace.id) } + let(:disabled_project) { projects.create!(namespace_id: namespace.id) } + + let!(:enabled_setting) { status_page_settings.create!(enabled: true, project_id: enabled_project.id, **status_page_setting_attrs) } + let!(:disabled_setting) { status_page_settings.create!(enabled: false, project_id: disabled_project.id, **status_page_setting_attrs) } + + let!(:published_issue) { issues.create!(confidential: false, project_id: enabled_project.id) } + let!(:nonpublished_issue_1) { issues.create!(confidential: true, project_id: enabled_project.id) } + let!(:nonpublished_issue_2) { issues.create!(confidential: false, project_id: disabled_project.id) } + let!(:nonpublished_issue_3) { issues.create!(confidential: false, project_id: project_without_status_page.id) } + + let(:current_time) { Time.current.change(usec: 0) } + let(:status_page_setting_attrs) do + { + aws_s3_bucket_name: 'bucket', + aws_region: 'region', + aws_access_key: 'key', + encrypted_aws_secret_key: 'abc123', + encrypted_aws_secret_key_iv: 'abc123' + } + end + + it 'creates a StatusPage::PublishedIncident record for each published issue' do + Timecop.freeze(current_time) do + expect(incidents.all).to be_empty + + migrate! + + incident = incidents.first + + expect(incidents.count).to eq(1) + expect(incident.issue_id).to eq(published_issue.id) + expect(incident.created_at).to eq(current_time) + expect(incident.updated_at).to eq(current_time) + end + end + end +end diff --git a/spec/models/concerns/limitable_spec.rb b/spec/models/concerns/limitable_spec.rb new file mode 100644 index 00000000000..ca0a257be7a --- /dev/null +++ b/spec/models/concerns/limitable_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Limitable do + let(:minimal_test_class) do + Class.new do + include ActiveModel::Model + + def self.name + 'TestClass' + end + + include Limitable + end + end + + before do + stub_const("MinimalTestClass", minimal_test_class) + end + + it { expect(MinimalTestClass.limit_name).to eq('test_classes') } + + context 'with scoped limit' do + before do + MinimalTestClass.limit_scope = :project + end + + it { expect(MinimalTestClass.limit_scope).to eq(:project) } + + it 'triggers scoped validations' do + instance = MinimalTestClass.new + + expect(instance).to receive(:validate_scoped_plan_limit_not_exceeded) + + instance.valid?(:create) + end + end + + context 'with global limit' do + before do + MinimalTestClass.limit_scope = Limitable::GLOBAL_SCOPE + end + + it { expect(MinimalTestClass.limit_scope).to eq(Limitable::GLOBAL_SCOPE) } + + it 'triggers scoped validations' do + instance = MinimalTestClass.new + + expect(instance).to receive(:validate_global_plan_limit_not_exceeded) + + instance.valid?(:create) + end + end +end 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) diff --git a/spec/support/shared_examples/models/concerns/limitable_shared_examples.rb b/spec/support/shared_examples/models/concerns/limitable_shared_examples.rb index 4bcea36fd42..d21823661f8 100644 --- a/spec/support/shared_examples/models/concerns/limitable_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/limitable_shared_examples.rb @@ -26,7 +26,7 @@ RSpec.shared_examples 'includes Limitable concern' do subject.dup.save end - it 'cannot create new models exceding the plan limits' do + it 'cannot create new models exceeding the plan limits' do expect { subject.save }.not_to change { described_class.count } expect(subject.errors[:base]).to contain_exactly("Maximum number of #{subject.class.limit_name.humanize(capitalize: false)} (1) exceeded") end |