From 06ecde12496cbb77e0218330756653a0bd152ad9 Mon Sep 17 00:00:00 2001 From: Felipe Santos Date: Fri, 29 Nov 2024 19:42:54 -0300 Subject: [PATCH] fix(gerrit): not auto-approving if change already had a Code-Review +1 --- lib/modules/platform/gerrit/client.spec.ts | 89 ++++++++++++++++++---- lib/modules/platform/gerrit/client.ts | 32 +++++--- lib/modules/platform/gerrit/scm.ts | 2 +- lib/modules/platform/gerrit/types.ts | 7 ++ 4 files changed, 102 insertions(+), 28 deletions(-) diff --git a/lib/modules/platform/gerrit/client.spec.ts b/lib/modules/platform/gerrit/client.spec.ts index 2d5ff5cddbf9fe6..72073d96bb19126 100644 --- a/lib/modules/platform/gerrit/client.spec.ts +++ b/lib/modules/platform/gerrit/client.spec.ts @@ -393,10 +393,20 @@ describe('modules/platform/gerrit/client', () => { describe('approveChange()', () => { it('already approved - do nothing', async () => { - const change = partial({}); + const change = partial({ + labels: { + 'Code-Review': { + all: [{ value: 2 }], + }, + }, + }); httpMock .scope(gerritEndpointUrl) - .get((url) => url.includes('/a/changes/123456?o=')) + .get( + (url) => + url.includes('/a/changes/123456?o=') && + url.includes('o=DETAILED_LABELS'), + ) .reply(200, gerritRestResponse(change), jsonResultHeader); await expect(client.approveChange(123456)).toResolve(); }); @@ -405,17 +415,53 @@ describe('modules/platform/gerrit/client', () => { const change = partial({ labels: {} }); httpMock .scope(gerritEndpointUrl) - .get((url) => url.includes('/a/changes/123456?o=')) + .get( + (url) => + url.includes('/a/changes/123456?o=') && + url.includes('o=DETAILED_LABELS'), + ) .reply(200, gerritRestResponse(change), jsonResultHeader); await expect(client.approveChange(123456)).toResolve(); }); it('not already approved - approve now', async () => { - const change = partial({ labels: { 'Code-Review': {} } }); + const change = partial({ + labels: { 'Code-Review': { all: [] } }, + }); + httpMock + .scope(gerritEndpointUrl) + .get( + (url) => + url.includes('/a/changes/123456?o=') && + url.includes('o=DETAILED_LABELS'), + ) + .reply(200, gerritRestResponse(change), jsonResultHeader); + const approveMock = httpMock + .scope(gerritEndpointUrl) + .post('/a/changes/123456/revisions/current/review', { + labels: { 'Code-Review': +2 }, + }) + .reply(200, gerritRestResponse(''), jsonResultHeader); + await expect(client.approveChange(123456)).toResolve(); + expect(approveMock.isDone()).toBeTrue(); + }); + + it('not already approved because of +1 - approve now', async () => { + const change = partial({ + labels: { + 'Code-Review': { + all: [{ value: 1 }], + }, + }, + }); httpMock .scope(gerritEndpointUrl) - .get((url) => url.includes('/a/changes/123456?o=')) + .get( + (url) => + url.includes('/a/changes/123456?o=') && + url.includes('o=DETAILED_LABELS'), + ) .reply(200, gerritRestResponse(change), jsonResultHeader); const approveMock = httpMock .scope(gerritEndpointUrl) @@ -432,7 +478,7 @@ describe('modules/platform/gerrit/client', () => { it('label not exists', () => { expect( client.wasApprovedBy(partial({}), 'user'), - ).toBeUndefined(); + ).toBeFalse(); }); it('not approved by anyone', () => { @@ -440,12 +486,29 @@ describe('modules/platform/gerrit/client', () => { client.wasApprovedBy( partial({ labels: { - 'Code-Review': {}, + 'Code-Review': { + all: [], + }, + }, + }), + 'user', + ), + ).toBeFalse(); + }); + + it('not approved but with +1', () => { + expect( + client.wasApprovedBy( + partial({ + labels: { + 'Code-Review': { + all: [{ value: 1, username: 'user' }], + }, }, }), 'user', ), - ).toBeUndefined(); + ).toBeFalse(); }); it('approved by given user', () => { @@ -454,10 +517,7 @@ describe('modules/platform/gerrit/client', () => { partial({ labels: { 'Code-Review': { - approved: { - _account_id: 1, - username: 'user', - }, + all: [{ value: 2, username: 'user' }], }, }, }), @@ -472,10 +532,7 @@ describe('modules/platform/gerrit/client', () => { partial({ labels: { 'Code-Review': { - approved: { - _account_id: 1, - username: 'other', - }, + all: [{ value: 2, username: 'other' }], }, }, }), diff --git a/lib/modules/platform/gerrit/client.ts b/lib/modules/platform/gerrit/client.ts index 89b4ebf3e66e3ce..efe7081ffc9f740 100644 --- a/lib/modules/platform/gerrit/client.ts +++ b/lib/modules/platform/gerrit/client.ts @@ -58,12 +58,14 @@ class GerritClient { repository: string, findPRConfig: GerritFindPRConfig, refreshCache?: boolean, + extraOptions?: string[], ): Promise { const filters = GerritClient.buildSearchFilters(repository, findPRConfig); + const options = [...this.requestDetails, ...(extraOptions ?? [])]; const changes = await this.gerritHttp.getJson( `a/changes/?q=` + filters.join('+') + - this.requestDetails.map((det) => `&o=${det}`).join(''), + options.map((det) => `&o=${det}`).join(''), { memCache: !refreshCache }, ); logger.trace( @@ -72,10 +74,13 @@ class GerritClient { return changes.body; } - async getChange(changeNumber: number): Promise { + async getChange( + changeNumber: number, + extraOptions?: string[], + ): Promise { + const options = [...this.requestDetails, ...(extraOptions ?? [])]; const changes = await this.gerritHttp.getJson( - `a/changes/${changeNumber}?` + - this.requestDetails.map((det) => `o=${det}`).join('&'), + `a/changes/${changeNumber}?` + options.map((det) => `o=${det}`).join('&'), ); return changes.body; } @@ -189,15 +194,20 @@ class GerritClient { } async checkIfApproved(changeId: number): Promise { - const change = await client.getChange(changeId); - const reviewLabels = change?.labels?.['Code-Review']; - return reviewLabels === undefined || reviewLabels.approved !== undefined; + const change = await client.getChange(changeId, ['DETAILED_LABELS']); + const reviewLabel = change?.labels?.['Code-Review']; + return ( + reviewLabel === undefined || + Boolean(reviewLabel.all?.some((label) => label.value === 2)) + ); } - wasApprovedBy(change: GerritChange, username: string): boolean | undefined { - return ( - change.labels?.['Code-Review'].approved && - change.labels['Code-Review'].approved.username === username + wasApprovedBy(change: GerritChange, username: string): boolean { + const reviewLabel = change?.labels?.['Code-Review']; + return Boolean( + reviewLabel?.all?.some( + (label) => label.value === 2 && label.username === username, + ), ); } diff --git a/lib/modules/platform/gerrit/scm.ts b/lib/modules/platform/gerrit/scm.ts index 6289f1f1f240472..646e975f1d93a8e 100644 --- a/lib/modules/platform/gerrit/scm.ts +++ b/lib/modules/platform/gerrit/scm.ts @@ -101,7 +101,7 @@ export class GerritScm extends DefaultGitScm { targetBranch: commit.baseBranch, }; const existingChange = await client - .findChanges(repository, searchConfig, true) + .findChanges(repository, searchConfig, true, ['DETAILED_LABELS']) .then((res) => res.pop()); let hasChanges = true; diff --git a/lib/modules/platform/gerrit/types.ts b/lib/modules/platform/gerrit/types.ts index 0d1b5d90fed354c..7f8603d275f11b8 100644 --- a/lib/modules/platform/gerrit/types.ts +++ b/lib/modules/platform/gerrit/types.ts @@ -77,6 +77,8 @@ export interface GerritChangeMessageInfo { export interface GerritLabelInfo { approved?: GerritAccountInfo; rejected?: GerritAccountInfo; + /** List of votes. Only set when o=DETAILED_LABELS. */ + all?: GerritApprovalInfo[]; } export interface GerritActionInfo { @@ -99,3 +101,8 @@ export interface GerritMergeableInfo { | 'CHERRY_PICK'; mergeable: boolean; } + +export interface GerritApprovalInfo { + value?: number; + username?: string; +}