Merge branch 'edit-ff-user-list-fe' into 'master'

Allow Users to Add and Delete IDs from User Lists

See merge request gitlab-org/gitlab!35373

ee/app/assets/javascripts/user_lists/components/add_user_modal.vue

+<script>
+import { s__ } from '~/locale';
+import { GlModal, GlFormGroup, GlFormTextarea } from '@gitlab/ui';
+import { ADD_USER_MODAL_ID } from '../constants/show';
+
+export default {
+  components: {
+    GlFormGroup,
+    GlFormTextarea,
+    GlModal,
+  },
+  props: {
+    visible: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  modalOptions: {
+    actionPrimary: {
+      text: s__('UserLists|Add'),
+      attributes: [{ 'data-testid': 'confirm-add-user-ids' }],
+    },
+    actionCancel: {
+      text: s__('UserLists|Cancel'),
+      attributes: [{ 'data-testid': 'cancel-add-user-ids' }],
+    },
+    modalId: ADD_USER_MODAL_ID,
+    static: true,
+  },
+  translations: {
+    title: s__('UserLists|Add users'),
+    description: s__(
+      'UserLists|Enter a comma separated list of user IDs. These IDs should be the users of the system in which the feature flag is set, not GitLab IDs',
+    ),
+    userIdsLabel: s__('UserLists|User IDs'),
+  },
+  data() {
+    return {
+      userIds: '',
+    };
+  },
+  methods: {
+    submitUsers() {
+      this.$emit('addUsers', this.userIds);
+      this.clearInput();
+    },
+    clearInput() {
+      this.userIds = '';
+    },
+  },
+};
+</script>
+<template>
+  <gl-modal
+    v-bind="$options.modalOptions"
+    :visible="visible"
+    data-testid="add-users-modal"
+    @primary="submitUsers"
+    @canceled="clearInput"
+  >
+    <template #modal-title>
+      {{ $options.translations.title }}
+    </template>
+    <template #default>
+      <p data-testid="add-userids-description">{{ $options.translations.description }}</p>
+      <gl-form-group label-for="add-user-ids" :label="$options.translations.userIdsLabel">
+        <gl-form-textarea id="add-user-ids" v-model="userIds" />
+      </gl-form-group>
+    </template>
+  </gl-modal>
+</template>

ee/app/assets/javascripts/user_lists/components/user_list.vue

 <script>
 import { mapActions, mapState } from 'vuex';
-import { GlAlert, GlEmptyState, GlLoadingIcon, GlModalDirective as GlModal } from '@gitlab/ui';
+import {
+  GlAlert,
+  GlButton,
+  GlEmptyState,
+  GlLoadingIcon,
+  GlModalDirective as GlModal,
+} from '@gitlab/ui';
 import { s__, __ } from '~/locale';
-import states from '../constants/show';
+import { states, ADD_USER_MODAL_ID } from '../constants/show';
+import AddUserModal from './add_user_modal.vue';
 
 const commonTableClasses = ['gl-py-5', 'gl-border-b-1', 'gl-border-b-solid', 'gl-border-gray-100'];
 
 export default {
   components: {
     GlAlert,
+    GlButton,
     GlEmptyState,
     GlLoadingIcon,
+    AddUserModal,
   },
   directives: {
     GlModal,
@@ -22,6 +31,7 @@ export default {
     },
   },
   translations: {
+    addUserButtonLabel: s__('UserLists|Add Users'),
     emptyStateTitle: s__('UserLists|There are no users'),
     emptyStateDescription: s__(
       'UserLists|Define a set of users to be used within feature flag strategies',
@@ -47,7 +57,7 @@ export default {
       'gl-align-items-center',
     ].join(' '),
   },
-  modalId: 'add-userids-modal',
+  ADD_USER_MODAL_ID,
   computed: {
     ...mapState(['userList', 'userIds', 'state']),
     name() {
@@ -67,7 +77,7 @@ export default {
     this.fetchUserList();
   },
   methods: {
-    ...mapActions(['fetchUserList', 'dismissErrorAlert']),
+    ...mapActions(['fetchUserList', 'dismissErrorAlert', 'removeUserId', 'addUserIds']),
   },
 };
 </script>
@@ -78,11 +88,21 @@ export default {
     </gl-alert>
     <gl-loading-icon v-if="isLoading" size="xl" class="mt-5" />
     <div v-else>
+      <add-user-modal @addUsers="addUserIds" />
       <div :class="$options.classes.headerClasses">
         <div>
           <h3>{{ name }}</h3>
           <h4 class="gl-text-gray-700">{{ $options.translations.userIdLabel }}</h4>
         </div>
+        <div class="mt-5">
+          <gl-button
+            v-gl-modal="$options.ADD_USER_MODAL_ID"
+            data-testid="add-users"
+            variant="success"
+          >
+            {{ $options.translations.addUserButtonLabel }}
+          </gl-button>
+        </div>
       </div>
       <div v-if="hasUserIds">
         <div :class="$options.classes.tableHeaderClasses">
@@ -95,6 +115,13 @@ export default {
           :class="$options.classes.tableRowClasses"
         >
           <span data-testid="user-id">{{ id }}</span>
+          <gl-button
+            category="secondary"
+            variant="danger"
+            icon="remove"
+            data-testid="delete-user-id"
+            @click="removeUserId(id)"
+          />
         </div>
       </div>
       <gl-empty-state

ee/app/assets/javascripts/user_lists/constants/show.js

-export default Object.freeze({
+export const states = Object.freeze({
   LOADING: 'LOADING',
   SUCCESS: 'SUCCESS',
   ERROR: 'ERROR',
   ERROR_DISMISSED: 'ERROR_DISMISSED',
 });
+
+export const ADD_USER_MODAL_ID = 'add-userids-modal';

ee/app/assets/javascripts/user_lists/store/show/actions.js

 import Api from 'ee/api';
+import { stringifyUserIds } from '../utils';
 import * as types from './mutation_types';
 
 export const fetchUserList = ({ commit, state }) => {
@@ -9,3 +10,23 @@ export const fetchUserList = ({ commit, state }) => {
 };
 
 export const dismissErrorAlert = ({ commit }) => commit(types.DISMISS_ERROR_ALERT);
+export const addUserIds = ({ dispatch, commit }, userIds) => {
+  commit(types.ADD_USER_IDS, userIds);
+  return dispatch('updateUserList');
+};
+
+export const removeUserId = ({ commit, dispatch }, userId) => {
+  commit(types.REMOVE_USER_ID, userId);
+  return dispatch('updateUserList');
+};
+
+export const updateUserList = ({ commit, state }) => {
+  commit(types.REQUEST_USER_LIST);
+
+  return Api.updateFeatureFlagUserList(state.projectId, {
+    ...state.userList,
+    user_xids: stringifyUserIds(state.userIds),
+  })
+    .then(response => commit(types.RECEIVE_USER_LIST_SUCCESS, response.data))
+    .catch(() => commit(types.RECEIVE_USER_LIST_ERROR));
+};

ee/app/assets/javascripts/user_lists/store/show/mutation_types.js

@@ -3,3 +3,6 @@ export const RECEIVE_USER_LIST_SUCCESS = 'RECEIVE_USER_LIST_SUCCESS';
 export const RECEIVE_USER_LIST_ERROR = 'RECEIVE_USER_LIST_ERROR';
 
 export const DISMISS_ERROR_ALERT = 'DISMISS_ERROR_ALERT';
+
+export const ADD_USER_IDS = 'ADD_USER_IDS';
+export const REMOVE_USER_ID = 'REMOVE_USER_ID';

ee/app/assets/javascripts/user_lists/store/show/mutations.js

-import states from '../../constants/show';
+import { states } from '../../constants/show';
 import * as types from './mutation_types';
+import { parseUserIds } from '../utils';
 
 export default {
   [types.REQUEST_USER_LIST](state) {
@@ -7,7 +8,7 @@ export default {
   },
   [types.RECEIVE_USER_LIST_SUCCESS](state, userList) {
     state.state = states.SUCCESS;
-    state.userIds = userList.user_xids?.length > 0 ? userList.user_xids.split(',') : [];
+    state.userIds = userList.user_xids?.length > 0 ? parseUserIds(userList.user_xids) : [];
     state.userList = userList;
   },
   [types.RECEIVE_USER_LIST_ERROR](state) {
@@ -16,4 +17,13 @@ export default {
   [types.DISMISS_ERROR_ALERT](state) {
     state.state = states.ERROR_DISMISSED;
   },
+  [types.ADD_USER_IDS](state, ids) {
+    state.userIds = [
+      ...state.userIds,
+      ...parseUserIds(ids).filter(id => id && !state.userIds.includes(id)),
+    ];
+  },
+  [types.REMOVE_USER_ID](state, id) {
+    state.userIds = state.userIds.filter(uid => uid !== id);
+  },
 };

ee/app/assets/javascripts/user_lists/store/show/state.js

-import states from '../../constants/show';
+import { states } from '../../constants/show';
 
 export default ({ projectId = '', userListIid = '' }) => ({
   state: states.LOADING,

ee/app/assets/javascripts/user_lists/store/utils.js

+export const parseUserIds = userIds => userIds.split(/\s*,\s*/g);
+
+export const stringifyUserIds = userIds => userIds.join(',');

ee/changelogs/unreleased/edit-ff-user-list-fe.yml

+---
+title: Let users modify feature flag user lists
+merge_request: 35373
+author:
+type: added

ee/spec/frontend/user_lists/components/add_user_modal_spec.js

+import { mount } from '@vue/test-utils';
+import AddUserModal from 'ee/user_lists/components/add_user_modal.vue';
+
+describe('Add User Modal', () => {
+  let wrapper;
+
+  const click = testId => wrapper.find(`[data-testid="${testId}"]`).trigger('click');
+
+  beforeEach(() => {
+    wrapper = mount(AddUserModal, {
+      propsData: { visible: true },
+    });
+  });
+
+  it('should explain the format of user IDs to enter', () => {
+    expect(wrapper.find('[data-testid="add-userids-description"]').text()).toContain(
+      'Enter a comma separated list of user IDs',
+    );
+  });
+
+  describe('events', () => {
+    beforeEach(() => {
+      wrapper.find('#add-user-ids').setValue('1, 2, 3, 4');
+    });
+
+    it('should emit the users entered when Add Users is clicked', () => {
+      click('confirm-add-user-ids');
+      expect(wrapper.emitted('addUsers')).toContainEqual(['1, 2, 3, 4']);
+    });
+
+    it('should clear the input after emitting', async () => {
+      click('confirm-add-user-ids');
+      await wrapper.vm.$nextTick();
+
+      expect(wrapper.find('#add-user-ids').element.value).toBe('');
+    });
+
+    it('should not emit the users entered if cancel is clicked', () => {
+      click('cancel-add-user-ids');
+      expect(wrapper.emitted('addUsers')).toBeUndefined();
+    });
+
+    it('should clear the input after cancelling', async () => {
+      click('cancel-add-user-ids');
+      await wrapper.vm.$nextTick();
+
+      expect(wrapper.find('#add-user-ids').element.value).toBe('');
+    });
+  });
+});

ee/spec/frontend/user_lists/components/user_list_spec.js

 import Vue from 'vue';
 import Vuex from 'vuex';
 import { mount } from '@vue/test-utils';
+import { uniq } from 'lodash';
 import { GlAlert, GlEmptyState, GlLoadingIcon } from '@gitlab/ui';
 import Api from 'ee/api';
 import { userList } from '../../feature_flags/mock_data';
+import { parseUserIds, stringifyUserIds } from 'ee/user_lists/store/utils';
 import createStore from 'ee/user_lists/store/show';
 import UserList from 'ee/user_lists/components/user_list.vue';
 
@@ -14,6 +16,8 @@ Vue.use(Vuex);
 describe('User List', () => {
   let wrapper;
 
+  const click = testId => wrapper.find(`[data-testid="${testId}"]`).trigger('click');
+
   const findUserIds = () => wrapper.findAll('[data-testid="user-id"]');
 
   const destroy = () => wrapper?.destroy();
@@ -54,7 +58,7 @@ describe('User List', () => {
     let userIds;
 
     beforeEach(() => {
-      userIds = userList.user_xids.split(',');
+      userIds = parseUserIds(userList.user_xids);
       Api.fetchFeatureFlagUserList.mockResolvedValueOnce({ data: userList });
       factory();
 
@@ -69,6 +73,10 @@ describe('User List', () => {
       expect(wrapper.find('h3').text()).toBe(userList.name);
     });
 
+    it('shows an add users button', () => {
+      expect(wrapper.find('[data-testid="add-users"]').text()).toBe('Add Users');
+    });
+
     it('shows a row for every id', () => {
       expect(wrapper.findAll('[data-testid="user-id-row"]')).toHaveLength(userIds.length);
     });
@@ -76,6 +84,72 @@ describe('User List', () => {
     it('shows one id on each row', () => {
       findUserIds().wrappers.forEach((w, i) => expect(w.text()).toBe(userIds[i]));
     });
+
+    it('shows a delete button for every row', () => {
+      expect(wrapper.findAll('[data-testid="delete-user-id"]')).toHaveLength(userIds.length);
+    });
+
+    describe('adding users', () => {
+      const newIds = ['user3', 'user4', 'user5', 'test', 'example', 'foo'];
+      let receivedUserIds;
+      let parsedReceivedUserIds;
+
+      beforeEach(async () => {
+        Api.updateFeatureFlagUserList.mockResolvedValue(userList);
+        click('add-users');
+        await wrapper.vm.$nextTick();
+        wrapper.find('#add-user-ids').setValue(`${stringifyUserIds(newIds)},`);
+        click('confirm-add-user-ids');
+        await wrapper.vm.$nextTick();
+        [[, { user_xids: receivedUserIds }]] = Api.updateFeatureFlagUserList.mock.calls;
+        parsedReceivedUserIds = parseUserIds(receivedUserIds);
+      });
+
+      it('should add user IDs to the user list', () => {
+        newIds.forEach(id => expect(receivedUserIds).toContain(id));
+      });
+
+      it('should not remove existing user ids', () => {
+        userIds.forEach(id => expect(receivedUserIds).toContain(id));
+      });
+
+      it('should not submit empty IDs', () => {
+        parsedReceivedUserIds.forEach(id => expect(id).not.toBe(''));
+      });
+
+      it('should not create duplicate entries', () => {
+        expect(uniq(parsedReceivedUserIds)).toEqual(parsedReceivedUserIds);
+      });
+
+      it('should display the new IDs', () => {
+        const userIdWrappers = findUserIds();
+        newIds.forEach(id => {
+          const userIdWrapper = userIdWrappers.wrappers.find(w => w.text() === id);
+          expect(userIdWrapper.exists()).toBe(true);
+        });
+      });
+    });
+
+    describe('deleting users', () => {
+      let receivedUserIds;
+
+      beforeEach(async () => {
+        Api.updateFeatureFlagUserList.mockResolvedValue(userList);
+        click('delete-user-id');
+        await wrapper.vm.$nextTick();
+        [[, { user_xids: receivedUserIds }]] = Api.updateFeatureFlagUserList.mock.calls;
+      });
+
+      it('should remove the ID clicked', () => {
+        expect(receivedUserIds).not.toContain(userIds[0]);
+      });
+
+      it('should not display the deleted user', () => {
+        const userIdWrappers = findUserIds();
+        const userIdWrapper = userIdWrappers.wrappers.find(w => w.text() === userIds[0]);
+        expect(userIdWrapper).toBeUndefined();
+      });
+    });
   });
 
   describe('error', () => {

ee/spec/frontend/user_lists/store/show/actions_spec.js

 import testAction from 'helpers/vuex_action_helper';
 import Api from 'ee/api';
+import { stringifyUserIds } from 'ee/user_lists/store/utils';
 import createState from 'ee/user_lists/store/show/state';
 import * as types from 'ee/user_lists/store/show/mutation_types';
 import * as actions from 'ee/user_lists/store/show/actions';
@@ -53,4 +54,64 @@ describe('User Lists Show Actions', () => {
       );
     });
   });
+
+  describe('addUserIds', () => {
+    it('adds the given IDs and tries to update the user list', () => {
+      return testAction(
+        actions.addUserIds,
+        '1,2,3',
+        mockState,
+        [{ type: types.ADD_USER_IDS, payload: '1,2,3' }],
+        [{ type: 'updateUserList' }],
+      );
+    });
+  });
+
+  describe('removeUserId', () => {
+    it('removes the given ID and tries to update the user list', () => {
+      return testAction(
+        actions.removeUserId,
+        'user3',
+        mockState,
+        [{ type: types.REMOVE_USER_ID, payload: 'user3' }],
+        [{ type: 'updateUserList' }],
+      );
+    });
+  });
+
+  describe('updateUserList', () => {
+    beforeEach(() => {
+      mockState.userList = userList;
+      mockState.userIds = ['user1', 'user2', 'user3'];
+    });
+
+    it('commits REQUEST_USER_LIST and RECEIVE_USER_LIST_SUCCESS on success', () => {
+      Api.updateFeatureFlagUserList.mockResolvedValue({ data: userList });
+      return testAction(
+        actions.updateUserList,
+        undefined,
+        mockState,
+        [
+          { type: types.REQUEST_USER_LIST },
+          { type: types.RECEIVE_USER_LIST_SUCCESS, payload: userList },
+        ],
+        [],
+        () =>
+          expect(Api.updateFeatureFlagUserList).toHaveBeenCalledWith('1', {
+            ...userList,
+            user_xids: stringifyUserIds(mockState.userIds),
+          }),
+      );
+    });
+    it('commits REQUEST_USER_LIST and RECEIVE_USER_LIST_ERROR on error', () => {
+      Api.updateFeatureFlagUserList.mockRejectedValue({ message: 'fail' });
+      return testAction(
+        actions.updateUserList,
+        undefined,
+        mockState,
+        [{ type: types.REQUEST_USER_LIST }, { type: types.RECEIVE_USER_LIST_ERROR }],
+        [],
+      );
+    });
+  });
 });

ee/spec/frontend/user_lists/store/show/mutations_spec.js

+import { uniq } from 'lodash';
 import createState from 'ee/user_lists/store/show/state';
 import mutations from 'ee/user_lists/store/show/mutations';
-import states from 'ee/user_lists/constants/show';
+import { states } from 'ee/user_lists/constants/show';
 import * as types from 'ee/user_lists/store/show/mutation_types';
 import { userList } from 'ee_jest/feature_flags/mock_data';
 
@@ -47,4 +48,39 @@ describe('User Lists Show Mutations', () => {
       expect(mockState.state).toBe(states.ERROR);
     });
   });
+  describe(types.ADD_USER_IDS, () => {
+    const newIds = ['user3', 'test1', '1', '3', ''];
+
+    beforeEach(() => {
+      mutations[types.RECEIVE_USER_LIST_SUCCESS](mockState, userList);
+      mutations[types.ADD_USER_IDS](mockState, newIds.join(', '));
+    });
+
+    it('adds the new IDs to the state unless empty', () => {
+      newIds.filter(id => id).forEach(id => expect(mockState.userIds).toContain(id));
+    });
+
+    it('does not add duplicate IDs to the state', () => {
+      expect(mockState.userIds).toEqual(uniq(mockState.userIds));
+    });
+  });
+  describe(types.REMOVE_USER_ID, () => {
+    let userIds;
+    let removedId;
+
+    beforeEach(() => {
+      mutations[types.RECEIVE_USER_LIST_SUCCESS](mockState, userList);
+      userIds = mockState.userIds;
+      removedId = 'user3';
+      mutations[types.REMOVE_USER_ID](mockState, removedId);
+    });
+
+    it('should remove the given id', () => {
+      expect(mockState).not.toContain(removedId);
+    });
+
+    it('should leave the rest of the IDs alone', () => {
+      userIds.filter(id => id !== removedId).forEach(id => expect(mockState.userIds).toContain(id));
+    });
+  });
 });

ee/spec/frontend/user_lists/store/utils_spec.js

+import { parseUserIds, stringifyUserIds } from 'ee/user_lists/store/utils';
+
+describe('User List Store Utils', () => {
+  describe('parseUserIds', () => {
+    it('should split comma-seperated user IDs into an array', () => {
+      expect(parseUserIds('1,2,3')).toEqual(['1', '2', '3']);
+    });
+
+    it('should filter whitespace before the comma', () => {
+      expect(parseUserIds('1\t,2     ,3')).toEqual(['1', '2', '3']);
+    });
+
+    it('should filter whitespace after the comma', () => {
+      expect(parseUserIds('1,\t2,    3')).toEqual(['1', '2', '3']);
+    });
+  });
+
+  describe('stringifyUserIds', () => {
+    it('should convert a list of user IDs into a comma-separated string', () => {
+      expect(stringifyUserIds(['1', '2', '3'])).toBe('1,2,3');
+    });
+  });
+});

locale/gitlab.pot

@@ -25442,9 +25442,24 @@ msgstr ""
 msgid "User was successfully updated."
 msgstr ""
 
+msgid "UserLists|Add"
+msgstr ""
+
+msgid "UserLists|Add Users"
+msgstr ""
+
+msgid "UserLists|Add users"
+msgstr ""
+
+msgid "UserLists|Cancel"
+msgstr ""
+
 msgid "UserLists|Define a set of users to be used within feature flag strategies"
 msgstr ""
 
+msgid "UserLists|Enter a comma separated list of user IDs. These IDs should be the users of the system in which the feature flag is set, not GitLab IDs"
+msgstr ""
+
 msgid "UserLists|There are no users"
 msgstr ""