From c0e15cb04478474897257435f64d152d816aef51 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Thu, 13 Dec 2018 11:50:40 -0600 Subject: [PATCH 01/17] Fix typo and i18n in mr approval settings form --- .../projects/_merge_request_approvals_settings_form.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml index 83bb1e4dbe247b..dbc2c5c29427d7 100644 --- a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml +++ b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml @@ -7,7 +7,7 @@ = hidden_field_tag "project[approver_group_ids]" .input-group.input-btn-group = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project', :style => "max-width: unset;" } - %button.btn.btn-success.js-add-approvers{ type: 'button', title: 'Add approvers(s)' } + %button.btn.btn-success.js-add-approvers{ type: 'button', title: _('Add approver(s)') } = _("Add") .form-text.text-muted = _("Add users or groups who are allowed to approve every merge request") -- GitLab From 2952ae9b098a66daf72d207d95d62fc455824afb Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Mon, 17 Dec 2018 16:07:56 -0600 Subject: [PATCH 02/17] Refactor "approvals" into directory **Why?** This directory will host the Vue components for multi rule approvals. https://gitlab.com/gitlab-org/gitlab-ee/issues/1979 --- .../setup_single_rule_approvals.js} | 0 .../pages/projects/merge_requests/shared/init_form.js | 2 +- ...pprovals_spec.js => setup_single_rule_approvals_spec.js} | 6 +++--- .../components}/approvals/approvals_body_spec.js | 0 .../components}/approvals/approvals_footer_spec.js | 0 5 files changed, 4 insertions(+), 4 deletions(-) rename ee/app/assets/javascripts/{approvals.js => approvals/setup_single_rule_approvals.js} (100%) rename ee/spec/javascripts/approvals/{approvals_spec.js => setup_single_rule_approvals_spec.js} (95%) rename ee/spec/javascripts/{ => vue_mr_widget/components}/approvals/approvals_body_spec.js (100%) rename ee/spec/javascripts/{ => vue_mr_widget/components}/approvals/approvals_footer_spec.js (100%) diff --git a/ee/app/assets/javascripts/approvals.js b/ee/app/assets/javascripts/approvals/setup_single_rule_approvals.js similarity index 100% rename from ee/app/assets/javascripts/approvals.js rename to ee/app/assets/javascripts/approvals/setup_single_rule_approvals.js diff --git a/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js b/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js index b3d9865a2d4f7b..234a74a5a8e827 100644 --- a/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js +++ b/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js @@ -1,3 +1,3 @@ -import initApprovals from '../../../../approvals'; +import initApprovals from 'ee/approvals/setup_single_rule_approvals'; export default () => initApprovals(); diff --git a/ee/spec/javascripts/approvals/approvals_spec.js b/ee/spec/javascripts/approvals/setup_single_rule_approvals_spec.js similarity index 95% rename from ee/spec/javascripts/approvals/approvals_spec.js rename to ee/spec/javascripts/approvals/setup_single_rule_approvals_spec.js index 137635cc872cb2..28a18bb9e77c46 100644 --- a/ee/spec/javascripts/approvals/approvals_spec.js +++ b/ee/spec/javascripts/approvals/setup_single_rule_approvals_spec.js @@ -1,7 +1,7 @@ import $ from 'jquery'; -import approvals from 'ee/approvals'; +import setup from 'ee/approvals/setup_single_rule_approvals'; -describe('Approvals', () => { +describe('EE setup_single_rule_approvals', () => { preloadFixtures('merge_requests_ee/merge_request_edit.html.raw'); let $approversEl; @@ -11,7 +11,7 @@ describe('Approvals', () => { loadFixtures('merge_requests_ee/merge_request_edit.html.raw'); $approversEl = $('ul.approver-list'); $suggestionEl = $('.suggested-approvers'); - approvals(); + setup(); }); describe('add suggested approver', () => { diff --git a/ee/spec/javascripts/approvals/approvals_body_spec.js b/ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_body_spec.js similarity index 100% rename from ee/spec/javascripts/approvals/approvals_body_spec.js rename to ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_body_spec.js diff --git a/ee/spec/javascripts/approvals/approvals_footer_spec.js b/ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_footer_spec.js similarity index 100% rename from ee/spec/javascripts/approvals/approvals_footer_spec.js rename to ee/spec/javascripts/vue_mr_widget/components/approvals/approvals_footer_spec.js -- GitLab From fafcb60fbb66edfe8291c46f2e5ba7bd2fff0b77 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Mon, 17 Dec 2018 16:39:22 -0600 Subject: [PATCH 03/17] Integrate multiple approval rules settings - This is behind the feature flag `:apporval_rule` --- .../javascripts/pages/projects/edit/index.js | 2 + ..._request_approvals_settings_form.html.haml | 59 ++----------------- .../_multiple_rules_form.html.haml | 6 ++ .../_single_rule_form.html.haml | 55 +++++++++++++++++ .../settings/merge_requests_settings_spec.rb | 2 + 5 files changed, 69 insertions(+), 55 deletions(-) create mode 100644 ee/app/views/shared/merge_request_approvals_settings/_multiple_rules_form.html.haml create mode 100644 ee/app/views/shared/merge_request_approvals_settings/_single_rule_form.html.haml diff --git a/ee/app/assets/javascripts/pages/projects/edit/index.js b/ee/app/assets/javascripts/pages/projects/edit/index.js index c62a23c057693b..5aeb5cb0640925 100644 --- a/ee/app/assets/javascripts/pages/projects/edit/index.js +++ b/ee/app/assets/javascripts/pages/projects/edit/index.js @@ -5,6 +5,7 @@ import UsersSelect from '~/users_select'; import UserCallout from '~/user_callout'; import groupsSelect from '~/groups_select'; import ApproversSelect from 'ee/approvers_select'; +import mountApprovalsSettings from 'ee/approvals'; import initServiceDesk from 'ee/projects/settings_service_desk'; document.addEventListener('DOMContentLoaded', () => { @@ -15,4 +16,5 @@ document.addEventListener('DOMContentLoaded', () => { new UserCallout({ className: 'js-mr-approval-callout' }); new ApproversSelect(); initServiceDesk(); + mountApprovalsSettings(document.getElementById('js-mr-approvals-settings')); }); diff --git a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml index dbc2c5c29427d7..9218c2ffb99f15 100644 --- a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml +++ b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml @@ -1,60 +1,9 @@ - can_override_approvers = project.can_override_approvers? -.form-group - = form.label :approver_ids, class: 'label-bold' do - = _("Approvers") - = hidden_field_tag "project[approver_ids]" - = hidden_field_tag "project[approver_group_ids]" - .input-group.input-btn-group - = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project', :style => "max-width: unset;" } - %button.btn.btn-success.js-add-approvers{ type: 'button', title: _('Add approver(s)') } - = _("Add") - .form-text.text-muted - = _("Add users or groups who are allowed to approve every merge request") - - .card.prepend-top-10.js-current-approvers - .load-wrapper.hidden - = icon('spinner spin', class: 'approver-list-loader') - .card-header - = _("Approvers") - %span.badge.badge-pill - - ids = [] - - project.approvers.each do |user| - - ids << user.user_id - - project.approver_groups.each do |group| - - group.users.each do |user| - - unless ids.include?(user.id) - - ids << user.id - = ids.count - %ul.content-list.approver-list - - project.approvers.each do |approver| - %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } } - = link_to approver.user.name, approver.user - .float-right - - confirm_message = _("Are you sure you want to remove approver %{name}?") % { name: approver.user.name } - %button{ href: project_approver_path(project, approver), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove approver") } - = icon("trash") - - project.approver_groups.each do |approver_group| - %li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } } - %span - %span.light - = _("Group:") - = link_to approver_group.group.name, approver_group.group - %span.badge.badge-pill - = approver_group.group.members.count - .float-right - - confirm_message = _("Are you sure you want to remove group %{name}?") % { name: approver_group.group.name } - %button{ href: project_approver_group_path(project, approver_group), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove group") } - = icon("trash") - - if project.approvers.empty? && project.approver_groups.empty? - %li= _("There are no approvers") - -.form-group - = form.label :approvals_before_merge, class: 'label-bold' do - = _("Approvals required") - = form.number_field :approvals_before_merge, class: "form-control", min: 0 - .form-text.text-muted - = _("Set number of approvers required before open merge requests can be merged") +- if Feature.enabled?(:approval_rule, project) + = render 'shared/merge_request_approvals_settings/multiple_rules_form', form: form, project: project +- else + = render 'shared/merge_request_approvals_settings/single_rule_form', form: form, project: project .form-group .form-check diff --git a/ee/app/views/shared/merge_request_approvals_settings/_multiple_rules_form.html.haml b/ee/app/views/shared/merge_request_approvals_settings/_multiple_rules_form.html.haml new file mode 100644 index 00000000000000..1de91e37206b6c --- /dev/null +++ b/ee/app/views/shared/merge_request_approvals_settings/_multiple_rules_form.html.haml @@ -0,0 +1,6 @@ +.form-group + = form.label :approver_ids, class: 'label-bold' do + = _("Approvers") + #js-mr-approvals-settings{ data: { 'project_id': @project.id } } + .text-center.prepend-top-default + = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') diff --git a/ee/app/views/shared/merge_request_approvals_settings/_single_rule_form.html.haml b/ee/app/views/shared/merge_request_approvals_settings/_single_rule_form.html.haml new file mode 100644 index 00000000000000..20981d58abc128 --- /dev/null +++ b/ee/app/views/shared/merge_request_approvals_settings/_single_rule_form.html.haml @@ -0,0 +1,55 @@ +.form-group + = form.label :approver_ids, class: 'label-bold' do + = _("Approvers") + = hidden_field_tag "project[approver_ids]" + = hidden_field_tag "project[approver_group_ids]" + .input-group.input-btn-group + = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project', :style => "max-width: unset;" } + %button.btn.btn-success.js-add-approvers{ type: 'button', title: _('Add approver(s)') } + = _("Add") + .form-text.text-muted + = _("Add users or groups who are allowed to approve every merge request") + + .card.prepend-top-10.js-current-approvers + .load-wrapper.hidden + = icon('spinner spin', class: 'approver-list-loader') + .card-header + = _("Approvers") + %span.badge.badge-pill + - ids = [] + - project.approvers.each do |user| + - ids << user.user_id + - project.approver_groups.each do |group| + - group.users.each do |user| + - unless ids.include?(user.id) + - ids << user.id + = ids.count + %ul.content-list.approver-list + - project.approvers.each do |approver| + %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } } + = link_to approver.user.name, approver.user + .float-right + - confirm_message = _("Are you sure you want to remove approver %{name}?") % { name: approver.user.name } + %button{ href: project_approver_path(project, approver), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove approver") } + = icon("trash") + - project.approver_groups.each do |approver_group| + %li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } } + %span + %span.light + = _("Group:") + = link_to approver_group.group.name, approver_group.group + %span.badge.badge-pill + = approver_group.group.members.count + .float-right + - confirm_message = _("Are you sure you want to remove group %{name}?") % { name: approver_group.group.name } + %button{ href: project_approver_group_path(project, approver_group), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove group") } + = icon("trash") + - if project.approvers.empty? && project.approver_groups.empty? + %li= _("There are no approvers") + +.form-group + = form.label :approvals_before_merge, class: 'label-bold' do + = _("Approvals required") + = form.number_field :approvals_before_merge, class: "form-control", min: 0 + .form-text.text-muted + = _("Set number of approvers required before open merge requests can be merged") diff --git a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb index dd5d1f3a9cb772..c3d416321d2e05 100644 --- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -10,6 +10,8 @@ let(:non_member) { create(:user) } before do + stub_feature_flags(approval_rule: false) + sign_in(user) project.add_maintainer(user) group.add_developer(user) -- GitLab From 42e956b0c8f53b7792b33dd4a939ab4b52ae2400 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Tue, 18 Dec 2018 14:50:18 -0600 Subject: [PATCH 04/17] Create empty state approvals settings form **Note:** - This includes a stubbed service in the FE - This is an iteration towards https://gitlab.com/gitlab-org/gitlab-ee/issues/1979 --- .../components/approval_rules_empty.vue | 28 +++++ .../approvals/components/settings.vue | 29 +++++ ee/app/assets/javascripts/approvals/index.js | 23 ++++ .../services/approvals_service_stub.js | 20 ++++ .../javascripts/approvals/stores/actions.js | 34 ++++++ .../javascripts/approvals/stores/index.js | 11 ++ .../approvals/stores/mutation_types.js | 3 + .../javascripts/approvals/stores/mutations.js | 13 +++ .../javascripts/approvals/stores/state.js | 5 + .../components/approval_rules_empty_spec.js | 42 ++++++++ .../approvals/components/settings_spec.js | 71 ++++++++++++ .../approvals/stores/actions_spec.js | 102 ++++++++++++++++++ .../approvals/stores/mutations_spec.js | 43 ++++++++ locale/gitlab.pot | 12 +++ 14 files changed, 436 insertions(+) create mode 100644 ee/app/assets/javascripts/approvals/components/approval_rules_empty.vue create mode 100644 ee/app/assets/javascripts/approvals/components/settings.vue create mode 100644 ee/app/assets/javascripts/approvals/index.js create mode 100644 ee/app/assets/javascripts/approvals/services/approvals_service_stub.js create mode 100644 ee/app/assets/javascripts/approvals/stores/actions.js create mode 100644 ee/app/assets/javascripts/approvals/stores/index.js create mode 100644 ee/app/assets/javascripts/approvals/stores/mutation_types.js create mode 100644 ee/app/assets/javascripts/approvals/stores/mutations.js create mode 100644 ee/app/assets/javascripts/approvals/stores/state.js create mode 100644 ee/spec/javascripts/approvals/components/approval_rules_empty_spec.js create mode 100644 ee/spec/javascripts/approvals/components/settings_spec.js create mode 100644 ee/spec/javascripts/approvals/stores/actions_spec.js create mode 100644 ee/spec/javascripts/approvals/stores/mutations_spec.js diff --git a/ee/app/assets/javascripts/approvals/components/approval_rules_empty.vue b/ee/app/assets/javascripts/approvals/components/approval_rules_empty.vue new file mode 100644 index 00000000000000..251f8b8e346832 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/approval_rules_empty.vue @@ -0,0 +1,28 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/settings.vue b/ee/app/assets/javascripts/approvals/components/settings.vue new file mode 100644 index 00000000000000..e0d9f85f159d56 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/settings.vue @@ -0,0 +1,29 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/index.js b/ee/app/assets/javascripts/approvals/index.js new file mode 100644 index 00000000000000..71dde3690c0e01 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/index.js @@ -0,0 +1,23 @@ +import Vue from 'vue'; +import Vuex from 'vuex'; +import createStore from './stores'; +import Settings from './components/settings.vue'; + +Vue.use(Vuex); + +export default function mountApprovalsSettings(el) { + if (!el) { + return null; + } + + const store = createStore(); + store.dispatch('setSettings', el.dataset); + + return new Vue({ + el, + store, + render(h) { + return h(Settings); + }, + }); +} diff --git a/ee/app/assets/javascripts/approvals/services/approvals_service_stub.js b/ee/app/assets/javascripts/approvals/services/approvals_service_stub.js new file mode 100644 index 00000000000000..4288f874520664 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/services/approvals_service_stub.js @@ -0,0 +1,20 @@ +/** + * This provides a stubbed API for approval rule requests. + * + * **PLEASE NOTE:** + * - This class will be removed when the BE is merged for https://gitlab.com/gitlab-org/gitlab-ee/issues/1979 + */ + +export function createApprovalsServiceStub() { + const projectApprovalRules = []; + + return { + getProjectApprovalRules() { + return Promise.resolve({ + data: { rules: projectApprovalRules }, + }); + }, + }; +} + +export default createApprovalsServiceStub(); diff --git a/ee/app/assets/javascripts/approvals/stores/actions.js b/ee/app/assets/javascripts/approvals/stores/actions.js new file mode 100644 index 00000000000000..1b121b508783b0 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/actions.js @@ -0,0 +1,34 @@ +import createFlash from '~/flash'; +import { __ } from '~/locale'; +import * as types from './mutation_types'; +import service from '../services/approvals_service_stub'; + +export const setSettings = ({ commit }, settings) => { + commit(types.SET_SETTINGS, settings); +}; + +export const requestRules = ({ commit }) => { + commit(types.SET_LOADING, true); +}; + +export const receiveRulesSuccess = ({ commit }, { rules }) => { + commit(types.SET_RULES, rules); + commit(types.SET_LOADING, false); +}; + +export const receiveRulesError = () => { + createFlash(__('An error occurred fetching the approval rules.')); +}; + +export const fetchRules = ({ state, dispatch }) => { + if (state.isLoading) { + return; + } + + dispatch('requestRules'); + + service + .getProjectApprovalRules() + .then(response => dispatch('receiveRulesSuccess', response.data)) + .catch(() => dispatch('receiveRulesError')); +}; diff --git a/ee/app/assets/javascripts/approvals/stores/index.js b/ee/app/assets/javascripts/approvals/stores/index.js new file mode 100644 index 00000000000000..64c78d2cb4ab7d --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/index.js @@ -0,0 +1,11 @@ +import Vuex from 'vuex'; +import state from './state'; +import mutations from './mutations'; +import * as actions from './actions'; + +export default () => + new Vuex.Store({ + state: state(), + mutations, + actions, + }); diff --git a/ee/app/assets/javascripts/approvals/stores/mutation_types.js b/ee/app/assets/javascripts/approvals/stores/mutation_types.js new file mode 100644 index 00000000000000..ed91b8e4bea363 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/mutation_types.js @@ -0,0 +1,3 @@ +export const SET_SETTINGS = 'SET_SETTINGS'; +export const SET_LOADING = 'SET_LOADING'; +export const SET_RULES = 'SET_RULES'; diff --git a/ee/app/assets/javascripts/approvals/stores/mutations.js b/ee/app/assets/javascripts/approvals/stores/mutations.js new file mode 100644 index 00000000000000..228a4765ab4232 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/mutations.js @@ -0,0 +1,13 @@ +import * as types from './mutation_types'; + +export default { + [types.SET_SETTINGS](state, settings) { + state.settings = { ...settings }; + }, + [types.SET_LOADING](state, isLoading) { + state.isLoading = isLoading; + }, + [types.SET_RULES](state, rules) { + state.rules = rules; + }, +}; diff --git a/ee/app/assets/javascripts/approvals/stores/state.js b/ee/app/assets/javascripts/approvals/stores/state.js new file mode 100644 index 00000000000000..34bde76e505335 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/state.js @@ -0,0 +1,5 @@ +export default () => ({ + settings: {}, + isLoading: false, + rules: [], +}); diff --git a/ee/spec/javascripts/approvals/components/approval_rules_empty_spec.js b/ee/spec/javascripts/approvals/components/approval_rules_empty_spec.js new file mode 100644 index 00000000000000..47c65198080935 --- /dev/null +++ b/ee/spec/javascripts/approvals/components/approval_rules_empty_spec.js @@ -0,0 +1,42 @@ +import { createLocalVue, shallowMount } from '@vue/test-utils'; +import { GlButton } from '@gitlab/ui'; +import ApprovalRulesEmpty from 'ee/approvals/components/approval_rules_empty.vue'; + +const localVue = createLocalVue(); + +describe('EE ApprovalsSettingsEmpty', () => { + let wrapper; + + const factory = options => { + wrapper = shallowMount(localVue.extend(ApprovalRulesEmpty), { + localVue, + ...options, + }); + }; + + afterEach(() => { + wrapper.destroy(); + }); + + it('shows message', () => { + factory(); + + expect(wrapper.text()).toContain(ApprovalRulesEmpty.message); + }); + + it('shows button', () => { + factory(); + + expect(wrapper.find(GlButton).exists()).toBe(true); + }); + + it('emits "click" on button press', () => { + factory(); + + expect(wrapper.emittedByOrder().length).toEqual(0); + + wrapper.find(GlButton).vm.$emit('click'); + + expect(wrapper.emittedByOrder().map(x => x.name)).toEqual(['click']); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/settings_spec.js b/ee/spec/javascripts/approvals/components/settings_spec.js new file mode 100644 index 00000000000000..a01ae979bb1f0f --- /dev/null +++ b/ee/spec/javascripts/approvals/components/settings_spec.js @@ -0,0 +1,71 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import Vuex from 'vuex'; +import { GlLoadingIcon } from '@gitlab/ui'; +import ApprovalRulesEmpty from 'ee/approvals/components/approval_rules_empty.vue'; +import Settings from 'ee/approvals/components/settings.vue'; + +const localVue = createLocalVue(); +localVue.use(Vuex); + +describe('EE ApprovalsSettingsForm', () => { + let state; + let actions; + let wrapper; + + const factory = () => { + const store = new Vuex.Store({ + state, + actions, + }); + + wrapper = shallowMount(localVue.extend(Settings), { + localVue, + store, + sync: false, + }); + }; + + beforeEach(() => { + state = {}; + + actions = { + fetchRules: jasmine.createSpy('fetchRules'), + }; + }); + + it('dispatches fetchRules action on created', () => { + expect(actions.fetchRules).not.toHaveBeenCalled(); + + factory(); + + expect(actions.fetchRules).toHaveBeenCalledTimes(1); + }); + + it('shows loading icon if loading', () => { + state.isLoading = true; + factory(); + + expect(wrapper.find(GlLoadingIcon).exists()).toBe(true); + }); + + it('does not show loading icon if not loading', () => { + state.isLoading = false; + factory(); + + expect(wrapper.find(GlLoadingIcon).exists()).toBe(false); + }); + + it('shows ApprovalsSettingsEmpty if empty', () => { + state.rules = []; + factory(); + + expect(wrapper.find(ApprovalRulesEmpty).exists()).toBe(true); + }); + + it('does not show ApprovalsSettingsEmpty is not empty', () => { + state.rules = [{ id: 1 }]; + factory(); + + expect(wrapper.find(ApprovalRulesEmpty).exists()).toBe(false); + }); +}); diff --git a/ee/spec/javascripts/approvals/stores/actions_spec.js b/ee/spec/javascripts/approvals/stores/actions_spec.js new file mode 100644 index 00000000000000..05ee18d8d75794 --- /dev/null +++ b/ee/spec/javascripts/approvals/stores/actions_spec.js @@ -0,0 +1,102 @@ +import testAction from 'spec/helpers/vuex_action_helper'; +import * as types from 'ee/approvals/stores/mutation_types'; +import actionsModule, * as actions from 'ee/approvals/stores/actions'; +import service from 'ee/approvals/services/approvals_service_stub'; + +describe('EE approvals store actions', () => { + let flashSpy; + + beforeEach(() => { + flashSpy = spyOnDependency(actionsModule, 'createFlash'); + spyOn(service, 'getProjectApprovalRules'); + }); + + describe('setSettings', () => { + it('sets the settings', done => { + const settings = { projectId: 7 }; + + testAction( + actions.setSettings, + settings, + {}, + [{ type: types.SET_SETTINGS, payload: settings }], + [], + done, + ); + }); + }); + + describe('requestRules', () => { + it('sets loading', done => { + testAction( + actions.requestRules, + null, + {}, + [{ type: types.SET_LOADING, payload: true }], + [], + done, + ); + }); + }); + + describe('receiveRulesSuccess', () => { + it('sets rules', done => { + const rules = [{ id: 1 }]; + + testAction( + actions.receiveRulesSuccess, + { rules }, + {}, + [{ type: types.SET_RULES, payload: rules }, { type: types.SET_LOADING, payload: false }], + [], + done, + ); + }); + }); + + describe('receiveRulesError', () => { + it('creates a flash', () => { + expect(flashSpy).not.toHaveBeenCalled(); + + actions.receiveRulesError(); + + expect(flashSpy).toHaveBeenCalledTimes(1); + expect(flashSpy).toHaveBeenCalledWith(jasmine.stringMatching('error occurred')); + }); + }); + + describe('fetchRules', () => { + it('does nothing if loading', done => { + testAction(actions.fetchRules, null, { isLoading: true }, [], [], done); + }); + + it('dispatches request/receive', done => { + const response = { + data: { rules: [] }, + }; + service.getProjectApprovalRules.and.returnValue(Promise.resolve(response)); + + testAction( + actions.fetchRules, + null, + {}, + [], + [{ type: 'requestRules' }, { type: 'receiveRulesSuccess', payload: response.data }], + done, + ); + }); + + it('dispatches request/receive on error', done => { + service.getProjectApprovalRules.and.returnValue(Promise.reject()); + + testAction( + actions.fetchRules, + null, + {}, + [], + [{ type: 'requestRules' }, { type: 'receiveRulesError' }], + done, + ); + }); + }); +}); diff --git a/ee/spec/javascripts/approvals/stores/mutations_spec.js b/ee/spec/javascripts/approvals/stores/mutations_spec.js new file mode 100644 index 00000000000000..9c5a4f95a86ea8 --- /dev/null +++ b/ee/spec/javascripts/approvals/stores/mutations_spec.js @@ -0,0 +1,43 @@ +import createState from 'ee/approvals/stores/state'; +import * as types from 'ee/approvals/stores/mutation_types'; +import mutations from 'ee/approvals/stores/mutations'; + +describe('EE approvals store mutations', () => { + let state; + + beforeEach(() => { + state = createState(); + }); + + describe(types.SET_SETTINGS, () => { + it('sets the settings', () => { + const newSettings = { projectId: 7 }; + + mutations[types.SET_SETTINGS](state, newSettings); + + expect(state.settings).toEqual(newSettings); + }); + }); + + describe(types.SET_LOADING, () => { + it('sets isLoading', () => { + state.isLoading = false; + + mutations[types.SET_LOADING](state, true); + + expect(state.isLoading).toBe(true); + }); + }); + + describe(types.SET_RULES, () => { + it('sets rules', () => { + const newRules = [{ id: 1 }, { id: 2 }]; + + state.rules = []; + + mutations[types.SET_RULES](state, newRules); + + expect(state.rules).toEqual(newRules); + }); + }); +}); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3c2bdacfc76e07..5b4d2515eefb57 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -464,6 +464,12 @@ msgstr "" msgid "Add additional text to appear in all email communications. %{character_limit} character limit" msgstr "" +msgid "Add approver(s)" +msgstr "" + +msgid "Add approvers" +msgstr "" + msgid "Add comment now" msgstr "" @@ -730,6 +736,9 @@ msgstr "" msgid "An error occurred creating the new branch." msgstr "" +msgid "An error occurred fetching the approval rules." +msgstr "" + msgid "An error occurred previewing the blob" msgstr "" @@ -9457,6 +9466,9 @@ msgstr "" msgid "There are no approvers" msgstr "" +msgid "There are no approvers explicitly added for this project. Members who are of Developer role or higher and code owners (if any) are eligible to approve." +msgstr "" + msgid "There are no archived projects yet" msgstr "" -- GitLab From bc206c329275d374a1133909d6dc14a3aa107b5f Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sat, 5 Jan 2019 00:54:58 -0600 Subject: [PATCH 05/17] FE store for MR approvals settings **Note:** - This contains the service stub which is removed in a later commit - Adds modal modules which will ref components in a later commit --- .../assets/javascripts/approvals/mappers.js | 19 +++ .../javascripts/approvals/stores/actions.js | 43 ++++- .../javascripts/approvals/stores/index.js | 5 + .../approvals/stores/actions_spec.js | 149 +++++++++++++++++- locale/gitlab.pot | 6 + 5 files changed, 217 insertions(+), 5 deletions(-) create mode 100644 ee/app/assets/javascripts/approvals/mappers.js diff --git a/ee/app/assets/javascripts/approvals/mappers.js b/ee/app/assets/javascripts/approvals/mappers.js new file mode 100644 index 00000000000000..4d67822cf7bbc8 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/mappers.js @@ -0,0 +1,19 @@ +export const mapApprovalRuleRequest = req => ({ + name: req.name, + approvals_required: req.approvalsRequired, + users: req.users, + groups: req.groups, +}); + +export const mapApprovalRuleResponse = res => ({ + id: res.id, + name: res.name, + approvalsRequired: res.approvals_required, + approvers: res.approvers, + users: res.users, + groups: res.groups, +}); + +export const mapApprovalRulesResponse = req => ({ + rules: req.rules.map(mapApprovalRuleResponse), +}); diff --git a/ee/app/assets/javascripts/approvals/stores/actions.js b/ee/app/assets/javascripts/approvals/stores/actions.js index 1b121b508783b0..39a6385c041dbb 100644 --- a/ee/app/assets/javascripts/approvals/stores/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/actions.js @@ -2,6 +2,7 @@ import createFlash from '~/flash'; import { __ } from '~/locale'; import * as types from './mutation_types'; import service from '../services/approvals_service_stub'; +import { mapApprovalRuleRequest, mapApprovalRulesResponse } from '../mappers'; export const setSettings = ({ commit }, settings) => { commit(types.SET_SETTINGS, settings); @@ -22,13 +23,49 @@ export const receiveRulesError = () => { export const fetchRules = ({ state, dispatch }) => { if (state.isLoading) { - return; + return Promise.resolve(); } dispatch('requestRules'); - service + return service .getProjectApprovalRules() - .then(response => dispatch('receiveRulesSuccess', response.data)) + .then(response => dispatch('receiveRulesSuccess', mapApprovalRulesResponse(response.data))) .catch(() => dispatch('receiveRulesError')); }; + +export const postRuleSuccess = ({ dispatch }) => { + dispatch('createModal/close'); + dispatch('fetchRules'); +}; + +export const postRuleError = () => { + createFlash(__('An error occurred while updating approvers')); +}; + +export const postRule = ({ dispatch }, rule) => + service + .postProjectApprovalRule(mapApprovalRuleRequest(rule)) + .then(() => dispatch('postRuleSuccess')) + .catch(() => dispatch('postRuleError')); + +export const putRule = ({ dispatch }, { id, ...newRule }) => + service + .putProjectApprovalRule(id, mapApprovalRuleRequest(newRule)) + .then(() => dispatch('postRuleSuccess')) + .catch(() => dispatch('postRuleError')); + +export const deleteRuleSuccess = ({ dispatch }) => { + dispatch('deleteModal/close'); + dispatch('fetchRules'); +}; + +export const deleteRuleError = () => { + createFlash(__('An error occurred while deleting the approvers group')); +}; + +export const deleteRule = ({ dispatch }, id) => + service + .deleteProjectApprovalRule(id) + .then(() => dispatch('deleteRuleSuccess')) + .catch(() => dispatch('deleteRuleError')); diff --git a/ee/app/assets/javascripts/approvals/stores/index.js b/ee/app/assets/javascripts/approvals/stores/index.js index 64c78d2cb4ab7d..8d34f826efc3f7 100644 --- a/ee/app/assets/javascripts/approvals/stores/index.js +++ b/ee/app/assets/javascripts/approvals/stores/index.js @@ -1,4 +1,5 @@ import Vuex from 'vuex'; +import modalModule from '~/vuex_shared/modules/modal'; import state from './state'; import mutations from './mutations'; import * as actions from './actions'; @@ -8,4 +9,8 @@ export default () => state: state(), mutations, actions, + modules: { + createModal: modalModule(), + deleteModal: modalModule(), + }, }); diff --git a/ee/spec/javascripts/approvals/stores/actions_spec.js b/ee/spec/javascripts/approvals/stores/actions_spec.js index 05ee18d8d75794..08348ca7459aea 100644 --- a/ee/spec/javascripts/approvals/stores/actions_spec.js +++ b/ee/spec/javascripts/approvals/stores/actions_spec.js @@ -2,6 +2,23 @@ import testAction from 'spec/helpers/vuex_action_helper'; import * as types from 'ee/approvals/stores/mutation_types'; import actionsModule, * as actions from 'ee/approvals/stores/actions'; import service from 'ee/approvals/services/approvals_service_stub'; +import { mapApprovalRuleRequest, mapApprovalRulesResponse } from 'ee/approvals/mappers'; + +const TEST_RULE_ID = 7; +const TEST_RULE_REQUEST = { + name: 'Lorem', + approvalsRequired: 1, + groups: [7], + users: [8, 9], +}; +const TEST_RULE_RESPONSE = { + id: 7, + name: 'Ipsum', + approvals_required: 2, + approvers: [{ id: 7 }, { id: 8 }, { id: 9 }], + groups: [{ id: 4 }], + users: [{ id: 7 }, { id: 8 }], +}; describe('EE approvals store actions', () => { let flashSpy; @@ -9,6 +26,9 @@ describe('EE approvals store actions', () => { beforeEach(() => { flashSpy = spyOnDependency(actionsModule, 'createFlash'); spyOn(service, 'getProjectApprovalRules'); + spyOn(service, 'postProjectApprovalRule'); + spyOn(service, 'putProjectApprovalRule'); + spyOn(service, 'deleteProjectApprovalRule'); }); describe('setSettings', () => { @@ -72,7 +92,7 @@ describe('EE approvals store actions', () => { it('dispatches request/receive', done => { const response = { - data: { rules: [] }, + data: { rules: [TEST_RULE_RESPONSE] }, }; service.getProjectApprovalRules.and.returnValue(Promise.resolve(response)); @@ -81,7 +101,10 @@ describe('EE approvals store actions', () => { null, {}, [], - [{ type: 'requestRules' }, { type: 'receiveRulesSuccess', payload: response.data }], + [ + { type: 'requestRules' }, + { type: 'receiveRulesSuccess', payload: mapApprovalRulesResponse(response.data) }, + ], done, ); }); @@ -99,4 +122,126 @@ describe('EE approvals store actions', () => { ); }); }); + + describe('postRuleSuccess', () => { + it('closes modal and fetches', done => { + testAction( + actions.postRuleSuccess, + null, + {}, + [], + [{ type: 'createModal/close' }, { type: 'fetchRules' }], + done, + ); + }); + }); + + describe('postRuleError', () => { + it('creates a flash', () => { + expect(flashSpy).not.toHaveBeenCalled(); + + actions.postRuleError(); + + expect(flashSpy.calls.allArgs()).toEqual([[jasmine.stringMatching('error occurred')]]); + }); + }); + + describe('postRule', () => { + it('dispatches success on success', done => { + service.postProjectApprovalRule.and.callFake(data => { + expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); + return Promise.resolve(); + }); + + testAction(actions.postRule, TEST_RULE_REQUEST, {}, [], [{ type: 'postRuleSuccess' }], done); + }); + + it('dispatches error on error', done => { + service.postProjectApprovalRule.and.callFake(data => { + expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); + return Promise.reject(); + }); + + testAction(actions.postRule, TEST_RULE_REQUEST, {}, [], [{ type: 'postRuleError' }], done); + }); + }); + + describe('putRule', () => { + it('dispatches success on success', done => { + service.putProjectApprovalRule.and.callFake((id, data) => { + expect(id).toBe(TEST_RULE_ID); + expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); + return Promise.resolve(); + }); + + testAction( + actions.putRule, + { id: TEST_RULE_ID, ...TEST_RULE_REQUEST }, + {}, + [], + [{ type: 'postRuleSuccess' }], + done, + ); + }); + + it('dispatches error on error', done => { + service.putProjectApprovalRule.and.callFake((id, data) => { + expect(id).toBe(TEST_RULE_ID); + expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); + return Promise.reject(); + }); + + testAction( + actions.putRule, + { id: TEST_RULE_ID, ...TEST_RULE_REQUEST }, + {}, + [], + [{ type: 'postRuleError' }], + done, + ); + }); + }); + + describe('deleteRuleSuccess', () => { + it('closes modal and fetches', done => { + testAction( + actions.deleteRuleSuccess, + null, + {}, + [], + [{ type: 'deleteModal/close' }, { type: 'fetchRules' }], + done, + ); + }); + }); + + describe('deleteRuleError', () => { + it('creates a flash', () => { + expect(flashSpy).not.toHaveBeenCalled(); + + actions.deleteRuleError(); + + expect(flashSpy.calls.allArgs()).toEqual([[jasmine.stringMatching('error occurred')]]); + }); + }); + + describe('deleteRule', () => { + it('dispatches success on success', done => { + service.deleteProjectApprovalRule.and.callFake(id => { + expect(id).toBe(TEST_RULE_ID); + return Promise.resolve(); + }); + + testAction(actions.deleteRule, TEST_RULE_ID, {}, [], [{ type: 'deleteRuleSuccess' }], done); + }); + + it('dispatches error on error', done => { + service.deleteProjectApprovalRule.and.callFake(id => { + expect(id).toBe(TEST_RULE_ID); + return Promise.reject(); + }); + + testAction(actions.deleteRule, TEST_RULE_ID, {}, [], [{ type: 'deleteRuleError' }], done); + }); + }); }); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5b4d2515eefb57..c271d9e049bf58 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -751,6 +751,9 @@ msgstr "" msgid "An error occurred while adding approver" msgstr "" +msgid "An error occurred while deleting the approvers group" +msgstr "" + msgid "An error occurred while deleting the comment" msgstr "" @@ -850,6 +853,9 @@ msgstr "" msgid "An error occurred while unsubscribing to notifications." msgstr "" +msgid "An error occurred while updating approvers" +msgstr "" + msgid "An error occurred while updating the comment" msgstr "" -- GitLab From c2cc451fc05b1bff1286002816ca4f248c29eba0 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sun, 30 Dec 2018 16:05:34 -0600 Subject: [PATCH 06/17] Integrate approval rules settings FE with BE - Adds endpoints to API - Removes service stub --- ee/app/assets/javascripts/api.js | 41 ++++++ .../services/approvals_service_stub.js | 20 --- .../javascripts/approvals/stores/actions.js | 31 +++-- ee/spec/javascripts/api_spec.js | 90 +++++++++++++ .../approvals/stores/actions_spec.js | 120 +++++++++++------- 5 files changed, 227 insertions(+), 75 deletions(-) create mode 100644 ee/app/assets/javascripts/api.js delete mode 100644 ee/app/assets/javascripts/approvals/services/approvals_service_stub.js create mode 100644 ee/spec/javascripts/api_spec.js diff --git a/ee/app/assets/javascripts/api.js b/ee/app/assets/javascripts/api.js new file mode 100644 index 00000000000000..aebea5a7f4bf09 --- /dev/null +++ b/ee/app/assets/javascripts/api.js @@ -0,0 +1,41 @@ +import CEApi from '~/api'; +import axios from '~/lib/utils/axios_utils'; + +export default { + ...CEApi, + projectApprovalRulesPath: '/api/:version/projects/:id/approval_rules', + projectApprovalRulePath: '/api/:version/projects/:id/approval_rules/:ruleid', + getProjectApprovalRules(projectId) { + const url = this.buildUrl(this.projectApprovalRulesPath).replace( + ':id', + encodeURIComponent(projectId), + ); + + return axios.get(url); + }, + + postProjectApprovalRule(projectId, rule) { + const url = this.buildUrl(this.projectApprovalRulesPath).replace( + ':id', + encodeURIComponent(projectId), + ); + + return axios.post(url, rule); + }, + + putProjectApprovalRule(projectId, ruleId, rule) { + const url = this.buildUrl(this.projectApprovalRulePath) + .replace(':id', encodeURIComponent(projectId)) + .replace(':ruleid', encodeURIComponent(ruleId)); + + return axios.put(url, rule); + }, + + deleteProjectApprovalRule(projectId, ruleId) { + const url = this.buildUrl(this.projectApprovalRulePath) + .replace(':id', encodeURIComponent(projectId)) + .replace(':ruleid', encodeURIComponent(ruleId)); + + return axios.delete(url); + }, +}; diff --git a/ee/app/assets/javascripts/approvals/services/approvals_service_stub.js b/ee/app/assets/javascripts/approvals/services/approvals_service_stub.js deleted file mode 100644 index 4288f874520664..00000000000000 --- a/ee/app/assets/javascripts/approvals/services/approvals_service_stub.js +++ /dev/null @@ -1,20 +0,0 @@ -/** - * This provides a stubbed API for approval rule requests. - * - * **PLEASE NOTE:** - * - This class will be removed when the BE is merged for https://gitlab.com/gitlab-org/gitlab-ee/issues/1979 - */ - -export function createApprovalsServiceStub() { - const projectApprovalRules = []; - - return { - getProjectApprovalRules() { - return Promise.resolve({ - data: { rules: projectApprovalRules }, - }); - }, - }; -} - -export default createApprovalsServiceStub(); diff --git a/ee/app/assets/javascripts/approvals/stores/actions.js b/ee/app/assets/javascripts/approvals/stores/actions.js index 39a6385c041dbb..6a133a851c7bc9 100644 --- a/ee/app/assets/javascripts/approvals/stores/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/actions.js @@ -1,7 +1,7 @@ import createFlash from '~/flash'; import { __ } from '~/locale'; +import Api from 'ee/api'; import * as types from './mutation_types'; -import service from '../services/approvals_service_stub'; import { mapApprovalRuleRequest, mapApprovalRulesResponse } from '../mappers'; export const setSettings = ({ commit }, settings) => { @@ -26,10 +26,11 @@ export const fetchRules = ({ state, dispatch }) => { return Promise.resolve(); } + const { projectId } = state.settings; + dispatch('requestRules'); - return service - .getProjectApprovalRules() + return Api.getProjectApprovalRules(projectId) .then(response => dispatch('receiveRulesSuccess', mapApprovalRulesResponse(response.data))) .catch(() => dispatch('receiveRulesError')); }; @@ -43,17 +44,21 @@ export const postRuleError = () => { createFlash(__('An error occurred while updating approvers')); }; -export const postRule = ({ dispatch }, rule) => - service - .postProjectApprovalRule(mapApprovalRuleRequest(rule)) +export const postRule = ({ state, dispatch }, rule) => { + const { projectId } = state.settings; + + return Api.postProjectApprovalRule(projectId, mapApprovalRuleRequest(rule)) .then(() => dispatch('postRuleSuccess')) .catch(() => dispatch('postRuleError')); +}; + +export const putRule = ({ state, dispatch }, { id, ...newRule }) => { + const { projectId } = state.settings; -export const putRule = ({ dispatch }, { id, ...newRule }) => - service - .putProjectApprovalRule(id, mapApprovalRuleRequest(newRule)) + return Api.putProjectApprovalRule(projectId, id, mapApprovalRuleRequest(newRule)) .then(() => dispatch('postRuleSuccess')) .catch(() => dispatch('postRuleError')); +}; export const deleteRuleSuccess = ({ dispatch }) => { dispatch('deleteModal/close'); @@ -64,8 +69,10 @@ export const deleteRuleError = () => { createFlash(__('An error occurred while deleting the approvers group')); }; -export const deleteRule = ({ dispatch }, id) => - service - .deleteProjectApprovalRule(id) +export const deleteRule = ({ state, dispatch }, id) => { + const { projectId } = state.settings; + + return Api.deleteProjectApprovalRule(projectId, id) .then(() => dispatch('deleteRuleSuccess')) .catch(() => dispatch('deleteRuleError')); +}; diff --git a/ee/spec/javascripts/api_spec.js b/ee/spec/javascripts/api_spec.js new file mode 100644 index 00000000000000..a0674c2539157a --- /dev/null +++ b/ee/spec/javascripts/api_spec.js @@ -0,0 +1,90 @@ +import MockAdapter from 'axios-mock-adapter'; +import axios from '~/lib/utils/axios_utils'; +import Api from 'ee/api'; +import { TEST_HOST } from 'spec/test_constants'; + +const TEST_API_VERSION = 'v3000'; +const TEST_PROJECT_ID = 17; +const TEST_RULE_ID = 22; + +describe('EE Api', () => { + let originalGon; + let mock; + + beforeEach(() => { + mock = new MockAdapter(axios); + originalGon = window.gon; + window.gon = { + api_version: TEST_API_VERSION, + relative_url_root: TEST_HOST, + }; + }); + + afterEach(() => { + mock.restore(); + window.gon = originalGon; + }); + + describe('getProjectApprovalRules', () => { + it('gets with projectApprovalRulesPath', done => { + const expectedData = { rules: [] }; + const expectedUrl = `${TEST_HOST}${Api.projectApprovalRulesPath}` + .replace(':version', TEST_API_VERSION) + .replace(':id', TEST_PROJECT_ID); + + mock.onGet(expectedUrl).reply(200, expectedData); + Api.getProjectApprovalRules(TEST_PROJECT_ID) + .then(response => { + expect(response.data).toEqual(expectedData); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('postProjectApprovalRule', () => { + it('posts with projectApprovalRulesPath', done => { + const expectedUrl = `${TEST_HOST}${Api.projectApprovalRulesPath}` + .replace(':version', TEST_API_VERSION) + .replace(':id', TEST_PROJECT_ID); + + mock.onPost(expectedUrl).reply(200); + Api.postProjectApprovalRule(TEST_PROJECT_ID) + .then(done) + .catch(done.fail); + }); + }); + + describe('putProjectApprovalRule', () => { + it('puts with projectApprovalRulePath', done => { + const rule = { name: 'Lorem' }; + const expectedUrl = `${TEST_HOST}${Api.projectApprovalRulePath}` + .replace(':version', TEST_API_VERSION) + .replace(':id', TEST_PROJECT_ID) + .replace(':ruleid', TEST_RULE_ID); + + mock.onPut(expectedUrl).reply(200); + Api.putProjectApprovalRule(TEST_PROJECT_ID, TEST_RULE_ID, rule) + .then(() => { + expect(mock.history.put.length).toBe(1); + expect(mock.history.put[0].data).toBe(JSON.stringify(rule)); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('deleteProjectApprovalRule', () => { + it('deletes with projectApprovalRulePath', done => { + const expectedUrl = `${TEST_HOST}${Api.projectApprovalRulePath}` + .replace(':version', TEST_API_VERSION) + .replace(':id', TEST_PROJECT_ID) + .replace(':ruleid', TEST_RULE_ID); + + mock.onDelete(expectedUrl).reply(200); + Api.deleteProjectApprovalRule(TEST_PROJECT_ID, TEST_RULE_ID) + .then(done) + .catch(done.fail); + }); + }); +}); diff --git a/ee/spec/javascripts/approvals/stores/actions_spec.js b/ee/spec/javascripts/approvals/stores/actions_spec.js index 08348ca7459aea..5a493f2db3632d 100644 --- a/ee/spec/javascripts/approvals/stores/actions_spec.js +++ b/ee/spec/javascripts/approvals/stores/actions_spec.js @@ -1,9 +1,10 @@ +import Api from 'ee/api'; import testAction from 'spec/helpers/vuex_action_helper'; import * as types from 'ee/approvals/stores/mutation_types'; import actionsModule, * as actions from 'ee/approvals/stores/actions'; -import service from 'ee/approvals/services/approvals_service_stub'; import { mapApprovalRuleRequest, mapApprovalRulesResponse } from 'ee/approvals/mappers'; +const TEST_PROJECT_ID = 9; const TEST_RULE_ID = 7; const TEST_RULE_REQUEST = { name: 'Lorem', @@ -21,14 +22,18 @@ const TEST_RULE_RESPONSE = { }; describe('EE approvals store actions', () => { + let state; let flashSpy; beforeEach(() => { + state = { + settings: { projectId: TEST_PROJECT_ID }, + }; flashSpy = spyOnDependency(actionsModule, 'createFlash'); - spyOn(service, 'getProjectApprovalRules'); - spyOn(service, 'postProjectApprovalRule'); - spyOn(service, 'putProjectApprovalRule'); - spyOn(service, 'deleteProjectApprovalRule'); + spyOn(Api, 'getProjectApprovalRules'); + spyOn(Api, 'postProjectApprovalRule'); + spyOn(Api, 'putProjectApprovalRule'); + spyOn(Api, 'deleteProjectApprovalRule'); }); describe('setSettings', () => { @@ -94,28 +99,32 @@ describe('EE approvals store actions', () => { const response = { data: { rules: [TEST_RULE_RESPONSE] }, }; - service.getProjectApprovalRules.and.returnValue(Promise.resolve(response)); + Api.getProjectApprovalRules.and.returnValue(Promise.resolve(response)); testAction( actions.fetchRules, null, - {}, + state, [], [ { type: 'requestRules' }, { type: 'receiveRulesSuccess', payload: mapApprovalRulesResponse(response.data) }, ], - done, + () => { + expect(Api.getProjectApprovalRules).toHaveBeenCalledWith(TEST_PROJECT_ID); + + done(); + }, ); }); it('dispatches request/receive on error', done => { - service.getProjectApprovalRules.and.returnValue(Promise.reject()); + Api.getProjectApprovalRules.and.returnValue(Promise.reject()); testAction( actions.fetchRules, null, - {}, + state, [], [{ type: 'requestRules' }, { type: 'receiveRulesError' }], done, @@ -148,53 +157,73 @@ describe('EE approvals store actions', () => { describe('postRule', () => { it('dispatches success on success', done => { - service.postProjectApprovalRule.and.callFake(data => { - expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); - return Promise.resolve(); - }); + Api.postProjectApprovalRule.and.returnValue(Promise.resolve()); - testAction(actions.postRule, TEST_RULE_REQUEST, {}, [], [{ type: 'postRuleSuccess' }], done); + testAction( + actions.postRule, + TEST_RULE_REQUEST, + state, + [], + [{ type: 'postRuleSuccess' }], + () => { + expect(Api.postProjectApprovalRule).toHaveBeenCalledWith( + TEST_PROJECT_ID, + mapApprovalRuleRequest(TEST_RULE_REQUEST), + ); + done(); + }, + ); }); it('dispatches error on error', done => { - service.postProjectApprovalRule.and.callFake(data => { - expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); - return Promise.reject(); - }); + Api.postProjectApprovalRule.and.returnValue(Promise.reject()); - testAction(actions.postRule, TEST_RULE_REQUEST, {}, [], [{ type: 'postRuleError' }], done); + testAction( + actions.postRule, + TEST_RULE_REQUEST, + state, + [], + [{ type: 'postRuleError' }], + () => { + expect(Api.postProjectApprovalRule).toHaveBeenCalledWith( + TEST_PROJECT_ID, + mapApprovalRuleRequest(TEST_RULE_REQUEST), + ); + + done(); + }, + ); }); }); describe('putRule', () => { it('dispatches success on success', done => { - service.putProjectApprovalRule.and.callFake((id, data) => { - expect(id).toBe(TEST_RULE_ID); - expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); - return Promise.resolve(); - }); + Api.putProjectApprovalRule.and.returnValue(Promise.resolve()); testAction( actions.putRule, { id: TEST_RULE_ID, ...TEST_RULE_REQUEST }, - {}, + state, [], [{ type: 'postRuleSuccess' }], - done, + () => { + expect(Api.putProjectApprovalRule).toHaveBeenCalledWith( + TEST_PROJECT_ID, + TEST_RULE_ID, + mapApprovalRuleRequest(TEST_RULE_REQUEST), + ); + done(); + }, ); }); it('dispatches error on error', done => { - service.putProjectApprovalRule.and.callFake((id, data) => { - expect(id).toBe(TEST_RULE_ID); - expect(data).toEqual(mapApprovalRuleRequest(TEST_RULE_REQUEST)); - return Promise.reject(); - }); + Api.putProjectApprovalRule.and.returnValue(Promise.reject()); testAction( actions.putRule, { id: TEST_RULE_ID, ...TEST_RULE_REQUEST }, - {}, + state, [], [{ type: 'postRuleError' }], done, @@ -227,21 +256,26 @@ describe('EE approvals store actions', () => { describe('deleteRule', () => { it('dispatches success on success', done => { - service.deleteProjectApprovalRule.and.callFake(id => { - expect(id).toBe(TEST_RULE_ID); - return Promise.resolve(); - }); + Api.deleteProjectApprovalRule.and.returnValue(Promise.resolve()); - testAction(actions.deleteRule, TEST_RULE_ID, {}, [], [{ type: 'deleteRuleSuccess' }], done); + testAction( + actions.deleteRule, + TEST_RULE_ID, + state, + [], + [{ type: 'deleteRuleSuccess' }], + () => { + expect(Api.deleteProjectApprovalRule).toHaveBeenCalledWith(TEST_PROJECT_ID, TEST_RULE_ID); + + done(); + }, + ); }); it('dispatches error on error', done => { - service.deleteProjectApprovalRule.and.callFake(id => { - expect(id).toBe(TEST_RULE_ID); - return Promise.reject(); - }); + Api.deleteProjectApprovalRule.and.returnValue(Promise.reject()); - testAction(actions.deleteRule, TEST_RULE_ID, {}, [], [{ type: 'deleteRuleError' }], done); + testAction(actions.deleteRule, TEST_RULE_ID, state, [], [{ type: 'deleteRuleError' }], done); }); }); }); -- GitLab From 9aef49710449116d67c70ece1c3370eeae347810 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sat, 5 Jan 2019 00:59:45 -0600 Subject: [PATCH 07/17] FE components for MR approvals settings See issue: https://gitlab.com/gitlab-org/gitlab-ee/issues/1979 --- .../approvals/components/approvers_list.vue | 35 ++++ .../components/approvers_list_empty.vue | 7 + .../components/approvers_list_item.vue | 44 ++++ .../approvals/components/approvers_select.vue | 153 ++++++++++++++ .../components/modal_rule_create.vue | 46 +++++ .../components/modal_rule_remove.vue | 71 +++++++ .../approvals/components/rule_form.vue | 168 ++++++++++++++++ .../approvals/components/rules.vue | 73 +++++++ ...proval_rules_empty.vue => rules_empty.vue} | 0 .../approvals/components/settings.vue | 46 ++++- .../assets/javascripts/approvals/constants.js | 2 + .../components/approvers_list_item_spec.js | 72 +++++++ .../components/approvers_list_spec.js | 73 +++++++ .../components/approvers_select_spec.js | 186 +++++++++++++++++ .../components/modal_rule_create_spec.js | 116 +++++++++++ .../components/modal_rule_remove_spec.js | 102 ++++++++++ .../approvals/components/rule_form_spec.js | 188 ++++++++++++++++++ ...ules_empty_spec.js => rules_empty_spec.js} | 6 +- .../approvals/components/rules_spec.js | 97 +++++++++ .../approvals/components/settings_spec.js | 135 +++++++++++-- locale/gitlab.pot | 54 +++++ 21 files changed, 1650 insertions(+), 24 deletions(-) create mode 100644 ee/app/assets/javascripts/approvals/components/approvers_list.vue create mode 100644 ee/app/assets/javascripts/approvals/components/approvers_list_empty.vue create mode 100644 ee/app/assets/javascripts/approvals/components/approvers_list_item.vue create mode 100644 ee/app/assets/javascripts/approvals/components/approvers_select.vue create mode 100644 ee/app/assets/javascripts/approvals/components/modal_rule_create.vue create mode 100644 ee/app/assets/javascripts/approvals/components/modal_rule_remove.vue create mode 100644 ee/app/assets/javascripts/approvals/components/rule_form.vue create mode 100644 ee/app/assets/javascripts/approvals/components/rules.vue rename ee/app/assets/javascripts/approvals/components/{approval_rules_empty.vue => rules_empty.vue} (100%) create mode 100644 ee/app/assets/javascripts/approvals/constants.js create mode 100644 ee/spec/javascripts/approvals/components/approvers_list_item_spec.js create mode 100644 ee/spec/javascripts/approvals/components/approvers_list_spec.js create mode 100644 ee/spec/javascripts/approvals/components/approvers_select_spec.js create mode 100644 ee/spec/javascripts/approvals/components/modal_rule_create_spec.js create mode 100644 ee/spec/javascripts/approvals/components/modal_rule_remove_spec.js create mode 100644 ee/spec/javascripts/approvals/components/rule_form_spec.js rename ee/spec/javascripts/approvals/components/{approval_rules_empty_spec.js => rules_empty_spec.js} (77%) create mode 100644 ee/spec/javascripts/approvals/components/rules_spec.js diff --git a/ee/app/assets/javascripts/approvals/components/approvers_list.vue b/ee/app/assets/javascripts/approvals/components/approvers_list.vue new file mode 100644 index 00000000000000..bf2d57f84bd4a0 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/approvers_list.vue @@ -0,0 +1,35 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/approvers_list_empty.vue b/ee/app/assets/javascripts/approvals/components/approvers_list_empty.vue new file mode 100644 index 00000000000000..d1ac017f5517aa --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/approvers_list_empty.vue @@ -0,0 +1,7 @@ + diff --git a/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue b/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue new file mode 100644 index 00000000000000..15cdbf86b0e9c0 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue @@ -0,0 +1,44 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/approvers_select.vue b/ee/app/assets/javascripts/approvals/components/approvers_select.vue new file mode 100644 index 00000000000000..07b5d3bc2a7982 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/approvers_select.vue @@ -0,0 +1,153 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/modal_rule_create.vue b/ee/app/assets/javascripts/approvals/components/modal_rule_create.vue new file mode 100644 index 00000000000000..ba4a90eb56e542 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/modal_rule_create.vue @@ -0,0 +1,46 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/modal_rule_remove.vue b/ee/app/assets/javascripts/approvals/components/modal_rule_remove.vue new file mode 100644 index 00000000000000..c45d8f07cf6baa --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/modal_rule_remove.vue @@ -0,0 +1,71 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/rule_form.vue b/ee/app/assets/javascripts/approvals/components/rule_form.vue new file mode 100644 index 00000000000000..10e6d8e148f819 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/rule_form.vue @@ -0,0 +1,168 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/rules.vue b/ee/app/assets/javascripts/approvals/components/rules.vue new file mode 100644 index 00000000000000..2e0720e897d6d7 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/rules.vue @@ -0,0 +1,73 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/approval_rules_empty.vue b/ee/app/assets/javascripts/approvals/components/rules_empty.vue similarity index 100% rename from ee/app/assets/javascripts/approvals/components/approval_rules_empty.vue rename to ee/app/assets/javascripts/approvals/components/rules_empty.vue diff --git a/ee/app/assets/javascripts/approvals/components/settings.vue b/ee/app/assets/javascripts/approvals/components/settings.vue index e0d9f85f159d56..d9b931553630f5 100644 --- a/ee/app/assets/javascripts/approvals/components/settings.vue +++ b/ee/app/assets/javascripts/approvals/components/settings.vue @@ -1,12 +1,22 @@ diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js new file mode 100644 index 00000000000000..23e2998dad21ca --- /dev/null +++ b/ee/app/assets/javascripts/approvals/constants.js @@ -0,0 +1,2 @@ +export const TYPE_USER = 'user'; +export const TYPE_GROUP = 'group'; diff --git a/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js b/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js new file mode 100644 index 00000000000000..ec1c3a3ddf96ac --- /dev/null +++ b/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js @@ -0,0 +1,72 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import { GlButton } from '@gitlab/ui'; +import Avatar from '~/vue_shared/components/project_avatar/default.vue'; +import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants'; +import ApproversListItem from 'ee/approvals/components/approvers_list_item.vue'; + +const localVue = createLocalVue(); +const TEST_USER = { + id: 1, + type: TYPE_USER, + name: 'Lorem Ipsum', +}; +const TEST_GROUP = { + id: 1, + type: TYPE_GROUP, + name: 'Lorem Group', + full_path: 'dolar/sit/amit', +}; + +describe('Approvals ApproversListItem', () => { + let wrapper; + + const factory = (options = {}) => { + wrapper = shallowMount(localVue.extend(ApproversListItem), { + ...options, + localVue, + }); + }; + + describe('when user', () => { + beforeEach(() => { + factory({ + propsData: { + approver: TEST_USER, + }, + }); + }); + + it('renders avatar', () => { + const avatar = wrapper.find(Avatar); + + expect(avatar.exists()).toBe(true); + expect(avatar.props('project')).toEqual(TEST_USER); + }); + + it('renders name', () => { + expect(wrapper.text()).toContain(TEST_USER.name); + }); + + it('when remove clicked, emits remove', () => { + const button = wrapper.find(GlButton); + button.vm.$emit('click'); + + expect(wrapper.emittedByOrder()).toEqual([{ name: 'remove', args: [TEST_USER] }]); + }); + }); + + describe('when group', () => { + beforeEach(() => { + factory({ + propsData: { + approver: TEST_GROUP, + }, + }); + }); + + it('renders full_path', () => { + expect(wrapper.text()).toContain(TEST_GROUP.full_path); + expect(wrapper.text()).not.toContain(TEST_GROUP.name); + }); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/approvers_list_spec.js b/ee/spec/javascripts/approvals/components/approvers_list_spec.js new file mode 100644 index 00000000000000..0ba9b4253fc0eb --- /dev/null +++ b/ee/spec/javascripts/approvals/components/approvers_list_spec.js @@ -0,0 +1,73 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import ApproversListEmpty from 'ee/approvals/components/approvers_list_empty.vue'; +import ApproversListItem from 'ee/approvals/components/approvers_list_item.vue'; +import ApproversList from 'ee/approvals/components/approvers_list.vue'; +import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants'; + +const localVue = createLocalVue(); +const TEST_APPROVERS = [ + { id: 1, type: TYPE_GROUP }, + { id: 1, type: TYPE_USER }, + { id: 2, type: TYPE_USER }, +]; + +describe('ApproversList', () => { + let propsData; + let wrapper; + + const factory = (options = {}) => { + wrapper = shallowMount(localVue.extend(ApproversList), { + ...options, + localVue, + propsData, + }); + }; + + beforeEach(() => { + propsData = {}; + }); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('when empty', () => { + beforeEach(() => { + propsData.value = []; + }); + + it('renders empty', () => { + factory(); + + expect(wrapper.find(ApproversListEmpty).exists()).toBe(true); + expect(wrapper.find('ul').exists()).toBe(false); + }); + }); + + describe('when not empty', () => { + beforeEach(() => { + propsData.value = TEST_APPROVERS; + }); + + it('renders items', () => { + factory(); + + const items = wrapper.findAll(ApproversListItem).wrappers.map(item => item.props('approver')); + + expect(items).toEqual(TEST_APPROVERS); + }); + + TEST_APPROVERS.forEach((approver, idx) => { + it(`when remove (${idx}), emits new input`, () => { + factory(); + + const item = wrapper.findAll(ApproversListItem).at(idx); + item.vm.$emit('remove', approver); + + const expected = TEST_APPROVERS.filter((x, i) => i !== idx); + + expect(wrapper.emittedByOrder()).toEqual([{ name: 'input', args: [expected] }]); + }); + }); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/approvers_select_spec.js b/ee/spec/javascripts/approvals/components/approvers_select_spec.js new file mode 100644 index 00000000000000..d54c0260e5caff --- /dev/null +++ b/ee/spec/javascripts/approvals/components/approvers_select_spec.js @@ -0,0 +1,186 @@ +import { createLocalVue, mount } from '@vue/test-utils'; +import $ from 'jquery'; +import Api from 'ee/api'; +import ApproversSelect from 'ee/approvals/components/approvers_select.vue'; +import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants'; +import { TEST_HOST } from 'spec/test_constants'; + +const DEBOUNCE_TIME = 250; +const TEST_PROJECT_ID = '17'; +const TEST_GROUP_AVATAR = `${TEST_HOST}/group-avatar.png`; +const TEST_USER_AVATAR = `${TEST_HOST}/user-avatar.png`; +const TEST_GROUPS = [ + { id: 1, full_name: 'GitLab Org', full_path: 'gitlab/org', avatar_url: null }, + { + id: 2, + full_name: 'Lorem Ipsum', + full_path: 'lorem-ipsum', + avatar_url: TEST_GROUP_AVATAR, + }, +]; +const TEST_USERS = [ + { id: 1, name: 'Dolar', username: 'dolar', avatar_url: TEST_USER_AVATAR }, + { id: 3, name: 'Sit', username: 'sit', avatar_url: TEST_USER_AVATAR }, +]; + +const localVue = createLocalVue(); + +const waitForEvent = ($input, event) => new Promise(resolve => $input.one(event, resolve)); +const parseAvatar = element => (element.classList.contains('identicon') ? null : element.src); +const select2Container = () => document.querySelector('.select2-container'); +const select2DropdownOptions = () => document.querySelectorAll('#select2-drop .user-result'); +const select2DropdownItems = () => + Array.prototype.map.call(select2DropdownOptions(), element => { + const isGroup = element.classList.contains('group-result'); + const avatar = parseAvatar(element.querySelector('.avatar')); + + return isGroup + ? { + avatar_url: avatar, + full_name: element.querySelector('.group-name').textContent, + full_path: element.querySelector('.group-path').textContent, + } + : { + avatar_url: avatar, + name: element.querySelector('.user-name').textContent, + username: element.querySelector('.user-username').textContent, + }; + }); + +describe('Approvals ApproversSelect', () => { + let wrapper; + let $input; + + const factory = (options = {}) => { + const propsData = { + projectId: TEST_PROJECT_ID, + ...options.propsData, + }; + + wrapper = mount(localVue.extend(ApproversSelect), { + ...options, + propsData, + localVue, + attachToDocument: true, + }); + + $input = $(wrapper.vm.$refs.input); + }; + const search = (term = '') => { + $input.select2('search', term); + jasmine.clock().tick(DEBOUNCE_TIME); + }; + + beforeEach(() => { + jasmine.clock().install(); + spyOn(Api, 'groups').and.returnValue(Promise.resolve(TEST_GROUPS)); + spyOn(Api, 'approverUsers').and.returnValue(Promise.resolve(TEST_USERS)); + }); + + afterEach(() => { + jasmine.clock().uninstall(); + wrapper.destroy(); + }); + + it('renders select2 input', () => { + expect(select2Container()).toBe(null); + + factory(); + + expect(select2Container()).not.toBe(null); + }); + + it('queries and displays groups and users', done => { + factory(); + + const expected = TEST_GROUPS.concat(TEST_USERS) + .map(({ id, ...obj }) => obj) + .map(({ username, ...obj }) => (!username ? obj : { ...obj, username: `@${username}` })); + + waitForEvent($input, 'select2-loaded') + .then(() => { + const items = select2DropdownItems(); + + expect(items).toEqual(expected); + }) + .then(done) + .catch(done.fail); + + search(); + }); + + it('searches with text and skips given ids', done => { + factory(); + + const term = 'lorem'; + + waitForEvent($input, 'select2-loaded') + .then(() => { + expect(Api.groups).toHaveBeenCalledWith(term, { skip_groups: [] }); + expect(Api.approverUsers).toHaveBeenCalledWith(term, { + skip_users: [], + project_id: TEST_PROJECT_ID, + }); + }) + .then(done) + .catch(done.fail); + + search(term); + }); + + it('searches and skips given groups and users', done => { + const skipGroupIds = [7, 8]; + const skipUserIds = [9, 10]; + + factory({ + propsData: { + skipGroupIds, + skipUserIds, + }, + }); + + waitForEvent($input, 'select2-loaded') + .then(() => { + expect(Api.groups).toHaveBeenCalledWith('', { skip_groups: skipGroupIds }); + expect(Api.approverUsers).toHaveBeenCalledWith('', { + skip_users: skipUserIds, + project_id: TEST_PROJECT_ID, + }); + }) + .then(done) + .catch(done.fail); + + search(); + }); + + it('emits input when data changes', done => { + factory(); + + const expectedFinal = [ + { ...TEST_USERS[0], type: TYPE_USER }, + { ...TEST_GROUPS[0], type: TYPE_GROUP }, + ]; + const expected = expectedFinal.map((x, idx) => ({ + name: 'input', + args: [expectedFinal.slice(0, idx + 1)], + })); + + waitForEvent($input, 'select2-loaded') + .then(() => { + const options = select2DropdownOptions(); + $(options[TEST_GROUPS.length]).trigger('mouseup'); + $(options[0]).trigger('mouseup'); + }) + .then(done) + .catch(done.fail); + + waitForEvent($input, 'change') + .then(() => { + expect(wrapper.emittedByOrder()).toEqual(expected); + }) + .then(done) + .catch(done.fail); + + search(); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/modal_rule_create_spec.js b/ee/spec/javascripts/approvals/components/modal_rule_create_spec.js new file mode 100644 index 00000000000000..ce614cd303b6dd --- /dev/null +++ b/ee/spec/javascripts/approvals/components/modal_rule_create_spec.js @@ -0,0 +1,116 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import Vuex from 'vuex'; +import GlModalVuex from '~/vue_shared/components/gl_modal_vuex.vue'; +import RuleForm from 'ee/approvals/components/rule_form.vue'; +import ModalRuleCreate from 'ee/approvals/components/modal_rule_create.vue'; + +const TEST_MODAL_ID = 'test-modal-create-id'; +const TEST_RULE = { id: 7 }; +const MODAL_MODULE = 'createModal'; + +const localVue = createLocalVue(); +localVue.use(Vuex); + +describe('Approvals ModalRuleCreate', () => { + let createModalState; + let wrapper; + + const factory = (options = {}) => { + const store = new Vuex.Store({ + modules: { + [MODAL_MODULE]: { + namespaced: true, + state: createModalState, + }, + }, + }); + + const propsData = { + modalId: TEST_MODAL_ID, + ...options.propsData, + }; + + wrapper = shallowMount(localVue.extend(ModalRuleCreate), { + ...options, + localVue, + store, + propsData, + }); + }; + + beforeEach(() => { + createModalState = {}; + }); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('without data', () => { + beforeEach(() => { + createModalState.data = null; + }); + + it('renders modal', () => { + factory(); + + const modal = wrapper.find(GlModalVuex); + + expect(modal.exists()).toBe(true); + + expect(modal.props('modalModule')).toEqual(MODAL_MODULE); + expect(modal.props('modalId')).toEqual(TEST_MODAL_ID); + expect(modal.attributes('title')).toEqual('Add approvers'); + expect(modal.attributes('ok-title')).toEqual('Add approvers'); + }); + + it('renders form', () => { + factory(); + + const modal = wrapper.find(GlModalVuex); + const form = modal.find(RuleForm); + + expect(form.exists()).toBe(true); + expect(form.props('initRule')).toEqual(null); + }); + + it('when modal emits ok, submits form', () => { + factory(); + + const form = wrapper.find(RuleForm); + form.vm.submit = jasmine.createSpy('submit'); + + const modal = wrapper.find(GlModalVuex); + modal.vm.$emit('ok', new Event('ok')); + + expect(form.vm.submit).toHaveBeenCalled(); + }); + }); + + describe('with data', () => { + beforeEach(() => { + createModalState.data = TEST_RULE; + }); + + it('renders modal', () => { + factory(); + + const modal = wrapper.find(GlModalVuex); + + expect(modal.exists()).toBe(true); + + expect(modal.attributes('title')).toEqual('Update approvers'); + expect(modal.attributes('ok-title')).toEqual('Update approvers'); + }); + + it('renders form', () => { + factory(); + + const modal = wrapper.find(GlModalVuex); + const form = modal.find(RuleForm); + + expect(form.exists()).toBe(true); + expect(form.props('initRule')).toEqual(TEST_RULE); + }); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/modal_rule_remove_spec.js b/ee/spec/javascripts/approvals/components/modal_rule_remove_spec.js new file mode 100644 index 00000000000000..02674f60697409 --- /dev/null +++ b/ee/spec/javascripts/approvals/components/modal_rule_remove_spec.js @@ -0,0 +1,102 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import Vuex from 'vuex'; +import ModalRuleRemove from 'ee/approvals/components/modal_rule_remove.vue'; +import GlModalVuex from '~/vue_shared/components/gl_modal_vuex.vue'; + +const MODAL_MODULE = 'deleteModal'; +const TEST_MODAL_ID = 'test-delete-modal-id'; +const TEST_RULE = { + id: 7, + name: 'Lorem', + approvers: Array(5) + .fill(1) + .map((x, id) => ({ id })), +}; + +const localVue = createLocalVue(); +localVue.use(Vuex); + +describe('Approvals ModalRuleRemove', () => { + let wrapper; + let actions; + let deleteModalState; + + const factory = (options = {}) => { + const store = new Vuex.Store({ + actions, + modules: { + [MODAL_MODULE]: { + namespaced: true, + state: deleteModalState, + }, + }, + }); + + const propsData = { + modalId: TEST_MODAL_ID, + ...options.propsData, + }; + + wrapper = shallowMount(localVue.extend(ModalRuleRemove), { + ...options, + localVue, + store, + propsData, + }); + }; + + beforeEach(() => { + deleteModalState = { + data: TEST_RULE, + }; + actions = { + deleteRule: jasmine.createSpy('deleteRule'), + }; + }); + + it('renders modal', () => { + factory(); + + const modal = wrapper.find(GlModalVuex); + + expect(modal.exists()).toBe(true); + expect(modal.props()).toEqual( + jasmine.objectContaining({ + modalModule: MODAL_MODULE, + modalId: TEST_MODAL_ID, + }), + ); + }); + + it('shows message', () => { + factory(); + + const modal = wrapper.find(GlModalVuex); + + expect(modal.text()).toContain(TEST_RULE.name); + expect(modal.text()).toContain(`${TEST_RULE.approvers.length} members`); + }); + + it('shows singular message', () => { + deleteModalState.data = { + ...TEST_RULE, + approvers: [{ id: 1 }], + }; + factory(); + + const modal = wrapper.find(GlModalVuex); + + expect(modal.text()).toContain('1 member'); + }); + + it('deletes rule when modal is submitted', () => { + factory(); + + expect(actions.deleteRule).not.toHaveBeenCalled(); + + const modal = wrapper.find(GlModalVuex); + modal.vm.$emit('ok', new Event('submit')); + + expect(actions.deleteRule).toHaveBeenCalledWith(jasmine.anything(), TEST_RULE.id, undefined); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/rule_form_spec.js b/ee/spec/javascripts/approvals/components/rule_form_spec.js new file mode 100644 index 00000000000000..2d1b18d6798e84 --- /dev/null +++ b/ee/spec/javascripts/approvals/components/rule_form_spec.js @@ -0,0 +1,188 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import Vuex from 'vuex'; +import { GlButton } from '@gitlab/ui'; +import ApproversSelect from 'ee/approvals/components/approvers_select.vue'; +import RuleForm from 'ee/approvals/components/rule_form.vue'; +import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants'; + +const TEST_PROJECT_ID = '7'; +const TEST_RULE = { + id: 10, + name: 'QA', + approvalsRequired: 2, + users: [{ id: 1 }, { id: 2 }, { id: 3 }], + groups: [{ id: 1 }, { id: 2 }], +}; + +const localVue = createLocalVue(); +localVue.use(Vuex); + +const wrapInput = node => ({ + node, + feedback: () => node.element.nextElementSibling.textContent, + isValid: () => !node.classes('is-invalid'), +}); +const findInput = (form, selector) => wrapInput(form.find(selector)); +const findNameInput = form => findInput(form, 'input[name=name]'); +const findApprovalsRequiredInput = form => findInput(form, 'input[name=approvals_required]'); +const findApproversSelect = form => { + const input = findInput(form, ApproversSelect); + + return { + ...input, + isValid() { + return !input.node.props('isInvalid'); + }, + }; +}; + +describe('Approvals RuleForm', () => { + let state; + let actions; + let wrapper; + + const factory = (options = {}) => { + const store = new Vuex.Store({ + state, + actions, + }); + + wrapper = shallowMount(localVue.extend(RuleForm), { + ...options, + localVue, + store, + }); + }; + + beforeEach(() => { + state = { + settings: { projectId: TEST_PROJECT_ID }, + }; + + actions = { + postRule: jasmine.createSpy('postRule'), + putRule: jasmine.createSpy('putRule'), + }; + }); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('without initRule', () => { + beforeEach(() => { + factory(); + }); + + it('at first, shows no validation', () => { + const inputs = [ + findNameInput(wrapper), + findApprovalsRequiredInput(wrapper), + findApproversSelect(wrapper), + ]; + const invalidInputs = inputs.filter(x => !x.isValid()); + const feedbacks = inputs.map(x => x.feedback()); + + expect(invalidInputs.length).toBe(0); + expect(feedbacks.every(str => !str.length)).toBe(true); + }); + + it('on submit, does not dispatch action', () => { + wrapper.vm.submit(); + + expect(actions.postRule).not.toHaveBeenCalled(); + }); + + it('on submit, shows name validation', () => { + const { node, isValid, feedback } = findNameInput(wrapper); + node.setValue(''); + + wrapper.vm.submit(); + + expect(isValid()).toBe(false); + expect(feedback()).toEqual('Please provide a name'); + }); + + it('on submit, shows approvalsRequired validation', () => { + const { node, isValid, feedback } = findApprovalsRequiredInput(wrapper); + node.setValue(-1); + + wrapper.vm.submit(); + + expect(isValid()).toBe(false); + expect(feedback()).toEqual('Please enter a non-negative number'); + }); + + it('on submit, shows approvers validation', () => { + const { isValid, feedback } = findApproversSelect(wrapper); + wrapper.vm.approvers = []; + + wrapper.vm.submit(); + + expect(isValid()).toBe(false); + expect(feedback()).toEqual('Please select and add a member'); + }); + + it('on submit with data, posts rule', () => { + const expected = { + name: 'Lorem', + approvalsRequired: 2, + users: [1, 2], + groups: [2, 3], + }; + const name = findNameInput(wrapper); + const approvalsRequired = findApprovalsRequiredInput(wrapper); + + const groups = expected.groups.map(id => ({ id, type: TYPE_GROUP })); + const users = expected.users.map(id => ({ id, type: TYPE_USER })); + + name.node.setValue(expected.name); + approvalsRequired.node.setValue(expected.approvalsRequired); + wrapper.vm.approvers = groups.concat(users); + + wrapper.vm.submit(); + + expect(actions.postRule).toHaveBeenCalledWith(jasmine.anything(), expected, undefined); + }); + + it('adds selected approvers on button click', () => { + const { node } = findApproversSelect(wrapper); + const selected = [ + { id: 1, type: TYPE_USER }, + { id: 2, type: TYPE_USER }, + { id: 1, type: TYPE_GROUP }, + ]; + const orig = [{ id: 7, type: TYPE_GROUP }]; + const expected = selected.concat(orig); + + wrapper.vm.approvers = orig; + + node.vm.$emit('input', selected); + wrapper.find(GlButton).vm.$emit('click'); + + expect(wrapper.vm.approvers).toEqual(expected); + }); + }); + + describe('with initRule', () => { + beforeEach(() => { + factory({ + propsData: { + initRule: TEST_RULE, + }, + }); + }); + + it('on submit, puts rule', () => { + const expected = { + ...TEST_RULE, + users: TEST_RULE.users.map(x => x.id), + groups: TEST_RULE.groups.map(x => x.id), + }; + + wrapper.vm.submit(); + + expect(actions.putRule).toHaveBeenCalledWith(jasmine.anything(), expected, undefined); + }); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/approval_rules_empty_spec.js b/ee/spec/javascripts/approvals/components/rules_empty_spec.js similarity index 77% rename from ee/spec/javascripts/approvals/components/approval_rules_empty_spec.js rename to ee/spec/javascripts/approvals/components/rules_empty_spec.js index 47c65198080935..950cafd34083b3 100644 --- a/ee/spec/javascripts/approvals/components/approval_rules_empty_spec.js +++ b/ee/spec/javascripts/approvals/components/rules_empty_spec.js @@ -1,6 +1,6 @@ import { createLocalVue, shallowMount } from '@vue/test-utils'; import { GlButton } from '@gitlab/ui'; -import ApprovalRulesEmpty from 'ee/approvals/components/approval_rules_empty.vue'; +import RulesEmpty from 'ee/approvals/components/rules_empty.vue'; const localVue = createLocalVue(); @@ -8,7 +8,7 @@ describe('EE ApprovalsSettingsEmpty', () => { let wrapper; const factory = options => { - wrapper = shallowMount(localVue.extend(ApprovalRulesEmpty), { + wrapper = shallowMount(localVue.extend(RulesEmpty), { localVue, ...options, }); @@ -21,7 +21,7 @@ describe('EE ApprovalsSettingsEmpty', () => { it('shows message', () => { factory(); - expect(wrapper.text()).toContain(ApprovalRulesEmpty.message); + expect(wrapper.text()).toContain(RulesEmpty.message); }); it('shows button', () => { diff --git a/ee/spec/javascripts/approvals/components/rules_spec.js b/ee/spec/javascripts/approvals/components/rules_spec.js new file mode 100644 index 00000000000000..b2c61d87878b15 --- /dev/null +++ b/ee/spec/javascripts/approvals/components/rules_spec.js @@ -0,0 +1,97 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import { GlButton } from '@gitlab/ui'; +import Icon from '~/vue_shared/components/icon.vue'; +import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; +import Rules from 'ee/approvals/components/rules.vue'; + +const TEST_RULES = [ + { id: 1, name: 'Lorem', approvalsRequired: 2, approvers: [{ id: 7 }, { id: 8 }] }, + { id: 2, name: 'Ipsum', approvalsRequired: 0, approvers: [{ id: 9 }] }, + { id: 3, name: 'Dolarsit', approvalsRequired: 3, approvers: [] }, +]; + +const localVue = createLocalVue(); +const getRowData = tr => { + const td = tr.findAll('td'); + const avatarList = td.at(1).find(UserAvatarList); + return { + name: td + .at(0) + .find('.d-sm-block.d-none') + .text(), + summary: td + .at(0) + .find('.d-sm-none.d-block') + .text(), + approvers: avatarList.exists() ? avatarList.props('items') : td.at(1).text(), + approvalsRequired: Number(td.at(2).text()), + }; +}; +const findButton = (tr, icon) => { + const buttons = tr.findAll(GlButton); + + return buttons.filter(x => x.find(Icon).props('name') === icon).at(0); +}; + +describe('Approvals Rules', () => { + let wrapper; + + const factory = (options = {}) => { + const propsData = { + rules: TEST_RULES, + ...options.propsData, + }; + + wrapper = shallowMount(localVue.extend(Rules), { + ...options, + localVue, + propsData, + }); + }; + + afterEach(() => { + wrapper.destroy(); + }); + + it('renders row for each rule', () => { + factory(); + + const rows = wrapper.findAll('tbody tr'); + const data = rows.wrappers.map(getRowData); + + expect(data).toEqual( + TEST_RULES.map(rule => ({ + name: rule.name, + summary: jasmine.stringMatching(`${rule.approvalsRequired} approval.*from ${rule.name}`), + approvalsRequired: rule.approvalsRequired, + approvers: rule.approvers.length ? rule.approvers : 'None', + })), + ); + }); + + it('when edit is clicked, emits edit', () => { + const idx = 2; + const rule = TEST_RULES[idx]; + + factory(); + + const tr = wrapper.findAll('tbody tr').at(idx); + const editButton = findButton(tr, 'pencil'); + editButton.vm.$emit('click'); + + expect(wrapper.emittedByOrder()).toEqual([{ name: 'edit', args: [rule] }]); + }); + + it('when remove is clicked, emits remove', () => { + const idx = 1; + const rule = TEST_RULES[idx]; + + factory(); + + const tr = wrapper.findAll('tbody tr').at(idx); + const removeButton = findButton(tr, 'remove'); + removeButton.vm.$emit('click'); + + expect(wrapper.emittedByOrder()).toEqual([{ name: 'remove', args: [rule] }]); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/settings_spec.js b/ee/spec/javascripts/approvals/components/settings_spec.js index a01ae979bb1f0f..43f0ec0fbc9f5a 100644 --- a/ee/spec/javascripts/approvals/components/settings_spec.js +++ b/ee/spec/javascripts/approvals/components/settings_spec.js @@ -1,7 +1,10 @@ import { shallowMount, createLocalVue } from '@vue/test-utils'; import Vuex from 'vuex'; -import { GlLoadingIcon } from '@gitlab/ui'; -import ApprovalRulesEmpty from 'ee/approvals/components/approval_rules_empty.vue'; +import { GlLoadingIcon, GlButton } from '@gitlab/ui'; +import ModalRuleCreate from 'ee/approvals/components/modal_rule_create.vue'; +import ModalRuleRemove from 'ee/approvals/components/modal_rule_remove.vue'; +import Rules from 'ee/approvals/components/rules.vue'; +import RulesEmpty from 'ee/approvals/components/rules_empty.vue'; import Settings from 'ee/approvals/components/settings.vue'; const localVue = createLocalVue(); @@ -30,6 +33,8 @@ describe('EE ApprovalsSettingsForm', () => { actions = { fetchRules: jasmine.createSpy('fetchRules'), + 'createModal/open': jasmine.createSpy('createModal/open'), + 'deleteModal/open': jasmine.createSpy('deleteModal/open'), }; }); @@ -41,31 +46,129 @@ describe('EE ApprovalsSettingsForm', () => { expect(actions.fetchRules).toHaveBeenCalledTimes(1); }); - it('shows loading icon if loading', () => { - state.isLoading = true; + it('renders create modal', () => { factory(); - expect(wrapper.find(GlLoadingIcon).exists()).toBe(true); + const modal = wrapper.find(ModalRuleCreate); + + expect(modal.exists()).toBe(true); + expect(modal.props('modalId')).toBe(wrapper.vm.$options.CREATE_MODAL_ID); }); - it('does not show loading icon if not loading', () => { - state.isLoading = false; + it('renders delete modal', () => { factory(); - expect(wrapper.find(GlLoadingIcon).exists()).toBe(false); + const modal = wrapper.find(ModalRuleRemove); + + expect(modal.exists()).toBe(true); + expect(modal.props('modalId')).toBe(wrapper.vm.$options.REMOVE_MODAL_ID); }); - it('shows ApprovalsSettingsEmpty if empty', () => { - state.rules = []; - factory(); + describe('if empty', () => { + beforeEach(() => { + state.rules = []; + }); + + it('shows RulesEmpty', () => { + factory(); + + expect(wrapper.find(RulesEmpty).exists()).toBe(true); + }); + + it('does not show Rules', () => { + factory(); + + expect(wrapper.find(Rules).exists()).toBe(false); + }); + + it('opens create modal if clicked', () => { + factory(); + + const empty = wrapper.find(RulesEmpty); + empty.vm.$emit('click'); - expect(wrapper.find(ApprovalRulesEmpty).exists()).toBe(true); + expect(actions['createModal/open']).toHaveBeenCalledWith(jasmine.anything(), null, undefined); + }); + + it('shows loading icon if loading', () => { + state.isLoading = true; + factory(); + + expect(wrapper.find(GlLoadingIcon).exists()).toBe(true); + }); + + it('does not show loading icon if not loading', () => { + state.isLoading = false; + factory(); + + expect(wrapper.find(GlLoadingIcon).exists()).toBe(false); + }); }); - it('does not show ApprovalsSettingsEmpty is not empty', () => { - state.rules = [{ id: 1 }]; - factory(); + describe('if not empty', () => { + beforeEach(() => { + state.rules = [{ id: 1 }]; + }); + + it('does not show RulesEmpty', () => { + factory(); + + expect(wrapper.find(RulesEmpty).exists()).toBe(false); + }); + + it('shows rules', () => { + factory(); + + const rules = wrapper.find(Rules); + + expect(rules.exists()).toBe(true); + expect(rules.props('rules')).toEqual(state.rules); + }); + + it('opens create modal when edit is clicked', () => { + factory(); - expect(wrapper.find(ApprovalRulesEmpty).exists()).toBe(false); + const rule = state.rules[0]; + const rules = wrapper.find(Rules); + rules.vm.$emit('edit', rule); + + expect(actions['createModal/open']).toHaveBeenCalledWith(jasmine.anything(), rule, undefined); + }); + + it('opens delete modal when remove is clicked', () => { + factory(); + + const { id } = state.rules[0]; + const rules = wrapper.find(Rules); + rules.vm.$emit('remove', id); + + expect(actions['deleteModal/open']).toHaveBeenCalledWith(jasmine.anything(), id, undefined); + }); + + it('renders add button', () => { + factory(); + + const button = wrapper.find(GlButton); + + expect(button.exists()).toBe(true); + expect(button.text()).toBe('Add approvers'); + }); + + it('opens create modal when add button is clicked', () => { + factory(); + + const button = wrapper.find(GlButton); + button.vm.$emit('click'); + + expect(actions['createModal/open']).toHaveBeenCalledWith(jasmine.anything(), null, undefined); + }); + + it('shows loading icon and rules if loading', () => { + state.isLoading = true; + factory(); + + expect(wrapper.find(Rules).exists()).toBe(true); + expect(wrapper.find(GlLoadingIcon).exists()).toBe(true); + }); }); }); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c271d9e049bf58..6f73fdcee0b813 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -44,6 +44,11 @@ msgstr "" msgid "\"%{query}\" in projects" msgstr "" +msgid "%d approval required from %{name}" +msgid_plural "%d approvals required from %{name}" +msgstr[0] "" +msgstr[1] "" + msgid "%d commit" msgid_plural "%d commits" msgstr[0] "" @@ -946,6 +951,31 @@ msgstr "" msgid "Apply suggestion" msgstr "" +msgid "ApprovalRuleForm|No. approvals required" +msgstr "" + +msgid "ApprovalRuleRemove|%d member" +msgid_plural "ApprovalRuleRemove|%d members" +msgstr[0] "" +msgstr[1] "" + +msgid "ApprovalRuleRemove|Approvals from this member are not revoked." +msgid_plural "ApprovalRuleRemove|Approvals from these members are not revoked." +msgstr[0] "" +msgstr[1] "" + +msgid "ApprovalRuleRemove|You are about to remove the %{name} approver group which has %{nMembers}." +msgstr "" + +msgid "ApprovalRule|Members" +msgstr "" + +msgid "ApprovalRule|Name" +msgstr "" + +msgid "ApprovalRule|e.g. QA, Security, etc." +msgstr "" + msgid "Approvals" msgstr "" @@ -7033,6 +7063,9 @@ msgstr "" msgid "Please enable and migrate to hashed storage to avoid security issues and ensure data integrity. %{migrate_link}" msgstr "" +msgid "Please enter a non-negative number" +msgstr "" + msgid "Please fill in a descriptive name for your group." msgstr "" @@ -7042,6 +7075,12 @@ msgstr "" msgid "Please note that this application is not provided by GitLab and you should verify its authenticity before allowing access." msgstr "" +msgid "Please provide a name" +msgstr "" + +msgid "Please select and add a member" +msgstr "" + msgid "Please select at least one filter to see results" msgstr "" @@ -7869,6 +7908,12 @@ msgstr "" msgid "Remove approver" msgstr "" +msgid "Remove approvers" +msgstr "" + +msgid "Remove approvers?" +msgstr "" + msgid "Remove avatar" msgstr "" @@ -8285,6 +8330,9 @@ msgstr "" msgid "Search users" msgstr "" +msgid "Search users or groups" +msgstr "" + msgid "Search your projects" msgstr "" @@ -10229,6 +10277,9 @@ msgstr "" msgid "Update" msgstr "" +msgid "Update approvers" +msgstr "" + msgid "Update failed" msgstr "" @@ -10883,6 +10934,9 @@ msgstr "" msgid "You have no permissions" msgstr "" +msgid "You have not added any approvers. Start by adding users or groups." +msgstr "" + msgid "You have reached your project limit" msgstr "" -- GitLab From ca08ffda69455e29b9af7885ef1102c70e4ded5d Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Tue, 15 Jan 2019 12:12:14 -0600 Subject: [PATCH 08/17] Extract modules from approvals store **Why?** - For the MR create/edit input, we'll need some common functionality and to override these action names. **Note:** - This also removes the `setSettings` action. Instead, we can simply init the settings when we create the store. --- .../javascripts/approvals/stores/index.js | 23 +++++++------- .../approvals/stores/modules/base/getters.js | 3 ++ .../approvals/stores/modules/base/index.js | 9 ++++++ .../{ => modules/base}/mutation_types.js | 1 - .../stores/{ => modules/base}/mutations.js | 3 -- .../approvals/stores/modules/base/state.js | 4 +++ .../{ => modules/project_settings}/actions.js | 30 ++++++++----------- .../stores/modules/project_settings/index.js | 7 +++++ .../javascripts/approvals/stores/state.js | 13 +++++--- .../stores/modules/base/getters_spec.js | 17 +++++++++++ .../{ => modules/base}/mutations_spec.js | 16 ++-------- .../project_settings}/actions_spec.js | 25 ++-------------- 12 files changed, 78 insertions(+), 73 deletions(-) create mode 100644 ee/app/assets/javascripts/approvals/stores/modules/base/getters.js create mode 100644 ee/app/assets/javascripts/approvals/stores/modules/base/index.js rename ee/app/assets/javascripts/approvals/stores/{ => modules/base}/mutation_types.js (64%) rename ee/app/assets/javascripts/approvals/stores/{ => modules/base}/mutations.js (71%) create mode 100644 ee/app/assets/javascripts/approvals/stores/modules/base/state.js rename ee/app/assets/javascripts/approvals/stores/{ => modules/project_settings}/actions.js (73%) create mode 100644 ee/app/assets/javascripts/approvals/stores/modules/project_settings/index.js create mode 100644 ee/spec/javascripts/approvals/stores/modules/base/getters_spec.js rename ee/spec/javascripts/approvals/stores/{ => modules/base}/mutations_spec.js (58%) rename ee/spec/javascripts/approvals/stores/{ => modules/project_settings}/actions_spec.js (91%) diff --git a/ee/app/assets/javascripts/approvals/stores/index.js b/ee/app/assets/javascripts/approvals/stores/index.js index 8d34f826efc3f7..b17514eab74678 100644 --- a/ee/app/assets/javascripts/approvals/stores/index.js +++ b/ee/app/assets/javascripts/approvals/stores/index.js @@ -1,16 +1,15 @@ import Vuex from 'vuex'; import modalModule from '~/vuex_shared/modules/modal'; import state from './state'; -import mutations from './mutations'; -import * as actions from './actions'; -export default () => - new Vuex.Store({ - state: state(), - mutations, - actions, - modules: { - createModal: modalModule(), - deleteModal: modalModule(), - }, - }); +export const createStoreOptions = (approvalsModule, settings) => ({ + state: state(settings), + modules: { + ...(approvalsModule ? { approvals: approvalsModule } : {}), + createModal: modalModule(), + deleteModal: modalModule(), + }, +}); + +export default (approvalsModule, settings = {}) => + new Vuex.Store(createStoreOptions(approvalsModule, settings)); diff --git a/ee/app/assets/javascripts/approvals/stores/modules/base/getters.js b/ee/app/assets/javascripts/approvals/stores/modules/base/getters.js new file mode 100644 index 00000000000000..851813512c3595 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/getters.js @@ -0,0 +1,3 @@ +export const isEmpty = state => !state.rules || !state.rules.length; + +export default () => {}; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/base/index.js b/ee/app/assets/javascripts/approvals/stores/modules/base/index.js new file mode 100644 index 00000000000000..a7bcc2b0b31fd5 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/index.js @@ -0,0 +1,9 @@ +import createState from './state'; +import * as getters from './getters'; +import mutations from './mutations'; + +export default () => ({ + state: createState(), + getters, + mutations, +}); diff --git a/ee/app/assets/javascripts/approvals/stores/mutation_types.js b/ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js similarity index 64% rename from ee/app/assets/javascripts/approvals/stores/mutation_types.js rename to ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js index ed91b8e4bea363..5e10c3f3f16218 100644 --- a/ee/app/assets/javascripts/approvals/stores/mutation_types.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/mutation_types.js @@ -1,3 +1,2 @@ -export const SET_SETTINGS = 'SET_SETTINGS'; export const SET_LOADING = 'SET_LOADING'; export const SET_RULES = 'SET_RULES'; diff --git a/ee/app/assets/javascripts/approvals/stores/mutations.js b/ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js similarity index 71% rename from ee/app/assets/javascripts/approvals/stores/mutations.js rename to ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js index 228a4765ab4232..e7490f063463e2 100644 --- a/ee/app/assets/javascripts/approvals/stores/mutations.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/mutations.js @@ -1,9 +1,6 @@ import * as types from './mutation_types'; export default { - [types.SET_SETTINGS](state, settings) { - state.settings = { ...settings }; - }, [types.SET_LOADING](state, isLoading) { state.isLoading = isLoading; }, diff --git a/ee/app/assets/javascripts/approvals/stores/modules/base/state.js b/ee/app/assets/javascripts/approvals/stores/modules/base/state.js new file mode 100644 index 00000000000000..6d6844f6363b80 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/modules/base/state.js @@ -0,0 +1,4 @@ +export default () => ({ + isLoading: false, + rules: [], +}); diff --git a/ee/app/assets/javascripts/approvals/stores/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js similarity index 73% rename from ee/app/assets/javascripts/approvals/stores/actions.js rename to ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js index 6a133a851c7bc9..26a5d2dd715e61 100644 --- a/ee/app/assets/javascripts/approvals/stores/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js @@ -1,12 +1,8 @@ import createFlash from '~/flash'; import { __ } from '~/locale'; import Api from 'ee/api'; -import * as types from './mutation_types'; -import { mapApprovalRuleRequest, mapApprovalRulesResponse } from '../mappers'; - -export const setSettings = ({ commit }, settings) => { - commit(types.SET_SETTINGS, settings); -}; +import * as types from '../base/mutation_types'; +import { mapApprovalRuleRequest, mapApprovalRulesResponse } from '../../../mappers'; export const requestRules = ({ commit }) => { commit(types.SET_LOADING, true); @@ -21,12 +17,8 @@ export const receiveRulesError = () => { createFlash(__('An error occurred fetching the approval rules.')); }; -export const fetchRules = ({ state, dispatch }) => { - if (state.isLoading) { - return Promise.resolve(); - } - - const { projectId } = state.settings; +export const fetchRules = ({ rootState, dispatch }) => { + const { projectId } = rootState.settings; dispatch('requestRules'); @@ -44,16 +36,16 @@ export const postRuleError = () => { createFlash(__('An error occurred while updating approvers')); }; -export const postRule = ({ state, dispatch }, rule) => { - const { projectId } = state.settings; +export const postRule = ({ rootState, dispatch }, rule) => { + const { projectId } = rootState.settings; return Api.postProjectApprovalRule(projectId, mapApprovalRuleRequest(rule)) .then(() => dispatch('postRuleSuccess')) .catch(() => dispatch('postRuleError')); }; -export const putRule = ({ state, dispatch }, { id, ...newRule }) => { - const { projectId } = state.settings; +export const putRule = ({ rootState, dispatch }, { id, ...newRule }) => { + const { projectId } = rootState.settings; return Api.putProjectApprovalRule(projectId, id, mapApprovalRuleRequest(newRule)) .then(() => dispatch('postRuleSuccess')) @@ -69,10 +61,12 @@ export const deleteRuleError = () => { createFlash(__('An error occurred while deleting the approvers group')); }; -export const deleteRule = ({ state, dispatch }, id) => { - const { projectId } = state.settings; +export const deleteRule = ({ rootState, dispatch }, id) => { + const { projectId } = rootState.settings; return Api.deleteProjectApprovalRule(projectId, id) .then(() => dispatch('deleteRuleSuccess')) .catch(() => dispatch('deleteRuleError')); }; + +export default () => {}; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/index.js b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/index.js new file mode 100644 index 00000000000000..657b31ea6c4a1b --- /dev/null +++ b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/index.js @@ -0,0 +1,7 @@ +import base from '../base'; +import * as actions from './actions'; + +export default () => ({ + ...base(), + actions, +}); diff --git a/ee/app/assets/javascripts/approvals/stores/state.js b/ee/app/assets/javascripts/approvals/stores/state.js index 34bde76e505335..5120f75ae33731 100644 --- a/ee/app/assets/javascripts/approvals/stores/state.js +++ b/ee/app/assets/javascripts/approvals/stores/state.js @@ -1,5 +1,10 @@ -export default () => ({ - settings: {}, - isLoading: false, - rules: [], +export const DEFAULT_SETTINGS = { + canEdit: true, +}; + +export default (settings = {}) => ({ + settings: { + ...DEFAULT_SETTINGS, + ...settings, + }, }); diff --git a/ee/spec/javascripts/approvals/stores/modules/base/getters_spec.js b/ee/spec/javascripts/approvals/stores/modules/base/getters_spec.js new file mode 100644 index 00000000000000..472919c6776a8e --- /dev/null +++ b/ee/spec/javascripts/approvals/stores/modules/base/getters_spec.js @@ -0,0 +1,17 @@ +import * as getters from 'ee/approvals/stores/modules/base/getters'; + +describe('EE store modules base getters', () => { + describe('isEmpty', () => { + it('when rules is falsey, is true', () => { + expect(getters.isEmpty({})).toBe(true); + }); + + it('when rules is empty, is true', () => { + expect(getters.isEmpty({ rules: [] })).toBe(true); + }); + + it('when rules has items, is false', () => { + expect(getters.isEmpty({ rules: [1] })).toBe(false); + }); + }); +}); diff --git a/ee/spec/javascripts/approvals/stores/mutations_spec.js b/ee/spec/javascripts/approvals/stores/modules/base/mutations_spec.js similarity index 58% rename from ee/spec/javascripts/approvals/stores/mutations_spec.js rename to ee/spec/javascripts/approvals/stores/modules/base/mutations_spec.js index 9c5a4f95a86ea8..b6753984d4f099 100644 --- a/ee/spec/javascripts/approvals/stores/mutations_spec.js +++ b/ee/spec/javascripts/approvals/stores/modules/base/mutations_spec.js @@ -1,24 +1,14 @@ import createState from 'ee/approvals/stores/state'; -import * as types from 'ee/approvals/stores/mutation_types'; -import mutations from 'ee/approvals/stores/mutations'; +import * as types from 'ee/approvals/stores/modules/base/mutation_types'; +import mutations from 'ee/approvals/stores/modules/base/mutations'; -describe('EE approvals store mutations', () => { +describe('EE approvals base module mutations', () => { let state; beforeEach(() => { state = createState(); }); - describe(types.SET_SETTINGS, () => { - it('sets the settings', () => { - const newSettings = { projectId: 7 }; - - mutations[types.SET_SETTINGS](state, newSettings); - - expect(state.settings).toEqual(newSettings); - }); - }); - describe(types.SET_LOADING, () => { it('sets isLoading', () => { state.isLoading = false; diff --git a/ee/spec/javascripts/approvals/stores/actions_spec.js b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js similarity index 91% rename from ee/spec/javascripts/approvals/stores/actions_spec.js rename to ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js index 5a493f2db3632d..08ed9dd4ba06c6 100644 --- a/ee/spec/javascripts/approvals/stores/actions_spec.js +++ b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js @@ -1,7 +1,7 @@ import Api from 'ee/api'; import testAction from 'spec/helpers/vuex_action_helper'; -import * as types from 'ee/approvals/stores/mutation_types'; -import actionsModule, * as actions from 'ee/approvals/stores/actions'; +import * as types from 'ee/approvals/stores/modules/base/mutation_types'; +import actionsModule, * as actions from 'ee/approvals/stores/modules/project_settings/actions'; import { mapApprovalRuleRequest, mapApprovalRulesResponse } from 'ee/approvals/mappers'; const TEST_PROJECT_ID = 9; @@ -21,7 +21,7 @@ const TEST_RULE_RESPONSE = { users: [{ id: 7 }, { id: 8 }], }; -describe('EE approvals store actions', () => { +describe('EE approvals project settings module actions', () => { let state; let flashSpy; @@ -36,21 +36,6 @@ describe('EE approvals store actions', () => { spyOn(Api, 'deleteProjectApprovalRule'); }); - describe('setSettings', () => { - it('sets the settings', done => { - const settings = { projectId: 7 }; - - testAction( - actions.setSettings, - settings, - {}, - [{ type: types.SET_SETTINGS, payload: settings }], - [], - done, - ); - }); - }); - describe('requestRules', () => { it('sets loading', done => { testAction( @@ -91,10 +76,6 @@ describe('EE approvals store actions', () => { }); describe('fetchRules', () => { - it('does nothing if loading', done => { - testAction(actions.fetchRules, null, { isLoading: true }, [], [], done); - }); - it('dispatches request/receive', done => { const response = { data: { rules: [TEST_RULE_RESPONSE] }, -- GitLab From 6e2064393ccab9ba8bf36ab80fa34464aae2ba41 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Tue, 15 Jan 2019 12:16:31 -0600 Subject: [PATCH 09/17] Update approvals components to support MR use **Why?** There's *alot* of common functionality between how these will be used in project settings and MR edit. This abstracts some of that so that it is reusable and extensible. --- .../components/{settings.vue => app.vue} | 37 +++---- .../components/project_settings/app.vue | 15 +++ .../project_settings/project_rules.vue | 55 +++++++++++ .../approvals/components/rule_controls.vue | 36 +++++++ .../approvals/components/rule_form.vue | 14 ++- .../approvals/components/rules.vue | 65 ++---------- .../approvals/components/rules_empty.vue | 8 +- .../{settings_spec.js => app_spec.js} | 99 +++++++++---------- .../components/approvers_list_item_spec.js | 1 + .../project_settings/project_rules_spec.js | 75 ++++++++++++++ .../components/rule_controls_spec.js | 88 +++++++++++++++++ .../approvals/components/rule_form_spec.js | 26 +++-- .../approvals/components/rules_empty_spec.js | 16 ++- .../approvals/components/rules_spec.js | 97 ------------------ 14 files changed, 388 insertions(+), 244 deletions(-) rename ee/app/assets/javascripts/approvals/components/{settings.vue => app.vue} (58%) create mode 100644 ee/app/assets/javascripts/approvals/components/project_settings/app.vue create mode 100644 ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue create mode 100644 ee/app/assets/javascripts/approvals/components/rule_controls.vue rename ee/spec/javascripts/approvals/components/{settings_spec.js => app_spec.js} (55%) create mode 100644 ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js create mode 100644 ee/spec/javascripts/approvals/components/rule_controls_spec.js delete mode 100644 ee/spec/javascripts/approvals/components/rules_spec.js diff --git a/ee/app/assets/javascripts/approvals/components/settings.vue b/ee/app/assets/javascripts/approvals/components/app.vue similarity index 58% rename from ee/app/assets/javascripts/approvals/components/settings.vue rename to ee/app/assets/javascripts/approvals/components/app.vue index d9b931553630f5..037a6355e31bc3 100644 --- a/ee/app/assets/javascripts/approvals/components/settings.vue +++ b/ee/app/assets/javascripts/approvals/components/app.vue @@ -1,27 +1,29 @@ @@ -44,13 +43,8 @@ export default { - - + + + diff --git a/ee/app/assets/javascripts/approvals/components/project_settings/app.vue b/ee/app/assets/javascripts/approvals/components/project_settings/app.vue new file mode 100644 index 00000000000000..3133244d61d7c3 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/project_settings/app.vue @@ -0,0 +1,15 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue b/ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue new file mode 100644 index 00000000000000..3def1cdf27d5da --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue @@ -0,0 +1,55 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/rule_controls.vue b/ee/app/assets/javascripts/approvals/components/rule_controls.vue new file mode 100644 index 00000000000000..e39d48b7ebd477 --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/rule_controls.vue @@ -0,0 +1,36 @@ + + + diff --git a/ee/app/assets/javascripts/approvals/components/rule_form.vue b/ee/app/assets/javascripts/approvals/components/rule_form.vue index 10e6d8e148f819..7bded15b27bb37 100644 --- a/ee/app/assets/javascripts/approvals/components/rule_form.vue +++ b/ee/app/assets/javascripts/approvals/components/rule_form.vue @@ -35,11 +35,17 @@ export default { approversByType() { return _.groupBy(this.approvers, x => x.type); }, + users() { + return this.approversByType[TYPE_USER] || []; + }, + groups() { + return this.approversByType[TYPE_GROUP] || []; + }, userIds() { - return (this.approversByType[TYPE_USER] || []).map(x => x.id); + return this.users.map(x => x.id); }, groupIds() { - return (this.approversByType[TYPE_GROUP] || []).map(x => x.id); + return this.groups.map(x => x.id); }, validation() { if (!this.showValidation) { @@ -84,6 +90,8 @@ export default { approvalsRequired: this.approvalsRequired, users: this.userIds, groups: this.groupIds, + userRecords: this.users, + groupRecords: this.groups, }; this.showValidation = true; @@ -131,7 +139,7 @@ export default {