From 84312039a411f8dc425db6e39ea686909b1ff418 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Tue, 9 Feb 2021 14:34:08 -0800 Subject: [PATCH 1/2] Update approvers dropdown to use new endpoint Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/28902 Use "/projects/:id/groups" endpoint --- app/assets/javascripts/api.js | 15 +++++++++++++++ .../approvals/components/approvers_select.vue | 10 +++++----- .../merge_request/user_sets_approvers_spec.rb | 12 +++++++++--- .../settings/merge_requests_settings_spec.rb | 2 +- .../approvals/components/approvers_select_spec.js | 14 ++++++++++---- .../project_approval_rules_shared_context.rb | 3 ++- spec/frontend/api_spec.js | 14 ++++++++++++++ 7 files changed, 56 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js index 48005787d81aab..2b589b71163295 100644 --- a/app/assets/javascripts/api.js +++ b/app/assets/javascripts/api.js @@ -31,6 +31,7 @@ const Api = { projectLabelsPath: '/:namespace_path/:project_path/-/labels', projectFileSchemaPath: '/:namespace_path/:project_path/-/schema/:ref/:filename', projectUsersPath: '/api/:version/projects/:id/users', + projectGroupsPath: '/api/:version/projects/:id/groups.json', projectInvitationsPath: '/api/:version/projects/:id/invitations', projectMembersPath: '/api/:version/projects/:id/members', projectMergeRequestsPath: '/api/:version/projects/:id/merge_requests', @@ -241,6 +242,20 @@ const Api = { .then(({ data }) => data); }, + projectGroups(id, options) { + const url = Api.buildUrl(this.projectGroupsPath).replace(':id', encodeURIComponent(id)); + + return axios + .get(url, { + params: { + ...options, + }, + }) + .then(({ data }) => { + return data; + }); + }, + addProjectMembersByUserId(id, data) { const url = Api.buildUrl(this.projectMembersPath).replace(':id', encodeURIComponent(id)); diff --git a/ee/app/assets/javascripts/approvals/components/approvers_select.vue b/ee/app/assets/javascripts/approvals/components/approvers_select.vue index fd2678c22b6b91..4172a492d0dafa 100644 --- a/ee/app/assets/javascripts/approvals/components/approvers_select.vue +++ b/ee/app/assets/javascripts/approvals/components/approvers_select.vue @@ -7,6 +7,8 @@ import { loadCSSFile } from '~/lib/utils/css_utils'; import { __ } from '~/locale'; import { TYPE_USER, TYPE_GROUP } from '../constants'; +const DEVELOPER_ACCESS_LEVEL = 30; + function addType(type) { return (items) => items.map((obj) => Object.assign(obj, { type })); } @@ -135,13 +137,11 @@ export default { .then((results) => ({ results })); }, fetchGroups(term) { - // Don't includeAll when search is empty. Otherwise, the user could get a lot of garbage choices. - // https://gitlab.com/gitlab-org/gitlab/issues/11566 - const includeAll = term.trim().length > 0; + const hasTerm = term.trim().length > 0; - return Api.groups(term, { + return Api.projectGroups(this.projectId, { skip_groups: this.skipGroupIds, - all_available: includeAll, + ...(hasTerm ? { search: term } : {}), }); }, fetchUsers(term) { diff --git a/ee/spec/features/merge_request/user_sets_approvers_spec.rb b/ee/spec/features/merge_request/user_sets_approvers_spec.rb index 09c8cc89fe7c08..1958ee0ffc3943 100644 --- a/ee/spec/features/merge_request/user_sets_approvers_spec.rb +++ b/ee/spec/features/merge_request/user_sets_approvers_spec.rb @@ -87,10 +87,12 @@ def non_collapse_approval_rules it 'allows setting groups as approvers' do group = create :group - group.add_developer(other_user) + group_project = create :project, :repository, group: group - visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' }) + group.add_developer(user) + group.add_developer(other_user) + visit project_new_merge_request_path(group_project, merge_request: { target_branch: 'master', source_branch: 'feature' }) open_modal(text: 'Add approval rule') open_approver_select @@ -156,9 +158,13 @@ def non_collapse_approval_rules it 'allows setting groups as approvers' do group = create :group + group_project = create :project, :repository, group: group + group_project_merge_request = create :merge_request, source_project: group_project + + group.add_developer(user) group.add_developer(other_user) - visit edit_project_merge_request_path(project, merge_request) + visit edit_project_merge_request_path(group_project, group_project_merge_request) open_modal(text: 'Add approval rule') open_approver_select 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 4efb445239f831..b5e828a1a653dc 100644 --- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -6,8 +6,8 @@ include FeatureApprovalHelper let(:user) { create(:user) } - let(:project) { create(:project) } let(:group) { create(:group) } + let(:project) { create(:project, group: group) } let(:group_member) { create(:user) } let(:non_member) { create(:user) } let!(:config_selector) { '.js-approval-rules' } diff --git a/ee/spec/frontend/approvals/components/approvers_select_spec.js b/ee/spec/frontend/approvals/components/approvers_select_spec.js index ce210f631a94d8..faed53273a3a18 100644 --- a/ee/spec/frontend/approvals/components/approvers_select_spec.js +++ b/ee/spec/frontend/approvals/components/approvers_select_spec.js @@ -76,7 +76,7 @@ describe('Approvals ApproversSelect', () => { }; beforeEach(() => { - jest.spyOn(Api, 'groups').mockResolvedValue(TEST_GROUPS); + jest.spyOn(Api, 'projectGroups').mockResolvedValue(TEST_GROUPS); jest.spyOn(Api, 'projectUsers').mockReturnValue(Promise.resolve(TEST_USERS)); }); @@ -126,7 +126,12 @@ describe('Approvals ApproversSelect', () => { }); it('fetches all available groups', () => { - expect(Api.groups).toHaveBeenCalledWith(term, { skip_groups: [], all_available: true }); + expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, { + skip_groups: [], + with_shared: true, + shared_min_access_level: 30, + search: term, + }); }); it('fetches users', () => { @@ -157,9 +162,10 @@ describe('Approvals ApproversSelect', () => { }); it('skips groups and does not fetch all available', () => { - expect(Api.groups).toHaveBeenCalledWith('', { + expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, { skip_groups: skipGroupIds, - all_available: false, + with_shared: true, + shared_min_access_level: 30, }); }); diff --git a/ee/spec/support/shared_contexts/project_approval_rules_shared_context.rb b/ee/spec/support/shared_contexts/project_approval_rules_shared_context.rb index cfda0e9dd6838e..24491e4c4ab90c 100644 --- a/ee/spec/support/shared_contexts/project_approval_rules_shared_context.rb +++ b/ee/spec/support/shared_contexts/project_approval_rules_shared_context.rb @@ -3,7 +3,8 @@ RSpec.shared_context 'with project with approval rules' do let_it_be(:approver) { create(:user) } let_it_be(:author) { create(:user) } - let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :public, :repository, group: group) } before do stub_licensed_features(multiple_approval_rules: true) diff --git a/spec/frontend/api_spec.js b/spec/frontend/api_spec.js index d6e1b170dd3d34..a03aabf9e4fb26 100644 --- a/spec/frontend/api_spec.js +++ b/spec/frontend/api_spec.js @@ -352,6 +352,20 @@ describe('Api', () => { }); }); + describe('projectGroups', () => { + it('fetches a project group', async () => { + const options = { unused: 'option' }; + const projectId = 1; + const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects/${projectId}/groups.json`; + mock.onGet(expectedUrl, { params: options }).reply(httpStatus.OK, { + name: 'test', + }); + + const { name } = await Api.projectGroups(projectId, options); + expect(name).toBe('test'); + }); + }); + describe('projectUsers', () => { it('fetches all users of a particular project', (done) => { const query = 'dummy query'; -- GitLab From 6e8761dc052c05d15e54e20621485eade20b0016 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Sat, 6 Mar 2021 01:16:48 -0800 Subject: [PATCH 2/2] Made some update to changed API usage Update spec for new API group return --- .../javascripts/approvals/components/approvers_select.vue | 2 -- .../merge_request/user_edits_approval_rules_mr_spec.rb | 4 ---- .../frontend/approvals/components/approvers_select_spec.js | 4 ---- 3 files changed, 10 deletions(-) diff --git a/ee/app/assets/javascripts/approvals/components/approvers_select.vue b/ee/app/assets/javascripts/approvals/components/approvers_select.vue index 4172a492d0dafa..bd203f4f78236a 100644 --- a/ee/app/assets/javascripts/approvals/components/approvers_select.vue +++ b/ee/app/assets/javascripts/approvals/components/approvers_select.vue @@ -7,8 +7,6 @@ import { loadCSSFile } from '~/lib/utils/css_utils'; import { __ } from '~/locale'; import { TYPE_USER, TYPE_GROUP } from '../constants'; -const DEVELOPER_ACCESS_LEVEL = 30; - function addType(type) { return (items) => items.map((obj) => Object.assign(obj, { type })); } diff --git a/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb b/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb index 7d7a651f20eec3..07ec86431fc7dc 100644 --- a/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb +++ b/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb @@ -73,11 +73,7 @@ def add_approval_rule_member(type, name) end context "with public group" do - let_it_be(:group) { create(:group, :public) } - before do - group.add_developer create(:user) - click_button 'Approval rules' click_button "Add approval rule" end diff --git a/ee/spec/frontend/approvals/components/approvers_select_spec.js b/ee/spec/frontend/approvals/components/approvers_select_spec.js index faed53273a3a18..c82e51f825cbd4 100644 --- a/ee/spec/frontend/approvals/components/approvers_select_spec.js +++ b/ee/spec/frontend/approvals/components/approvers_select_spec.js @@ -128,8 +128,6 @@ describe('Approvals ApproversSelect', () => { it('fetches all available groups', () => { expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, { skip_groups: [], - with_shared: true, - shared_min_access_level: 30, search: term, }); }); @@ -164,8 +162,6 @@ describe('Approvals ApproversSelect', () => { it('skips groups and does not fetch all available', () => { expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, { skip_groups: skipGroupIds, - with_shared: true, - shared_min_access_level: 30, }); }); -- GitLab