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 218e782717a7e52b07256a0bff517022d35bc56e..06ec49de53c05b4a644359f02d629108806bd196 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 0000000000000000000000000000000000000000..ce07c3c6e81b918496ae53417242d0340b10d462 --- /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 c48d46748f3a7e01412b90de191b9a9f00f6b1a8..9a4447526bff9638998dc61c69c14f8d4a96d351 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 1b5157e93ffeb7ffcc47a3e705bf1f3de211190d..c49b25f9763b89e1ad935daae88274645af279d1 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 defb1df6c7ceb0d0eec63067061733955473db1b..b492fbbc18dbfcb8add72ca2d14b6ae38bc9f7b8 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" /> 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 f9af086b0bcfaf42b44f334d670305c957498418..6d7857e632fa45edeade5ce289829c298f72c06a 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'; @@ -234,6 +235,7 @@ describe('Reviewer dropdown component', () => { 'user_selects_reviewer_from_mr_sidebar', { value: 1, + suggested_position: 0, selectable_reviewers_count: 2, }, undefined, @@ -255,6 +257,7 @@ describe('Reviewer dropdown component', () => { 'user_selects_reviewer_from_mr_sidebar', { value: 1, + suggested_position: 0, selectable_reviewers_count: 1, }, undefined, @@ -270,6 +273,7 @@ describe('Reviewer dropdown component', () => { 'user_selects_reviewer_from_mr_sidebar_after_search', { value: 2, + suggested_position: 0, selectable_reviewers_count: 2, }, undefined, @@ -306,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, + ); + }); + }); }); }); 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 0000000000000000000000000000000000000000..0ab3c14c85f08f8e11b789977038e0f6f3fae3f7 --- /dev/null +++ b/spec/frontend/merge_requests/utils/reviewer_positions_spec.js @@ -0,0 +1,103 @@ +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; + + beforeEach(() => { + const mockSessionStorage = { + setItem: jest.fn(), + getItem: jest.fn().mockImplementation((key) => { + const vals = { + 'MergeRequest/123/test-list-id': mockReviewersString, + }; + + return vals[key]; + }), + }; + + Object.defineProperty(window, 'sessionStorage', { + value: mockSessionStorage, + writable: true, + }); + + setSpy = mockSessionStorage.setItem; + }); + + 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', () => { + 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: 'some-issuable-id-that-doesnt-exist', + listId: 'some-list-that-doesnt-exist', + }); + + expect(result).toEqual([]); + }); + }); + + 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); + }); + }); +});