[FEATURE] Mettre à jour le check de commit lorsqu'une PR n'est plus m…

…anagée par la merge queue

 #502

build/controllers/github.js

@@ -7,7 +7,7 @@ import { logger } from '../../common/services/logger.js';
 import ScalingoClient from '../../common/services/scalingo-client.js';
 import { config } from '../../config.js';
 import * as reviewAppRepo from '../repositories/review-app-repository.js';
-import { mergeQueue as _mergeQueue } from '../services/merge-queue.js';
+import { MERGE_STATUS, mergeQueue as _mergeQueue } from '../services/merge-queue.js';
 
 const repositoryToScalingoAppsReview = {
   'pix-api-data': ['pix-api-data-integration'],
@@ -260,7 +260,9 @@ async function processWebhook(
     }
     if (request.payload.action === 'closed') {
       const repositoryName = request.payload.repository.full_name;
-      await mergeQueue.unmanagePullRequest({ repositoryName, number: request.payload.number });
+      const isMerged = request.payload.pull_request.merged;
+      const status = isMerged ? MERGE_STATUS.MERGED : MERGE_STATUS.ABORTED;
+      await mergeQueue.unmanagePullRequest({ repositoryName, number: request.payload.number, status });
       return handleCloseRA(request);
     }
     if (request.payload.action === 'labeled' && request.payload.label.name == 'no-review-app') {
@@ -279,7 +281,11 @@ async function processWebhook(
     }
     if (request.payload.action === 'unlabeled' && request.payload.label.name === ':rocket: Ready to Merge') {
       const repositoryName = request.payload.repository.full_name;
-      await mergeQueue.unmanagePullRequest({ repositoryName, number: request.payload.number });
+      await mergeQueue.unmanagePullRequest({
+        repositoryName,
+        number: request.payload.number,
+        status: MERGE_STATUS.ABORTED,
+      });
     }
     return `Ignoring ${request.payload.action} action`;
   } else if (eventName === 'check_suite') {
@@ -292,7 +298,7 @@ async function processWebhook(
 
       const prNumber = request.payload.check_suite.pull_requests[0].number;
       if (request.payload.check_suite.conclusion !== 'success') {
-        await mergeQueue.unmanagePullRequest({ repositoryName, number: prNumber });
+        await mergeQueue.unmanagePullRequest({ repositoryName, number: prNumber, status: MERGE_STATUS.ABORTED });
       } else {
         const hasReadyToMergeLabel = await githubService.isPrLabelledWith({
           repositoryName,

build/controllers/merge.js

@@ -1,4 +1,4 @@
-import { mergeQueue } from '../services/merge-queue.js';
+import { MERGE_STATUS, mergeQueue } from '../services/merge-queue.js';
 import { config } from '../../config.js';
 import Boom from '@hapi/boom';
 
@@ -12,7 +12,11 @@ const mergeController = {
     const { pullRequest } = request.payload;
     const [organisation, repository, pullRequestNumber] = pullRequest.split('/');
     const repositoryName = `${organisation}/${repository}`;
-    await dependencies.mergeQueue.unmanagePullRequest({ repositoryName, number: Number(pullRequestNumber) });
+    await dependencies.mergeQueue.unmanagePullRequest({
+      repositoryName,
+      number: Number(pullRequestNumber),
+      status: MERGE_STATUS.ERROR,
+    });
     return h.response().code(204);
   },
 };

build/services/merge-queue.js

@@ -3,6 +3,18 @@ import { config } from '../../config.js';
 import * as pullRequestRepository from '../repositories/pull-request-repository.js';
 import githubService from '../../common/services/github.js';
 
+export const MERGE_STATUS = {
+  ABORTED: 'ABORTED',
+  ERROR: 'ERROR',
+  MERGED: 'MERGED',
+};
+
+export const MERGE_STATUS_DETAILS = {
+  ABORTED: { description: 'Merge plus géré', checkStatus: 'success' },
+  ERROR: { description: 'Error, merci de visiter la liste des essais', checkStatus: 'error' },
+  MERGED: { description: '', checkStatus: 'success' },
+};
+
 export class MergeQueue {
   #pullRequestRepository;
   #githubService;
@@ -64,7 +76,15 @@ export class MergeQueue {
     await this.manage({ repositoryName });
   }
 
-  async unmanagePullRequest({ repositoryName, number }) {
+  async unmanagePullRequest({ repositoryName, number, status }) {
+    const statusDetails = MERGE_STATUS_DETAILS[status];
+    await this.#githubService.setMergeQueueStatus({
+      status: statusDetails.checkStatus,
+      description: statusDetails.description,
+      repositoryFullName: repositoryName,
+      prNumber: number,
+    });
+
     await this.#pullRequestRepository.remove({
       repositoryName,
       number,

common/services/github.js

@@ -359,7 +359,7 @@ async function _getPullRequestsDetailsByCommitShas({ repoOwner, repoName, commit
   return pullRequestsForCommitShaFilteredDetails;
 }
 
-async function createCommitStatus({ context, repo, pull_number, description, state }) {
+async function createCommitStatus({ context, repo, pull_number, description, state, target_url }) {
   const owner = '1024pix';
   const octokit = _createOctokit();
 
@@ -373,6 +373,7 @@ async function createCommitStatus({ context, repo, pull_number, description, sta
     repo,
     sha,
     state,
+    target_url,
   });
 
   const startedAt = response.data.started_at;
@@ -389,6 +390,7 @@ async function setMergeQueueStatus({ repositoryFullName, prNumber, status, descr
     pull_number: prNumber,
     state: status,
     description,
+    target_url: 'https://github.com/1024pix/pix-actions/actions/workflows/auto-merge-dispatch.yml',
   });
 }
 

test/acceptance/build/github_test.js

@@ -66,6 +66,17 @@ describe('Acceptance | Build | Github', function () {
         .reply(returnCode);
     }
 
+    function getMergeCheckStatusNock({ repositoryFullName }) {
+      const prHeadCommit = 'sha1';
+      nock('https://api.github.com')
+        .get(`/repos/1024pix/pix/pulls/123`)
+        .reply(200, { head: { sha: prHeadCommit } });
+
+      return nock('https://api.github.com')
+        .post(`/repos/${repositoryFullName}/statuses/${prHeadCommit}`)
+        .reply(201, { started_at: '2024-01-01' });
+    }
+
     let body;
 
     ['opened', 'reopened'].forEach((action) => {
@@ -556,9 +567,12 @@ describe('Acceptance | Build | Github', function () {
                 fork: false,
               },
             },
+            merged: false,
           },
         };
 
+        getMergeCheckStatusNock({ repositoryFullName: body.repository.full_name });
+
         // when
         const response = await server.inject({
           method: 'POST',
@@ -682,14 +696,7 @@ describe('Acceptance | Build | Github', function () {
             })
             .reply(200, {});
 
-          const prHeadCommit = 'aaaa';
-          const getPullRequestNock = nock('https://api.github.com')
-            .get(`/repos/1024pix/pix/pulls/123`)
-            .reply(200, { head: { sha: prHeadCommit } });
-
-          const createCheckNock = nock('https://api.github.com')
-            .post(`/repos/${body.repository.full_name}/statuses/${prHeadCommit}`)
-            .reply(201, { started_at: '2024-01-01' });
+          const mergeCheckNock = getMergeCheckStatusNock({ repositoryFullName: body.repository.full_name });
 
           const response = await server.inject({
             method: 'POST',
@@ -703,8 +710,7 @@ describe('Acceptance | Build | Github', function () {
 
           expect(checkUserBelongsToPixNock.isDone()).to.be.true;
           expect(callGitHubAction.isDone()).to.be.true;
-          expect(getPullRequestNock.isDone()).to.be.true;
-          expect(createCheckNock.isDone()).to.be.true;
+          expect(mergeCheckNock.isDone()).to.be.true;
           expect(response.statusCode).equal(200);
         });
 
@@ -1059,6 +1065,7 @@ describe('Acceptance | Build | Github', function () {
               ],
             },
           };
+          getMergeCheckStatusNock({ repositoryFullName: body.repository.full_name });
 
           const response = await server.inject({
             method: 'POST',

test/acceptance/build/merge_test.js

@@ -1,5 +1,5 @@
 import server from '../../../server.js';
-import { expect } from '../../test-helper.js';
+import { expect, nock } from '../../test-helper.js';
 import { knex } from '../../../db/knex-database-connection.js';
 import { describe } from 'mocha';
 import { config } from '../../../config.js';
@@ -28,6 +28,14 @@ describe('Acceptance | Build | Merge', function () {
       it('responds with 200 and remove PR in database', async function () {
         await knex('pull_requests').insert({ repositoryName: '1024pix/pix-test', number: 1 });
         const body = { pullRequest: '1024pix/pix-test/1' };
+        const prHeadCommit = 'sha1';
+        nock('https://api.github.com')
+          .get('/repos/1024pix/pix-test/pulls/1')
+          .reply(200, { head: { sha: prHeadCommit } });
+
+        nock('https://api.github.com')
+          .post(`/repos/1024pix/pix-test/statuses/${prHeadCommit}`)
+          .reply(201, { started_at: '2024-01-01' });
 
         const res = await server.inject({
           method: 'POST',

test/unit/build/controllers/github_test.js

@@ -5,6 +5,7 @@ import url from 'node:url';
 import * as githubController from '../../../../build/controllers/github.js';
 import { config } from '../../../../config.js';
 import { catchErr, expect, sinon } from '../../../test-helper.js';
+import { MERGE_STATUS } from '../../../../build/services/merge-queue.js';
 
 describe('Unit | Controller | Github', function () {
   describe('#getMessageTemplate', function () {
@@ -262,19 +263,52 @@ Les variables d'environnement seront accessibles sur scalingo https://dashboard.
               expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({
                 number: 123,
                 repositoryName: repositoryName,
+                status: MERGE_STATUS.ABORTED,
               });
             });
           });
         });
 
-        describe('when action is `closed`', function () {
+        describe('when action is `closed` and pr is not merged', function () {
           it('should call pullRequestRepository.remove() method', async function () {
             // given
             const repositoryName = '1024pix/pix-sample-repo';
             sinon.stub(request, 'payload').value({
               action: 'closed',
               number: 123,
               repository: { full_name: repositoryName },
+              pull_request: {
+                merged: false,
+              },
+            });
+
+            const handleCloseRA = sinon.stub();
+            const mergeQueue = { unmanagePullRequest: sinon.stub() };
+
+            // when
+            await githubController.processWebhook(request, { handleCloseRA, mergeQueue });
+
+            // then
+            expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({
+              number: 123,
+              repositoryName,
+              status: MERGE_STATUS.ABORTED,
+            });
+            expect(handleCloseRA.calledOnceWithExactly(request)).to.be.true;
+          });
+        });
+
+        describe('when action is `closed` and pr is merged', function () {
+          it('should call pullRequestRepository.remove() method', async function () {
+            // given
+            const repositoryName = '1024pix/pix-sample-repo';
+            sinon.stub(request, 'payload').value({
+              action: 'closed',
+              number: 123,
+              repository: { full_name: repositoryName },
+              pull_request: {
+                merged: true,
+              },
             });
 
             const handleCloseRA = sinon.stub();
@@ -287,6 +321,7 @@ Les variables d'environnement seront accessibles sur scalingo https://dashboard.
             expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({
               number: 123,
               repositoryName,
+              status: MERGE_STATUS.MERGED,
             });
             expect(handleCloseRA.calledOnceWithExactly(request)).to.be.true;
           });
@@ -328,6 +363,7 @@ Les variables d'environnement seront accessibles sur scalingo https://dashboard.
             expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({
               number: 123,
               repositoryName,
+              status: MERGE_STATUS.ABORTED,
             });
           });
         });

test/unit/build/controllers/merge_test.js

@@ -1,6 +1,7 @@
 import { catchErr, expect, sinon } from '../../../test-helper.js';
 import mergeController from '../../../../build/controllers/merge.js';
 import { config } from '../../../../config.js';
+import { MERGE_STATUS } from '../../../../build/services/merge-queue.js';
 
 describe('Unit | Controller | Merge', function () {
   describe('#handle', function () {
@@ -39,6 +40,7 @@ describe('Unit | Controller | Merge', function () {
       expect(mergeQueue.unmanagePullRequest).to.be.calledOnceWithExactly({
         number: 123,
         repositoryName: '1024pix/pix',
+        status: MERGE_STATUS.ERROR,
       });
 
       expect(hStub.response).to.be.calledOnce;

test/unit/build/services/github_test.js

@@ -621,6 +621,7 @@ describe('Unit | Build | github-test', function () {
           context: 'merge-queue-status',
           state: 'pending',
           description: 'fuu',
+          target_url: 'https://github.com/1024pix/pix-actions/actions/workflows/auto-merge-dispatch.yml',
         };
 
         const createCheckNock = nock('https://api.github.com')

test/unit/run/services/merge-queue_test.js

@@ -1,5 +1,5 @@
 import { expect, sinon } from '../../../test-helper.js';
-import { MergeQueue } from '../../../../build/services/merge-queue.js';
+import { MERGE_STATUS, MERGE_STATUS_DETAILS, MergeQueue } from '../../../../build/services/merge-queue.js';
 import { config } from '../../../../config.js';
 
 describe('Unit | Build | Services | merge-queue', function () {
@@ -133,17 +133,65 @@ describe('Unit | Build | Services | merge-queue', function () {
       const pullRequestRepository = {
         remove: sinon.stub(),
       };
+      const githubService = {
+        setMergeQueueStatus: sinon.stub(),
+      };
 
-      const mergeQueue = new MergeQueue({ pullRequestRepository });
+      const mergeQueue = new MergeQueue({ pullRequestRepository, githubService });
       mergeQueue.manage = sinon.stub();
 
-      await mergeQueue.unmanagePullRequest({ repositoryName, number: pullRequestNumber });
+      await mergeQueue.unmanagePullRequest({ repositoryName, number: pullRequestNumber, status: MERGE_STATUS.ERROR });
 
       expect(pullRequestRepository.remove).to.have.been.calledOnceWithExactly({
         repositoryName,
         number: pullRequestNumber,
       });
       expect(mergeQueue.manage).to.have.been.calledOnceWithExactly({ repositoryName });
+      expect(githubService.setMergeQueueStatus).to.have.been.calledOnce;
+    });
+
+    const testCases = [
+      {
+        status: MERGE_STATUS.MERGED,
+        checkDescription: MERGE_STATUS_DETAILS.MERGED.description,
+        checkStatus: 'success',
+      },
+      { status: MERGE_STATUS.ERROR, checkDescription: MERGE_STATUS_DETAILS.ERROR.description, checkStatus: 'error' },
+      {
+        status: MERGE_STATUS.ABORTED,
+        checkDescription: MERGE_STATUS_DETAILS.ABORTED.description,
+        checkStatus: 'success',
+      },
+    ];
+
+    testCases.forEach((testCase) => {
+      it(`should update check status when pr status is '${testCase.status}'`, async function () {
+        const repositoryName = Symbol('repository-name');
+        const pullRequestNumber = Symbol('pull-request-number');
+
+        const pullRequestRepository = {
+          remove: sinon.stub(),
+        };
+        const githubService = {
+          setMergeQueueStatus: sinon.stub(),
+        };
+
+        const mergeQueue = new MergeQueue({ pullRequestRepository, githubService });
+        mergeQueue.manage = sinon.stub();
+
+        await mergeQueue.unmanagePullRequest({
+          repositoryName,
+          number: pullRequestNumber,
+          status: testCase.status,
+        });
+
+        expect(githubService.setMergeQueueStatus).to.have.been.calledOnceWithExactly({
+          status: testCase.checkStatus,
+          repositoryFullName: repositoryName,
+          prNumber: pullRequestNumber,
+          description: testCase.checkDescription,
+        });
+      });
     });
   });
 });