From 9616cdbfb0d74c4c4b42cf2c2b312c1964b6c432 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 17 Oct 2024 17:09:24 -0600 Subject: [PATCH 1/7] Limit users who can appear in approval rule reviers dropdown --- .../components/reviewers/reviewer_dropdown.vue | 12 ++++++++++-- .../components/reviewers/approval_rules.vue | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue index 62106936db838b..d98beee6ab567d 100644 --- a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue +++ b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue @@ -42,6 +42,11 @@ export default { required: false, default: () => [], }, + visibleReviewers: { + type: Array, + required: false, + default: () => [], + }, }, data() { return { @@ -55,12 +60,15 @@ export default { computed: { mappedUsers() { const items = []; + const visibleReviewers = this.selectedReviewers.filter((user) => + this.visibleReviewers.map((gqlUser) => gqlUser.id).includes(user.id), + ); let users; - if (this.selectedReviewers.length && !this.search) { + if (visibleReviewers.length && !this.search) { items.push({ text: __('Reviewers'), - options: this.selectedReviewers.map((user) => this.mapUser(user)), + options: visibleReviewers.map((user) => this.mapUser(user)), }); } diff --git a/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue b/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue index d0cc7f85e340ef..8254f87b3badf8 100644 --- a/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue +++ b/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue @@ -148,6 +148,7 @@ export default {
-- GitLab From fd4bf3ddfde8dab700ef9acae327ee5c116bb9bf Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 18 Oct 2024 15:14:08 -0600 Subject: [PATCH 2/7] Test the limitation of not showing pre-selected users The dropdown present in the test scenario is the system-wide "Any eligible user" approval rule, which indicates that NO user is eligible for that rule, which is why all selected users are filtered out. --- .../reviewers/reviewer_dropdown_spec.js | 58 +++++++++++++------ 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js b/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js index 3ca5fc9de95381..56a1ea9d462837 100644 --- a/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js +++ b/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js @@ -131,25 +131,49 @@ describe('Reviewer dropdown component', () => { }); }); - it('renders users from autocomplete endpoint', async () => { - findDropdown().vm.$emit('shown'); + describe('with the user already selected', () => { + it('renders users from autocomplete endpoint and skips "ineligible" pre-selected reviewers', async () => { + findDropdown().vm.$emit('shown'); + + await waitForPromises(); + + expect(findDropdown().props('items')).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + options: [], + }), + ]), + ); + }); + }); - await waitForPromises(); + describe('with the user not already selected', () => { + beforeEach(async () => { + createComponent(true, {}); - expect(findDropdown().props('items')).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - options: expect.arrayContaining([ - expect.objectContaining({ - secondaryText: '@root', - text: 'Administrator', - value: 'root', - mergeRequestInteraction: { canMerge: true }, - }), - ]), - }), - ]), - ); + await waitForPromises(); + }); + + it('renders users from autocomplete endpoint', async () => { + findDropdown().vm.$emit('shown'); + + await waitForPromises(); + + expect(findDropdown().props('items')).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + options: expect.arrayContaining([ + expect.objectContaining({ + secondaryText: '@root', + text: 'Administrator', + value: 'root', + mergeRequestInteraction: { canMerge: true }, + }), + ]), + }), + ]), + ); + }); }); it('updates reviewers when dropdown is closed', () => { -- GitLab From b557a702365fbc4e983714c48c30641b4c88d7af Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 21 Oct 2024 14:41:15 -0600 Subject: [PATCH 3/7] Remove the "Any eligible user" rule from filtering --- .../components/reviewers/approval_rules.vue | 11 ++++++- .../reviewers/reviewer_dropdown_spec.js | 29 ++++++++++++++----- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue b/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue index 8254f87b3badf8..39e2bb1059faa2 100644 --- a/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue +++ b/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue @@ -69,6 +69,15 @@ export default { toggleApprovalSections() { this.showApprovalSections = !this.showApprovalSections; }, + visibleReviewersForRule(rule) { + let visible = rule.reviewers; + + if (rule.type.toLowerCase() === 'any_approver') { + visible = this.reviewers; + } + + return visible; + }, }, ANY_APPROVER: RULE_TYPE_ANY_APPROVER.toUpperCase(), CODE_OWNERS: RULE_TYPE_CODE_OWNER.toUpperCase(), @@ -148,7 +157,7 @@ export default {
diff --git a/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js b/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js index 56a1ea9d462837..4d881ccf517589 100644 --- a/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js +++ b/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js @@ -16,19 +16,19 @@ let setReviewersMutationMock; Vue.use(VueApollo); -const createMockUser = () => ({ +const createMockUser = ({ id = 1, name = 'Administrator', username = 'root' } = {}) => ({ __typename: 'UserCore', - id: 'gid://gitlab/User/1', + id: `gid://gitlab/User/${id}`, avatarUrl: 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon', - name: 'Administrator', - username: 'root', - webUrl: '/root', - webPath: '/root', + webUrl: `/${username}`, + webPath: `/${username}`, status: null, mergeRequestInteraction: { canMerge: true, }, + username, + name, }); function createComponent( @@ -39,7 +39,7 @@ function createComponent( data: { workspace: { id: 1, - users: [createMockUser()], + users: [createMockUser(), createMockUser({ id: 2, name: 'Nonadmin', username: 'bob' })], }, }, }); @@ -140,7 +140,14 @@ describe('Reviewer dropdown component', () => { expect(findDropdown().props('items')).toEqual( expect.arrayContaining([ expect.objectContaining({ - options: [], + options: expect.arrayContaining([ + expect.objectContaining({ + secondaryText: '@bob', + text: 'Nonadmin', + value: 'bob', + mergeRequestInteraction: { canMerge: true }, + }), + ]), }), ]), ); @@ -169,6 +176,12 @@ describe('Reviewer dropdown component', () => { value: 'root', mergeRequestInteraction: { canMerge: true }, }), + expect.objectContaining({ + secondaryText: '@bob', + text: 'Nonadmin', + value: 'bob', + mergeRequestInteraction: { canMerge: true }, + }), ]), }), ]), -- GitLab From d35b1cc9389b6447dead91b2b8914cc2464419fb Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 21 Oct 2024 14:52:33 -0600 Subject: [PATCH 4/7] Move visibleReviewers ("which users to display") to a computed This should avoid continually recomputing a value which is static per-rule. --- .../components/reviewers/reviewer_dropdown.vue | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue index d98beee6ab567d..29de3fc32332da 100644 --- a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue +++ b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue @@ -60,15 +60,12 @@ export default { computed: { mappedUsers() { const items = []; - const visibleReviewers = this.selectedReviewers.filter((user) => - this.visibleReviewers.map((gqlUser) => gqlUser.id).includes(user.id), - ); let users; - if (visibleReviewers.length && !this.search) { + if (this.selectedReviewersToShow.length && !this.search) { items.push({ text: __('Reviewers'), - options: visibleReviewers.map((user) => this.mapUser(user)), + options: this.selectedReviewersToShow.map((user) => this.mapUser(user)), }); } @@ -90,6 +87,11 @@ export default { return items; }, + selectedReviewersToShow() { + return this.selectedReviewers.filter((user) => + this.visibleReviewers.map((gqlUser) => gqlUser.id).includes(user.id), + ); + } }, watch: { selectedReviewers(newVal) { -- GitLab From 06f93ad5aa3b8cc600152df4f70caab81dcde7f6 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 21 Oct 2024 15:49:19 -0600 Subject: [PATCH 5/7] Make prettier happy --- .../merge_requests/components/reviewers/reviewer_dropdown.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue index 29de3fc32332da..90df79503369a8 100644 --- a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue +++ b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue @@ -91,7 +91,7 @@ export default { return this.selectedReviewers.filter((user) => this.visibleReviewers.map((gqlUser) => gqlUser.id).includes(user.id), ); - } + }, }, watch: { selectedReviewers(newVal) { -- GitLab From 4777e8a89cc835a145a89ab4b0418864d564f377 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 21 Oct 2024 15:50:01 -0600 Subject: [PATCH 6/7] Tests for approval rules must pass `type` now, as it's used directly --- .../components/reviewers/approval_rules_spec.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ee/spec/frontend/merge_requests/components/reviewers/approval_rules_spec.js b/ee/spec/frontend/merge_requests/components/reviewers/approval_rules_spec.js index 5f09f344f19ae5..63b446dfcc0091 100644 --- a/ee/spec/frontend/merge_requests/components/reviewers/approval_rules_spec.js +++ b/ee/spec/frontend/merge_requests/components/reviewers/approval_rules_spec.js @@ -39,6 +39,7 @@ describe('Reviewer drawer approval rules component', () => { { approvalsRequired, name: 'Optional rule', + type: 'any_approver', approvedBy: { nodes: [], }, @@ -46,6 +47,7 @@ describe('Reviewer drawer approval rules component', () => { { approvalsRequired, name: 'Required rule', + type: 'code_owner', approvedBy: { nodes: [], }, @@ -53,6 +55,7 @@ describe('Reviewer drawer approval rules component', () => { { approvalsRequired, name: 'Approved rule', + type: 'regular', approvedBy: { nodes: [{ id: 1 }], }, @@ -108,6 +111,7 @@ describe('Reviewer drawer approval rules component', () => { approvalsRequired: 1, name: 'Approved rule', section: 'Frontend', + type: 'code_owner', approvedBy: { nodes: [{ id: 1 }], }, @@ -123,6 +127,7 @@ describe('Reviewer drawer approval rules component', () => { approvalsRequired: 1, name: 'Approved rule', section: 'codeowners', + type: 'code_owner', approvedBy: { nodes: [{ id: 1 }], }, -- GitLab From 40caf1d78b85cfef847636ba79058768a869a449 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 23 Oct 2024 10:18:21 -0600 Subject: [PATCH 7/7] Use constant for string matching to exclude the "any approver" rule --- .../merge_requests/components/reviewers/approval_rules.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue b/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue index 39e2bb1059faa2..012d7c30c93eab 100644 --- a/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue +++ b/ee/app/assets/javascripts/merge_requests/components/reviewers/approval_rules.vue @@ -72,7 +72,7 @@ export default { visibleReviewersForRule(rule) { let visible = rule.reviewers; - if (rule.type.toLowerCase() === 'any_approver') { + if (rule.type.toLowerCase() === RULE_TYPE_ANY_APPROVER) { visible = this.reviewers; } -- GitLab