Merge branch 'fix-group-mr-approvals-vuex-usage' into 'master'

Updated approval settings to use local form data to stop mutating outside the store

See merge request gitlab-org/gitlab!59887

app/assets/javascripts/vuex_shared/bindings.js

@@ -6,7 +6,7 @@
  * @param {string} list[].getter - the name of the getter, leave it empty to not use a getter
  * @param {string} list[].updateFn - the name of the action, leave it empty to use the default action
  * @param {string} defaultUpdateFn - the default function to dispatch
- * @param {string} root - the key of the state where to search fo they keys described in list
+ * @param {string|function} root - the key of the state where to search for the keys described in list
  * @returns {Object} a dictionary with all the computed properties generated
  */
 export const mapComputed = (list, defaultUpdateFn, root) => {
@@ -21,6 +21,10 @@ export const mapComputed = (list, defaultUpdateFn, root) => {
         if (getter) {
           return this.$store.getters[getter];
         } else if (root) {
+          if (typeof root === 'function') {
+            return root(this.$store.state)[key];
+          }
+
           return this.$store.state[root][key];
         }
         return this.$store.state[key];

doc/development/fe_guide/vuex.md

@@ -540,11 +540,11 @@ export default {
     foo: ''
   },
   actions: {
-    updateBar() {...}
-    updateAll() {...}
+    updateBar() {...},
+    updateAll() {...},
   },
   getters: {
-    getFoo() {...}
+    getFoo() {...},
   }
 }
 ```
@@ -559,13 +559,13 @@ export default {
      * @param {string} list[].getter - the name of the getter, leave it empty to not use a getter
      * @param {string} list[].updateFn - the name of the action, leave it empty to use the default action
      * @param {string} defaultUpdateFn - the default function to dispatch
-     * @param {string} root - optional key of the state where to search fo they keys described in list
+     * @param {string|function} root - optional key of the state where to search for they keys described in list
      * @returns {Object} a dictionary with all the computed properties generated
     */
     ...mapComputed(
       [
         'baz',
-        { key: 'bar', updateFn: 'updateBar' }
+        { key: 'bar', updateFn: 'updateBar' },
         { key: 'foo', getter: 'getFoo' },
       ],
       'updateAll',
@@ -575,3 +575,48 @@ export default {
 ```
 
 `mapComputed` then generates the appropriate computed properties that get the data from the store and dispatch the correct action when updated.
+
+In the event that the `root` of the key is more than one-level deep you can use a function to retrieve the relevant state object.
+
+For instance, with a store like:
+
+```javascript
+// this store is non-functional and only used to give context to the example
+export default {
+  state: {
+    foo: {
+      qux: {
+        baz: '',
+        bar: '',
+        foo: '',
+      },
+    },
+  },
+  actions: {
+    updateBar() {...},
+    updateAll() {...},
+  },
+  getters: {
+    getFoo() {...},
+  }
+}
+```
+
+The `root` could be:
+
+```javascript
+import { mapComputed } from '~/vuex_shared/bindings'
+export default {
+  computed: {
+    ...mapComputed(
+      [
+        'baz',
+        { key: 'bar', updateFn: 'updateBar' },
+        { key: 'foo', getter: 'getFoo' },
+      ],
+      'updateAll',
+      (state) => state.foo.qux,
+    ),
+  }
+}
+```

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

 <script>
 import { GlButton, GlForm, GlFormGroup } from '@gitlab/ui';
 import { mapActions, mapState } from 'vuex';
+import { mapComputed } from '~/vuex_shared/bindings';
 import { APPROVAL_SETTINGS_I18N } from '../constants';
 import ApprovalSettingsCheckbox from './approval_settings_checkbox.vue';
 
@@ -17,14 +18,30 @@ export default {
       required: true,
     },
   },
