From d3bb52886824f0672f8cb6dce45d9ea756e41c6c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 8 Apr 2025 21:58:14 -0600 Subject: [PATCH 1/6] Send the previous (sans-search) position of a chosen reviewer --- .../reviewers/reviewer_dropdown.vue | 28 +++++++++++++++++++ .../utils/reviewer_positions.js | 20 +++++++++++++ .../user_selects_reviewer_from_mr_sidebar.yml | 2 ++ ..._reviewer_from_mr_sidebar_after_search.yml | 2 ++ .../components/reviewers/approval_rules.vue | 1 + 5 files changed, 53 insertions(+) create mode 100644 app/assets/javascripts/merge_requests/utils/reviewer_positions.js 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 218e782717a7e5..06ec49de53c05b 100644 --- a/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue +++ b/app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue @@ -4,6 +4,7 @@ import { GlCollapsibleListbox, GlButton, GlAvatar, GlIcon } from '@gitlab/ui'; import { __ } from '~/locale'; import { InternalEvents } from '~/tracking'; import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; +import { uuids } from '~/lib/utils/uuids'; import { TYPENAME_MERGE_REQUEST } from '~/graphql_shared/constants'; import { convertToGraphQLId } from '~/graphql_shared/utils'; import userAutocompleteWithMRPermissionsQuery from '~/graphql_shared/queries/project_autocomplete_users_with_mr_permissions.query.graphql'; @@ -14,6 +15,11 @@ import { SEARCH_SELECT_REVIEWER_EVENT, SELECT_REVIEWER_EVENT, } from '../../constants'; +import { + getReviewersForList, + suggestedPosition, + setReviewersForList, +} from '../../utils/reviewer_positions'; import UpdateReviewers from './update_reviewers.vue'; import userPermissionsQuery from './queries/user_permissions.query.graphql'; @@ -66,6 +72,11 @@ export default { required: false, default: () => 'complex', }, + uniqueId: { + type: String, + required: false, + default: () => uuids()[0], + }, }, data() { return { @@ -159,6 +170,17 @@ export default { }, }); + if (!search) { + const theseUsers = toUsernames(users); + const newOptions = difference(theseUsers, this.currentSelectedReviewers); + + setReviewersForList({ + issuableId: this.issuableId, + listId: this.uniqueId, + reviewers: newOptions, + }); + } + this.fetchedUsers = users; this.searching = false; }, @@ -169,6 +191,10 @@ export default { const telemetryEvent = this.search ? SEARCH_SELECT_REVIEWER_EVENT : SELECT_REVIEWER_EVENT; const previousUsernames = toUsernames(this.selectedReviewers); const listUsernames = toUsernames(this.usersForList); + const suggested = getReviewersForList({ + issuableId: this.issuableId, + listId: this.uniqueId, + }); // Reviewers are always shown first if they are in the list, // so we should exclude them for when we check the position const selectableList = difference(listUsernames, previousUsernames); @@ -177,9 +203,11 @@ export default { additions.forEach((added) => { // Convert from 0- to 1-index const listPosition = selectableList.findIndex((user) => user === added) + 1; + const suggestedPos = suggestedPosition({ username: added, list: suggested }); this.trackEvent(telemetryEvent, { value: listPosition, + suggested_position: suggestedPos, selectable_reviewers_count: selectableList.length, }); }); diff --git a/app/assets/javascripts/merge_requests/utils/reviewer_positions.js b/app/assets/javascripts/merge_requests/utils/reviewer_positions.js new file mode 100644 index 00000000000000..7a6f6797119970 --- /dev/null +++ b/app/assets/javascripts/merge_requests/utils/reviewer_positions.js @@ -0,0 +1,20 @@ +function cacheId({ issuableId, listId } = {}) { + return `MergeRequest/${issuableId}/${listId}`; +} + +export function setReviewersForList({ issuableId, listId, reviewers } = {}) { + const id = cacheId({ issuableId, listId }); + + window.sessionStorage.setItem(id, JSON.stringify(reviewers, null, 0)); +} + +export function getReviewersForList({ issuableId, listId } = {}) { + const id = cacheId({ issuableId, listId }); + const list = window.sessionStorage.getItem(id); + + return list ? JSON.parse(list) : []; +} + +export function suggestedPosition({ username, list } = {}) { + return list.indexOf(username) + 1; // 1-index, so that "0" means they weren't in the list +} diff --git a/config/events/user_selects_reviewer_from_mr_sidebar.yml b/config/events/user_selects_reviewer_from_mr_sidebar.yml index c48d46748f3a7e..9a4447526bff96 100644 --- a/config/events/user_selects_reviewer_from_mr_sidebar.yml +++ b/config/events/user_selects_reviewer_from_mr_sidebar.yml @@ -9,6 +9,8 @@ identifiers: additional_properties: value: description: List position of the selected user + suggested_position: + description: The original position of this user in the suggested list, or 0 if they weren't included selectable_reviewers_count: description: The total number of selectable reviewers in the list this reviewer was selected from product_group: code_review diff --git a/config/events/user_selects_reviewer_from_mr_sidebar_after_search.yml b/config/events/user_selects_reviewer_from_mr_sidebar_after_search.yml index 1b5157e93ffeb7..c49b25f9763b89 100644 --- a/config/events/user_selects_reviewer_from_mr_sidebar_after_search.yml +++ b/config/events/user_selects_reviewer_from_mr_sidebar_after_search.yml @@ -9,6 +9,8 @@ identifiers: additional_properties: value: description: List position of the selected user + suggested_position: + description: The original position of this user in the suggested list, or 0 if they weren't included selectable_reviewers_count: description: The total number of selectable reviewers in the list this reviewer was selected from product_group: code_review 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 defb1df6c7ceb0..b492fbbc18dbfc 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 @@ -159,6 +159,7 @@ export default { :selected-reviewers="reviewers" :visible-reviewers="visibleReviewersForRule(rule)" :users="rule.eligibleApprovers" + :unique-id="rule.id" /> -- GitLab From 7149c4c9953126596891139c0e307be4a3f08de5 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 16:24:43 -0600 Subject: [PATCH 2/6] Update existing tests with new tracking parameter --- .../components/reviewers/reviewer_dropdown_spec.js | 3 +++ 1 file changed, 3 insertions(+) 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 f9af086b0bcfaf..a9282be06896c6 100644 --- a/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js +++ b/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js @@ -234,6 +234,7 @@ describe('Reviewer dropdown component', () => { 'user_selects_reviewer_from_mr_sidebar', { value: 1, + suggested_position: 0, selectable_reviewers_count: 2, }, undefined, @@ -255,6 +256,7 @@ describe('Reviewer dropdown component', () => { 'user_selects_reviewer_from_mr_sidebar', { value: 1, + suggested_position: 0, selectable_reviewers_count: 1, }, undefined, @@ -270,6 +272,7 @@ describe('Reviewer dropdown component', () => { 'user_selects_reviewer_from_mr_sidebar_after_search', { value: 2, + suggested_position: 0, selectable_reviewers_count: 2, }, undefined, -- GitLab From dc379e67b8653a0197cdaeb654e0fef2838d0dcc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 17:22:09 -0600 Subject: [PATCH 3/6] Test new "historical" suggestion position reporting --- .../reviewers/reviewer_dropdown_spec.js | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) 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 a9282be06896c6..6d7857e632fa45 100644 --- a/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js +++ b/spec/frontend/merge_requests/components/reviewers/reviewer_dropdown_spec.js @@ -6,6 +6,7 @@ import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; import { mockTracking, triggerEvent } from 'helpers/tracking_helper'; import { useMockInternalEventsTracking } from 'helpers/tracking_internal_events_helper'; +import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; import ReviewerDropdown from '~/merge_requests/components/reviewers/reviewer_dropdown.vue'; import UpdateReviewers from '~/merge_requests/components/reviewers/update_reviewers.vue'; import userPermissionsQuery from '~/merge_requests/components/reviewers/queries/user_permissions.query.graphql'; @@ -309,6 +310,95 @@ describe('Reviewer dropdown component', () => { ); }); }); + + describe('"suggested" historical position', () => { + it('reports the correct prior suggested position', async () => { + createComponent(true, { + selectedReviewers: [], + }); + + await waitForPromises(); + + findDropdown().vm.$emit('shown'); + + await waitForPromises(); + + findDropdown().vm.$emit('select', ['root']); + findDropdown().vm.$emit('hidden'); + + expect(trackEventSpy).toHaveBeenCalledWith( + 'user_selects_reviewer_from_mr_sidebar', + { + value: 1, + suggested_position: 1, + selectable_reviewers_count: 2, + }, + undefined, + ); + }); + + it('reports the correct prior suggested position - discounting already selected reviewers', async () => { + createComponent(true, { + selectedReviewers: [createMockUser()], + }); + + await waitForPromises(); + + findDropdown().vm.$emit('shown'); + + await waitForPromises(); + + findDropdown().vm.$emit('select', ['bob']); + findDropdown().vm.$emit('hidden'); + + expect(trackEventSpy).toHaveBeenCalledWith( + 'user_selects_reviewer_from_mr_sidebar', + { + value: 1, + suggested_position: 1, + selectable_reviewers_count: 1, + }, + undefined, + ); + }); + + it("reports 0 as the prior suggested position of the reviewer if they weren't in the initial suggested list", async () => { + createComponent(true, { + selectedReviewers: [], + }); + + await waitForPromises(); + + findDropdown().vm.$emit('shown'); + await waitForPromises(); + + autocompleteUsersMock.mockReset(); + autocompleteUsersMock.mockResolvedValue({ + data: { + workspace: { + id: 1, + users: [createMockUser({ id: 3, name: 'Friend', username: 'coolguy' })], + }, + }, + }); + findDropdown().vm.$emit('search', 'coolguy'); + jest.advanceTimersByTime(DEFAULT_DEBOUNCE_AND_THROTTLE_MS); + await waitForPromises(); + + findDropdown().vm.$emit('select', ['coolguy']); + findDropdown().vm.$emit('hidden'); + + expect(trackEventSpy).toHaveBeenCalledWith( + 'user_selects_reviewer_from_mr_sidebar_after_search', + { + value: 1, + suggested_position: 0, + selectable_reviewers_count: 1, + }, + undefined, + ); + }); + }); }); }); -- GitLab From 0435fa41b91b2caa71123245d6ecaeb2f9e679a3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 19:03:06 -0600 Subject: [PATCH 4/6] Test the utility that does basic management of the reviewer positions --- .../utils/reviewer_positions_spec.js | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 spec/frontend/merge_requests/utils/reviewer_positions_spec.js diff --git a/spec/frontend/merge_requests/utils/reviewer_positions_spec.js b/spec/frontend/merge_requests/utils/reviewer_positions_spec.js new file mode 100644 index 00000000000000..0ee3fec6da9e7f --- /dev/null +++ b/spec/frontend/merge_requests/utils/reviewer_positions_spec.js @@ -0,0 +1,61 @@ +import { + setReviewersForList, + getReviewersForList, + suggestedPosition, +} from '~/merge_requests/utils/reviewer_positions'; + +describe('reviewer_positions utility', () => { + const mockIssuableId = '123'; + const mockListId = 'test-list-id'; + const mockReviewers = ['user1', 'user2', 'user3']; + const mockReviewersString = '["user1","user2","user3"]'; + let setSpy; + let getSpy; + + beforeEach(() => { + window.sessionStorage.clear(); + + setSpy = jest.spyOn(window.sessionStorage, 'setItem'); + getSpy = jest.spyOn(window.sessionStorage, 'getItem'); + }); + + describe('setReviewersForList', () => { + it('stores reviewers in session storage with the correct key', () => { + setReviewersForList({ + issuableId: mockIssuableId, + listId: mockListId, + reviewers: mockReviewers, + }); + + expect(setSpy).toHaveBeenCalledWith( + `MergeRequest/${mockIssuableId}/${mockListId}`, + mockReviewersString, + ); + }); + }); + + describe('getReviewersForList', () => { + it('retrieves reviewers from session storage with the correct key', () => { + window.sessionStorage.setItem( + `MergeRequest/${mockIssuableId}/${mockListId}`, + mockReviewersString, + ); + + const result = getReviewersForList({ + issuableId: mockIssuableId, + listId: mockListId, + }); + + expect(result).toEqual(mockReviewers); + }); + + it('returns an empty array when no data exists in storage', () => { + const result = getReviewersForList({ + issuableId: mockIssuableId, + listId: mockListId, + }); + + expect(result).toEqual([]); + }); + }); +}); \ No newline at end of file -- GitLab From 37a82b311039ad7276ed13e4a807c4da0d181f56 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 19:17:02 -0600 Subject: [PATCH 5/6] Properly mock sessionStorage since it doesn't work out of the box --- .../utils/reviewer_positions_spec.js | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/spec/frontend/merge_requests/utils/reviewer_positions_spec.js b/spec/frontend/merge_requests/utils/reviewer_positions_spec.js index 0ee3fec6da9e7f..07a53946a0cd49 100644 --- a/spec/frontend/merge_requests/utils/reviewer_positions_spec.js +++ b/spec/frontend/merge_requests/utils/reviewer_positions_spec.js @@ -10,13 +10,25 @@ describe('reviewer_positions utility', () => { const mockReviewers = ['user1', 'user2', 'user3']; const mockReviewersString = '["user1","user2","user3"]'; let setSpy; - let getSpy; beforeEach(() => { - window.sessionStorage.clear(); + const mockSessionStorage = { + setItem: jest.fn(), + getItem: jest.fn().mockImplementation((key) => { + const vals = { + 'MergeRequest/123/test-list-id': mockReviewersString + }; - setSpy = jest.spyOn(window.sessionStorage, 'setItem'); - getSpy = jest.spyOn(window.sessionStorage, 'getItem'); + return vals[key]; + }) + }; + + Object.defineProperty(window, 'sessionStorage', { + value: mockSessionStorage, + writable: true + }); + + setSpy = mockSessionStorage.setItem; }); describe('setReviewersForList', () => { @@ -36,11 +48,6 @@ describe('reviewer_positions utility', () => { describe('getReviewersForList', () => { it('retrieves reviewers from session storage with the correct key', () => { - window.sessionStorage.setItem( - `MergeRequest/${mockIssuableId}/${mockListId}`, - mockReviewersString, - ); - const result = getReviewersForList({ issuableId: mockIssuableId, listId: mockListId, @@ -51,8 +58,8 @@ describe('reviewer_positions utility', () => { it('returns an empty array when no data exists in storage', () => { const result = getReviewersForList({ - issuableId: mockIssuableId, - listId: mockListId, + issuableId: 'some-issuable-id-that-doesnt-exist', + listId: 'some-list-that-doesnt-exist', }); expect(result).toEqual([]); -- GitLab From 2fbc4e0715bfddb4e17f70484c353d6e354aed2d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 19:23:34 -0600 Subject: [PATCH 6/6] Test the "suggested position" function --- .../utils/reviewer_positions.js | 4 +- .../utils/reviewer_positions_spec.js | 43 +++++++++++++++++-- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/merge_requests/utils/reviewer_positions.js b/app/assets/javascripts/merge_requests/utils/reviewer_positions.js index 7a6f6797119970..ce07c3c6e81b91 100644 --- a/app/assets/javascripts/merge_requests/utils/reviewer_positions.js +++ b/app/assets/javascripts/merge_requests/utils/reviewer_positions.js @@ -2,7 +2,7 @@ function cacheId({ issuableId, listId } = {}) { return `MergeRequest/${issuableId}/${listId}`; } -export function setReviewersForList({ issuableId, listId, reviewers } = {}) { +export function setReviewersForList({ issuableId, listId, reviewers = [] } = {}) { const id = cacheId({ issuableId, listId }); window.sessionStorage.setItem(id, JSON.stringify(reviewers, null, 0)); @@ -15,6 +15,6 @@ export function getReviewersForList({ issuableId, listId } = {}) { return list ? JSON.parse(list) : []; } -export function suggestedPosition({ username, list } = {}) { +export function suggestedPosition({ username, list = [] } = {}) { return list.indexOf(username) + 1; // 1-index, so that "0" means they weren't in the list } diff --git a/spec/frontend/merge_requests/utils/reviewer_positions_spec.js b/spec/frontend/merge_requests/utils/reviewer_positions_spec.js index 07a53946a0cd49..0ab3c14c85f08f 100644 --- a/spec/frontend/merge_requests/utils/reviewer_positions_spec.js +++ b/spec/frontend/merge_requests/utils/reviewer_positions_spec.js @@ -16,16 +16,16 @@ describe('reviewer_positions utility', () => { setItem: jest.fn(), getItem: jest.fn().mockImplementation((key) => { const vals = { - 'MergeRequest/123/test-list-id': mockReviewersString + 'MergeRequest/123/test-list-id': mockReviewersString, }; return vals[key]; - }) + }), }; Object.defineProperty(window, 'sessionStorage', { value: mockSessionStorage, - writable: true + writable: true, }); setSpy = mockSessionStorage.setItem; @@ -65,4 +65,39 @@ describe('reviewer_positions utility', () => { expect(result).toEqual([]); }); }); -}); \ No newline at end of file + + describe('suggestedPosition', () => { + it('returns the 1-indexed position of a username in the list', () => { + const result = suggestedPosition({ + username: 'user2', + list: mockReviewers, + }); + + expect(result).toBe(2); + }); + + it('returns 0 when the username is not in the list', () => { + const result = suggestedPosition({ + username: 'nonexistent-user', + list: mockReviewers, + }); + + expect(result).toBe(0); + }); + + it('returns 0 when the list is empty', () => { + const result = suggestedPosition({ + username: 'user1', + list: [], + }); + + expect(result).toBe(0); + }); + + it('handles undefined parameters gracefully', () => { + const result = suggestedPosition(); + + expect(result).toBe(0); + }); + }); +}); -- GitLab