From 32cfe98990f6cb1a3641147d4824a5ab071f0696 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 15 Nov 2024 11:12:58 -0700 Subject: [PATCH 1/4] Avoid shadowing the "visibleReviewers" variable name - When passing in the lists of reviewers, use "displayable" - Once the selected reviewers have been filtered to just the displayable ones, that's when they're "visible" --- .../components/reviewers/reviewer_dropdown.vue | 10 +++++----- .../components/reviewers/approval_rules.vue | 4 ++-- 2 files changed, 7 insertions(+), 7 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 6bf2b0b298194b..9fc1de913c05c1 100644 --- a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue +++ b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue @@ -62,7 +62,7 @@ export default { required: false, default: () => [], }, - visibleReviewers: { + displayableReviewers: { type: Array, required: false, default: () => [], @@ -103,10 +103,10 @@ export default { const items = []; const users = this.usersForList; - if (this.selectedReviewersToShow.length && !this.search) { + if (this.visibleReviewers.length && !this.search) { items.push({ text: __('Reviewers'), - options: this.selectedReviewersToShow.map((user) => this.mapUser(user)), + options: this.visibleReviewers.map((user) => this.mapUser(user)), }); } @@ -122,9 +122,9 @@ export default { return items; }, - selectedReviewersToShow() { + visibleReviewers() { return this.selectedReviewers.filter((user) => - this.visibleReviewers.map((gqlUser) => gqlUser.id).includes(user.id), + this.displayableReviewers.map((gqlUser) => gqlUser.id).includes(user.id), ); }, }, 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 555b790fa1f459..d037f25a401c68 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 @@ -75,7 +75,7 @@ export default { toggleApprovalSections() { this.showApprovalSections = !this.showApprovalSections; }, - visibleReviewersForRule(rule) { + reviewersEligibleToDisplayForRule(rule) { let visible = rule.reviewers; if (rule.type.toLowerCase() === RULE_TYPE_ANY_APPROVER) { @@ -163,7 +163,7 @@ export default {
-- GitLab From 1c0be96b70349591de35c5a7082306111ef52285 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 15 Nov 2024 11:27:03 -0700 Subject: [PATCH 2/4] Use "eligible" instead of "displayable" --- .../merge_requests/components/reviewers/reviewer_dropdown.vue | 4 ++-- .../merge_requests/components/reviewers/approval_rules.vue | 4 ++-- 2 files changed, 4 insertions(+), 4 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 9fc1de913c05c1..26c805087e2238 100644 --- a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue +++ b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue @@ -62,7 +62,7 @@ export default { required: false, default: () => [], }, - displayableReviewers: { + eligibleReviewers: { type: Array, required: false, default: () => [], @@ -124,7 +124,7 @@ export default { }, visibleReviewers() { return this.selectedReviewers.filter((user) => - this.displayableReviewers.map((gqlUser) => gqlUser.id).includes(user.id), + this.eligibleReviewers.map((gqlUser) => gqlUser.id).includes(user.id), ); }, }, 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 d037f25a401c68..6bc9a97f09d11e 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 @@ -75,7 +75,7 @@ export default { toggleApprovalSections() { this.showApprovalSections = !this.showApprovalSections; }, - reviewersEligibleToDisplayForRule(rule) { + reviewersEligibleForRule(rule) { let visible = rule.reviewers; if (rule.type.toLowerCase() === RULE_TYPE_ANY_APPROVER) { @@ -163,7 +163,7 @@ export default {
-- GitLab From a94a6a1062e2aeb607d63eafc59e06031df0bfdf Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 14 May 2025 16:11:43 -0600 Subject: [PATCH 3/4] Fix missed prop name change --- .../javascripts/sidebar/components/reviewers/reviewer_title.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/sidebar/components/reviewers/reviewer_title.vue b/app/assets/javascripts/sidebar/components/reviewers/reviewer_title.vue index 6cb0e78c7ae682..6952b0b44615ac 100644 --- a/app/assets/javascripts/sidebar/components/reviewers/reviewer_title.vue +++ b/app/assets/javascripts/sidebar/components/reviewers/reviewer_title.vue @@ -54,7 +54,7 @@ export default { class="gl-ml-auto" usage="simple" :selected-reviewers="reviewers" - :visible-reviewers="reviewers" + :eligible-reviewers="reviewers" />
-- GitLab From 5b3fe63fd83347dc6d4c6dffd229f6596cca95e8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 22 May 2025 12:19:06 -0600 Subject: [PATCH 4/4] Describe eligible versus visible reviewers in comments --- .../merge_requests/components/reviewers/reviewer_dropdown.vue | 2 ++ 1 file changed, 2 insertions(+) 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 26c805087e2238..187ce37a2867d2 100644 --- a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue +++ b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue @@ -62,6 +62,7 @@ export default { required: false, default: () => [], }, + // Any user eligible to be a reviewer for this list (based on approval rule, etc.) eligibleReviewers: { type: Array, required: false, @@ -123,6 +124,7 @@ export default { return items; }, visibleReviewers() { + // Eligible users filtered to only show the previously selected users return this.selectedReviewers.filter((user) => this.eligibleReviewers.map((gqlUser) => gqlUser.id).includes(user.id), ); -- GitLab