Merge branch 'ph/212754/resetApprovalsToProjectDefaults' into 'master'

Allow approval rules to be reset to project defaults

See merge request gitlab-org/gitlab!32657

ee/app/assets/javascripts/approvals/components/app.vue

 <script>
 import { mapState, mapActions } from 'vuex';
-import { GlLoadingIcon, GlDeprecatedButton } from '@gitlab/ui';
+import { GlLoadingIcon, GlButton } from '@gitlab/ui';
+import showToast from '~/vue_shared/plugins/global_toast';
+import { __ } from '~/locale';
 import ModalRuleCreate from './modal_rule_create.vue';
 import ModalRuleRemove from './modal_rule_remove.vue';
 
@@ -8,7 +10,7 @@ export default {
   components: {
     ModalRuleCreate,
     ModalRuleRemove,
-    GlDeprecatedButton,
+    GlButton,
     GlLoadingIcon,
   },
   props: {
@@ -21,7 +23,9 @@ export default {
   computed: {
     ...mapState({
       settings: 'settings',
+      isLoading: state => state.approvals.isLoading,
       hasLoaded: state => state.approvals.hasLoaded,
+      targetBranch: state => state.approvals.targetBranch,
     }),
     createModalId() {
       return `${this.settings.prefix}-approvals-create-modal`;
@@ -29,19 +33,28 @@ export default {
     removeModalId() {
       return `${this.settings.prefix}-approvals-remove-modal`;
     },
-    targetBranch() {
-      if (this.settings.prefix === 'mr-edit' && !this.settings.mrSettingsPath) {
-        return this.settings.mrCreateTargetBranch;
-      }
-      return null;
-    },
   },
   mounted() {
-    return this.fetchRules(this.targetBranch);
+    return this.fetchRules({ targetBranch: this.targetBranch });
   },
   methods: {
-    ...mapActions(['fetchRules']),
+    ...mapActions(['fetchRules', 'undoRulesChange']),
     ...mapActions({ openCreateModal: 'createModal/open' }),
+    resetToProjectDefaults() {
+      const { targetBranch } = this;
+
+      return this.fetchRules({ targetBranch, resetToDefault: true }).then(() => {
+        const toast = showToast(__('Approval rules reset to project defaults'), {
+          action: {
+            text: __('Undo'),
+            onClick: () => {
+              this.undoRulesChange();
+              toast.goAway(0);
+            },
+          },
+        });
+      });
+    },
   },
 };
 </script>
@@ -54,14 +67,27 @@ export default {
         <slot name="rules"></slot>
       </div>
       <div v-if="settings.canEdit && settings.allowMultiRule" class="border-bottom py-3 px-2">
-        <div v-if="settings.allowMultiRule" class="d-flex">
-          <gl-deprecated-button
-            class="ml-auto btn-info btn-inverted"
+        <div class="d-flex">
+          <gl-button
+            v-if="targetBranch"
+            :disabled="isLoading"
+            class="gl-ml-auto"
+            data-testid="reset-to-defaults"
+            @click="resetToProjectDefaults"
+          >
+            {{ __('Reset to project defaults') }}
+          </gl-button>
+          <gl-button
+            :class="{ 'gl-ml-3': targetBranch, 'gl-ml-auto': !targetBranch }"
+            :disabled="isLoading"
+            category="secondary"
+            variant="info"
             data-qa-selector="add_approvers_button"
+            data-testid="add-approval-rule"
             @click="openCreateModal(null)"
           >
             {{ __('Add approval rule') }}
-          </gl-deprecated-button>
+          </gl-button>
         </div>
       </div>
       <slot name="footer"></slot>

ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules.vue

@@ -18,15 +18,11 @@ export default {
     EmptyRule,
     RuleInput,
   },
-  data() {
-    return {
-      targetBranch: '',
-    };
-  },
   computed: {
     ...mapState(['settings']),
     ...mapState({
       rules: state => state.approvals.rules,
+      targetBranch: state => state.approvals.targetBranch,
     }),
     hasNamedRule() {
       if (this.settings.allowMultiRule) {
@@ -60,14 +56,18 @@ export default {
       },
       immediate: true,
     },
+    targetBranch() {
+      this.fetchRules({ targetBranch: this.targetBranch });
+    },
   },
   mounted() {
     if (this.isEditPath) {
       this.mergeRequestTargetBranchElement = document.querySelector('#merge_request_target_branch');
+      const targetBranch = this.mergeRequestTargetBranchElement?.value;
 
-      this.targetBranch = this.mergeRequestTargetBranchElement?.value;
+      this.setTargetBranch(targetBranch);
 
-      if (this.targetBranch) {
+      if (targetBranch) {
         targetBranchMutationObserver = new MutationObserver(this.onTargetBranchMutation);
         targetBranchMutationObserver.observe(this.mergeRequestTargetBranchElement, {
           attributes: true,
@@ -85,13 +85,12 @@ export default {
     }
   },
   methods: {
-    ...mapActions(['setEmptyRule', 'addEmptyRule', 'fetchRules']),
+    ...mapActions(['setEmptyRule', 'addEmptyRule', 'fetchRules', 'setTargetBranch']),
     onTargetBranchMutation() {
       const selectedTargetBranchValue = this.mergeRequestTargetBranchElement.value;
 
       if (this.targetBranch !== selectedTargetBranchValue) {
-        this.targetBranch = selectedTargetBranchValue;
-        this.fetchRules(this.targetBranch);
+        this.setTargetBranch(selectedTargetBranchValue);
       }
     },
     indicatorText(rule) {

ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue

@@ -11,11 +11,13 @@ const INPUT_DELETE = 'merge_request[approval_rules_attributes][][_destroy]';
 const INPUT_REMOVE_HIDDEN_GROUPS =
   'merge_request[approval_rules_attributes][][remove_hidden_groups]';
 const INPUT_FALLBACK_APPROVALS_REQUIRED = 'merge_request[approvals_before_merge]';
+const INPUT_RESET_TO_DEFAULTS_ID = 'merge_request[reset_approval_rules_to_defaults]';
 
 export default {
   computed: {
     ...mapState(['settings']),
     ...mapState({
+      resetToDefault: state => state.approvals.resetToDefault,
       rules: state => state.approvals.rules,
       rulesToDelete: state => state.approvals.rulesToDelete,
       fallbackApprovalsRequired: state => state.approvals.fallbackApprovalsRequired,
@@ -30,11 +32,18 @@ export default {
   INPUT_DELETE,
   INPUT_REMOVE_HIDDEN_GROUPS,
   INPUT_FALLBACK_APPROVALS_REQUIRED,
+  INPUT_RESET_TO_DEFAULTS_ID,
 };
 </script>
 
 <template>
   <div v-if="settings.canEdit">
+    <input
+      v-if="resetToDefault"
+      type="hidden"
+      value="true"
+      :name="$options.INPUT_RESET_TO_DEFAULTS_ID"
+    />
     <div v-for="id in rulesToDelete" :key="id">
       <input :value="id" :name="$options.INPUT_ID" type="hidden" />
       <input :value="1" :name="$options.INPUT_DELETE" type="hidden" />

ee/app/assets/javascripts/approvals/mount_mr_edit.js

@@ -12,18 +12,19 @@ export default function mountApprovalInput(el) {
     return null;
   }
 
-  const mrCreateTargetBranch = el.dataset.mrSettingsPath
-    ? null
-    : document.querySelector('#js-target-branch-title')?.textContent;
+  const targetBranch =
+    document.querySelector('#js-target-branch-title')?.textContent ||
+    document.querySelector('#merge_request_target_branch')?.value;
 
   const store = createStore(mrEditModule(), {
     ...el.dataset,
     prefix: 'mr-edit',
     canEdit: parseBoolean(el.dataset.canEdit),
     allowMultiRule: parseBoolean(el.dataset.allowMultiRule),
-    mrCreateTargetBranch,
   });
 
+  store.dispatch('setTargetBranch', targetBranch);
+
   return new Vue({
     el,
     store,

ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js

 export const SET_LOADING = 'SET_LOADING';
 export const SET_APPROVAL_SETTINGS = 'SET_APPROVAL_SETTINGS';
 export const ADD_EMPTY_RULE = 'ADD_EMPTY_RULE';
+export const SET_RESET_TO_DEFAULT = 'SET_RESET_TO_DEFAULT';
+export const UNDO_RULES = 'UNDO_RULES';

ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js

@@ -8,7 +8,7 @@ export default {
   [types.SET_APPROVAL_SETTINGS](state, settings) {
     state.hasLoaded = true;
     state.rules = settings.rules;
-    state.initialRules = settings.rules;
+    state.initialRules = [...settings.rules];
     state.fallbackApprovalsRequired = settings.fallbackApprovalsRequired;
     state.minFallbackApprovalsRequired = settings.minFallbackApprovalsRequired;
   },
@@ -26,4 +26,12 @@ export default {
       isNew: true,
     });
   },
+  [types.SET_RESET_TO_DEFAULT](state, resetToDefault) {
+    state.resetToDefault = resetToDefault;
+    state.oldRules = [...state.rules];
+  },
+  [types.UNDO_RULES](state) {
+    state.resetToDefault = false;
+    state.rules = [...state.oldRules];
+  },
 };

ee/app/assets/javascripts/approvals/stores/modules/base/state.js

@@ -5,4 +5,6 @@ export default () => ({
   fallbackApprovalsRequired: 0,
   minFallbackApprovalsRequired: 0,
   initialRules: [],
+  oldRules: [],
+  resetToDefault: false,
 });

ee/app/assets/javascripts/approvals/stores/modules/mr_edit/actions.js

@@ -52,8 +52,9 @@ export const requestRules = ({ commit }) => {
   commit(types.SET_LOADING, true);
 };
 
-export const receiveRulesSuccess = ({ commit }, settings) => {
+export const receiveRulesSuccess = ({ commit }, { resetToDefault, settings }) => {
   commit(types.SET_LOADING, false);
+  commit(types.SET_RESET_TO_DEFAULT, resetToDefault);
   commit(types.SET_APPROVAL_SETTINGS, settings);
 };
 
@@ -61,11 +62,14 @@ export const receiveRulesError = () => {
   createFlash(__('An error occurred fetching the approval rules.'));
 };
 
-export const fetchRules = ({ rootState, dispatch }, targetBranch = '') => {
+export const fetchRules = (
+  { rootState, dispatch },
+  { targetBranch = '', resetToDefault = false },
+) => {
   dispatch('requestRules');
 
   const { mrSettingsPath, projectSettingsPath } = rootState.settings;
-  const path = mrSettingsPath || projectSettingsPath;
+  const path = resetToDefault ? projectSettingsPath : mrSettingsPath || projectSettingsPath;
 
   const params = targetBranch
     ? {
@@ -82,7 +86,7 @@ export const fetchRules = ({ rootState, dispatch }, targetBranch = '') => {
       ...settings,
       rules: settings.rules.map(x => (x.id ? x : seedNewRule(x))),
     }))
-    .then(settings => dispatch('receiveRulesSuccess', settings))
+    .then(settings => dispatch('receiveRulesSuccess', { settings, resetToDefault }))
     .catch(() => dispatch('receiveRulesError'));
 };
 
@@ -148,4 +152,9 @@ export const addEmptyRule = ({ commit }) => {
   commit(types.ADD_EMPTY_RULE);
 };
 
+export const setTargetBranch = ({ commit }, targetBranch) =>
+  commit(types.SET_TARGET_BRANCH, targetBranch);
+
+export const undoRulesChange = ({ commit }) => commit(types.UNDO_RULES);
+
 export default () => {};

ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutation_types.js

@@ -7,3 +7,4 @@ export const SET_FALLBACK_RULE = 'SET_FALLBACK_RULE';
 export const POST_REGULAR_RULE = 'POST_REGULAR_RULE';
 export const DELETE_ANY_RULE = 'DELETE_ANY_RULE';
 export const SET_EMPTY_RULE = 'SET_EMPTY_RULE';
+export const SET_TARGET_BRANCH = 'SET_TARGET_BRANCH';

ee/app/assets/javascripts/approvals/stores/modules/mr_edit/mutations.js

@@ -85,4 +85,7 @@ export default {
       ];
     }
   },
+  [types.SET_TARGET_BRANCH](state, targetBranch) {
+    state.targetBranch = targetBranch;
+  },
 };

ee/app/assets/javascripts/approvals/stores/modules/mr_edit/state.js

@@ -3,4 +3,5 @@ import baseState from '../base/state';
 export default () => ({
   ...baseState(),
   rulesToDelete: [],
+  targetBranch: '',
 });

ee/changelogs/unreleased/ph-212754-resetApprovalsToProjectDefaults.yml

+---
+title: Allow approval rules to be reset to project defaults
+merge_request: 32657
+author:
+type: added

ee/spec/features/merge_requests/user_resets_approvers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Merge Requests > User resets approvers', :js do
+  include FeatureApprovalHelper
+
+  let(:project) { create(:project, :repository) }
+  let(:user) { project.creator }
+  let(:first_user) { create(:user) }
+  let(:project_approvers) { create_list(:user, 3) }
+  let(:merge_request) do
+    create(:merge_request, approval_users: [first_user], title: 'Bugfix1', source_project: project)
+  end
+  let!(:rule) { create(:approval_project_rule, project: project, users: project_approvers, approvals_required: 1 )}
+
+  before do
+    stub_licensed_features(multiple_approval_rules: true)
+
+    project_approvers.each do |approver|
+      project.add_developer(approver)
+    end
+
+    merge_request.approvals.create!(user: first_user)
+
+    project.add_developer(user)
+    sign_in(user)
+    visit edit_project_merge_request_path(project, merge_request)
+
+    wait_for_requests
+  end
+
+  it 'resets approvers for merge requests' do
+    expect_avatar(find('.js-members'), first_user)
+
+    click_button 'Reset to project defaults'
+
+    wait_for_requests
+
+    expect_avatar(find('.js-members'), project_approvers)
+
+    click_button 'Save changes'
+
+    wait_for_requests
+
+    expect(page).to have_content 'Requires approval'
+  end
+end

ee/spec/frontend/approvals/components/app_spec.js

 import { shallowMount, createLocalVue } from '@vue/test-utils';
 import Vuex from 'vuex';
-import { GlLoadingIcon, GlDeprecatedButton } from '@gitlab/ui';
+import { GlLoadingIcon } from '@gitlab/ui';
+import showToast from '~/vue_shared/plugins/global_toast';
 import App from 'ee/approvals/components/app.vue';
 import ModalRuleCreate from 'ee/approvals/components/modal_rule_create.vue';
 import ModalRuleRemove from 'ee/approvals/components/modal_rule_remove.vue';
 import { createStoreOptions } from 'ee/approvals/stores';
 import settingsModule from 'ee/approvals/stores/modules/project_settings';
 
+jest.mock('~/vue_shared/plugins/global_toast');
+
 const localVue = createLocalVue();
 localVue.use(Vuex);
 
@@ -18,6 +21,7 @@ describe('EE Approvals App', () => {
   let wrapper;
   let slots;
 
+  const targetBranchName = 'development';
   const factory = () => {
     wrapper = shallowMount(localVue.extend(App), {
       localVue,
@@ -25,7 +29,8 @@ describe('EE Approvals App', () => {
       store: new Vuex.Store(store),
     });
   };
-  const findAddButton = () => wrapper.find(GlDeprecatedButton);
+  const findAddButton = () => wrapper.find('[data-testid="add-approval-rule"]');
+  const findResetButton = () => wrapper.find('[data-testid="reset-to-defaults"]');
   const findLoadingIcon = () => wrapper.find(GlLoadingIcon);
   const findRules = () => wrapper.find(`.${TEST_RULES_CLASS}`);
 
@@ -40,20 +45,16 @@ describe('EE Approvals App', () => {
     });
 
     store.modules.approvals.actions = {
-      fetchRules: jest.fn(),
+      fetchRules: jest.fn().mockResolvedValue(),
     };
 
+    store.modules.approvals.state.targetBranch = targetBranchName;
+
     jest.spyOn(store.modules.approvals.actions, 'fetchRules');
     jest.spyOn(store.modules.createModal.actions, 'open');
   });
 
   describe('targetBranch', () => {
-    const targetBranchName = 'development';
-
-    beforeEach(() => {
-      store.state.settings.mrCreateTargetBranch = targetBranchName;
-    });
-
     it('passes the target branch name in fetchRules for MR create path', () => {
       store.state.settings.prefix = 'mr-edit';
       store.state.settings.mrSettingsPath = null;
@@ -61,30 +62,31 @@ describe('EE Approvals App', () => {
 
       expect(store.modules.approvals.actions.fetchRules).toHaveBeenCalledWith(
         expect.anything(),
-        targetBranchName,
+        { targetBranch: targetBranchName },
         undefined,
       );
     });
 
-    it('does not pass the target branch name in fetchRules for MR edit path', () => {
+    it('passes the target branch name in fetchRules for MR edit path', () => {
       store.state.settings.prefix = 'mr-edit';
       store.state.settings.mrSettingsPath = 'some/path';
       factory();
 
       expect(store.modules.approvals.actions.fetchRules).toHaveBeenCalledWith(
         expect.anything(),
-        null,
+        { targetBranch: targetBranchName },
         undefined,
       );
     });
 
     it('does not pass the target branch name in fetchRules for project settings path', () => {
       store.state.settings.prefix = 'project-settings';
+      store.modules.approvals.state.targetBranch = null;
       factory();
 
       expect(store.modules.approvals.actions.fetchRules).toHaveBeenCalledWith(
         expect.anything(),
-        null,
+        { targetBranch: null },
         undefined,
       );
     });
@@ -203,4 +205,36 @@ describe('EE Approvals App', () => {
       expect(findAddButton().exists()).toBe(false);
     });
   });
+
+  describe('when resetting to project defaults', () => {
+    const targetBranch = 'development';
+
+    beforeEach(() => {
+      store.state.settings.targetBranch = targetBranch;
+      store.state.settings.prefix = 'mr-edit';
+      store.state.settings.allowMultiRule = true;
+      store.modules.approvals.state.hasLoaded = true;
+      store.modules.approvals.state.rules = [{ id: 1 }];
+    });
+
+    it('calls fetchRules to reset to defaults', () => {
+      factory();
+
+      findResetButton().vm.$emit('click');
+
+      return wrapper.vm.$nextTick().then(() => {
+        expect(store.modules.approvals.actions.fetchRules).toHaveBeenLastCalledWith(
+          expect.anything(),
+          { targetBranch, resetToDefault: true },
+          undefined,
+        );
+        expect(showToast).toHaveBeenCalledWith('Approval rules reset to project defaults', {
+          action: {
+            text: 'Undo',
+            onClick: expect.anything(),
+          },
+        });
+      });
+    });
+  });
 });

ee/spec/frontend/approvals/components/mr_edit/mr_rules_spec.js

@@ -40,12 +40,6 @@ describe('EE Approvals MRRules', () => {
       .find(UserAvatarList)
       .props('items');
   const findRuleControls = () => wrapper.find('td.js-controls').find(RuleControls);
-  const setTargetBranchInputValue = () => {
-    const value = 'new value';
-    const element = document.querySelector('#merge_request_target_branch');
-    element.value = value;
-    return value;
-  };
   const callTargetBranchHandler = MutationObserverSpy => {
     const onTargetBranchMutationHandler = MutationObserverSpy.mock.calls[0][0];
     return onTargetBranchMutationHandler();
@@ -61,6 +55,7 @@ describe('EE Approvals MRRules', () => {
     store.modules.approvals.state = {
       hasLoaded: true,
       rules: [],
+      targetBranch: 'master',
     };
     store.modules.approvals.actions.putRule = jest.fn();
   });
@@ -87,6 +82,7 @@ describe('EE Approvals MRRules', () => {
         fetchRules: jest.fn(),
         addEmptyRule: jest.fn(),
         setEmptyRule: jest.fn(),
+        setTargetBranch: jest.fn(),
       };
       store.state.settings.mrSettingsPath = 'some/path';
       store.state.settings.eligibleApproversDocsPath = 'some/path';
@@ -97,24 +93,19 @@ describe('EE Approvals MRRules', () => {
       targetBranchInputElement.parentNode.removeChild(targetBranchInputElement);
     });
 
-    it('sets the target branch data to be the same value as the target branch dropdown', () => {
-      factory();
-
-      expect(wrapper.vm.targetBranch).toBe(initialTargetBranch);
-    });
-
     it('updates the target branch data when the target branch dropdown is changed', () => {
       factory();
-      const newValue = setTargetBranchInputValue();
       callTargetBranchHandler(global.MutationObserver);
-      expect(wrapper.vm.targetBranch).toBe(newValue);
+      expect(store.modules.approvals.actions.setTargetBranch).toHaveBeenCalled();
     });
 
     it('re-fetches rules when target branch has changed', () => {
       factory();
-      setTargetBranchInputValue();
-      callTargetBranchHandler(global.MutationObserver);
-      expect(store.modules.approvals.actions.fetchRules).toHaveBeenCalled();
+      store.modules.approvals.state.targetBranch = 'master123';
+
+      return wrapper.vm.$nextTick().then(() => {
+        expect(store.modules.approvals.actions.fetchRules).toHaveBeenCalled();
+      });
     });
 
     it('disconnects MutationObserver when component gets destroyed', () => {

ee/spec/frontend/approvals/stores/modules/base/mutations_spec.js

@@ -35,4 +35,28 @@ describe('EE approvals base module mutations', () => {
       expect(state).toEqual(expect.objectContaining(settings));
     });
   });
+
+  describe(types.SET_RESET_TO_DEFAULT, () => {
+    it('resets rules', () => {
+      state.rules = ['test'];
+
+      mutations[types.SET_RESET_TO_DEFAULT](state, true);
+
+      expect(state.resetToDefault).toBe(true);
+      expect(state.oldRules).toEqual(['test']);
+    });
+  });
+
+  describe(types.UNDO_RULES, () => {
+    it('undos rules', () => {
+      const oldRules = ['old'];
+      state.rules = ['new'];
+      state.oldRules = oldRules;
+
+      mutations[types.UNDO_RULES](state, true);
+
+      expect(state.resetToDefault).toBe(false);
+      expect(state.rules).toEqual(oldRules);
+    });
+  });
 });

ee/spec/frontend/approvals/stores/modules/mr_edit/actions_spec.js

+import testAction from 'helpers/vuex_action_helper';
+import * as types from 'ee/approvals/stores/modules/mr_edit/mutation_types';
+import * as actions from 'ee/approvals/stores/modules/mr_edit/actions';
+
+describe('Approval MR edit module actions', () => {
+  describe('setTargetBranch', () => {
+    it('commits SET_TARGET_BRANCH', done => {
+      testAction(
+        actions.setTargetBranch,
+        'master',
+        {},
+        [{ type: types.SET_TARGET_BRANCH, payload: 'master' }],
+        [],
+        done,
+      );
+    });
+  });
+
+  describe('undoRulesChange', () => {
+    it('commits UNDO_RULES', done => {
+      testAction(actions.undoRulesChange, null, {}, [{ type: types.UNDO_RULES }], [], done);
+    });
+  });
+});

locale/gitlab.pot

@@ -2671,6 +2671,9 @@ msgstr ""
 msgid "Approval rules"
 msgstr ""
 
+msgid "Approval rules reset to project defaults"
+msgstr ""
+
 msgid "ApprovalRuleRemove|%d member"
 msgid_plural "ApprovalRuleRemove|%d members"
 msgstr[0] ""
@@ -18962,6 +18965,9 @@ msgstr ""
 msgid "Reset template"
 msgstr ""
 
+msgid "Reset to project defaults"
+msgstr ""
+
 msgid "Resetting the authorization key for this project will require updating the authorization key in every alert source it is enabled in."
 msgstr ""