From 078d483ee467500d6a21217455a21b078cc93eb7 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Fri, 5 May 2023 14:31:53 +0800 Subject: [PATCH 1/7] fix: close pull request (#751) --- src/services/review/ReviewRequestService.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/services/review/ReviewRequestService.ts b/src/services/review/ReviewRequestService.ts index a3c2c6cd1..614524359 100644 --- a/src/services/review/ReviewRequestService.ts +++ b/src/services/review/ReviewRequestService.ts @@ -601,9 +601,14 @@ export default class ReviewRequestService { } closeReviewRequest = async (reviewRequest: ReviewRequest): Promise => { - const siteName = reviewRequest.site.name + const { repo } = reviewRequest.site + if (!repo) throw new RequestNotFoundError("Repo not found") + const repoNameInGithub = repo.name const { pullRequestNumber } = reviewRequest.reviewMeta - await this.apiService.closeReviewRequest(siteName, pullRequestNumber) + await this.apiService.closeReviewRequest( + repoNameInGithub, + pullRequestNumber + ) reviewRequest.reviewStatus = ReviewRequestStatus.Closed await reviewRequest.save() From 6ad8e8da97419ed87503444550839c84c392f215 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Fri, 5 May 2023 15:23:09 +0800 Subject: [PATCH 2/7] Fix: publish button delay (#752) * Fix: publish button delay * Fix: tests * Fix: move review request status check into router --- src/integration/Reviews.spec.ts | 28 +++++++++++++++++++ .../v2/authenticated/__tests__/review.spec.ts | 6 ++-- src/routes/v2/authenticated/review.ts | 10 +++++-- 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts index 7fad6b8f0..4cb47120f 100644 --- a/src/integration/Reviews.spec.ts +++ b/src/integration/Reviews.spec.ts @@ -1265,6 +1265,23 @@ 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( @@ -1272,6 +1289,17 @@ describe("Review Requests Integration Tests", () => { 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/routes/v2/authenticated/__tests__/review.spec.ts b/src/routes/v2/authenticated/__tests__/review.spec.ts index 81a76fb20..16e56c057 100644 --- 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 index 0a71aedfb..934698f62 100644 --- 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, From fd30cbe2377c2254aeb3ce3bbdd4ebe72919e95e Mon Sep 17 00:00:00 2001 From: Harish Date: Fri, 5 May 2023 16:29:21 +0800 Subject: [PATCH 3/7] 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 --- src/integration/Sites.spec.ts | 20 ------------- src/routes/v2/authenticated/sites.ts | 1 - src/services/identity/SitesService.ts | 30 ++++++++++++------- .../identity/__tests__/SitesService.spec.ts | 7 ++--- src/types/repoInfo.ts | 6 ++-- 5 files changed, 24 insertions(+), 40 deletions(-) diff --git a/src/integration/Sites.spec.ts b/src/integration/Sites.spec.ts index cf8020a92..f92795fcd 100644 --- 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/sites.ts b/src/routes/v2/authenticated/sites.ts index 4bdf886e0..506c8c374 100644 --- 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/identity/SitesService.ts b/src/services/identity/SitesService.ts index 2698ad80b..17a73c2fe 100644 --- 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 index fd78b6359..c0025b0ae 100644 --- 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) }) diff --git a/src/types/repoInfo.ts b/src/types/repoInfo.ts index a36ce7f9d..ba9ead182 100644 --- a/src/types/repoInfo.ts +++ b/src/types/repoInfo.ts @@ -15,10 +15,10 @@ export type GitHubRepositoryData = { } export type RepositoryData = { - lastUpdated: GitHubRepositoryData["pushed_at"] - permissions: GitHubRepositoryData["permissions"] + lastUpdated?: GitHubRepositoryData["pushed_at"] + permissions?: GitHubRepositoryData["permissions"] repoName: GitHubRepositoryData["name"] - isPrivate: GitHubRepositoryData["private"] + isPrivate?: GitHubRepositoryData["private"] } type SiteUrlTypes = "staging" | "prod" From 34839e2600223f472531e706f79832080e0c67b3 Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Mon, 8 May 2023 17:19:41 +0800 Subject: [PATCH 4/7] 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 --- src/services/db/GitHubService.js | 33 +++++++++++++++++++ .../db/__tests__/GitHubService.spec.js | 7 ++++ 2 files changed, 40 insertions(+) diff --git a/src/services/db/GitHubService.js b/src/services/db/GitHubService.js index 5fe7df9e0..e770fb734 100644 --- 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,8 @@ 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 + 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 index 65846c1d7..88705b774 100644 --- 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, } From a2fec7bf395754b33074cd3b6d6b835c19c0c3a7 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Tue, 9 May 2023 09:59:16 +0800 Subject: [PATCH 5/7] Fix: handle updating of files in root directory (#761) --- src/services/db/GitHubService.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/services/db/GitHubService.js b/src/services/db/GitHubService.js index e770fb734..a78c05be3 100644 --- a/src/services/db/GitHubService.js +++ b/src/services/db/GitHubService.js @@ -233,8 +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 - await this.readDirectory(sessionData, { 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 From fb704a1a8dc626c387268e26398604a4f1c8285b Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Tue, 9 May 2023 14:35:26 +0800 Subject: [PATCH 6/7] fix: create .keep first when rename subfolder (#762) --- .../directoryServices/SubcollectionDirectoryService.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/services/directoryServices/SubcollectionDirectoryService.js b/src/services/directoryServices/SubcollectionDirectoryService.js index 9899b6e2a..02fa507ab 100644 --- 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, From ed53b714ece7740ee755a3c275733ccfa1f7afff Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Tue, 9 May 2023 14:47:56 +0800 Subject: [PATCH 7/7] 0.26.0 --- CHANGELOG.md | 19 ++++++++++++++++--- package-lock.json | 2 +- package.json | 2 +- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec060843b..370142be9 100644 --- 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 index 280b72286..b2e740b66 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "isomercms", - "version": "0.25.0", + "version": "0.26.0", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/package.json b/package.json index 82584b423..fb89265d6 100644 --- 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",