fix(gerrit): not auto-approving if change already had a Code-Review +1

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<GerritChange>({});
+      const change = partial<GerritChange>({
+        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<GerritChange>({ 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<GerritChange>({ labels: { 'Code-Review': {} } });
+      const change = partial<GerritChange>({
+        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<GerritChange>({
+        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,20 +478,37 @@ describe('modules/platform/gerrit/client', () => {
     it('label not exists', () => {
       expect(
         client.wasApprovedBy(partial<GerritChange>({}), 'user'),
-      ).toBeUndefined();
+      ).toBeFalse();
     });
 
     it('not approved by anyone', () => {
       expect(
         client.wasApprovedBy(
           partial<GerritChange>({
             labels: {
-              'Code-Review': {},
+              'Code-Review': {
+                all: [],
+              },
+            },
+          }),
+          'user',
+        ),
+      ).toBeFalse();
+    });
+
+    it('not approved but with +1', () => {
+      expect(
+        client.wasApprovedBy(
+          partial<GerritChange>({
+            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<GerritChange>({
             labels: {
               'Code-Review': {
-                approved: {
-                  _account_id: 1,
-                  username: 'user',
-                },
+                all: [{ value: 2, username: 'user' }],
               },
             },
           }),
@@ -472,10 +532,7 @@ describe('modules/platform/gerrit/client', () => {
           partial<GerritChange>({
             labels: {
               'Code-Review': {
-                approved: {
-                  _account_id: 1,
-                  username: 'other',
-                },
+                all: [{ value: 2, username: 'other' }],
               },
             },
           }),

lib/modules/platform/gerrit/client.ts

@@ -58,12 +58,14 @@ class GerritClient {
     repository: string,
     findPRConfig: GerritFindPRConfig,
     refreshCache?: boolean,
+    extraOptions?: string[],
   ): Promise<GerritChange[]> {
     const filters = GerritClient.buildSearchFilters(repository, findPRConfig);
+    const options = [...this.requestDetails, ...(extraOptions ?? [])];
     const changes = await this.gerritHttp.getJson<GerritChange[]>(
       `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<GerritChange> {
+  async getChange(
+    changeNumber: number,
+    extraOptions?: string[],
+  ): Promise<GerritChange> {
+    const options = [...this.requestDetails, ...(extraOptions ?? [])];
     const changes = await this.gerritHttp.getJson<GerritChange>(
-      `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<boolean> {
-    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,
+      ),
     );
   }
 

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;

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;
+}