From 901c4129c0babbcec90e1e5046e9b76392f5ed76 Mon Sep 17 00:00:00 2001 From: Paul Slaughter <pslaughter@gitlab.com> Date: Mon, 4 Feb 2019 22:04:23 -0600 Subject: [PATCH 1/5] Add `sync: false` to project_rules_spec --- .../approvals/components/project_settings/project_rules_spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js b/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js index 8fb0886a54c6a6b1..ae4017b8dc0b520b 100644 --- a/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js +++ b/ee/spec/javascripts/approvals/components/project_settings/project_rules_spec.js @@ -41,6 +41,7 @@ describe('Approvals ProjectRules', () => { wrapper = mount(localVue.extend(ProjectRules), { ...options, + sync: false, store, localVue, }); -- GitLab From d22c6a3c67c51c018f07c880c6c7167d7d78451e Mon Sep 17 00:00:00 2001 From: Paul Slaughter <pslaughter@gitlab.com> Date: Mon, 4 Feb 2019 22:27:29 -0600 Subject: [PATCH 2/5] Add `sync: false` to rule_controls_spec --- ee/spec/javascripts/approvals/components/rule_controls_spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/ee/spec/javascripts/approvals/components/rule_controls_spec.js b/ee/spec/javascripts/approvals/components/rule_controls_spec.js index 486ad3c993f63a23..2b425afe1a38ff3f 100644 --- a/ee/spec/javascripts/approvals/components/rule_controls_spec.js +++ b/ee/spec/javascripts/approvals/components/rule_controls_spec.js @@ -21,6 +21,7 @@ describe('EE Approvals RuleControls', () => { }, localVue, store: new Vuex.Store(store), + sync: false, }); }; -- GitLab From 8cf5a25e17052535a461477b849f3b38b4449060 Mon Sep 17 00:00:00 2001 From: Paul Slaughter <pslaughter@gitlab.com> Date: Tue, 5 Feb 2019 01:11:44 -0600 Subject: [PATCH 3/5] Stub approval_rules feature flag in specs --- .../projects/settings/forked_project_settings_spec.rb | 1 + spec/features/projects_spec.rb | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/spec/features/projects/settings/forked_project_settings_spec.rb b/spec/features/projects/settings/forked_project_settings_spec.rb index df33d21560292cc3..dc0278370aa2ad6a 100644 --- a/spec/features/projects/settings/forked_project_settings_spec.rb +++ b/spec/features/projects/settings/forked_project_settings_spec.rb @@ -7,6 +7,7 @@ let(:forked_project) { fork_project(original_project, user) } before do + stub_feature_flags(approval_rules: false) original_project.add_maintainer(user) forked_project.add_maintainer(user) sign_in(user) diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index f7efc3f325c60fa7..3a67c5581d06ab47 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -4,6 +4,10 @@ include ProjectForksHelper include MobileHelpers + before do + stub_feature_flags(approval_rules: false) + end + describe 'creating from template' do let(:user) { create(:user) } let(:template) { Gitlab::ProjectTemplate.find(:rails) } -- GitLab From f5b939f70d2f8f25e802dd948b8eb779aaec817b Mon Sep 17 00:00:00 2001 From: Paul Slaughter <pslaughter@gitlab.com> Date: Sat, 2 Feb 2019 15:02:27 -0600 Subject: [PATCH 4/5] Rename feature approval_rule to approval_rules --- .../projects/_merge_request_approvals_settings_form.html.haml | 2 +- .../features/projects/settings/merge_requests_settings_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 9218c2ffb99f15e7..e0c20e8a142e8a70 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,6 +1,6 @@ - can_override_approvers = project.can_override_approvers? -- if Feature.enabled?(:approval_rule, project) +- if Feature.enabled?(:approval_rules, 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 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 c3d416321d2e05ab..64409ec0944c6a01 100644 --- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -10,7 +10,7 @@ let(:non_member) { create(:user) } before do - stub_feature_flags(approval_rule: false) + stub_feature_flags(approval_rules: false) sign_in(user) project.add_maintainer(user) -- GitLab From 6e66baed42134c5391c440520baa25e044d97f7d Mon Sep 17 00:00:00 2001 From: Paul Slaughter <pslaughter@gitlab.com> Date: Mon, 4 Feb 2019 15:40:01 -0600 Subject: [PATCH 5/5] Replace ee/api.js with paths in HAML **Why?** It is much more maintainable if we get these from the BE. --- ee/app/assets/javascripts/api.js | 41 -------- .../approvals/components/approvers_select.vue | 2 +- .../modules/project_settings/actions.js | 22 +++-- .../_multiple_rules_form.html.haml | 4 +- ee/spec/javascripts/api_spec.js | 90 ----------------- .../components/approvers_select_spec.js | 2 +- .../modules/project_settings/actions_spec.js | 99 ++++++++++--------- 7 files changed, 70 insertions(+), 190 deletions(-) delete mode 100644 ee/app/assets/javascripts/api.js delete mode 100644 ee/spec/javascripts/api_spec.js diff --git a/ee/app/assets/javascripts/api.js b/ee/app/assets/javascripts/api.js deleted file mode 100644 index aebea5a7f4bf093d..0000000000000000 --- a/ee/app/assets/javascripts/api.js +++ /dev/null @@ -1,41 +0,0 @@ -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/components/approvers_select.vue b/ee/app/assets/javascripts/approvals/components/approvers_select.vue index 07b5d3bc2a798224..2dfa5172573ef70e 100644 --- a/ee/app/assets/javascripts/approvals/components/approvers_select.vue +++ b/ee/app/assets/javascripts/approvals/components/approvers_select.vue @@ -2,7 +2,7 @@ import $ from 'jquery'; import _ from 'underscore'; import { __ } from '~/locale'; -import Api from 'ee/api'; +import Api from '~/api'; import { TYPE_USER, TYPE_GROUP } from '../constants'; import { renderAvatar } from '~/helpers/avatar_helper'; diff --git a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js index 26a5d2dd715e6125..81a0e0acba1d5d82 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js @@ -1,6 +1,6 @@ import createFlash from '~/flash'; import { __ } from '~/locale'; -import Api from 'ee/api'; +import axios from '~/lib/utils/axios_utils'; import * as types from '../base/mutation_types'; import { mapApprovalRuleRequest, mapApprovalRulesResponse } from '../../../mappers'; @@ -18,11 +18,12 @@ export const receiveRulesError = () => { }; export const fetchRules = ({ rootState, dispatch }) => { - const { projectId } = rootState.settings; + const { settingsPath } = rootState.settings; dispatch('requestRules'); - return Api.getProjectApprovalRules(projectId) + return axios + .get(settingsPath) .then(response => dispatch('receiveRulesSuccess', mapApprovalRulesResponse(response.data))) .catch(() => dispatch('receiveRulesError')); }; @@ -37,17 +38,19 @@ export const postRuleError = () => { }; export const postRule = ({ rootState, dispatch }, rule) => { - const { projectId } = rootState.settings; + const { rulesPath } = rootState.settings; - return Api.postProjectApprovalRule(projectId, mapApprovalRuleRequest(rule)) + return axios + .post(rulesPath, mapApprovalRuleRequest(rule)) .then(() => dispatch('postRuleSuccess')) .catch(() => dispatch('postRuleError')); }; export const putRule = ({ rootState, dispatch }, { id, ...newRule }) => { - const { projectId } = rootState.settings; + const { rulesPath } = rootState.settings; - return Api.putProjectApprovalRule(projectId, id, mapApprovalRuleRequest(newRule)) + return axios + .put(`${rulesPath}/${id}`, mapApprovalRuleRequest(newRule)) .then(() => dispatch('postRuleSuccess')) .catch(() => dispatch('postRuleError')); }; @@ -62,9 +65,10 @@ export const deleteRuleError = () => { }; export const deleteRule = ({ rootState, dispatch }, id) => { - const { projectId } = rootState.settings; + const { rulesPath } = rootState.settings; - return Api.deleteProjectApprovalRule(projectId, id) + return axios + .delete(`${rulesPath}/${id}`) .then(() => dispatch('deleteRuleSuccess')) .catch(() => dispatch('deleteRuleError')); }; 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 index 1de91e37206b6c85..e86bc047b5abb8ea 100644 --- 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 @@ -1,6 +1,8 @@ .form-group = form.label :approver_ids, class: 'label-bold' do = _("Approvers") - #js-mr-approvals-settings{ data: { 'project_id': @project.id } } + #js-mr-approvals-settings{ data: { 'project_id': @project.id, + 'settings_path': api_v4_projects_approval_settings_path(id: @project.id), + 'rules_path': api_v4_projects_approval_settings_rules_path(id: @project.id) } } .text-center.prepend-top-default = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') diff --git a/ee/spec/javascripts/api_spec.js b/ee/spec/javascripts/api_spec.js deleted file mode 100644 index a0674c2539157a4f..0000000000000000 --- a/ee/spec/javascripts/api_spec.js +++ /dev/null @@ -1,90 +0,0 @@ -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/components/approvers_select_spec.js b/ee/spec/javascripts/approvals/components/approvers_select_spec.js index d54c0260e5caff3c..813dcdf41304c93a 100644 --- a/ee/spec/javascripts/approvals/components/approvers_select_spec.js +++ b/ee/spec/javascripts/approvals/components/approvers_select_spec.js @@ -1,6 +1,6 @@ import { createLocalVue, mount } from '@vue/test-utils'; import $ from 'jquery'; -import Api from 'ee/api'; +import Api from '~/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'; diff --git a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js index 08ed9dd4ba06c690..e6d6fa80a60728aa 100644 --- a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js +++ b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js @@ -1,4 +1,5 @@ -import Api from 'ee/api'; +import MockAdapter from 'axios-mock-adapter'; +import axios from '~/lib/utils/axios_utils'; import testAction from 'spec/helpers/vuex_action_helper'; import * as types from 'ee/approvals/stores/modules/base/mutation_types'; import actionsModule, * as actions from 'ee/approvals/stores/modules/project_settings/actions'; @@ -20,20 +21,28 @@ const TEST_RULE_RESPONSE = { groups: [{ id: 4 }], users: [{ id: 7 }, { id: 8 }], }; +const TEST_SETTINGS_PATH = 'projects/9/approval_settings'; +const TEST_RULES_PATH = 'projects/9/approval_settings/rules'; describe('EE approvals project settings module actions', () => { let state; let flashSpy; + let mock; beforeEach(() => { state = { - settings: { projectId: TEST_PROJECT_ID }, + settings: { + projectId: TEST_PROJECT_ID, + settingsPath: TEST_SETTINGS_PATH, + rulesPath: TEST_RULES_PATH, + }, }; flashSpy = spyOnDependency(actionsModule, 'createFlash'); - spyOn(Api, 'getProjectApprovalRules'); - spyOn(Api, 'postProjectApprovalRule'); - spyOn(Api, 'putProjectApprovalRule'); - spyOn(Api, 'deleteProjectApprovalRule'); + mock = new MockAdapter(axios); + }); + + afterEach(() => { + mock.restore(); }); describe('requestRules', () => { @@ -51,13 +60,16 @@ describe('EE approvals project settings module actions', () => { describe('receiveRulesSuccess', () => { it('sets rules', done => { - const rules = [{ id: 1 }]; + const settings = { rules: [{ id: 1 }] }; testAction( actions.receiveRulesSuccess, - { rules }, + settings, {}, - [{ type: types.SET_RULES, payload: rules }, { type: types.SET_LOADING, payload: false }], + [ + { type: types.SET_RULES, payload: settings.rules }, + { type: types.SET_LOADING, payload: false }, + ], [], done, ); @@ -77,10 +89,8 @@ describe('EE approvals project settings module actions', () => { describe('fetchRules', () => { it('dispatches request/receive', done => { - const response = { - data: { rules: [TEST_RULE_RESPONSE] }, - }; - Api.getProjectApprovalRules.and.returnValue(Promise.resolve(response)); + const data = { rules: [TEST_RULE_RESPONSE] }; + mock.onGet(TEST_SETTINGS_PATH).replyOnce(200, data); testAction( actions.fetchRules, @@ -89,10 +99,10 @@ describe('EE approvals project settings module actions', () => { [], [ { type: 'requestRules' }, - { type: 'receiveRulesSuccess', payload: mapApprovalRulesResponse(response.data) }, + { type: 'receiveRulesSuccess', payload: mapApprovalRulesResponse(data) }, ], () => { - expect(Api.getProjectApprovalRules).toHaveBeenCalledWith(TEST_PROJECT_ID); + expect(mock.history.get.map(x => x.url)).toEqual([TEST_SETTINGS_PATH]); done(); }, @@ -100,7 +110,7 @@ describe('EE approvals project settings module actions', () => { }); it('dispatches request/receive on error', done => { - Api.getProjectApprovalRules.and.returnValue(Promise.reject()); + mock.onGet(TEST_SETTINGS_PATH).replyOnce(500); testAction( actions.fetchRules, @@ -138,7 +148,7 @@ describe('EE approvals project settings module actions', () => { describe('postRule', () => { it('dispatches success on success', done => { - Api.postProjectApprovalRule.and.returnValue(Promise.resolve()); + mock.onPost(TEST_RULES_PATH).replyOnce(200); testAction( actions.postRule, @@ -147,39 +157,28 @@ describe('EE approvals project settings module actions', () => { [], [{ type: 'postRuleSuccess' }], () => { - expect(Api.postProjectApprovalRule).toHaveBeenCalledWith( - TEST_PROJECT_ID, - mapApprovalRuleRequest(TEST_RULE_REQUEST), - ); + expect(mock.history.post).toEqual([ + jasmine.objectContaining({ + url: TEST_RULES_PATH, + data: JSON.stringify(mapApprovalRuleRequest(TEST_RULE_REQUEST)), + }), + ]); + done(); }, ); }); it('dispatches error on error', done => { - Api.postProjectApprovalRule.and.returnValue(Promise.reject()); + mock.onPost(TEST_RULES_PATH).replyOnce(500); - testAction( - actions.postRule, - TEST_RULE_REQUEST, - state, - [], - [{ type: 'postRuleError' }], - () => { - expect(Api.postProjectApprovalRule).toHaveBeenCalledWith( - TEST_PROJECT_ID, - mapApprovalRuleRequest(TEST_RULE_REQUEST), - ); - - done(); - }, - ); + testAction(actions.postRule, TEST_RULE_REQUEST, state, [], [{ type: 'postRuleError' }], done); }); }); describe('putRule', () => { it('dispatches success on success', done => { - Api.putProjectApprovalRule.and.returnValue(Promise.resolve()); + mock.onPut(`${TEST_RULES_PATH}/${TEST_RULE_ID}`).replyOnce(200); testAction( actions.putRule, @@ -188,18 +187,20 @@ describe('EE approvals project settings module actions', () => { [], [{ type: 'postRuleSuccess' }], () => { - expect(Api.putProjectApprovalRule).toHaveBeenCalledWith( - TEST_PROJECT_ID, - TEST_RULE_ID, - mapApprovalRuleRequest(TEST_RULE_REQUEST), - ); + expect(mock.history.put).toEqual([ + jasmine.objectContaining({ + url: `${TEST_RULES_PATH}/${TEST_RULE_ID}`, + data: JSON.stringify(mapApprovalRuleRequest(TEST_RULE_REQUEST)), + }), + ]); + done(); }, ); }); it('dispatches error on error', done => { - Api.putProjectApprovalRule.and.returnValue(Promise.reject()); + mock.onPut(`${TEST_RULES_PATH}/${TEST_RULE_ID}`).replyOnce(500); testAction( actions.putRule, @@ -237,7 +238,7 @@ describe('EE approvals project settings module actions', () => { describe('deleteRule', () => { it('dispatches success on success', done => { - Api.deleteProjectApprovalRule.and.returnValue(Promise.resolve()); + mock.onDelete(`${TEST_RULES_PATH}/${TEST_RULE_ID}`).replyOnce(200); testAction( actions.deleteRule, @@ -246,7 +247,11 @@ describe('EE approvals project settings module actions', () => { [], [{ type: 'deleteRuleSuccess' }], () => { - expect(Api.deleteProjectApprovalRule).toHaveBeenCalledWith(TEST_PROJECT_ID, TEST_RULE_ID); + expect(mock.history.delete).toEqual([ + jasmine.objectContaining({ + url: `${TEST_RULES_PATH}/${TEST_RULE_ID}`, + }), + ]); done(); }, @@ -254,7 +259,7 @@ describe('EE approvals project settings module actions', () => { }); it('dispatches error on error', done => { - Api.deleteProjectApprovalRule.and.returnValue(Promise.reject()); + mock.onDelete(`${TEST_RULES_PATH}/${TEST_RULE_ID}`).replyOnce(500); testAction(actions.deleteRule, TEST_RULE_ID, state, [], [{ type: 'deleteRuleError' }], done); }); -- GitLab