hotfix(0.18.1): merge into develop

CHANGELOG.md

@@ -4,8 +4,15 @@ All notable changes to this project will be documented in this file. Dates are d
 
 Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
+#### [v0.18.1](https://github.com/isomerpages/isomercms-backend/compare/v0.18.0...v0.18.1)
+
+- fix(review): return 200 for unmigrated sites [`bd69c29`](https://github.com/isomerpages/isomercms-backend/commit/bd69c29023554c5dcf8cb361227ba4ebf0d1ac08)
+- fix(sanitize): use same setup for dompurify as FE [`c25f448`](https://github.com/isomerpages/isomercms-backend/commit/c25f448813e1addce67d0209375ec35ef9cb6c7b)
+
 #### [v0.18.0](https://github.com/isomerpages/isomercms-backend/compare/v0.17.0...v0.18.0)
 
+> 30 March 2023
+
 - feat(identity): phase 2  [`#509`](https://github.com/isomerpages/isomercms-backend/pull/509)
 - chore(docker compose): remove local emulation of lambdas [`#666`](https://github.com/isomerpages/isomercms-backend/pull/666)
 - Chore(Site launch microservices):managing cloud environments [`#657`](https://github.com/isomerpages/isomercms-backend/pull/657)
@@ -141,15 +148,15 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 - Feat/add access token table [`#441`](https://github.com/isomerpages/isomercms-backend/pull/441)
 - Refactor/consolidate misc GitHub requests [`#440`](https://github.com/isomerpages/isomercms-backend/pull/440)
 - 0.5.0 [`#434`](https://github.com/isomerpages/isomercms-backend/pull/434)
-- Fix/migrate script [`#432`](https://github.com/isomerpages/isomercms-backend/pull/432)
-- Feat/move whitelist into database [`#422`](https://github.com/isomerpages/isomercms-backend/pull/422)
-- Refactor/use test fixture [`#430`](https://github.com/isomerpages/isomercms-backend/pull/430)
-- Chore: remove duplicate validation in User model [`#429`](https://github.com/isomerpages/isomercms-backend/pull/429)
 
 #### [v0.5.0](https://github.com/isomerpages/isomercms-backend/compare/v0.4.0...v0.5.0)
 
-> 14 April 2022
+> 21 April 2022
 
+- Fix/migrate script [`#432`](https://github.com/isomerpages/isomercms-backend/pull/432)
+- Feat/move whitelist into database [`#422`](https://github.com/isomerpages/isomercms-backend/pull/422)
+- Refactor/use test fixture [`#430`](https://github.com/isomerpages/isomercms-backend/pull/430)
+- Chore: remove duplicate validation in User model [`#429`](https://github.com/isomerpages/isomercms-backend/pull/429)
 - build(deps-dev): bump eslint-config-prettier from 8.1.0 to 8.5.0 [`#371`](https://github.com/isomerpages/isomercms-backend/pull/371)
 - build(deps): bump async from 3.2.0 to 3.2.3 [`#424`](https://github.com/isomerpages/isomercms-backend/pull/424)
 - chore(mergify): change to lower case [`#425`](https://github.com/isomerpages/isomercms-backend/pull/425)

package.json

@@ -1,6 +1,6 @@
 {
   "name": "isomercms",
-  "version": "0.18.0",
+  "version": "0.18.1",
   "private": true,
   "scripts": {
     "build": "tsc -p tsconfig.build.json",

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, "checkIfSiteIsUnmigrated")
+  .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,14 +96,29 @@ 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" })
+
+    const siteMembers = await this.collaboratorsService.list(siteName)
+    // NOTE: This is an initial migrated site but
+    // we haven't migrated the users.
+    if (siteMembers.length === 0 && site) {
+      return res.status(200).json({ message: "Unmigrated users" })
+    }
+
     // Check if they have access to site
     const possibleSiteMember = await this.identityUsersService.getSiteMember(
       userWithSiteSessionData.isomerUserId,
       siteName
     )
 
     if (!possibleSiteMember) {
-      return res.status(404).send()
+      return res.status(404).json({ message: "No site members found" })
     }
 
     const files = await this.reviewRequestService.compareDiff(
@@ -226,6 +267,19 @@ 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" })
+
+    const siteMembers = await this.collaboratorsService.list(siteName)
+    // NOTE: This is an initial migrated site but
+    // we haven't migrated the users.
+    if (siteMembers.length === 0 && site) {
+      return res.status(200).json({ message: "Unmigrated users" })
+    }
 
     if (!site) {
       logger.error({
@@ -395,6 +449,20 @@ 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" })
+    }
+
+    const siteMembers = await this.collaboratorsService.list(siteName)
+    // NOTE: This is an initial migrated site but
+    // we haven't migrated the users.
+    if (siteMembers.length === 0 && site) {
+      return res.status(200).json({ message: "Unmigrated users" })
+    }
 
     if (!site) {
       logger.error({
@@ -760,6 +828,21 @@ 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" })
+    }
+
+    const siteMembers = await this.collaboratorsService.list(siteName)
+    // NOTE: This is an initial migrated site but
+    // we haven't migrated the users.
+    if (siteMembers.length === 0 && site) {
+      return res.status(200).json({ message: "Unmigrated users" })
+    }
+
     if (!site) {
       logger.error({
         message: "Invalid site requested",
@@ -932,11 +1015,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 +1237,20 @@ 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" })
+    }
+
+    const siteMembers = await this.collaboratorsService.list(siteName)
+    // NOTE: This is an initial migrated site but
+    // we haven't migrated the users.
+    if (siteMembers.length === 0 && site) {
+      return res.status(200).json({ message: "Unmigrated users" })
+    }
 
     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,