Merge branch '6760-fix-unsaved-approver-cancel-prompt' into 'master'

Fix remove approver prompt cancel

Closes #6760

See merge request gitlab-org/gitlab-ee!8178

ee/app/assets/javascripts/approvals.js

@@ -3,42 +3,46 @@ import _ from 'underscore';
 
 export default () => {
   $('.approver-list').on(
-    'click',
+    'confirm:complete',
     '.unsaved-approvers.approver .btn-remove',
-    function approverListClickCallback(ev) {
-      const removeElement = $(this).closest('li');
-      const approverId = parseInt(removeElement.attr('id').replace('user_', ''), 10);
-      const approverIds = $('input#merge_request_approver_ids');
-      const skipUsers = approverIds.data('skipUsers') || [];
-      const approverIndex = skipUsers.indexOf(approverId);
+    function approverListClickCallback(ev, answer) {
+      if (answer) {
+        const removeElement = $(this).closest('li');
+        const approverId = parseInt(removeElement.attr('id').replace('user_', ''), 10);
+        const approverIds = $('input#merge_request_approver_ids');
+        const skipUsers = approverIds.data('skipUsers') || [];
+        const approverIndex = skipUsers.indexOf(approverId);
 
-      removeElement.remove();
+        removeElement.remove();
 
-      if (approverIndex > -1) {
-        approverIds.data('skipUsers', skipUsers.splice(approverIndex, 1));
+        if (approverIndex > -1) {
+          approverIds.data('skipUsers', skipUsers.splice(approverIndex, 1));
+        }
       }
 
-      ev.preventDefault();
+      return false;
     },
   );
 
   $('.approver-list').on(
-    'click',
+    'confirm:complete',
     '.unsaved-approvers.approver-group .btn-remove',
-    function approverListRemoveClickCallback(ev) {
-      const removeElement = $(this).closest('li');
-      const approverGroupId = parseInt(removeElement.attr('id').replace('group_', ''), 10);
-      const approverGroupIds = $('input#merge_request_approver_group_ids');
-      const skipGroups = approverGroupIds.data('skipGroups') || [];
-      const approverGroupIndex = skipGroups.indexOf(approverGroupId);
+    function approverListRemoveClickCallback(ev, answer) {
+      if (answer) {
+        const removeElement = $(this).closest('li');
+        const approverGroupId = parseInt(removeElement.attr('id').replace('group_', ''), 10);
+        const approverGroupIds = $('input#merge_request_approver_group_ids');
+        const skipGroups = approverGroupIds.data('skipGroups') || [];
+        const approverGroupIndex = skipGroups.indexOf(approverGroupId);
 
-      removeElement.remove();
+        removeElement.remove();
 
-      if (approverGroupIndex > -1) {
-        approverGroupIds.data('skipGroups', skipGroups.splice(approverGroupIndex, 1));
+        if (approverGroupIndex > -1) {
+          approverGroupIds.data('skipGroups', skipGroups.splice(approverGroupIndex, 1));
+        }
       }
 
-      ev.preventDefault();
+      return false;
     },
   );
 

ee/changelogs/unreleased/6760-fix-unsaved-approver-cancel-prompt.yml

+---
+title: Fix approver removal still being conducted even when "Cancel" is clicked in confirmation prompt
+merge_request: 8178
+author:
+type: fixed

ee/spec/javascripts/approvals/approvals_spec.js

+import $ from 'jquery';
+import approvals from 'ee/approvals';
+
+describe('Approvals', () => {
+  preloadFixtures('merge_requests_ee/merge_request_edit.html.raw');
+
+  let $approversEl;
+  let $suggestionEl;
+
+  beforeEach(() => {
+    loadFixtures('merge_requests_ee/merge_request_edit.html.raw');
+    $approversEl = $('ul.approver-list');
+    $suggestionEl = $('.suggested-approvers');
+    approvals();
+  });
+
+  describe('add suggested approver', () => {
+    it('should add approver when suggested user is clicked', () => {
+      expect($approversEl.find('li.approver').length).toEqual(0);
+
+      $suggestionEl
+        .find('a')
+        .first()
+        .click();
+
+      $suggestionEl
+        .find('a')
+        .last()
+        .click();
+
+      expect($approversEl.find('li.approver').length).toEqual(2);
+    });
+
+    it('only adds approver once when the same suggested user is clicked multiple times', () => {
+      expect($approversEl.find('li.approver').length).toEqual(0);
+
+      $suggestionEl
+        .find('a')
+        .first()
+        .click();
+
+      $suggestionEl
+        .find('a')
+        .first()
+        .click();
+
+      expect($approversEl.find('li.approver').length).toEqual(1);
+    });
+  });
+
+  describe('remove unsaved approver', () => {
+    beforeEach(() => {
+      $suggestionEl.find('a').click(); // Adds two approvers
+    });
+
+    it('should remove approver if confirm window result is positive', () => {
+      spyOn(window, 'confirm').and.returnValue(true);
+
+      $approversEl
+        .find('.unsaved-approvers.approver a.btn-remove')
+        .first()
+        .click();
+
+      expect($approversEl.find('li.approver').length).toEqual(1);
+    });
+
+    it('should not remove approver if confirm window result is negative', () => {
+      spyOn(window, 'confirm').and.returnValue(false);
+
+      $approversEl
+        .find('.unsaved-approvers.approver a.btn-remove')
+        .first()
+        .click();
+
+      expect($approversEl.find('li.approver').length).toEqual(2);
+    });
+  });
+
+  describe('remove unsaved approver group', () => {
+    it('should remove approver group if confirm window result is positive', () => {
+      spyOn(window, 'confirm').and.returnValue(true);
+
+      expect($approversEl.find('li.approver-group').length).toEqual(1);
+
+      $approversEl
+        .find('.unsaved-approvers.approver-group a.btn-remove')
+        .first()
+        .click();
+
+      expect($approversEl.find('li.approver-group').length).toEqual(0);
+    });
+
+    it('should not remove approver group if confirm window result is negative', () => {
+      spyOn(window, 'confirm').and.returnValue(false);
+
+      expect($approversEl.find('li.approver-group').length).toEqual(1);
+
+      $approversEl
+        .find('.unsaved-approvers.approver-group a.btn-remove')
+        .first()
+        .click();
+
+      expect($approversEl.find('li.approver-group').length).toEqual(1);
+    });
+  });
+});

ee/spec/javascripts/fixtures/merge_requests.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :controller do
+  include JavaScriptFixturesHelpers
+
+  let(:admin) { create(:admin) }
+  let(:namespace) { create(:namespace, name: 'frontend-fixtures' )}
+  let(:project) { create(:project, :repository, namespace: namespace, path: 'merge-requests-project') }
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: admin, approvals_before_merge: 2) }
+  let(:suggested_approvers) { create_list(:user, 3) }
+
+  render_views
+
+  before(:all) do
+    clean_frontend_fixtures('merge_requests_ee/')
+  end
+
+  before do
+    # Ensure some approver suggestions are displayed
+    service = double(:service)
+    expect(::Gitlab::AuthorityAnalyzer).to receive(:new).and_return(service)
+    expect(service).to receive(:calculate).and_return(suggested_approvers)
+
+    # Ensure a project level group is present (but unsaved)
+    approver_group = create(:approver_group, target: project)
+    approver_group.group.add_owner(create(:owner))
+
+    sign_in(admin)
+  end
+
+  after do
+    remove_repository(project)
+  end
+
+  it 'merge_requests_ee/merge_request_edit.html.raw' do |example|
+    get :edit,
+      id: merge_request.id,
+      namespace_id: project.namespace.to_param,
+      project_id: project,
+      format: :html
+
+    expect(merge_request.all_approvers_including_groups.size).to eq(1)
+    expect(response).to be_success
+    store_frontend_fixture(response, example.description)
+  end
+end