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

lib/modules/platform/azure/__snapshots__/index.spec.ts.snap

@@ -217,6 +217,10 @@ exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOpti
 ]
 `;
 
+exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOptions) should not re-approve if the PR is already approved 1`] = `undefined`;
+
+exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOptions) should re-approve if the PR was approved by another user 1`] = `undefined`;
+
 exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOptions) should re-approve the PR 1`] = `undefined`;
 
 exports[`modules/platform/azure/index updatePr(prNo, title, body, platformPrOptions) should reopen the PR 1`] = `

lib/modules/platform/azure/index.spec.ts

@@ -1149,7 +1149,7 @@ describe('modules/platform/azure/index', () => {
       const updateFn = jest
         .fn(() => prUpdateResult)
         .mockName('createPullRequestReviewer');
-      azureApi.gitApi.mockImplementationOnce(
+      azureApi.gitApi.mockImplementation(
         () =>
           ({
             createPullRequest: jest.fn(() => prResult),
@@ -1293,14 +1293,90 @@ describe('modules/platform/azure/index', () => {
         isRequired: false,
       };
       const updateFn = jest.fn(() => prUpdateResult);
-      azureApi.gitApi.mockImplementationOnce(
+      azureApi.gitApi.mockImplementation(
+        () =>
+          ({
+            updatePullRequest: jest.fn(() => prResult),
+            createPullRequestReviewer: updateFn,
+            getPullRequestById: jest.fn(() => ({
+              pullRequestId: prResult.pullRequestId,
+              createdBy: prResult.createdBy,
+            })),
+          }) as any,
+      );
+      const pr = await azure.updatePr({
+        number: prResult.pullRequestId,
+        prTitle: 'The Title',
+        prBody: 'Hello world',
+        platformPrOptions: { autoApprove: true },
+      });
+      expect(updateFn).toHaveBeenCalled();
+      expect(pr).toMatchSnapshot();
+    });
+
+    it('should not re-approve if the PR is already approved', async () => {
+      await initRepo({ repository: 'some/repo' });
+      const prResult = {
+        pullRequestId: 456,
+        createdBy: {
+          id: 123,
+          url: 'user-url',
+        },
+      };
+      const prUpdateResult = {
+        reviewerUrl: prResult.createdBy.url,
+        vote: AzurePrVote.Approved,
+        isFlagged: false,
+        isRequired: false,
+      };
+      const updateFn = jest.fn(() => prUpdateResult);
+      azureApi.gitApi.mockImplementation(
+        () =>
+          ({
+            updatePullRequest: jest.fn(() => prResult),
+            createPullRequestReviewer: updateFn,
+            getPullRequestById: jest.fn(() => ({
+              pullRequestId: prResult.pullRequestId,
+              createdBy: prResult.createdBy,
+              reviewers: [{ vote: AzurePrVote.Approved, id: 123 }],
+            })),
+          }) as any,
+      );
+      const pr = await azure.updatePr({
+        number: prResult.pullRequestId,
+        prTitle: 'The Title',
+        prBody: 'Hello world',
+        platformPrOptions: { autoApprove: true },
+      });
+      expect(updateFn).not.toHaveBeenCalled();
+      expect(pr).toMatchSnapshot();
+    });
+
+    it('should re-approve if the PR was approved by another user', async () => {
+      await initRepo({ repository: 'some/repo' });
+      const prResult = {
+        pullRequestId: 456,
+        createdBy: {
+          id: 123,
+          url: 'user-url',
+        },
+      };
+      const prUpdateResult = {
+        reviewerUrl: prResult.createdBy.url,
+        vote: AzurePrVote.Approved,
+        isFlagged: false,
+        isRequired: false,
+      };
+      const updateFn = jest.fn(() => prUpdateResult);
+      azureApi.gitApi.mockImplementation(
         () =>
           ({
             updatePullRequest: jest.fn(() => prResult),
             createPullRequestReviewer: updateFn,
             getPullRequestById: jest.fn(() => ({
               pullRequestId: prResult.pullRequestId,
               createdBy: prResult.createdBy,
+              reviewers: [{ vote: AzurePrVote.Approved, id: 321 }],
             })),
           }) as any,
       );

lib/modules/platform/azure/index.ts

@@ -513,18 +513,7 @@ export async function createPr({
     );
   }
   if (platformPrOptions?.autoApprove) {
-    await azureApiGit.createPullRequestReviewer(
-      {
-        reviewerUrl: pr.createdBy!.url,
-        vote: AzurePrVote.Approved,
-        isFlagged: false,
-        isRequired: false,
-      },
-      config.repoId,
-      // TODO #22198
-      pr.pullRequestId!,
-      pr.createdBy!.id!,
-    );
+    await approvePr(pr);
   }
   await Promise.all(
     labels!.map((label) =>
@@ -581,19 +570,7 @@ export async function updatePr({
     objToUpdate.status = PullRequestStatus.Abandoned;
   }
   if (platformPrOptions?.autoApprove) {
-    const pr = await azureApiGit.getPullRequestById(prNo, config.project);
-    await azureApiGit.createPullRequestReviewer(
-      {
-        reviewerUrl: pr.createdBy!.url,
-        vote: AzurePrVote.Approved,
-        isFlagged: false,
-        isRequired: false,
-      },
-      config.repoId,
-      // TODO #22198
-      pr.pullRequestId!,
-      pr.createdBy!.id!,
-    );
+    await approvePr(prNo);
   }
 
   const updatedPr = await azureApiGit.updatePullRequest(
@@ -1015,3 +992,37 @@ export async function deleteLabel(
   const azureApiGit = await azureApi.gitApi();
   await azureApiGit.deletePullRequestLabels(config.repoId, prNumber, label);
 }
+
+export async function approvePr(
+  prNumberOrPr: number | GitPullRequest,
+): Promise<void> {
+  const azureApiGit = await azureApi.gitApi();
+  const pr =
+    typeof prNumberOrPr === 'number'
+      ? await azureApiGit.getPullRequestById(prNumberOrPr, config.project)
+      : prNumberOrPr;
+
+  const isApproved = pr.reviewers?.some(
+    (reviewer) =>
+      reviewer.vote === AzurePrVote.Approved &&
+      reviewer.id === pr.createdBy?.id,
+  );
+
+  if (isApproved) {
+    logger.debug('PR is already approved');
+    return;
+  }
+
+  await azureApiGit.createPullRequestReviewer(
+    {
+      reviewerUrl: pr.createdBy!.url,
+      vote: AzurePrVote.Approved,
+      isFlagged: false,
+      isRequired: false,
+    },
+    config.repoId,
+    // TODO #22198
+    pr.pullRequestId!,
+    pr.createdBy!.id!,
+  );
+}

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

@@ -4,11 +4,12 @@ import { REPOSITORY_ARCHIVED } from '../../../constants/error-messages';
 import { setBaseUrl } from '../../../util/http/gerrit';
 import type { FindPRConfig } from '../types';
 import { client } from './client';
-import type {
-  GerritChange,
-  GerritChangeMessageInfo,
-  GerritFindPRConfig,
-  GerritMergeableInfo,
+import {
+  type GerritAccountInfo,
+  type GerritChange,
+  type GerritChangeMessageInfo,
+  type GerritFindPRConfig,
+  type GerritMergeableInfo,
 } from './types';
 
 const gerritEndpointUrl = 'https://dev.gerrit.com/renovate/';
@@ -399,10 +400,22 @@ describe('modules/platform/gerrit/client', () => {
 
   describe('approveChange()', () => {
     it('already approved - do nothing', async () => {
-      const change = partial<GerritChange>({});
+      const owner = partial<GerritAccountInfo>({ username: 'user' });
+      const change = partial<GerritChange>({
+        labels: {
+          'Code-Review': {
+            all: [{ value: 2, username: owner.username }],
+          },
+        },
+        owner,
+      });
       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 +424,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 +456,61 @@ 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('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();
     });
 
-    it('approved by given other', () => {
-      expect(
-        client.wasApprovedBy(
-          partial<GerritChange>({
-            labels: {
-              'Code-Review': {
-                approved: {
-                  _account_id: 1,
-                  username: 'other',
-                },
-              },
-            },
-          }),
-          'user',
-        ),
-      ).toBeFalse();
+    it('already approved by another user - approve again', async () => {
+      const owner = partial<GerritAccountInfo>({ username: 'user' });
+      const change = partial<GerritChange>({
+        labels: {
+          'Code-Review': {
+            all: [{ value: 2, username: 'other user' }],
+          },
+        },
+        owner,
+      });
+      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();
     });
   });
 });