fix: comment logic (#1460)

* fix: comment lgoic

* fix: update logic for construcvtion of reviewMeta

* chore: add tests + minor cleanpu

* chore: fix broken test

Author: seaerchin

src/integration/NotificationOnEditHandler.spec.ts

@@ -298,7 +298,7 @@ describe("Notifications Router", () => {
       await ReviewMeta.create({
         reviewId: 1,
         pullRequestNumber: 1,
-        reviewLink: "test",
+        reviewLink: `/${mockSiteName}/review/1`,
       })
       mockGithubService.getComments.mockResolvedValueOnce([])
       mockGithubService.getCommitDiff.mockResolvedValueOnce({

src/integration/Reviews.spec.ts

@@ -39,13 +39,11 @@ import {
   MOCK_GITHUB_PULL_REQUEST_NUMBER,
   MOCK_GITHUB_RAWCOMMENT_ONE,
   MOCK_GITHUB_RAWCOMMENT_TWO,
-  MOCK_GITHUB_FRONTMATTER,
   MOCK_PAGE_PERMALINK,
 } from "@fixtures/github"
 import { MOCK_GITHUB_DATE_ONE } from "@fixtures/identity"
 import {
   MOCK_PULL_REQUEST_BODY_ONE,
-  MOCK_PULL_REQUEST_CHANGED_FILES_ONE,
   MOCK_PULL_REQUEST_ONE,
   MOCK_PULL_REQUEST_TITLE_ONE,
 } from "@fixtures/review"
@@ -1764,11 +1762,71 @@ describe("Review Requests Integration Tests", () => {
       await Reviewer.destroy({
         where: {},
       })
+      await ReviewComment.destroy({ where: {} })
       await ReviewRequest.destroy({
         where: {},
       })
     })
 
+    it("should not retrieve comments for review requests with the same number across different sites", async () => {
+      // Arrange
+      const FAKE_COMMENT = "This is a comment that should not appear for site 1"
+      const duplicatedReviewRequest = await ReviewRequest.create({
+        requestorId: MOCK_USER_ID_ONE,
+        siteId: MOCK_SITE_ID_TWO,
+      })
+      await ReviewMeta.create({
+        reviewId: duplicatedReviewRequest?.id,
+        pullRequestNumber: MOCK_GITHUB_PULL_REQUEST_NUMBER,
+        // NOTE: the only relevant difference here is that the `siteName` is different
+        // but importantly, the pr number is the same across both this and the one we created
+        // earlier in the `beforeAll` block
+        reviewLink: `cms.isomer.gov.sg/sites/${MOCK_REPO_NAME_TWO}/review/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`,
+      })
+
+      await ReviewComment.create({
+        reviewId: duplicatedReviewRequest.id,
+        reviewerId: MOCK_USER_ID_ONE,
+        comment: FAKE_COMMENT,
+        createdAt: new Date().getTime(),
+        updatedAt: new Date().getTime(),
+      })
+
+      const app = generateRouterForUserWithSite(
+        subrouter,
+        MOCK_USER_SESSION_DATA_TWO,
+        MOCK_REPO_NAME_ONE
+      )
+
+      mockGenericAxios.get.mockResolvedValueOnce({
+        data: [MOCK_GITHUB_RAWCOMMENT_ONE, MOCK_GITHUB_RAWCOMMENT_TWO],
+      })
+
+      const expected = [
+        {
+          user: MOCK_USER_EMAIL_ONE,
+          message: MOCK_GITHUB_COMMENT_BODY_ONE,
+          createdAt: new Date(MOCK_GITHUB_COMMIT_DATE_ONE).getTime(),
+          isRead: false,
+        },
+        {
+          user: MOCK_USER_EMAIL_TWO,
+          message: MOCK_GITHUB_COMMENT_BODY_TWO,
+          createdAt: new Date(MOCK_GITHUB_COMMIT_DATE_THREE).getTime(),
+          isRead: false,
+        },
+      ]
+
+      // Act
+      const actual = await request(app).get(
+        `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments`
+      )
+
+      // Assert
+      expect(actual.statusCode).toEqual(200)
+      expect(actual.body).toEqual(expected)
+    })
+
     it("should retrieve the comments for the review request", async () => {
       // Arrange
       const app = generateRouterForUserWithSite(

src/services/review/ReviewRequestService.ts

@@ -1,4 +1,4 @@
-import _, { sortBy, unionBy, zipObject } from "lodash"
+import _, { pull, sortBy, unionBy, zipObject } from "lodash"
 import { err, errAsync, ok, okAsync, Result, ResultAsync } from "neverthrow"
 import { Op, ModelStatic } from "sequelize"
 import { Sequelize } from "sequelize-typescript"
@@ -1083,28 +1083,47 @@ export default class ReviewRequestService {
     const { siteName, isomerUserId } = sessionData
 
     logger.info(
-      `Creating comment for PR ${pullRequestNumber}, site: ${siteName}`
+      `Creating comment for PR with pullRequestNumber: ${pullRequestNumber}, site: ${siteName}`
     )
-    // get id of review request
+    // NOTE: We need to do this to ensure that users are creating comments
+    // for a review request that exists.
+    // Without this check, users can ping our backend
+    // to create comments for any review request,
+    // even if the review request doesn't exist
     const reviewMeta = await this.reviewMeta.findOne({
-      where: { pullRequestNumber },
+      where: {
+        pullRequestNumber,
+        reviewLink: {
+          // NOTE: The `/${requestId}/` that the frontend
+          // sends across is not actually the `reviewId`.
+          // This is actually the GITHUB `pullRequestNumber`
+          // and hence, we have to artifically construct
+          // the `reviewLink`.
+          // We need the starting `/` to prevent cases where `siteName`
+          // is potentially a substring of another site (eg: `moe-ny` and `moe-moe-ny`)
+          [Op.endsWith]: `/${siteName}/review/${pullRequestNumber}`,
+        },
+      },
     })
 
     if (reviewMeta?.reviewId) {
       try {
         return await this.reviewCommentService.createCommentForReviewRequest(
-          reviewMeta?.reviewId,
+          reviewMeta.reviewId,
           isomerUserId,
           message
         )
       } catch (e) {
         logger.error(
-          `Error creating comment in DB for PR ${pullRequestNumber}, site: ${siteName}`
+          `Error creating comment in DB for PR ${reviewMeta.reviewId}, site: ${siteName}`
         )
         throw new DatabaseError("Error creating comment in DB")
       }
     }
-    logger.info(`No review request found for PR ${pullRequestNumber}`)
+
+    logger.info(
+      `No review request found for PR with reviewId: ${reviewMeta?.reviewId}, pullRequestNumber: ${pullRequestNumber}, site: ${siteName}`
+    )
     throw new RequestNotFoundError("Review Request not found")
   }
 
@@ -1115,17 +1134,33 @@ export default class ReviewRequestService {
   ): Promise<CommentItem[]> => {
     const { siteName, isomerUserId: userId } = sessionData
 
-    // get review request id
+    // NOTE: We need to do this to ensure that users are creating comments
+    // for a review request that exists.
+    // Without this check, users can ping our backend
+    // to create comments for any review request,
+    // even if the review request doesn't exist
     const reviewMeta = await this.reviewMeta.findOne({
-      where: { pullRequestNumber },
+      where: {
+        pullRequestNumber,
+        // NOTE: The `/${requestId}/` that the frontend
+        // sends across is not actually the `reviewId`.
+        // This is actually the GITHUB `pullRequestNumber`
+        // and hence, we have to artifically construct
+        // the `reviewLink`.
+        // We need the starting `/` to prevent cases where `siteName`
+        // is potentially a substring of another site (eg: `moe-ny` and `moe-moe-ny`)
+        reviewLink: {
+          [Op.endsWith]: `/${siteName}/review/${pullRequestNumber}`,
+        },
+      },
     })
     if (!reviewMeta || !reviewMeta.reviewId) {
       throw new RequestNotFoundError("Review Request not found")
     }
 
     const comments = await this.apiService.getComments(
       siteName,
-      pullRequestNumber
+      reviewMeta.pullRequestNumber
     )
 
     const requestsView = await this.reviewRequestView.findOne({