Add latest changes from gitlab-org/gitlab@master

12 files changed, 505 insertions, 24 deletions

diff --git a/spec/frontend/jira_connect/index_spec.js b/spec/frontend/jira_connect/index_spec.js
new file mode 100644
index 00000000000..eb54fe6476f
--- /dev/null
+++ b/spec/frontend/jira_connect/index_spec.js
@@ -0,0 +1,56 @@
+import waitForPromises from 'helpers/wait_for_promises';
+import { initJiraConnect } from '~/jira_connect';
+import { removeSubscription } from '~/jira_connect/api';
+
+jest.mock('~/jira_connect/api', () => ({
+  removeSubscription: jest.fn().mockResolvedValue(),
+  getLocation: jest.fn().mockResolvedValue('test/location'),
+}));
+
+describe('initJiraConnect', () => {
+  window.AP = {
+    navigator: {
+      reload: jest.fn(),
+    },
+  };
+
+  beforeEach(async () => {
+    setFixtures(`
+      <a class="js-jira-connect-sign-in" href="https://gitlab.com">Sign In</a>
+      <a class="js-jira-connect-sign-in" href="https://gitlab.com">Another Sign In</a>
+
+      <a href="https://gitlab.com/sub1" class="js-jira-connect-remove-subscription">Remove</a>
+      <a href="https://gitlab.com/sub2" class="js-jira-connect-remove-subscription">Remove</a>
+      <a href="https://gitlab.com/sub3" class="js-jira-connect-remove-subscription">Remove</a>
+    `);
+
+    await initJiraConnect();
+  });
+
+  describe('Sign in links', () => {
+    it('have `return_to` query parameter', () => {
+      Array.from(document.querySelectorAll('.js-jira-connect-sign-in')).forEach((el) => {
+        expect(el.href).toContain('return_to=test/location');
+      });
+    });
+  });
+
+  describe('`remove subscription` buttons', () => {
+    describe('on click', () => {
+      it('calls `removeSubscription`', () => {
+        Array.from(document.querySelectorAll('.js-jira-connect-remove-subscription')).forEach(
+          (removeSubscriptionButton) => {
+            removeSubscriptionButton.dispatchEvent(new Event('click'));
+
+            waitForPromises();
+
+            expect(removeSubscription).toHaveBeenCalledWith(removeSubscriptionButton.href);
+            expect(removeSubscription).toHaveBeenCalledTimes(1);
+
+            removeSubscription.mockClear();
+          },
+        );
+      });
+    });
+  });
+});
diff --git a/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js b/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js
index cf3745f4156..3f1cf67127e 100644
--- a/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js
+++ b/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js
@@ -34,6 +34,7 @@ describe('Pipeline New Form', () => {
   const findForm = () => wrapper.find(GlForm);
   const findDropdown = () => wrapper.find(GlDropdown);
   const findDropdownItems = () => wrapper.findAll(GlDropdownItem);
+  const findSubmitButton = () => wrapper.find('[data-testid="run_pipeline_button"]');
   const findVariableRows = () => wrapper.findAll('[data-testid="ci-variable-row"]');
   const findRemoveIcons = () => wrapper.findAll('[data-testid="remove-ci-variable-row"]');
   const findKeyInputs = () => wrapper.findAll('[data-testid="pipeline-form-ci-variable-key"]');
@@ -155,6 +156,18 @@ describe('Pipeline New Form', () => {
 
       await waitForPromises();
     });
+
+    it('disables the submit button immediately after submitting', async () => {
+      createComponent();
+
+      expect(findSubmitButton().props('disabled')).toBe(false);
+
+      findForm().vm.$emit('submit', dummySubmitEvent);
+      await waitForPromises();
+
+      expect(findSubmitButton().props('disabled')).toBe(true);
+    });
+
     it('creates pipeline with full ref and variables', async () => {
       createComponent();
 
@@ -167,6 +180,7 @@ describe('Pipeline New Form', () => {
       expect(getExpectedPostParams().ref).toEqual(wrapper.vm.$data.refValue.fullName);
       expect(redirectTo).toHaveBeenCalledWith(`${pipelinesPath}/${postResponse.id}`);
     });
+
     it('creates a pipeline with short ref and variables', async () => {
       // query params are used
       createComponent('', mockParams);
@@ -312,31 +326,55 @@ describe('Pipeline New Form', () => {
   describe('Form errors and warnings', () => {
     beforeEach(() => {
       createComponent();
+    });
 
-      mock.onPost(pipelinesPath).reply(httpStatusCodes.BAD_REQUEST, mockError);
+    describe('when the error response can be handled', () => {
+      beforeEach(async () => {
+        mock.onPost(pipelinesPath).reply(httpStatusCodes.BAD_REQUEST, mockError);
 
-      findForm().vm.$emit('submit', dummySubmitEvent);
+        findForm().vm.$emit('submit', dummySubmitEvent);
 
-      return waitForPromises();
-    });
+        await waitForPromises();
+      });
 
-    it('shows both error and warning', () => {
-      expect(findErrorAlert().exists()).toBe(true);
-      expect(findWarningAlert().exists()).toBe(true);
-    });
+      it('shows both error and warning', () => {
+        expect(findErrorAlert().exists()).toBe(true);
+        expect(findWarningAlert().exists()).toBe(true);
+      });
 
-    it('shows the correct error', () => {
-      expect(findErrorAlert().text()).toBe(mockError.errors[0]);
-    });
+      it('shows the correct error', () => {
+        expect(findErrorAlert().text()).toBe(mockError.errors[0]);
+      });
 
-    it('shows the correct warning title', () => {
-      const { length } = mockError.warnings;
+      it('shows the correct warning title', () => {
+        const { length } = mockError.warnings;
+
+        expect(findWarningAlertSummary().attributes('message')).toBe(`${length} warnings found:`);
+      });
+
+      it('shows the correct amount of warnings', () => {
+        expect(findWarnings()).toHaveLength(mockError.warnings.length);
+      });
 
-      expect(findWarningAlertSummary().attributes('message')).toBe(`${length} warnings found:`);
+      it('re-enables the submit button', () => {
+        expect(findSubmitButton().props('disabled')).toBe(false);
+      });
     });
 
-    it('shows the correct amount of warnings', () => {
-      expect(findWarnings()).toHaveLength(mockError.warnings.length);
+    describe('when the error response cannot be handled', () => {
+      beforeEach(async () => {
+        mock
+          .onPost(pipelinesPath)
+          .reply(httpStatusCodes.INTERNAL_SERVER_ERROR, 'something went wrong');
+
+        findForm().vm.$emit('submit', dummySubmitEvent);
+
+        await waitForPromises();
+      });
+
+      it('re-enables the submit button', () => {
+        expect(findSubmitButton().props('disabled')).toBe(false);
+      });
     });
   });
 });
diff --git a/spec/frontend/vue_mr_widget/mr_widget_options_spec.js b/spec/frontend/vue_mr_widget/mr_widget_options_spec.js
index 8c642799fb8..872dcd2af7f 100644
--- a/spec/frontend/vue_mr_widget/mr_widget_options_spec.js
+++ b/spec/frontend/vue_mr_widget/mr_widget_options_spec.js
@@ -825,14 +825,11 @@ describe('MrWidgetOptions', () => {
 
   describe('security widget', () => {
     describe.each`
-      context                             | hasPipeline | isFlagEnabled | shouldRender
-      ${'has pipeline and flag enabled'}  | ${true}     | ${true}       | ${true}
-      ${'has pipeline and flag disabled'} | ${true}     | ${false}      | ${false}
-      ${'no pipeline and flag enabled'}   | ${false}    | ${true}       | ${false}
-    `('given $context', ({ hasPipeline, isFlagEnabled, shouldRender }) => {
+      context                  | hasPipeline | shouldRender
+      ${'there is a pipeline'} | ${true}     | ${true}
+      ${'no pipeline'}         | ${false}    | ${false}
+    `('given $context', ({ hasPipeline, shouldRender }) => {
       beforeEach(() => {
-        gon.features.coreSecurityMrWidget = isFlagEnabled;
-
         const mrData = {
           ...mockData,
           ...(hasPipeline ? {} : { pipeline: null }),
diff --git a/spec/requests/api/graphql/mutations/merge_requests/reviewer_rereview_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/reviewer_rereview_spec.rb
new file mode 100644
index 00000000000..2e4f35cbcde
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/merge_requests/reviewer_rereview_spec.rb
@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Setting assignees of a merge request' do
+  include GraphqlHelpers
+
+  let(:current_user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [user]) }
+  let(:project) { merge_request.project }
+  let(:user) { create(:user) }
+  let(:input) { { user_id: global_id_of(user) } }
+
+  let(:mutation) do
+    variables = {
+      project_path: project.full_path,
+      iid: merge_request.iid.to_s
+    }
+    graphql_mutation(:merge_request_reviewer_rereview, variables.merge(input),
+                     <<-QL.strip_heredoc
+                       clientMutationId
+                       errors
+    QL
+    )
+  end
+
+  def mutation_response
+    graphql_mutation_response(:merge_request_reviewer_rereview)
+  end
+
+  def mutation_errors
+    mutation_response['errors']
+  end
+
+  before do
+    project.add_developer(current_user)
+    project.add_developer(user)
+  end
+
+  it 'returns an error if the user is not allowed to update the merge request' do
+    post_graphql_mutation(mutation, current_user: create(:user))
+
+    expect(graphql_errors).not_to be_empty
+  end
+
+  describe 'reviewer does not exist' do
+    let(:input) { { user_id: global_id_of(create(:user)) } }
+
+    it 'returns an error' do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(mutation_errors).not_to be_empty
+    end
+  end
+
+  describe 'reviewer exists' do
+    it 'does not return an error' do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(mutation_errors).to be_empty
+    end
+  end
+end
diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb
index e04f63befd0..be4ecd0a734 100644
--- a/spec/requests/api/internal/base_spec.rb
+++ b/spec/requests/api/internal/base_spec.rb
@@ -1094,6 +1094,104 @@ RSpec.describe API::Internal::Base do
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
+
+    context 'admin mode' do
+      shared_examples 'pushes succeed for ssh and http' do
+        it 'accepts the SSH push' do
+          push(key, project)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        it 'accepts the HTTP push' do
+          push(key, project, 'http')
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      shared_examples 'pushes fail for ssh and http' do
+        it 'rejects the SSH push' do
+          push(key, project)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+
+        it 'rejects the HTTP push' do
+          push(key, project, 'http')
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+
+      context 'feature flag :user_mode_in_session is enabled' do
+        context 'with an admin user' do
+          let(:user) { create(:admin) }
+
+          context 'is member of the project' do
+            before do
+              project.add_developer(user)
+            end
+
+            it_behaves_like 'pushes succeed for ssh and http'
+          end
+
+          context 'is not member of the project' do
+            it_behaves_like 'pushes succeed for ssh and http'
+          end
+        end
+
+        context 'with a regular user' do
+          context 'is member of the project' do
+            before do
+              project.add_developer(user)
+            end
+
+            it_behaves_like 'pushes succeed for ssh and http'
+          end
+
+          context 'is not member of the project' do
+            it_behaves_like 'pushes fail for ssh and http'
+          end
+        end
+      end
+
+      context 'feature flag :user_mode_in_session is disabled' do
+        before do
+          stub_feature_flags(user_mode_in_session: false)
+        end
+
+        context 'with an admin user' do
+          let(:user) { create(:admin) }
+
+          context 'is member of the project' do
+            before do
+              project.add_developer(user)
+            end
+
+            it_behaves_like 'pushes succeed for ssh and http'
+          end
+
+          context 'is not member of the project' do
+            it_behaves_like 'pushes succeed for ssh and http'
+          end
+        end
+
+        context 'with a regular user' do
+          context 'is member of the project' do
+            before do
+              project.add_developer(user)
+            end
+
+            it_behaves_like 'pushes succeed for ssh and http'
+          end
+
+          context 'is not member of the project' do
+            it_behaves_like 'pushes fail for ssh and http'
+          end
+        end
+      end
+    end
   end
 
   describe 'POST /internal/post_receive', :clean_gitlab_redis_shared_state do
diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb
index d54f33b6a23..f2ef1508098 100644
--- a/spec/serializers/user_serializer_spec.rb
+++ b/spec/serializers/user_serializer_spec.rb
@@ -9,7 +9,7 @@ RSpec.describe UserSerializer do
   context 'serializer with merge request context' do
     let(:merge_request) { create(:merge_request) }
     let(:project) { merge_request.project }
-    let(:serializer) { described_class.new(merge_request_iid: merge_request.iid) }
+    let(:serializer) { described_class.new(current_user: user1, merge_request_iid: merge_request.iid) }
 
     before do
       allow(project).to(
diff --git a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb
new file mode 100644
index 00000000000..1075f6f9034
--- /dev/null
+++ b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::MarkReviewerReviewedService do
+  let(:current_user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [current_user]) }
+  let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) }
+  let(:project) { merge_request.project }
+  let(:service) { described_class.new(project, current_user) }
+  let(:result) { service.execute(merge_request) }
+
+  before do
+    project.add_developer(current_user)
+  end
+
+  describe '#execute' do
+    describe 'invalid permissions' do
+      let(:service) { described_class.new(project, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer does not exist' do
+      let(:service) { described_class.new(project, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer exists' do
+      it 'returns success' do
+        expect(result[:status]).to eq :success
+      end
+
+      it 'updates reviewers state' do
+        expect(result[:status]).to eq :success
+        expect(reviewer.state).to eq 'reviewed'
+      end
+    end
+  end
+end
diff --git a/spec/services/merge_requests/request_review_service_spec.rb b/spec/services/merge_requests/request_review_service_spec.rb
new file mode 100644
index 00000000000..5cb4120852a
--- /dev/null
+++ b/spec/services/merge_requests/request_review_service_spec.rb
@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::RequestReviewService do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [user]) }
+  let(:reviewer) { merge_request.find_reviewer(user) }
+  let(:project) { merge_request.project }
+  let(:service) { described_class.new(project, current_user) }
+  let(:result) { service.execute(merge_request, user) }
+  let(:todo_service) { spy('todo service') }
+  let(:notification_service) { spy('notification service') }
+
+  before do
+    allow(NotificationService).to receive(:new) { notification_service }
+    allow(service).to receive(:todo_service).and_return(todo_service)
+    allow(service).to receive(:notification_service).and_return(notification_service)
+
+    reviewer.update!(state: MergeRequestReviewer.states[:reviewed])
+
+    project.add_developer(current_user)
+    project.add_developer(user)
+  end
+
+  describe '#execute' do
+    describe 'invalid permissions' do
+      let(:service) { described_class.new(project, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer does not exist' do
+      let(:result) { service.execute(merge_request, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer exists' do
+      it 'returns success' do
+        expect(result[:status]).to eq :success
+      end
+
+      it 'updates reviewers state' do
+        service.execute(merge_request, user)
+        reviewer.reload
+
+        expect(reviewer.state).to eq 'unreviewed'
+      end
+
+      it 'sends email to reviewer' do
+        expect(notification_service).to receive_message_chain(:async, :review_requested_of_merge_request).with(merge_request, current_user, user)
+
+        service.execute(merge_request, user)
+      end
+
+      it 'creates a new todo for the reviewer' do
+        expect(todo_service).to receive(:create_request_review_todo).with(merge_request, current_user, user)
+
+        service.execute(merge_request, user)
+      end
+    end
+  end
+end
diff --git a/spec/services/notification_recipients/build_service_spec.rb b/spec/services/notification_recipients/build_service_spec.rb
index cc08f9fceff..ff54d6ccd2f 100644
--- a/spec/services/notification_recipients/build_service_spec.rb
+++ b/spec/services/notification_recipients/build_service_spec.rb
@@ -110,4 +110,28 @@ RSpec.describe NotificationRecipients::BuildService do
       end
     end
   end
+
+  describe '#build_requested_review_recipients' do
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+    before do
+      merge_request.reviewers.push(assignee)
+    end
+
+    shared_examples 'no N+1 queries' do
+      it 'avoids N+1 queries', :request_store do
+        create_user
+
+        service.build_requested_review_recipients(note)
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          service.build_requested_review_recipients(note)
+        end
+
+        create_user
+
+        expect { service.build_requested_review_recipients(note) }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
 end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 85234077b1f..b67c37ba02d 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -2177,6 +2177,46 @@ RSpec.describe NotificationService, :mailer do
         let(:notification_trigger) { notification.merge_when_pipeline_succeeds(merge_request, @u_disabled) }
       end
     end
+
+    describe '#review_requested_of_merge_request' do
+      let(:merge_request) { create(:merge_request, author: author, source_project: project, reviewers: [reviewer]) }
+
+      let_it_be(:current_user) { create(:user) }
+      let_it_be(:reviewer) { create(:user) }
+
+      it 'sends email to reviewer', :aggregate_failures do
+        notification.review_requested_of_merge_request(merge_request, current_user, reviewer)
+
+        merge_request.reviewers.each { |reviewer| should_email(reviewer) }
+        should_not_email(merge_request.author)
+        should_not_email(@u_watcher)
+        should_not_email(@u_participant_mentioned)
+        should_not_email(@subscriber)
+        should_not_email(@watcher_and_subscriber)
+        should_not_email(@u_guest_watcher)
+        should_not_email(@u_guest_custom)
+        should_not_email(@u_custom_global)
+        should_not_email(@unsubscriber)
+        should_not_email(@u_participating)
+        should_not_email(@u_disabled)
+        should_not_email(@u_lazy_participant)
+      end
+
+      it 'adds "review requested" reason for new reviewer' do
+        notification.review_requested_of_merge_request(merge_request, current_user, [reviewer])
+
+        merge_request.reviewers.each do |reviewer|
+          email = find_email_for(reviewer)
+
+          expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::REVIEW_REQUESTED)
+        end
+      end
+
+      it_behaves_like 'project emails are disabled' do
+        let(:notification_target)  { merge_request }
+        let(:notification_trigger) { notification.review_requested_of_merge_request(merge_request, current_user, reviewer) }
+      end
+    end
   end
 
   describe 'Projects', :deliver_mails_inline do
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 83d233a8112..743dc080b06 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -1193,6 +1193,17 @@ RSpec.describe TodoService do
     end
   end
 
+  describe '#create_request_review_todo' do
+    let(:target) { create(:merge_request, author: author, source_project: project) }
+    let(:reviewer) { create(:user) }
+
+    it 'creates a todo for reviewer' do
+      service.create_request_review_todo(target, author, reviewer)
+
+      should_create_todo(user: reviewer, target: target, action: Todo::REVIEW_REQUESTED)
+    end
+  end
+
   def should_create_todo(attributes = {})
     attributes.reverse_merge!(
       project: project,
diff --git a/spec/views/layouts/_head.html.haml_spec.rb b/spec/views/layouts/_head.html.haml_spec.rb
index 15fdfaaaa65..0b06309deff 100644
--- a/spec/views/layouts/_head.html.haml_spec.rb
+++ b/spec/views/layouts/_head.html.haml_spec.rb
@@ -102,6 +102,44 @@ RSpec.describe 'layouts/_head' do
       expect(rendered).to match(/<script.*>.*var u="\/\/#{matomo_host}\/".*<\/script>/m)
       expect(rendered).to match(%r(<noscript>.*<img src="//#{matomo_host}/matomo.php.*</noscript>))
     end
+
+    context 'matomo_disable_cookies' do
+      context 'when true' do
+        before do
+          stub_config(extra: { matomo_url: matomo_host, matomo_site_id: 12345, matomo_disable_cookies: true })
+        end
+
+        it 'disables cookies' do
+          render
+
+          expect(rendered).to include('_paq.push(["disableCookies"])')
+        end
+      end
+
+      context 'when false' do
+        before do
+          stub_config(extra: { matomo_url: matomo_host, matomo_site_id: 12345, matomo_disable_cookies: false })
+        end
+
+        it 'does not disable cookies' do
+          render
+
+          expect(rendered).not_to include('_paq.push(["disableCookies"])')
+        end
+      end
+
+      context 'when absent' do
+        before do
+          stub_config(extra: { matomo_url: matomo_host, matomo_site_id: 12345 })
+        end
+
+        it 'does not disable cookies' do
+          render
+
+          expect(rendered).not_to include('_paq.push(["disableCookies"])')
+        end
+      end
+    end
   end
 
   def stub_helper_with_safe_string(method)