From 3dd33e1d5cacbc08bd6b3496221d1c517a357d4f Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho Date: Tue, 8 Feb 2022 15:01:54 -0500 Subject: [PATCH 1/5] Add policy action builder into scan result policy page. There is another MR in parallel to this one dealing with policy rules. This is applicable to both new and existing policies. EE: true --- .../approver_deletable_avatar.vue | 30 ++++ .../scan_result_policy/lib/actions.js | 52 ++++++ .../policy_action_builder.vue | 146 +++++++++++++++++ .../scan_result_policy_editor.vue | 14 ++ .../approver_deletable_avatar_spec.js | 87 ++++++++++ .../scan_result_policy/lib/actions_spec.js | 148 ++++++++++++++++++ .../policy_action_builder_spec.js | 98 ++++++++++++ .../scan_result_policy_editor_spec.js | 28 +++- locale/gitlab.pot | 6 + 9 files changed, 608 insertions(+), 1 deletion(-) create mode 100644 ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approver_deletable_avatar.vue create mode 100644 ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js create mode 100644 ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue create mode 100644 ee/spec/frontend/threat_monitoring/components/policy_editor/approver_deletable_avatar_spec.js create mode 100644 ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions_spec.js create mode 100644 ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder_spec.js diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approver_deletable_avatar.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approver_deletable_avatar.vue new file mode 100644 index 00000000000000..8b8715f91fdd9b --- /dev/null +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approver_deletable_avatar.vue @@ -0,0 +1,30 @@ + + + diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js new file mode 100644 index 00000000000000..7086ab79dfb644 --- /dev/null +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js @@ -0,0 +1,52 @@ +const USER_TYPE = 'user'; +const GROUP_TYPE = 'group'; + +/* + Return the ids for all approvers of the group type. +*/ +export function groupIds(approvers) { + return approvers + .filter((approver) => approver.type === GROUP_TYPE) + .map((approver) => approver.id); +} + +/* + Return the ids for all approvers of the user type. +*/ +export function userIds(approvers) { + return approvers.filter((approver) => approver.type === USER_TYPE).map((approver) => approver.id); +} + +/* + Group existing approvers into a single array. +*/ +export function groupApprovers(existingApprovers) { + const approvers = [...existingApprovers]; + const userUniqKeys = ['state', 'username']; + const groupUniqKeys = ['full_name', 'full_path']; + + return approvers.map((approver) => { + const approverKeys = Object.keys(approver); + + if (approverKeys.includes(...groupUniqKeys)) { + return { ...approver, type: GROUP_TYPE }; + } else if (approverKeys.includes(...userUniqKeys)) { + return { ...approver, type: USER_TYPE }; + } + return approver; + }); +} + +/* + Convert approvers into yaml fields (user_approvers, users_approvers_ids) in relation to action. +*/ +export function decomposeApprovers(action, approvers) { + const newAction = { ...action }; + delete newAction.group_approvers; + delete newAction.user_approvers; + return { + ...newAction, + user_approvers_ids: userIds(approvers), + group_approvers_ids: groupIds(approvers), + }; +} diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue new file mode 100644 index 00000000000000..8162bb5f62541e --- /dev/null +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue @@ -0,0 +1,146 @@ + + + diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue index b5eb31e20efe53..f11806712d019b 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor.vue @@ -19,6 +19,7 @@ import { import PolicyEditorLayout from '../policy_editor_layout.vue'; import { assignSecurityPolicyProject, modifyPolicy } from '../utils'; import DimDisableContainer from '../dim_disable_container.vue'; +import PolicyActionBuilder from './policy_action_builder.vue'; import { DEFAULT_SCAN_RESULT_POLICY, fromYaml, toYaml } from './lib'; export default { @@ -49,6 +50,7 @@ export default { GlFormInput, GlFormTextarea, GlAlert, + PolicyActionBuilder, PolicyEditorLayout, DimDisableContainer, }, @@ -116,6 +118,9 @@ export default { }, }, methods: { + updateAction(actionIndex, values) { + this.policy.actions.splice(actionIndex, 1, values); + }, handleError(error) { if (error.message.toLowerCase().includes('graphql')) { this.$emit('error', GRAPHQL_ERROR_MESSAGE); @@ -264,6 +269,15 @@ export default { + + diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/approver_deletable_avatar_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/approver_deletable_avatar_spec.js new file mode 100644 index 00000000000000..e3a0b20acda386 --- /dev/null +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/approver_deletable_avatar_spec.js @@ -0,0 +1,87 @@ +import { GlButton, GlAvatarLabeled } from '@gitlab/ui'; +import { shallowMount } from '@vue/test-utils'; +import ApproverDeletableAvatar from 'ee/threat_monitoring/components/policy_editor/approver_deletable_avatar.vue'; +import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants'; + +const TEST_USER = { + id: 1, + type: TYPE_USER, + name: 'Lorem Ipsum', + avatar_url: '/asd/1', +}; +const TEST_GROUP = { + id: 1, + type: TYPE_GROUP, + name: 'Lorem Group', + full_path: 'dolar/sit/amit', + avatar_url: '/asd/2', +}; + +describe('ApproverDeletableAvatar', () => { + let wrapper; + + const factory = (options = {}) => { + wrapper = shallowMount(ApproverDeletableAvatar, { + ...options, + }); + }; + + const findAvatar = () => wrapper.findComponent(GlAvatarLabeled); + const findRemoveButton = () => wrapper.findComponent(GlButton); + + describe('when user', () => { + beforeEach(() => { + factory({ + propsData: { + approver: TEST_USER, + }, + }); + }); + + it('renders GlAvatar for user', () => { + const avatar = findAvatar(); + expect(avatar.exists()).toBe(true); + expect(avatar.attributes()).toMatchObject({ + 'entity-name': TEST_USER.name, + src: TEST_USER.avatar_url, + shape: 'circle', + alt: TEST_USER.name, + }); + expect(avatar.props('label')).toBe(TEST_USER.name); + }); + + it('when remove clicked, emits remove', async () => { + await findRemoveButton().vm.$emit('click'); + + expect(wrapper.emitted().input).toEqual([[TEST_USER]]); + }); + }); + + describe('when group', () => { + beforeEach(() => { + factory({ + propsData: { + approver: TEST_GROUP, + }, + }); + }); + + it('renders ProjectAvatar for group', () => { + const avatar = findAvatar(); + expect(avatar.exists()).toBe(true); + expect(avatar.attributes()).toMatchObject({ + 'entity-name': TEST_GROUP.name, + src: TEST_GROUP.avatar_url, + shape: 'rect', + alt: TEST_GROUP.name, + }); + expect(avatar.props('label')).toBe(TEST_GROUP.full_path); + }); + + it('when remove clicked, emits remove', async () => { + await findRemoveButton().vm.$emit('click'); + + expect(wrapper.emitted().input).toEqual([[TEST_GROUP]]); + }); + }); +}); diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions_spec.js new file mode 100644 index 00000000000000..26729159fce146 --- /dev/null +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions_spec.js @@ -0,0 +1,148 @@ +import { + groupIds, + userIds, + groupApprovers, + decomposeApprovers, +} from 'ee/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions'; + +// As returned by endpoints based on API::Entities::UserBasic +const userApprover = { + id: 1, + name: null, + state: null, + username: null, + avatar_url: null, + web_url: null, +}; + +// As returned by endpoints based on API::Entities::PublicGroupDetails +const groupApprover = { + id: 2, + name: null, + full_name: null, + full_path: null, + avatar_url: null, + web_url: null, +}; + +const unknownApprover = { id: 3, name: null }; + +const allApprovers = [userApprover, groupApprover]; + +const groupedApprovers = groupApprovers(allApprovers); + +describe('groupApprovers', () => { + describe('with mixed approvers', () => { + it('returns a copy of the input values with their proper type attribute', () => { + expect(groupApprovers(allApprovers)).toStrictEqual([ + { + avatar_url: null, + id: userApprover.id, + name: null, + state: null, + type: 'user', + username: null, + web_url: null, + }, + { + avatar_url: null, + full_name: null, + full_path: null, + id: groupApprover.id, + name: null, + type: 'group', + web_url: null, + }, + ]); + }); + + it('sets types depending on whether the approver is a group or a user', () => { + const approvers = groupApprovers(allApprovers); + expect(approvers.find((approver) => approver.id === userApprover.id)).toEqual( + expect.objectContaining({ type: 'user' }), + ); + expect(approvers.find((approver) => approver.id === groupApprover.id)).toEqual( + expect.objectContaining({ type: 'group' }), + ); + }); + }); + + it('sets group as a type for group related approvers', () => { + expect(groupApprovers([groupApprover])).toStrictEqual([ + { + avatar_url: null, + full_name: null, + full_path: null, + id: groupApprover.id, + name: null, + type: 'group', + web_url: null, + }, + ]); + }); + + it('sets user as a type for user related approvers', () => { + expect(groupApprovers([userApprover])).toStrictEqual([ + { + avatar_url: null, + id: userApprover.id, + name: null, + state: null, + type: 'user', + username: null, + web_url: null, + }, + ]); + }); + + it('does not set a type if neither group or user keys are present', () => { + expect(groupApprovers([unknownApprover])).toStrictEqual([ + { id: unknownApprover.id, name: null }, + ]); + }); +}); + +describe('decomposeApprovers', () => { + it('returns a copy of approvers adding id fields for both group and users', () => { + expect(decomposeApprovers({}, groupedApprovers)).toStrictEqual({ + group_approvers_ids: [groupApprover.id], + user_approvers_ids: [userApprover.id], + }); + }); + + it('removes group_approvers and user_approvers keys only keeping the id fields', () => { + expect( + decomposeApprovers({ user_approvers: null, group_approvers: null }, groupedApprovers), + ).toStrictEqual({ + group_approvers_ids: [groupApprover.id], + user_approvers_ids: [userApprover.id], + }); + }); + + it('preserves any other keys in addition to the id fields', () => { + expect(decomposeApprovers({ existingKey: null }, groupedApprovers)).toStrictEqual({ + group_approvers_ids: [groupApprover.id], + user_approvers_ids: [userApprover.id], + existingKey: null, + }); + }); + + it('returns empty id fields if there is only unknown types', () => { + expect(decomposeApprovers({}, [unknownApprover])).toStrictEqual({ + group_approvers_ids: [], + user_approvers_ids: [], + }); + }); +}); + +describe('userIds', () => { + it('returns only approver with type set to user', () => { + expect(userIds(groupedApprovers)).toStrictEqual([userApprover.id]); + }); +}); + +describe('groupIds', () => { + it('returns only approver with type set to group', () => { + expect(groupIds(groupedApprovers)).toStrictEqual([groupApprover.id]); + }); +}); diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder_spec.js new file mode 100644 index 00000000000000..f77652422302f5 --- /dev/null +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder_spec.js @@ -0,0 +1,98 @@ +import { GlFormInput } from '@gitlab/ui'; +import { mount } from '@vue/test-utils'; +import { nextTick } from 'vue'; +import ApproverDeletableAvatar from 'ee/threat_monitoring/components/policy_editor/approver_deletable_avatar.vue'; +import PolicyActionBuilder from 'ee/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue'; + +const APPROVER_1 = { + id: 1, + name: 'name', + state: 'active', + username: 'username', + web_url: '', + avatar_url: '', +}; + +const APPROVER_2 = { + id: 2, + name: 'name2', + state: 'active', + username: 'username2', + web_url: '', + avatar_url: '', +}; + +const APPROVERS = [APPROVER_1, APPROVER_2]; + +const APPROVERS_IDS = APPROVERS.map((approver) => approver.id); + +const ACTION = { + approvals_required: 1, + user_approvers_ids: APPROVERS_IDS, +}; + +describe('PolicyActionBuilder', () => { + let wrapper; + + const factory = (propsData = {}) => { + wrapper = mount(PolicyActionBuilder, { + propsData: { + initAction: ACTION, + existingApprovers: APPROVERS, + ...propsData, + }, + provide: { + projectId: '1', + }, + }); + }; + + const findApprovalsRequiredInput = () => wrapper.findComponent(GlFormInput); + const findAllDeletableAvatars = () => wrapper.findAllComponents(ApproverDeletableAvatar); + const findAddApproverBtn = () => wrapper.find('[data-testid="add-approver"]'); + + it('renders approvals required form input, the deletable avatars and add approver button', async () => { + factory(); + await nextTick(); + + expect(findApprovalsRequiredInput().exists()).toBe(true); + expect(findAllDeletableAvatars().length).toBe(APPROVERS.length); + expect(findAddApproverBtn().exists()).toBe(true); + }); + + it('triggers an update when changing approvals required', async () => { + const newValue = ACTION.approvals_required + 1; + factory(); + await nextTick(); + + const formInput = findApprovalsRequiredInput(); + + await formInput.vm.$emit('input', newValue); + + expect(wrapper.emitted().changed).toEqual([ + [{ approvals_required: newValue, user_approvers_ids: APPROVERS_IDS }], + ]); + }); + + it('removes one approver when triggering a deletable avatar', async () => { + factory(); + await nextTick(); + const alldeletableAvatars = findAllDeletableAvatars(); + const deletableAvatar = alldeletableAvatars.at(0); + + expect(alldeletableAvatars.length).toBe(APPROVERS.length); + + await deletableAvatar.vm.$emit('input', { ...APPROVER_1, type: 'user' }); + + expect(wrapper.emitted().changed).toEqual([ + [ + { + approvals_required: ACTION.approvals_required, + user_approvers_ids: [APPROVER_2.id], + group_approvers_ids: [], + }, + ], + ]); + expect(findAllDeletableAvatars()).toHaveLength(APPROVERS.length - 1); + }); +}); diff --git a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js index eb000a656b3184..610f99bfc81f49 100644 --- a/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js +++ b/ee/spec/frontend/threat_monitoring/components/policy_editor/scan_result_policy/scan_result_policy_editor_spec.js @@ -21,6 +21,7 @@ import { EDITOR_MODE_YAML, } from 'ee/threat_monitoring/components/policy_editor/constants'; import DimDisableContainer from 'ee/threat_monitoring/components/policy_editor/dim_disable_container.vue'; +import PolicyActionBuilder from 'ee/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue'; jest.mock('~/lib/utils/url_utility', () => ({ joinPaths: jest.requireActual('~/lib/utils/url_utility').joinPaths, @@ -49,7 +50,7 @@ describe('ScanResultPolicyEditor', () => { branch: 'main', fullPath: 'path/to/existing-project', }; - const scanResultPolicyApprovers = []; + const scanResultPolicyApprovers = [{ id: 1, username: 'username', state: 'active' }]; const factory = ({ propsData = {}, provide = {} } = {}) => { wrapper = shallowMount(ScanResultPolicyEditor, { @@ -82,6 +83,8 @@ describe('ScanResultPolicyEditor', () => { const findEmptyState = () => wrapper.findComponent(GlEmptyState); const findPolicyEditorLayout = () => wrapper.findComponent(PolicyEditorLayout); + const findPolicyActionBuilder = () => wrapper.findComponent(PolicyActionBuilder); + const findAllPolicyActionBuilders = () => wrapper.findAllComponents(PolicyActionBuilder); const findAddRuleButton = () => wrapper.find('[data-testid="add-rule"]'); const findAlert = () => wrapper.findComponent(GlAlert); const findNameInput = () => wrapper.findComponent(GlFormInput); @@ -208,4 +211,27 @@ describe('ScanResultPolicyEditor', () => { expect(emptyState.props('svgPath')).toBe(policyEditorEmptyStateSvgPath); }); }); + + describe('with policy action builder', () => { + it('renders a single policy action builder', async () => { + factory(); + + await nextTick(); + + expect(findAllPolicyActionBuilders()).toHaveLength(1); + expect(findPolicyActionBuilder().props('existingApprovers')).toEqual( + scanResultPolicyApprovers, + ); + }); + + it('updates policy action when edited', async () => { + const UPDATED_ACTION = { type: 'required_approval', group_approvers_ids: [1] }; + factory(); + + await nextTick(); + await findPolicyActionBuilder().vm.$emit('changed', UPDATED_ACTION); + + expect(findPolicyActionBuilder().props('initAction')).toEqual(UPDATED_ACTION); + }); + }); }); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 44d04b63afced2..23ffb2c1bdd399 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -31699,6 +31699,12 @@ msgstr "" msgid "Saving project." msgstr "" +msgid "ScanResultPolicy|%{thenLabelStart}Then%{thenLabelEnd} Require approval from %{approvalsRequired} of the following approvers: %{approvers}" +msgstr "" + +msgid "ScanResultPolicy|add an approver" +msgstr "" + msgid "Scanner" msgstr "" -- GitLab From a336f0f6874134bf66c39336f06879420bf5cab8 Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho Date: Thu, 10 Feb 2022 12:08:18 -0500 Subject: [PATCH 2/5] Address UX comment in relation to modal and user flow. Replaces modal by a custom dropdown. --- .../approvers_select_dropdown.vue | 163 +++++++++++++ .../scan_result_policy/lib/actions.js | 2 +- .../policy_action_builder.vue | 50 +--- .../approvers_select_dropdown_spec.js | 220 ++++++++++++++++++ .../policy_action_builder_spec.js | 35 ++- 5 files changed, 429 insertions(+), 41 deletions(-) create mode 100644 ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approvers_select_dropdown.vue create mode 100644 ee/spec/frontend/threat_monitoring/components/policy_editor/approvers_select_dropdown_spec.js diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approvers_select_dropdown.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approvers_select_dropdown.vue new file mode 100644 index 00000000000000..d9ceff100af92a --- /dev/null +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/approvers_select_dropdown.vue @@ -0,0 +1,163 @@ + + + diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js index 7086ab79dfb644..30f610bdcb66fe 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/lib/actions.js @@ -1,4 +1,4 @@ -const USER_TYPE = 'user'; +export const USER_TYPE = 'user'; const GROUP_TYPE = 'group'; /* diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue index 8162bb5f62541e..ddac33b1dd7459 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue @@ -1,21 +1,17 @@ - - diff --git a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue index ddac33b1dd7459..883df9c1d8d710 100644 --- a/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue +++ b/ee/app/assets/javascripts/threat_monitoring/components/policy_editor/scan_result_policy/policy_action_builder.vue @@ -1,17 +1,25 @@ @@ -81,7 +96,7 @@ export default { class="gl-bg-gray-10 gl-border-solid gl-border-1 gl-border-gray-100 gl-rounded-base px-3 pt-3 gl-relative gl-pb-4" > - +