feat: send status when unmanage pr

1024pix · Jan 8, 2025 · 4939191 · 4939191
1 parent f154d2c
commit 4939191
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 22 deletions.
diff --git a/build/controllers/github.js b/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,

diff --git a/build/controllers/merge.js b/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);
   },
 };

diff --git a/test/acceptance/build/github_test.js b/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',

diff --git a/test/acceptance/build/merge_test.js b/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',

diff --git a/test/unit/build/controllers/github_test.js b/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,
             });
           });
         });

diff --git a/test/unit/build/controllers/merge_test.js b/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;

diff --git a/test/unit/run/services/merge-queue_test.js b/test/unit/run/services/merge-queue_test.js
@@ -150,12 +150,19 @@ describe('Unit | Build | Services | merge-queue', function () {
       expect(githubService.setMergeQueueStatus).to.have.been.calledOnce;
     });
 
-
     const testCases = [
-      { status: MERGE_STATUS.MERGED, checkDescription: MERGE_STATUS_DETAILS.MERGED.description, checkStatus: 'success' },
+      {
+        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' },
-    ]
+      {
+        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 () {