Merge pull request #758 from isomerpages/release/0.26.0

* fix: close pull request (#751) * Fix: publish button delay (#752) * Fix: publish button delay * Fix: tests * Fix: move review request status check into router * feat: return sites from db for email login (#754) * feat: return sites from db for email login * fix: tests * fix: use lodash function for type guard * fix(githhub service): get call to github to prevent race condition (#756) * fix(githhub service): get call to github to prevent race condition * fix(github service): throw error if directory name not defined * style(github service): add comments for clarity * fix: tests --------- Co-authored-by: Alexander Lee <[email protected]> * Fix: handle updating of files in root directory (#761) * fix: create .keep first when rename subfolder (#762) * 0.26.0 --------- Co-authored-by: seaerchin <[email protected]> Co-authored-by: Alexander Lee <[email protected]> Co-authored-by: Harish <[email protected]>
isomerpages · May 9, 2023 · f59390f · f59390f
2 parents 0276882 + ed53b71
commit f59390f
Show file tree

Hide file tree

Showing 15 changed files with 134 additions and 57 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,8 +4,20 @@ 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.26.0](https://github.com/isomerpages/isomercms-backend/compare/v0.25.0...v0.26.0)
+
+- fix: create .keep first when rename subfolder [`#762`](https://github.com/isomerpages/isomercms-backend/pull/762)
+- Fix: handle updating of files in root directory [`#761`](https://github.com/isomerpages/isomercms-backend/pull/761)
+- fix(githhub service): get call to github to prevent race condition [`#756`](https://github.com/isomerpages/isomercms-backend/pull/756)
+- feat: return sites from db for email login [`#754`](https://github.com/isomerpages/isomercms-backend/pull/754)
+- Fix: publish button delay [`#752`](https://github.com/isomerpages/isomercms-backend/pull/752)
+- fix: close pull request [`#751`](https://github.com/isomerpages/isomercms-backend/pull/751)
+- release(0.25.0): merge to develop [`#749`](https://github.com/isomerpages/isomercms-backend/pull/749)
+
 #### [v0.25.0](https://github.com/isomerpages/isomercms-backend/compare/v0.24.2...v0.25.0)
 
+> 4 May 2023
+
 - fix(markdown-utils): add check for falsy values [`#746`](https://github.com/isomerpages/isomercms-backend/pull/746)
 - chore: add logging to endpoints being called [`#744`](https://github.com/isomerpages/isomercms-backend/pull/744)
 - feat(datadog): add tracing for http requests out [`#745`](https://github.com/isomerpages/isomercms-backend/pull/745)
@@ -100,12 +112,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 - build(deps): bump vm2 from 3.9.12 to 3.9.15 in /microservices [`#688`](https://github.com/isomerpages/isomercms-backend/pull/688)
 - build(deps): bump vm2 from 3.9.11 to 3.9.15 [`#687`](https://github.com/isomerpages/isomercms-backend/pull/687)
 - release(0.19.0): merge to develop [`#684`](https://github.com/isomerpages/isomercms-backend/pull/684)
+- fix(utils): sanitize empty string + trim [`#686`](https://github.com/isomerpages/isomercms-backend/pull/686)
 
 #### [v0.19.0](https://github.com/isomerpages/isomercms-backend/compare/v0.18.2...v0.19.0)
 
 > 6 April 2023
 
-- fix(utils): sanitize empty string + trim [`#686`](https://github.com/isomerpages/isomercms-backend/pull/686)
 - fix(utils): change order of ops and rec sanitization [`#680`](https://github.com/isomerpages/isomercms-backend/pull/680)
 - chore(review): fix tests for review router [`#683`](https://github.com/isomerpages/isomercms-backend/pull/683)
 - Feat(site launch): support for multiple sites [`#665`](https://github.com/isomerpages/isomercms-backend/pull/665)
@@ -117,14 +129,15 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
 > 3 April 2023
 
+- 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)
 - Fix: change response for github users accessing collaborator endpoints [`db1130f`](https://github.com/isomerpages/isomercms-backend/commit/db1130f96a6c9cda99df43d5774134aefffeee3e)
 
 #### [v0.18.1](https://github.com/isomerpages/isomercms-backend/compare/v0.18.0...v0.18.1)
 
 > 31 March 2023
 
-- 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)
+- fix(review): return 200 for unmigrated sites [`073cab8`](https://github.com/isomerpages/isomercms-backend/commit/073cab8c6704178ee5061b8582b4f999720dfc95)
 
 #### [v0.18.0](https://github.com/isomerpages/isomercms-backend/compare/v0.17.0...v0.18.0)
 

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "isomercms",
-  "version": "0.25.0",
+  "version": "0.26.0",
   "private": true,
   "scripts": {
     "build": "tsc -p tsconfig.build.json",

diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts
@@ -1265,13 +1265,41 @@ describe("Review Requests Integration Tests", () => {
       })
     })
 
+    it("should return 404 if review request is not approved", async () => {
+      // Arrange
+      const app = generateRouterForUserWithSite(
+        subrouter,
+        MOCK_USER_SESSION_DATA_ONE,
+        MOCK_REPO_NAME_TWO
+      )
+
+      // Act
+      const actual = await request(app).post(
+        `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/merge`
+      )
+
+      // Assert
+      expect(actual.statusCode).toEqual(404)
+    })
+
     it("should merge the pull request successfully", async () => {
       // Arrange
       const app = generateRouterForUserWithSite(
         subrouter,
         MOCK_USER_SESSION_DATA_ONE,
         MOCK_REPO_NAME_ONE
       )
+      const activeReviewRequest = await ReviewRequest.findOne({
+        where: {
+          requestorId: MOCK_USER_ID_ONE,
+          siteId: MOCK_SITE_ID_ONE,
+        },
+      })
+      if (!activeReviewRequest) fail("Should not reach here")
+      await activeReviewRequest.set({
+        reviewStatus: ReviewRequestStatus.Approved,
+      })
+      await activeReviewRequest.save()
       mockGenericAxios.post.mockResolvedValueOnce(null)
       mockGenericAxios.put.mockResolvedValueOnce(null)
 

diff --git a/src/integration/Sites.spec.ts b/src/integration/Sites.spec.ts
@@ -220,31 +220,11 @@ describe("Sites Router", () => {
       const expected = {
         siteNames: [
           {
-            lastUpdated: mockUpdatedAt,
             repoName: mockSite,
-            isPrivate: mockPrivate,
-            permissions: mockPermissions,
           },
         ],
       }
 
-      mockGenericAxios.get.mockResolvedValueOnce({
-        data: [
-          {
-            pushed_at: mockUpdatedAt,
-            permissions: mockPermissions,
-            name: mockSite,
-            private: mockPrivate,
-          },
-          {
-            pushed_at: mockUpdatedAt,
-            permissions: mockPermissions,
-            name: mockAdminSite,
-            private: mockPrivate,
-          },
-        ],
-      })
-
       // Act
       const actual = await request(app).get("/")
 

diff --git a/src/routes/v2/authenticated/__tests__/review.spec.ts b/src/routes/v2/authenticated/__tests__/review.spec.ts
@@ -738,9 +738,9 @@ describe("Review Requests Router", () => {
       // Arrange
 
       mockCollaboratorsService.getRole.mockResolvedValueOnce("role")
-      mockReviewRequestService.getReviewRequest.mockResolvedValueOnce(
-        "review request"
-      )
+      mockReviewRequestService.getReviewRequest.mockResolvedValueOnce({
+        reviewStatus: ReviewRequestStatus.Approved,
+      })
       mockReviewRequestService.mergeReviewRequest.mockResolvedValueOnce(
         undefined
       )

diff --git a/src/routes/v2/authenticated/review.ts b/src/routes/v2/authenticated/review.ts
@@ -716,13 +716,19 @@ export class ReviewsRouter {
       return res.status(404).json({ message: possibleReviewRequest.message })
     }
 
-    // Step 4: Merge review request
+    // Step 4: Check if review request is valid
+    if (possibleReviewRequest.reviewStatus !== ReviewRequestStatus.Approved)
+      return res
+        .status(404)
+        .json({ message: "Approved review request not found!" })
+
+    // Step 5: Merge review request
     // NOTE: We are not checking for existence of PR
     // as the underlying Github API returns 404 if
     // the requested review could not be found.
     await this.reviewRequestService.mergeReviewRequest(possibleReviewRequest)
 
-    // Step 5: Clean up the review request view records
+    // Step 6: Clean up the review request view records
     // The error is discarded as we are guaranteed to have a review request
     await this.reviewRequestService.deleteAllReviewRequestViews(
       site.value,

diff --git a/src/routes/v2/authenticated/sites.ts b/src/routes/v2/authenticated/sites.ts
@@ -128,7 +128,6 @@ export class SitesRouter {
 
     router.get(
       "/",
-      this.statsMiddleware.countGithubSites,
       this.statsMiddleware.countMigratedSites,
       attachReadRouteHandlerWrapper(this.getSites)
     )

diff --git a/src/services/db/GitHubService.js b/src/services/db/GitHubService.js
@@ -94,6 +94,37 @@ class GitHubService {
     const { accessToken, siteName, isomerUserId: userId } = sessionData
     try {
       const endpoint = this.getFilePath({ siteName, fileName, directoryName })
+
+      /**
+       * Currently, this rides on the assumption that creating a top level
+       * directly will create a collection.yml file, and creating a new resource
+       * folder will create an index.html file.
+       */
+      const isCreatingTopLevelDirectory = fileName === "collection.yml"
+      const isCreatingNewResourceFolder = fileName === "index.html"
+      const checkDirectoryExist =
+        !isCreatingTopLevelDirectory && !isCreatingNewResourceFolder
+
+      if (checkDirectoryExist) {
+        /**
+         * When we are creating a new resource post or creating a new subDirectory,
+         * we create a _posts and a .keep folder respectively. However, we still need to check if
+         * parent directory still exists.
+         */
+        const isCreatingSubDirectory = fileName === ".keep"
+        const isCreatingPostResource = directoryName.endsWith("_posts")
+        if (!directoryName) {
+          throw new NotFoundError("Directory name is not defined")
+        }
+        let pathToCheck = directoryName
+        if (isCreatingSubDirectory || isCreatingPostResource) {
+          // get parent directory
+          pathToCheck = directoryName.split("/").slice(0, -1).join("/")
+        }
+        // this is to check if the file path still exists, else this will throw a 404
+        await this.readDirectory(sessionData, { directoryName: pathToCheck })
+      }
+
       // Validation and sanitisation of media already done
       const encodedContent = isMedia ? content : Base64.encode(content)
 
@@ -202,6 +233,9 @@ class GitHubService {
     const { accessToken, siteName, isomerUserId: userId } = sessionData
     try {
       const endpoint = this.getFilePath({ siteName, fileName, directoryName })
+      // this is to check if the file path still exists, else this will throw a 404. Only needed for paths outside of root
+      if (directoryName)
+        await this.readDirectory(sessionData, { directoryName })
       const encodedNewContent = Base64.encode(fileContent)
 
       let fileSha = sha

diff --git a/src/services/db/__tests__/GitHubService.spec.js b/src/services/db/__tests__/GitHubService.spec.js
@@ -121,6 +121,7 @@ describe("Github Service", () => {
         },
       }
       mockAxiosInstance.put.mockResolvedValueOnce(resp)
+      mockAxiosInstance.get.mockResolvedValueOnce("")
       await expect(
         service.create(sessionData, {
           content,
@@ -145,6 +146,7 @@ describe("Github Service", () => {
           },
         },
       }
+      mockAxiosInstance.get.mockResolvedValueOnce("")
       mockAxiosInstance.put.mockResolvedValueOnce(resp)
       await expect(
         service.create(sessionData, {
@@ -167,6 +169,7 @@ describe("Github Service", () => {
     })
 
     it("Create parses and throws the correct error in case of a conflict", async () => {
+      mockAxiosInstance.get.mockResolvedValueOnce("")
       mockAxiosInstance.put.mockImplementation(() => {
         const err = new Error()
         err.response = {
@@ -346,6 +349,7 @@ describe("Github Service", () => {
     }
 
     it("Updating a file works correctly", async () => {
+      mockAxiosInstance.get.mockResolvedValueOnce("")
       const resp = {
         data: {
           content: {
@@ -372,6 +376,7 @@ describe("Github Service", () => {
     })
 
     it("Update throws the correct error if file cannot be found", async () => {
+      mockAxiosInstance.get.mockResolvedValueOnce("")
       mockAxiosInstance.put.mockImplementation(() => {
         const err = new Error()
         err.response = {
@@ -395,6 +400,7 @@ describe("Github Service", () => {
     })
 
     it("Updating a file with no sha works correctly", async () => {
+      mockAxiosInstance.get.mockResolvedValueOnce("")
       const getResp = {
         data: {
           content: encodedContent,
@@ -435,6 +441,7 @@ describe("Github Service", () => {
     })
 
     it("Update with no sha provided throws the correct error if file cannot be found", async () => {
+      mockAxiosInstance.get.mockResolvedValueOnce("")
       const readParams = {
         ref: BRANCH_REF,
       }

diff --git a/src/services/directoryServices/SubcollectionDirectoryService.js b/src/services/directoryServices/SubcollectionDirectoryService.js
@@ -91,6 +91,11 @@ class SubcollectionDirectoryService {
     const files = await this.baseDirectoryService.list(sessionData, {
       directoryName: dir,
     })
+    await this.gitHubService.create(sessionData, {
+      content: "",
+      fileName: PLACEHOLDER_FILE_NAME,
+      directoryName: `_${collectionName}/${parsedNewName}`,
+    })
     // We can't perform these operations concurrently because of conflict issues
     /* eslint-disable no-await-in-loop, no-restricted-syntax */
     for (const file of files) {
@@ -111,11 +116,6 @@ class SubcollectionDirectoryService {
         newSubcollectionName: parsedNewName,
       })
     }
-    await this.gitHubService.create(sessionData, {
-      content: "",
-      fileName: PLACEHOLDER_FILE_NAME,
-      directoryName: `_${collectionName}/${parsedNewName}`,
-    })
     await this.collectionYmlService.renameSubfolderInOrder(sessionData, {
       collectionName,
       oldSubfolder: subcollectionName,

diff --git a/src/services/identity/SitesService.ts b/src/services/identity/SitesService.ts
@@ -331,6 +331,17 @@ class SitesService {
       })
     )
 
+    // get sites from DB for email login users
+    if (!isAdminUser && isEmailUser) {
+      const retrievedSitesByEmail = await this.getSitesForEmailUser(userId)
+      const filteredValidSites = retrievedSitesByEmail.filter(_.isString)
+
+      const repoData: RepositoryData[] = filteredValidSites.map((site) => ({
+        repoName: site,
+      }))
+      return repoData
+    }
+
     const allSites = await Promise.all(
       paramsArr.map(async (params) => {
         const {
@@ -358,25 +369,22 @@ class SitesService {
               isPrivate,
             } as RepositoryData
           })
-          .filter(
-            (repoData) =>
+          .filter((repoData) => {
+            if (!repoData || !repoData.permissions) {
+              return false
+            }
+            return (
               repoData.permissions.push === true &&
               !ISOMER_ADMIN_REPOS.includes(repoData.repoName)
-          )
+            )
+          })
       })
     )
 
     const flattenedAllSites = _.flatten(allSites)
     // Github users are using their own access token, which already filters sites to only those they have write access to
     // Admin users should have access to all sites regardless
-    if (isAdminUser || !isEmailUser) return flattenedAllSites
-
-    // Email users need to have the list of sites filtered to those they have access to in our db, since our centralised token returns all sites
-    const retrievedSitesByEmail = await this.getSitesForEmailUser(userId)
-
-    return flattenedAllSites.filter((repoData) =>
-      retrievedSitesByEmail.includes(repoData.repoName)
-    )
+    return flattenedAllSites
   }
 
   async checkHasAccessForGitHubUser(sessionData: UserWithSiteSessionData) {

diff --git a/src/services/identity/__tests__/SitesService.spec.ts b/src/services/identity/__tests__/SitesService.spec.ts
@@ -916,10 +916,7 @@ describe("SitesService", () => {
 
       const expectedResp: RepositoryData[] = [
         {
-          lastUpdated: repoInfo.pushed_at,
-          permissions: repoInfo.permissions,
           repoName: repoInfo.name,
-          isPrivate: repoInfo.private,
         },
       ]
       MockIsomerAdminsService.getByUserId.mockImplementationOnce(() => null)
@@ -939,7 +936,7 @@ describe("SitesService", () => {
       expect(MockIsomerAdminsService.getByUserId).toHaveBeenCalledWith(
         mockIsomerUserId
       )
-      expect(mockAxios.get).toHaveBeenCalledTimes(3)
+      expect(mockAxios.get).toHaveBeenCalledTimes(0)
       config.set("sites.pageCount", currRepoCount)
       expect(config.get("sites.pageCount")).toBe(currRepoCount)
     })
@@ -968,7 +965,7 @@ describe("SitesService", () => {
       expect(MockUsersService.findSitesByUserId).toHaveBeenCalledWith(
         mockIsomerUserId
       )
-      expect(mockAxios.get).toHaveBeenCalledTimes(3)
+      expect(mockAxios.get).toHaveBeenCalledTimes(0)
       config.set("sites.pageCount", currRepoCount)
       expect(config.get("sites.pageCount")).toBe(currRepoCount)
     })