+  data() {
+    return {
+      hasFormLoaded: false,
+    };
+  },
   computed: {
     ...mapState({
-      settings: (state) => state.approvals.settings,
       isLoading: (state) => state.approvals.isLoading,
     }),
+    ...mapComputed(
+      [
+        { key: 'preventAuthorApproval', updateFn: 'setPreventAuthorApproval' },
+        { key: 'preventCommittersApproval', updateFn: 'setPreventCommittersApproval' },
+        { key: 'preventMrApprovalRuleEdit', updateFn: 'setPreventMrApprovalRuleEdit' },
+        { key: 'removeApprovalsOnPush', updateFn: 'setRemoveApprovalsOnPush' },
+        { key: 'requireUserPassword', updateFn: 'setRequireUserPassword' },
+      ],
+      undefined,
+      (state) => state.approvals.settings,
+    ),
   },
-  created() {
-    this.fetchSettings(this.approvalSettingsPath);
+  async created() {
+    await this.fetchSettings(this.approvalSettingsPath);
+    this.hasFormLoaded = true;
   },
   methods: {
     ...mapActions(['fetchSettings', 'updateSettings']),
@@ -45,34 +62,34 @@ export default {
 </script>
 
 <template>
-  <gl-form @submit.prevent="onSubmit">
+  <gl-form v-if="hasFormLoaded" @submit.prevent="onSubmit">
     <gl-form-group>
       <approval-settings-checkbox
-        v-model="settings.preventAuthorApproval"
+        v-model="preventAuthorApproval"
         :label="$options.i18n.authorApprovalLabel"
         :anchor="$options.links.preventAuthorApprovalDocsAnchor"
         data-testid="prevent-author-approval"
       />
       <approval-settings-checkbox
-        v-model="settings.preventMrApprovalRuleEdit"
+        v-model="preventMrApprovalRuleEdit"
         :label="$options.i18n.preventMrApprovalRuleEditLabel"
         :anchor="$options.links.preventMrApprovalRuleEditDocsAnchor"
         data-testid="prevent-mr-approval-rule-edit"
       />
       <approval-settings-checkbox
-        v-model="settings.requireUserPassword"
+        v-model="requireUserPassword"
         :label="$options.i18n.requireUserPasswordLabel"
         :anchor="$options.links.requireUserPasswordDocsAnchor"
         data-testid="require-user-password"
       />
       <approval-settings-checkbox
-        v-model="settings.removeApprovalsOnPush"
+        v-model="removeApprovalsOnPush"
         :label="$options.i18n.removeApprovalsOnPushLabel"
         :anchor="$options.links.removeApprovalsOnPushDocsAnchor"
         data-testid="remove-approvals-on-push"
       />
       <approval-settings-checkbox
-        v-model="settings.preventCommittersApproval"
+        v-model="preventCommittersApproval"
         :label="$options.i18n.preventCommittersApprovalLabel"
         :anchor="$options.links.preventCommittersApprovalAnchor"
         data-testid="prevent-committers-approval"

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

@@ -3,6 +3,14 @@ import axios from '~/lib/utils/axios_utils';
 import { __ } from '~/locale';
 import * as types from './mutation_types';
 
+const mapStateToPayload = (state) => ({
+  allow_author_approval: !state.settings.preventAuthorApproval,
+  allow_overrides_to_approver_list_per_merge_request: !state.settings.preventMrApprovalRuleEdit,
+  require_password_to_approve: state.settings.requireUserPassword,
+  retain_approvals_on_push: !state.settings.removeApprovalsOnPush,
+  allow_committer_approval: !state.settings.preventCommittersApproval,
+});
+
 export const fetchSettings = ({ commit }, endpoint) => {
   commit(types.REQUEST_SETTINGS);
 
@@ -24,18 +32,10 @@ export const fetchSettings = ({ commit }, endpoint) => {
 };
 
 export const updateSettings = ({ commit, state }, endpoint) => {
-  const payload = {
-    allow_author_approval: !state.settings.preventAuthorApproval,
-    allow_overrides_to_approver_list_per_merge_request: !state.settings.preventMrApprovalRuleEdit,
-    require_password_to_approve: state.settings.requireUserPassword,
-    retain_approvals_on_push: !state.settings.removeApprovalsOnPush,
-    allow_committer_approval: !state.settings.preventCommittersApproval,
-  };
-
   commit(types.REQUEST_UPDATE_SETTINGS);
 
   return axios
-    .put(endpoint, payload)
+    .put(endpoint, { ...mapStateToPayload(state) })
     .then(({ data }) => {
       commit(types.UPDATE_SETTINGS_SUCCESS, data);
       createFlash({
@@ -54,3 +54,23 @@ export const updateSettings = ({ commit, state }, endpoint) => {
       });
     });
 };
+
+export const setPreventAuthorApproval = ({ commit }, { preventAuthorApproval }) => {
+  commit(types.SET_PREVENT_AUTHOR_APPROVAL, preventAuthorApproval);
+};
+
+export const setPreventCommittersApproval = ({ commit }, { preventCommittersApproval }) => {
+  commit(types.SET_PREVENT_COMMITTERS_APPROVAL, preventCommittersApproval);
+};
+
+export const setPreventMrApprovalRuleEdit = ({ commit }, { preventMrApprovalRuleEdit }) => {
+  commit(types.SET_PREVENT_MR_APPROVAL_RULE_EDIT, preventMrApprovalRuleEdit);
+};
+
+export const setRemoveApprovalsOnPush = ({ commit }, { removeApprovalsOnPush }) => {
+  commit(types.SET_REMOVE_APPROVALS_ON_PUSH, removeApprovalsOnPush);
+};
+
+export const setRequireUserPassword = ({ commit }, { requireUserPassword }) => {
+  commit(types.SET_REQUIRE_USER_PASSWORD, requireUserPassword);
+};

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

@@ -6,3 +6,9 @@ export const RECEIVE_SETTINGS_ERROR = 'RECEIVE_SETTINGS_ERROR';
 export const REQUEST_UPDATE_SETTINGS = 'REQUEST_UPDATE_SETTINGS';
 export const UPDATE_SETTINGS_SUCCESS = 'UPDATE_SETTINGS_SUCCESS';
 export const UPDATE_SETTINGS_ERROR = 'UPDATE_SETTINGS_ERROR';
+
+export const SET_PREVENT_AUTHOR_APPROVAL = 'SET_PREVENT_AUTHOR_APPROVAL';
+export const SET_PREVENT_COMMITTERS_APPROVAL = 'SET_PREVENT_COMMITTERS_APPROVAL';
+export const SET_PREVENT_MR_APPROVAL_RULE_EDIT = 'SET_PREVENT_MR_APPROVAL_RULE_EDIT';
+export const SET_REMOVE_APPROVALS_ON_PUSH = 'SET_REMOVE_APPROVALS_ON_PUSH';
+export const SET_REQUIRE_USER_PASSWORD = 'SET_REQUIRE_USER_PASSWORD';

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

 import * as types from './mutation_types';
 
+const mapDataToState = (data) => ({
+  preventAuthorApproval: !data.allow_author_approval,
+  preventMrApprovalRuleEdit: !data.allow_overrides_to_approver_list_per_merge_request,
+  requireUserPassword: data.require_password_to_approve,
+  removeApprovalsOnPush: !data.retain_approvals_on_push,
+  preventCommittersApproval: !data.allow_committer_approval,
+});
+
 export default {
   [types.REQUEST_SETTINGS](state) {
     state.isLoading = true;
   },
   [types.RECEIVE_SETTINGS_SUCCESS](state, data) {
-    state.settings.preventAuthorApproval = !data.allow_author_approval;
-    state.settings.preventMrApprovalRuleEdit = !data.allow_overrides_to_approver_list_per_merge_request;
-    state.settings.requireUserPassword = data.require_password_to_approve;
-    state.settings.removeApprovalsOnPush = !data.retain_approvals_on_push;
-    state.settings.preventCommittersApproval = !data.allow_committer_approval;
+    state.settings = { ...mapDataToState(data) };
     state.isLoading = false;
   },
   [types.RECEIVE_SETTINGS_ERROR](state) {
@@ -19,14 +23,25 @@ export default {
     state.isLoading = true;
   },
   [types.UPDATE_SETTINGS_SUCCESS](state, data) {
-    state.settings.preventAuthorApproval = !data.allow_author_approval;
-    state.settings.preventMrApprovalRuleEdit = !data.allow_overrides_to_approver_list_per_merge_request;
-    state.settings.requireUserPassword = data.require_password_to_approve;
-    state.settings.removeApprovalsOnPush = !data.retain_approvals_on_push;
-    state.settings.preventCommittersApproval = !data.allow_committer_approval;
+    state.settings = { ...mapDataToState(data) };
     state.isLoading = false;
   },
   [types.UPDATE_SETTINGS_ERROR](state) {
     state.isLoading = false;
   },
+  [types.SET_PREVENT_AUTHOR_APPROVAL](state, preventAuthorApproval) {
+    state.settings.preventAuthorApproval = preventAuthorApproval;
+  },
+  [types.SET_PREVENT_COMMITTERS_APPROVAL](state, preventCommittersApproval) {
+    state.settings.preventCommittersApproval = preventCommittersApproval;
+  },
+  [types.SET_PREVENT_MR_APPROVAL_RULE_EDIT](state, preventMrApprovalRuleEdit) {
+    state.settings.preventMrApprovalRuleEdit = preventMrApprovalRuleEdit;
+  },
+  [types.SET_REMOVE_APPROVALS_ON_PUSH](state, removeApprovalsOnPush) {
+    state.settings.removeApprovalsOnPush = removeApprovalsOnPush;
+  },
+  [types.SET_REQUIRE_USER_PASSWORD](state, requireUserPassword) {
+    state.settings.requireUserPassword = requireUserPassword;
+  },
 };

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

@@ -7,6 +7,7 @@ import { APPROVAL_SETTINGS_I18N } from 'ee/approvals/constants';
 import { createStoreOptions } from 'ee/approvals/stores';
 import groupSettingsModule from 'ee/approvals/stores/modules/group_settings';
 import { extendedWrapper } from 'helpers/vue_test_utils_helper';
+import waitForPromises from 'helpers/wait_for_promises';
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -60,9 +61,10 @@ describe('ApprovalSettings', () => {
   `('with $testid checkbox', ({ testid, setting, labelKey, anchor }) => {
     let checkbox = null;
 
-    beforeEach(() => {
+    beforeEach(async () => {
       store.modules.approvals.state.settings[setting] = false;
       createWrapper();
+      await waitForPromises();
       checkbox = wrapper.findByTestId(testid);
     });
 
@@ -83,24 +85,33 @@ describe('ApprovalSettings', () => {
 
     it('updates the store when the value is changed', async () => {
       await checkbox.vm.$emit('input', true);
+      await waitForPromises();
 
       expect(store.modules.approvals.state.settings[setting]).toBe(true);
     });
   });
 
   describe('loading', () => {
-    it('renders enabled button when not loading', () => {
+    it('does not render the form if the settings are not there yet', () => {
+      createWrapper();
+
+      expect(findForm().exists()).toBe(false);
+    });
+
+    it('renders enabled button when not loading', async () => {
       store.modules.approvals.state.isLoading = false;
 
       createWrapper();
+      await waitForPromises();
 
       expect(findSaveButton().props('disabled')).toBe(false);
     });
 
-    it('renders disabled button when loading', () => {
+    it('renders disabled button when loading', async () => {
       store.modules.approvals.state.isLoading = true;
 
       createWrapper();
+      await waitForPromises();
 
       expect(findSaveButton().props('disabled')).toBe(true);
     });
@@ -109,7 +120,7 @@ describe('ApprovalSettings', () => {
   describe('form submission', () => {
     it('update settings via API', async () => {
       createWrapper();
-
+      await waitForPromises();
       await findForm().vm.$emit('submit', { preventDefault: () => {} });
 
       expect(actions.updateSettings).toHaveBeenCalledWith(expect.any(Object), approvalSettingsPath);

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

@@ -135,4 +135,19 @@ describe('EE approvals group settings module actions', () => {
       });
     });
   });
+
+  describe.each`
+    action                            | type                                       | prop
+    ${'setPreventAuthorApproval'}     | ${types.SET_PREVENT_AUTHOR_APPROVAL}       | ${'preventAuthorApproval'}
+    ${'setPreventCommittersApproval'} | ${types.SET_PREVENT_COMMITTERS_APPROVAL}   | ${'preventCommittersApproval'}
+    ${'setPreventMrApprovalRuleEdit'} | ${types.SET_PREVENT_MR_APPROVAL_RULE_EDIT} | ${'preventMrApprovalRuleEdit'}
+    ${'setRemoveApprovalsOnPush'}     | ${types.SET_REMOVE_APPROVALS_ON_PUSH}      | ${'removeApprovalsOnPush'}
+    ${'setRequireUserPassword'}       | ${types.SET_REQUIRE_USER_PASSWORD}         | ${'requireUserPassword'}
+  `('$action', ({ action, type, prop }) => {
+    it(`commits ${type}`, () => {
+      const payload = { [prop]: true };
+
+      return testAction(actions[action], payload, state, [{ type, payload: true }], []);
+    });
+  });
 });

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

@@ -75,4 +75,19 @@ describe('Group settings store mutations', () => {
       expect(state.isLoading).toBe(false);
     });
   });
+
+  describe.each`
+    mutation                               | prop
+    ${'SET_PREVENT_AUTHOR_APPROVAL'}       | ${'preventAuthorApproval'}
+    ${'SET_PREVENT_COMMITTERS_APPROVAL'}   | ${'preventCommittersApproval'}
+    ${'SET_PREVENT_MR_APPROVAL_RULE_EDIT'} | ${'preventMrApprovalRuleEdit'}
+    ${'SET_REMOVE_APPROVALS_ON_PUSH'}      | ${'removeApprovalsOnPush'}
+    ${'SET_REQUIRE_USER_PASSWORD'}         | ${'requireUserPassword'}
+  `('$mutation', ({ mutation, prop }) => {
+    it(`sets the ${prop}`, () => {
+      mutations[mutation](state, true);
+
+      expect(state.settings[prop]).toBe(true);
+    });
+  });
 });

spec/frontend/vuex_shared/bindings_spec.js

@@ -3,7 +3,7 @@ import { mapComputed } from '~/vuex_shared/bindings';
 
 describe('Binding utils', () => {
   describe('mapComputed', () => {
-    const defaultArgs = [['baz'], 'bar', 'foo'];
+    const defaultArgs = [['baz'], 'bar', 'foo', 'qux'];
 
     const createDummy = (mapComputedArgs = defaultArgs) => ({
       computed: {
@@ -29,12 +29,18 @@ describe('Binding utils', () => {
       },
     };
 
-    it('returns an object with keys equal to the first fn parameter ', () => {
+    it('returns an object with keys equal to the first fn parameter', () => {
       const keyList = ['foo1', 'foo2'];
       const result = mapComputed(keyList, 'foo', 'bar');
       expect(Object.keys(result)).toEqual(keyList);
     });
 
+    it('returns an object with keys equal to the first fn parameter when the root is a function', () => {
+      const keyList = ['foo1', 'foo2'];
+      const result = mapComputed(keyList, 'foo', (state) => state.bar);
+      expect(Object.keys(result)).toEqual(keyList);
+    });
+
     it('returned object has set and get function', () => {
       const result = mapComputed(['baz'], 'foo', 'bar');
       expect(result.baz.set).toBeDefined();