fix(gerrit): not auto-merging if Code-Review changed from +2 to +1

lib/modules/platform/gerrit/client.spec.ts

@@ -399,10 +399,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();
     });
@@ -411,17 +421,27 @@ 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='))
+        .get(
+          (url) =>
+            url.includes('/a/changes/123456?o=') &&
+            url.includes('o=DETAILED_LABELS'),
+        )
         .reply(200, gerritRestResponse(change), jsonResultHeader);
       const approveMock = httpMock
         .scope(gerritEndpointUrl)
@@ -433,62 +453,32 @@ describe('modules/platform/gerrit/client', () => {
       await expect(client.approveChange(123456)).toResolve();
       expect(approveMock.isDone()).toBeTrue();
     });
-  });
-
-  describe('wasApprovedBy()', () => {
-    it('label not exists', () => {
-      expect(
-        client.wasApprovedBy(partial<GerritChange>({}), 'user'),
-      ).toBeUndefined();
-    });
-
-    it('not approved by anyone', () => {
-      expect(
-        client.wasApprovedBy(
-          partial<GerritChange>({
-            labels: {
-              'Code-Review': {},
-            },
-          }),
-          'user',
-        ),
-      ).toBeUndefined();
-    });
 
-    it('approved by given user', () => {
-      expect(
-        client.wasApprovedBy(
-          partial<GerritChange>({
-            labels: {
-              'Code-Review': {
-                approved: {
-                  _account_id: 1,
-                  username: 'user',
-                },
-              },
-            },
-          }),
-          'user',
-        ),
-      ).toBeTrue();
-    });
-
-    it('approved by given other', () => {
-      expect(
-        client.wasApprovedBy(
-          partial<GerritChange>({
-            labels: {
-              'Code-Review': {
-                approved: {
-                  _account_id: 1,
-                  username: 'other',
-                },
-              },
-            },
-          }),
-          'user',
-        ),
-      ).toBeFalse();
+    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=') &&
+            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 },
+          notify: 'NONE',
+        })
+        .reply(200, gerritRestResponse(''), jsonResultHeader);
+      await expect(client.approveChange(123456)).toResolve();
+      expect(approveMock.isDone()).toBeTrue();
     });
   });
 });

lib/modules/platform/gerrit/client.ts

@@ -72,10 +72,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;
   }
@@ -196,15 +199,11 @@ 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;
-  }
-
-  wasApprovedBy(change: GerritChange, username: string): boolean | undefined {
+    const change = await client.getChange(changeId, ['DETAILED_LABELS']);
+    const reviewLabel = change?.labels?.['Code-Review'];
     return (
-      change.labels?.['Code-Review'].approved &&
-      change.labels['Code-Review'].approved.username === username
+      reviewLabel === undefined ||
+      Boolean(reviewLabel.all?.some((label) => label.value === 2))
     );
   }
 

lib/modules/platform/gerrit/index.ts

@@ -434,3 +434,7 @@
 export function getIssueList(): Promise<Issue[]> {
   return Promise.resolve([]);
 }
+
+export async function approvePr(number: number): Promise<void> {
+  await client.approveChange(number);
+}

lib/modules/platform/gerrit/scm.spec.ts

@@ -369,7 +369,6 @@ describe('modules/platform/gerrit/scm', () => {
         },
       });
       clientMock.findChanges.mockResolvedValueOnce([existingChange]);
-      clientMock.wasApprovedBy.mockReturnValueOnce(true);
       git.prepareCommit.mockResolvedValueOnce({
         commitSha: 'commitSha' as LongCommitSha,
         parentCommitSha: 'parentSha' as LongCommitSha,
@@ -385,6 +384,7 @@ describe('modules/platform/gerrit/scm', () => {
           message: 'commit msg',
           files: [],
           prTitle: 'pr title',
+          autoApprove: true,
         }),
       ).toBe('commitSha');
       expect(git.prepareCommit).toHaveBeenCalledWith({
@@ -396,6 +396,7 @@ describe('modules/platform/gerrit/scm', () => {
           'Renovate-Branch: renovate/dependency-1.x\nChange-Id: ...',
         ],
         prTitle: 'pr title',
+        autoApprove: true,
         force: true,
       });
       expect(git.fetchRevSpec).toHaveBeenCalledWith('refs/changes/1/2');
@@ -404,10 +405,6 @@ describe('modules/platform/gerrit/scm', () => {
         sourceRef: 'renovate/dependency-1.x',
         targetRef: 'refs/for/main%notify=NONE',
       });
-      expect(clientMock.wasApprovedBy).toHaveBeenCalledWith(
-        existingChange,
-        'user',
-      );
       expect(clientMock.approveChange).toHaveBeenCalledWith(123456);
     });
   });

lib/modules/platform/gerrit/scm.ts

@@ -135,11 +135,8 @@ export class GerritScm extends DefaultGitScm {
           files: commit.files,
         });
         if (pushResult) {
-          //existingChange was the old change before commit/push. we need to approve again, if it was previously approved from renovate
-          if (
-            existingChange &&
-            client.wasApprovedBy(existingChange, username)
-          ) {
+          // New patchsets dismisses the approval, so we need to approve it again
+          if (existingChange && commit.autoApprove) {
             await client.approveChange(existingChange._number);
           }
           return commitSha;

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

lib/modules/platform/types.ts

@@ -282,6 +282,7 @@ export interface Platform {
 
   maxBodyLength(): number;
   labelCharLimit?(): number;
+  approvePr?(number: number): Promise<void>;
 }
 
 export interface PlatformScm {

lib/util/git/types.ts

@@ -85,6 +85,8 @@ export interface CommitFilesConfig {
   platformCommit?: PlatformCommitOptions;
   /** Only needed by Gerrit platform */
   prTitle?: string;
+  /** Only needed by Gerrit platform */
+  autoApprove?: boolean;
 }
 
 export interface PushFilesConfig {

lib/workers/repository/update/branch/commit.ts

@@ -62,5 +62,7 @@ export function commitFilesToBranch(
     platformCommit: config.platformCommit,
     // Only needed by Gerrit platform
     prTitle: config.prTitle,
+    // Only needed by Gerrit platform
+    autoApprove: config.autoApprove,
   });
 }

lib/workers/repository/update/pr/automerge.ts

@@ -69,6 +69,12 @@
       prAutomergeBlockReason: 'PlatformNotReady',
     };
   }
+  // Usually the PR will already be approved, this is a last resort in case the
+  // approval was lost for some reason
+  if (config.autoApprove && platform.approvePr) {
+    logger.debug('Auto-approving PR if needed before automerge');
+    await platform.approvePr(pr.number);
+  }
   const branchStatus = await resolveBranchStatus(
     branchName,
     !!config.internalChecksAsSuccess,