fix(review): return 200 for unmigrated sites

chore(review): remove check on POST route

tests(review): fixed tests

tests(integration): fixed review tests

chore(review): update function name

src/integration/Reviews.spec.ts

@@ -113,13 +113,17 @@ const ReviewsRouter = new _ReviewsRouter(
   usersService,
   sitesService,
   collaboratorsService,
-  notificationsService
+  notificationsService,
+  gitHubService
 )
 const reviewsSubrouter = ReviewsRouter.getRouter()
 const subrouter = express()
 subrouter.use("/:siteName", reviewsSubrouter)
 
 const mockGenericAxios = mockAxios.create()
+const migrateSpy = jest
+  .spyOn(ReviewsRouter, "doesUnmigratedSiteExist")
+  .mockResolvedValue(true)
 
 describe("Review Requests Integration Tests", () => {
   beforeAll(async () => {
@@ -477,6 +481,7 @@ describe("Review Requests Integration Tests", () => {
         MOCK_USER_SESSION_DATA_ONE,
         MOCK_REPO_NAME_TWO
       )
+      migrateSpy.mockResolvedValueOnce(false)
 
       // Act
       const actual = await request(app).get(`/${MOCK_REPO_NAME_TWO}/summary`)
@@ -722,6 +727,7 @@ describe("Review Requests Integration Tests", () => {
         MOCK_USER_SESSION_DATA_ONE,
         MOCK_REPO_NAME_TWO
       )
+      migrateSpy.mockResolvedValueOnce(false)
 
       // Act
       const actual = await request(app).get(
@@ -1617,6 +1623,7 @@ describe("Review Requests Integration Tests", () => {
         MOCK_USER_SESSION_DATA_TWO,
         MOCK_REPO_NAME_TWO
       )
+      migrateSpy.mockResolvedValueOnce(false)
 
       // Act
       const actual = await request(app).get(

src/routes/v2/authenticated/__tests__/review.spec.ts

@@ -11,13 +11,17 @@ import { generateRouterForDefaultUserWithSite } from "@fixtures/app"
 import { mockUserId } from "@fixtures/identity"
 import { MOCK_USER_EMAIL_ONE, MOCK_USER_EMAIL_TWO } from "@fixtures/users"
 import { CollaboratorRoles, ReviewRequestStatus } from "@root/constants"
+import { GitHubService } from "@root/services/db/GitHubService"
 import CollaboratorsService from "@services/identity/CollaboratorsService"
 import NotificationsService from "@services/identity/NotificationsService"
 import SitesService from "@services/identity/SitesService"
 import UsersService from "@services/identity/UsersService"
 import ReviewRequestService from "@services/review/ReviewRequestService"
 
 describe("Review Requests Router", () => {
+  const mockGithubService = {
+    getRepoInfo: jest.fn().mockResolvedValue(true),
+  }
   const mockReviewRequestService = {
     approveReviewRequest: jest.fn(),
     closeReviewRequest: jest.fn(),
@@ -60,7 +64,8 @@ describe("Review Requests Router", () => {
     (mockIdentityUsersService as unknown) as UsersService,
     (mockSitesService as unknown) as SitesService,
     (mockCollaboratorsService as unknown) as CollaboratorsService,
-    (mockNotificationsService as unknown) as NotificationsService
+    (mockNotificationsService as unknown) as NotificationsService,
+    (mockGithubService as unknown) as GitHubService
   )
 
   const subrouter = express()
@@ -138,6 +143,7 @@ describe("Review Requests Router", () => {
       mockReviewRequestService.compareDiff.mockResolvedValueOnce(
         mockFilesChanged
       )
+      mockSitesService.getBySiteName.mockResolvedValueOnce(true)
 
       // Act
       const response = await request(app).get("/mockSite/review/compare")
@@ -152,6 +158,7 @@ describe("Review Requests Router", () => {
     it("should return 404 if user is not a site member", async () => {
       // Arrange
       mockIdentityUsersService.getSiteMember.mockResolvedValueOnce(null)
+      mockSitesService.getBySiteName.mockResolvedValueOnce(true)
 
       // Act
       const response = await request(app).get("/mockSite/review/compare")
@@ -335,6 +342,7 @@ describe("Review Requests Router", () => {
     it("should return 404 if the site does not exist", async () => {
       // Arrange
       mockSitesService.getBySiteName.mockResolvedValueOnce(null)
+      mockGithubService.getRepoInfo.mockRejectedValueOnce(false)
 
       // Act
       const response = await request(app).get("/mockSite/review/summary")
@@ -519,6 +527,7 @@ describe("Review Requests Router", () => {
     it("should return 404 if the site does not exist", async () => {
       // Arrange
       mockSitesService.getBySiteName.mockResolvedValueOnce(null)
+      mockGithubService.getRepoInfo.mockRejectedValueOnce(false)
 
       // Act
       const response = await request(app).get(`/mockSite/review/12345`)
@@ -913,6 +922,7 @@ describe("Review Requests Router", () => {
     it("should return 404 if the site does not exist", async () => {
       // Arrange
       mockSitesService.getBySiteName.mockResolvedValueOnce(null)
+      mockGithubService.getRepoInfo.mockRejectedValueOnce(false)
 
       // Act
       const response = await request(app).get(`/mockSite/review/12345/comments`)

src/routes/v2/authenticated/review.ts

@@ -14,6 +14,7 @@ import UserWithSiteSessionData from "@classes/UserWithSiteSessionData"
 
 import { CollaboratorRoles, ReviewRequestStatus } from "@root/constants"
 import { SiteMember, User } from "@root/database/models"
+import { GitHubService } from "@root/services/db/GitHubService"
 import CollaboratorsService from "@root/services/identity/CollaboratorsService"
 import NotificationsService from "@root/services/identity/NotificationsService"
 import SitesService from "@root/services/identity/SitesService"
@@ -41,25 +42,50 @@ export class ReviewsRouter {
 
   private readonly notificationsService
 
+  private readonly githubService
+
   constructor(
     reviewRequestService: ReviewRequestService,
     identityUsersService: UsersService,
     sitesService: SitesService,
     collaboratorsService: CollaboratorsService,
-    notificationsService: NotificationsService
+    notificationsService: NotificationsService,
+    githubService: GitHubService
   ) {
     this.reviewRequestService = reviewRequestService
     this.identityUsersService = identityUsersService
     this.sitesService = sitesService
     this.collaboratorsService = collaboratorsService
     this.notificationsService = notificationsService
+    this.githubService = githubService
 
     autoBind(this)
   }
 
+  // NOTE: This is required because we want to distinguish
+  // between unmigrated sites that don't exist in our db
+  // vs sites that truly don't exist.
+  // This is to avoid flooding the frontend with 404s
+  // and triggering false alarms
+  // TODO (IS-58): Remove this function and related calls when
+  // all sites are migrated over to our db
+  checkIfSiteIsUnmigrated = async (
+    sessionData: UserWithSiteSessionData
+  ): Promise<boolean> => {
+    try {
+      // Site exists on github but isn't migrated
+      await this.githubService.getRepoInfo(sessionData)
+      return true
+    } catch {
+      // If there are any errors, we failed to fetch the site
+      // and the site is assumed to not exist.
+      return false
+    }
+  }
+
   compareDiff: RequestHandler<
     { siteName: string },
-    { items: EditedItemDto[] },
+    { items: EditedItemDto[] } | ResponseErrorBody,
     unknown,
     unknown,
     { userWithSiteSessionData: UserWithSiteSessionData }
@@ -70,6 +96,14 @@ export class ReviewsRouter {
     const { userWithSiteSessionData } = res.locals
     const { siteName } = req.params
 
+    const site = await this.sitesService.getBySiteName(siteName)
+    const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated(
+      userWithSiteSessionData
+    )
+
+    if (!site && doesUnmigratedSiteExist)
+      return res.status(200).json({ message: "Unmigrated site" })
+
     // Check if they have access to site
     const possibleSiteMember = await this.identityUsersService.getSiteMember(
       userWithSiteSessionData.isomerUserId,
@@ -226,6 +260,12 @@ export class ReviewsRouter {
     const { siteName } = req.params
     const site = await this.sitesService.getBySiteName(siteName)
     const { userWithSiteSessionData } = res.locals
+    const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated(
+      userWithSiteSessionData
+    )
+
+    if (!site && doesUnmigratedSiteExist)
+      return res.status(200).json({ message: "Unmigrated site" })
 
     if (!site) {
       logger.error({
@@ -395,6 +435,12 @@ export class ReviewsRouter {
     const { siteName, requestId } = req.params
     const { userWithSiteSessionData } = res.locals
     const site = await this.sitesService.getBySiteName(siteName)
+    const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated(
+      userWithSiteSessionData
+    )
+
+    if (!site && doesUnmigratedSiteExist)
+      return res.status(200).json({ message: "Unmigrated site" })
 
     if (!site) {
       logger.error({
@@ -760,6 +806,13 @@ export class ReviewsRouter {
     const { userWithSiteSessionData } = res.locals
     // Step 1: Check that the site exists
     const site = await this.sitesService.getBySiteName(siteName)
+    const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated(
+      userWithSiteSessionData
+    )
+
+    if (!site && doesUnmigratedSiteExist)
+      return res.status(200).json({ message: "Unmigrated site" })
+
     if (!site) {
       logger.error({
         message: "Invalid site requested",
@@ -932,11 +985,10 @@ export class ReviewsRouter {
       })
     }
 
-    // Step 2: Check that user exists.
+    // Step 2. Check that user exists.
     // Having session data is proof that this user exists
     // as otherwise, they would be rejected by our middleware
     const { userWithSiteSessionData } = res.locals
-
     // Check if they are a collaborator
     const role = await this.collaboratorsService.getRole(
       siteName,
@@ -1155,6 +1207,12 @@ export class ReviewsRouter {
     const { path } = req.query
     const { userWithSiteSessionData } = res.locals
     const site = await this.sitesService.getBySiteName(siteName)
+    const doesUnmigratedSiteExist = await this.checkIfSiteIsUnmigrated(
+      userWithSiteSessionData
+    )
+
+    if (!site && doesUnmigratedSiteExist)
+      return res.status(200).json({ message: "Unmigrated site" })
 
     if (!site) {
       logger.error({

src/server.js

@@ -198,7 +198,8 @@ const reviewRouter = new ReviewsRouter(
   usersService,
   sitesService,
   collaboratorsService,
-  notificationsService
+  notificationsService,
+  gitHubService
 )
 const authenticatedSubrouterV1 = getAuthenticatedSubrouterV1({
   authenticationMiddleware,