diff --git a/src/integration/NotificationOnEditHandler.spec.ts b/src/integration/NotificationOnEditHandler.spec.ts index 8a058202a..55b231f2f 100644 --- a/src/integration/NotificationOnEditHandler.spec.ts +++ b/src/integration/NotificationOnEditHandler.spec.ts @@ -68,7 +68,7 @@ const collectionYmlService = new CollectionYmlService({ gitHubService: mockGithubService, }) const baseDirectoryService = new BaseDirectoryService({ - gitHubService: mockGithubService, + repoService: mockGithubService, }) const contactUsService = new ContactUsPageService({ diff --git a/src/integration/Notifications.spec.ts b/src/integration/Notifications.spec.ts index 35531782d..2ff7bf16c 100644 --- a/src/integration/Notifications.spec.ts +++ b/src/integration/Notifications.spec.ts @@ -75,7 +75,9 @@ const usersService = getUsersService(sequelize) const configYmlService = new ConfigYmlService({ gitHubService }) const footerYmlService = new FooterYmlService({ gitHubService }) const collectionYmlService = new CollectionYmlService({ gitHubService }) -const baseDirectoryService = new BaseDirectoryService({ gitHubService }) +const baseDirectoryService = new BaseDirectoryService({ + repoService: gitHubService, +}) const contactUsService = new ContactUsPageService({ gitHubService, diff --git a/src/integration/Privatisation.spec.ts b/src/integration/Privatisation.spec.ts index ebf591c84..407616654 100644 --- a/src/integration/Privatisation.spec.ts +++ b/src/integration/Privatisation.spec.ts @@ -106,7 +106,9 @@ const usersService = getUsersService(sequelize) const isomerAdminsService = new IsomerAdminsService({ repository: IsomerAdmin }) const footerYmlService = new FooterYmlService({ gitHubService }) const collectionYmlService = new CollectionYmlService({ gitHubService }) -const baseDirectoryService = new BaseDirectoryService({ gitHubService }) +const baseDirectoryService = new BaseDirectoryService({ + repoService: gitHubService, +}) const contactUsService = new ContactUsPageService({ gitHubService, diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts index 47264fb59..8075f2ebc 100644 --- a/src/integration/Reviews.spec.ts +++ b/src/integration/Reviews.spec.ts @@ -110,7 +110,9 @@ const usersService = getUsersService(sequelize) const isomerAdminsService = new IsomerAdminsService({ repository: IsomerAdmin }) const footerYmlService = new FooterYmlService({ gitHubService }) const collectionYmlService = new CollectionYmlService({ gitHubService }) -const baseDirectoryService = new BaseDirectoryService({ gitHubService }) +const baseDirectoryService = new BaseDirectoryService({ + repoService: gitHubService, +}) const contactUsService = new ContactUsPageService({ gitHubService, diff --git a/src/integration/Sites.spec.ts b/src/integration/Sites.spec.ts index 275044085..76fad18d0 100644 --- a/src/integration/Sites.spec.ts +++ b/src/integration/Sites.spec.ts @@ -89,7 +89,9 @@ const contactUsService = new ContactUsPageService({ gitHubService, footerYmlService, }) -const baseDirectoryService = new BaseDirectoryService({ gitHubService }) +const baseDirectoryService = new BaseDirectoryService({ + repoService: gitHubService, +}) const resourcePageService = new ResourcePageService({ gitHubService }) const resourceRoomDirectoryService = new ResourceRoomDirectoryService({ baseDirectoryService, diff --git a/src/routes/v2/authenticatedSites/index.js b/src/routes/v2/authenticatedSites/index.js index 3f2e9fcd6..5e3ec19e4 100644 --- a/src/routes/v2/authenticatedSites/index.js +++ b/src/routes/v2/authenticatedSites/index.js @@ -107,13 +107,15 @@ const getAuthenticatedSitesSubrouter = ({ }) const unlinkedPageService = new UnlinkedPageService({ gitHubService }) const resourcePageService = new ResourcePageService({ gitHubService }) - const mediaFileService = new MediaFileService({ gitHubService }) + const mediaFileService = new MediaFileService({ repoService: gitHubService }) const moverService = new MoverService({ unlinkedPageService, collectionPageService, subcollectionPageService, }) - const baseDirectoryService = new BaseDirectoryService({ gitHubService }) + const baseDirectoryService = new BaseDirectoryService({ + repoService: gitHubService, + }) const unlinkedPagesDirectoryService = new UnlinkedPagesDirectoryService({ baseDirectoryService, moverService, diff --git a/src/server.js b/src/server.js index bb8c979c3..19c785ce1 100644 --- a/src/server.js +++ b/src/server.js @@ -158,7 +158,9 @@ const gitHubService = new RepoService( const configYmlService = new ConfigYmlService({ gitHubService }) const footerYmlService = new FooterYmlService({ gitHubService }) const collectionYmlService = new CollectionYmlService({ gitHubService }) -const baseDirectoryService = new BaseDirectoryService({ gitHubService }) +const baseDirectoryService = new BaseDirectoryService({ + repoService: gitHubService, +}) const contactUsService = new ContactUsPageService({ gitHubService, diff --git a/src/services/db/GitFileSystemService.ts b/src/services/db/GitFileSystemService.ts index 358f2c0e8..ef8467927 100644 --- a/src/services/db/GitFileSystemService.ts +++ b/src/services/db/GitFileSystemService.ts @@ -34,11 +34,6 @@ import type { import type { IsomerCommitMessage } from "@root/types/github" import { ALLOWED_FILE_EXTENSIONS } from "@root/utils/file-upload-utils" -/** - * Some notes: - * - Seems like getTree, updateTree and updateRepoState is always used together - */ - const EFS_VOL_PATH = config.get("aws.efs.volPath") const BRANCH_REF = config.get("github.branchRef") @@ -49,6 +44,19 @@ export default class GitFileSystemService { this.git = git } + isDefaultLogFields(logFields: unknown): logFields is DefaultLogFields { + const c = logFields as DefaultLogFields + return ( + !!logFields && + typeof logFields === "object" && + typeof c.author_name === "string" && + typeof c.author_email === "string" && + typeof c.date === "string" && + typeof c.message === "string" && + typeof c.hash === "string" + ) + } + isGitInitialized(repoName: string): ResultAsync { return ResultAsync.fromPromise( this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).checkIsRepo(), @@ -871,17 +879,187 @@ export default class GitFileSystemService { }) } - isDefaultLogFields(logFields: unknown): logFields is DefaultLogFields { - const c = logFields as DefaultLogFields - return ( - !!logFields && - typeof logFields === "object" && - typeof c.author_name === "string" && - typeof c.author_email === "string" && - typeof c.date === "string" && - typeof c.message === "string" && - typeof c.hash === "string" - ) + // Rename a single file or directory + renameSinglePath( + repoName: string, + oldPath: string, + newPath: string, + userId: string, + message?: string + ): ResultAsync { + let oldStateSha = "" + + return this.getLatestCommitOfBranch(repoName, BRANCH_REF) + .andThen((latestCommit) => { + // It is guaranteed that the latest commit contains the SHA hash + oldStateSha = latestCommit.sha as string + return okAsync(true) + }) + .andThen(() => this.getFilePathStats(repoName, oldPath)) + .andThen(() => + // We expect to see an error here, since the new path should not exist + this.getFilePathStats(repoName, newPath) + .andThen(() => + errAsync(new ConflictError("File path already exists")) + ) + .map(() => true) + .orElse((error) => { + if (error instanceof NotFoundError) { + return okAsync(true) + } + + return errAsync(error) + }) + ) + .andThen(() => + ResultAsync.fromPromise( + this.git.cwd(`${EFS_VOL_PATH}/${repoName}`).mv(oldPath, newPath), + (error) => { + logger.error(`Error when moving ${oldPath} to ${newPath}: ${error}`) + + if (error instanceof GitError) { + return new GitFileSystemNeedsRollbackError( + `Unable to rename ${oldPath} to ${newPath}` + ) + } + + return new GitFileSystemNeedsRollbackError( + "An unknown error occurred" + ) + } + ) + ) + .andThen(() => + this.commit( + repoName, + [oldPath, newPath], + userId, + message || `Renamed ${oldPath} to ${newPath}` + ) + ) + .orElse((error) => { + if (error instanceof GitFileSystemNeedsRollbackError) { + return this.rollback(repoName, oldStateSha).andThen(() => + errAsync(new GitFileSystemError(error.message)) + ) + } + + return errAsync(error) + }) + } + + // Move multiple files from oldPath to newPath without renaming them + moveFiles( + repoName: string, + oldPath: string, + newPath: string, + userId: string, + targetFiles: string[], + message?: string + ): ResultAsync { + let oldStateSha = "" + + return this.getLatestCommitOfBranch(repoName, BRANCH_REF) + .andThen((latestCommit) => { + // It is guaranteed that the latest commit contains the SHA hash + oldStateSha = latestCommit.sha as string + return okAsync(true) + }) + .andThen(() => this.getFilePathStats(repoName, oldPath)) + .andThen((stats) => { + if (!stats.isDirectory()) { + return errAsync( + new GitFileSystemError( + `Path "${oldPath}" is not a valid directory in repo "${repoName}"` + ) + ) + } + return okAsync(true) + }) + .andThen(() => + // Ensure that the new path exists + ResultAsync.fromPromise( + fs.promises.mkdir(`${EFS_VOL_PATH}/${repoName}/${newPath}`, { + recursive: true, + }), + (error) => { + logger.error(`Error when creating ${newPath} during move: ${error}`) + + if (error instanceof Error) { + return new GitFileSystemNeedsRollbackError( + `Unable to create ${newPath}` + ) + } + + return new GitFileSystemNeedsRollbackError( + "An unknown error occurred" + ) + } + ) + ) + .andThen(() => + combine( + targetFiles.map((targetFile) => + // We expect to see an error here, since the new path should not exist + this.getFilePathStats(repoName, `${newPath}/${targetFile}`) + .andThen(() => + errAsync(new ConflictError("File path already exists")) + ) + .map(() => true) + .orElse((error) => { + if (error instanceof NotFoundError) { + return okAsync(true) + } + + return errAsync(error) + }) + .andThen(() => { + const oldFilePath = + oldPath === "" ? targetFile : `${oldPath}/${targetFile}` + const newFilePath = + newPath === "" ? targetFile : `${newPath}/${targetFile}` + + return ResultAsync.fromPromise( + this.git + .cwd(`${EFS_VOL_PATH}/${repoName}`) + .mv(`${oldFilePath}`, `${newFilePath}`), + (error) => { + logger.error( + `Error when moving ${targetFile} in ${oldPath} to ${newPath}: ${error}` + ) + + if (error instanceof GitError) { + return new GitFileSystemNeedsRollbackError( + `Unable to move ${targetFile} to ${newPath}` + ) + } + + return new GitFileSystemNeedsRollbackError( + "An unknown error occurred" + ) + } + ) + }) + ) + ) + ) + .andThen(() => + this.commit( + repoName, + [oldPath, newPath], + userId, + message || `Moved selected files from ${oldPath} to ${newPath}` + ) + ) + .orElse((error) => { + if (error instanceof GitFileSystemNeedsRollbackError) { + return this.rollback(repoName, oldStateSha).andThen(() => + errAsync(new GitFileSystemError(error.message)) + ) + } + + return errAsync(error) + }) } getLatestCommitOfBranch( diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts index 68cb04c6c..2f7d85bc2 100644 --- a/src/services/db/RepoService.ts +++ b/src/services/db/RepoService.ts @@ -4,8 +4,9 @@ import config from "@config/config" import logger from "@logger/logger" -import { GithubSessionDataProps } from "@root/classes" +import GithubSessionData from "@root/classes/GithubSessionData" import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" +import { ConflictError } from "@root/errors/ConflictError" import { GitHubCommitData } from "@root/types/commitData" import type { GitCommitResult, @@ -356,7 +357,7 @@ export default class RepoService extends GitHubService { }: { directoryName: string message: string - githubSessionData: GithubSessionDataProps + githubSessionData: GithubSessionData } ): Promise { if (this.isRepoWhitelisted(sessionData.siteName)) { @@ -449,6 +450,169 @@ export default class RepoService extends GitHubService { }) } + async renameSinglePath( + sessionData: UserWithSiteSessionData, + githubSessionData: GithubSessionData, + oldPath: string, + newPath: string, + message?: string + ): Promise { + if (this.isRepoWhitelisted(sessionData.siteName)) { + logger.info("Renaming file/directory in local Git file system") + const result = await this.gitFileSystemService.renameSinglePath( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + message + ) + + if (result.isErr()) { + throw result.error + } + + this.gitFileSystemService.push(sessionData.siteName) + return { newSha: result.value } + } + + const gitTree = await super.getTree(sessionData, githubSessionData, { + isRecursive: true, + }) + const newGitTree: any[] = [] + const isMovingDirectory = + gitTree.find((item: any) => item.path === oldPath)?.type === "tree" || + false + + gitTree.forEach((item: any) => { + if (isMovingDirectory) { + if (item.path === newPath && item.type === "tree") { + throw new ConflictError("Target directory already exists") + } else if (item.path === oldPath && item.type === "tree") { + // Rename old subdirectory to new name + newGitTree.push({ + ...item, + path: newPath, + }) + } else if ( + item.path.startsWith(`${oldPath}/`) && + item.type !== "tree" + ) { + // Delete old files + newGitTree.push({ + ...item, + sha: null, + }) + } + } else if (item.path === newPath && item.type !== "tree") { + throw new ConflictError("Target file already exists") + } else if (item.path === oldPath && item.type !== "tree") { + // Add file to new directory + newGitTree.push({ + ...item, + path: newPath, + }) + // Delete old file + newGitTree.push({ + ...item, + sha: null, + }) + } + }) + + const newCommitSha = await super.updateTree( + sessionData, + githubSessionData, + { + gitTree: newGitTree, + message, + } + ) + await super.updateRepoState(sessionData, { + commitSha: newCommitSha, + }) + + return { newSha: newCommitSha } + } + + async moveFiles( + sessionData: UserWithSiteSessionData, + githubSessionData: GithubSessionData, + oldPath: string, + newPath: string, + targetFiles: string[], + message?: string + ): Promise { + if (this.isRepoWhitelisted(sessionData.siteName)) { + logger.info("Moving files in local Git file system") + const result = await this.gitFileSystemService.moveFiles( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + targetFiles, + message + ) + + if (result.isErr()) { + throw result.error + } + + this.gitFileSystemService.push(sessionData.siteName) + return { newSha: result.value } + } + + const gitTree = await super.getTree(sessionData, githubSessionData, { + isRecursive: true, + }) + const newGitTree: any[] = [] + + gitTree.forEach((item: any) => { + if (item.path.startsWith(`${newPath}/`) && item.type !== "tree") { + const fileName = item.path + .split(`${newPath}/`) + .slice(1) + .join(`${newPath}/`) + if (targetFiles.includes(fileName)) { + // Conflicting file + throw new ConflictError("File already exists in target directory") + } + } + if (item.path.startsWith(`${oldPath}/`) && item.type !== "tree") { + const fileName = item.path + .split(`${oldPath}/`) + .slice(1) + .join(`${oldPath}/`) + if (targetFiles.includes(fileName)) { + // Add file to target directory + newGitTree.push({ + ...item, + path: `${newPath}/${fileName}`, + }) + // Delete old file + newGitTree.push({ + ...item, + sha: null, + }) + } + } + }) + + const newCommitSha = await super.updateTree( + sessionData, + githubSessionData, + { + gitTree: newGitTree, + message, + } + ) + + await super.updateRepoState(sessionData, { + commitSha: newCommitSha, + }) + + return { newSha: newCommitSha } + } + async getRepoInfo(sessionData: any): Promise { return await super.getRepoInfo(sessionData) } diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index be63a0c91..048b7dea0 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -37,13 +37,16 @@ const GitFileSystemService = new _GitFileSystemService( const BRANCH_REF = config.get("github.branchRef") describe("GitFileSystemService", () => { - beforeAll(() => { + beforeEach(() => { mockFs({ [config.get("aws.efs.volPath")]: { "fake-repo": { "fake-dir": { "fake-file": "fake content", }, + "another-fake-dir": { + "fake-file": "duplicate fake file", + }, "fake-empty-dir": {}, "another-fake-file": "Another fake content", }, @@ -54,14 +57,14 @@ describe("GitFileSystemService", () => { // Prevent inter-test pollution of mocks afterEach(() => { jest.clearAllMocks() - }) - - afterAll(() => { mockFs.restore() }) describe("listDirectoryContents", () => { it("should return the contents of a directory successfully", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce("another-fake-dir-hash"), + }) MockSimpleGit.cwd.mockReturnValueOnce({ revparse: jest.fn().mockResolvedValueOnce("another-fake-file-hash"), }) @@ -79,6 +82,13 @@ describe("GitFileSystemService", () => { path: "fake-dir", size: 0, } + const expectedAnotherFakeDir: GitDirectoryItem = { + name: "another-fake-dir", + type: "dir", + sha: "another-fake-dir-hash", + path: "another-fake-dir", + size: 0, + } const expectedFakeEmptyDir: GitDirectoryItem = { name: "fake-empty-dir", type: "dir", @@ -103,6 +113,7 @@ describe("GitFileSystemService", () => { .sort((a, b) => a.name.localeCompare(b.name)) expect(actual).toMatchObject([ + expectedAnotherFakeDir, expectedAnotherFakeFile, expectedFakeDir, expectedFakeEmptyDir, @@ -110,6 +121,9 @@ describe("GitFileSystemService", () => { }) it("should return only results of files that are tracked by Git", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockRejectedValueOnce(new GitError()), + }) MockSimpleGit.cwd.mockReturnValueOnce({ revparse: jest.fn().mockResolvedValueOnce("another-fake-file-hash"), }) @@ -157,6 +171,9 @@ describe("GitFileSystemService", () => { MockSimpleGit.cwd.mockReturnValueOnce({ revparse: jest.fn().mockRejectedValueOnce(new GitError()), }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockRejectedValueOnce(new GitError()), + }) const actual = await GitFileSystemService.listDirectoryContents( "fake-repo", @@ -1264,6 +1281,514 @@ describe("GitFileSystemService", () => { }) }) + describe("renameSinglePath", () => { + it("should rename a file successfully", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + // Note: This will not cause the actual file to be renamed + const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: mockGitMv, + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), + }), + }) + + const actual = await GitFileSystemService.renameSinglePath( + "fake-repo", + "fake-dir/fake-file", + "fake-dir/fake-file-renamed", + "fake-user-id", + "fake-message" + ) + + expect(actual._unsafeUnwrap()).toEqual("fake-new-hash") + expect(mockGitMv).toHaveBeenCalledWith( + "fake-dir/fake-file", + "fake-dir/fake-file-renamed" + ) + }) + + it("should rename a directory successfully", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + // Note: This will not cause the actual file to be renamed + const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: mockGitMv, + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), + }), + }) + + const actual = await GitFileSystemService.renameSinglePath( + "fake-repo", + "fake-dir", + "fake-dir-renamed", + "fake-user-id", + "fake-message" + ) + + expect(actual._unsafeUnwrap()).toEqual("fake-new-hash") + expect(mockGitMv).toHaveBeenCalledWith("fake-dir", "fake-dir-renamed") + }) + + it("should rollback changes if an error occurred when committing", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: jest.fn().mockResolvedValueOnce(undefined), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockReturnValueOnce({ + commit: jest.fn().mockRejectedValueOnce(new GitError()), + }), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + reset: jest.fn().mockReturnValueOnce({ + clean: jest.fn().mockResolvedValueOnce(undefined), + }), + }) + + const spyRollback = jest.spyOn(GitFileSystemService, "rollback") + + const actual = await GitFileSystemService.renameSinglePath( + "fake-repo", + "fake-dir", + "fake-dir-renamed", + "fake-user-id", + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) + expect(spyRollback).toHaveBeenCalledWith("fake-repo", "fake-hash") + }) + + it("should rollback changes if an error occurred when moving the file", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: jest.fn().mockRejectedValueOnce(new GitError()), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + reset: jest.fn().mockReturnValueOnce({ + clean: jest.fn().mockResolvedValueOnce(undefined), + }), + }) + + const spyRollback = jest.spyOn(GitFileSystemService, "rollback") + + const actual = await GitFileSystemService.renameSinglePath( + "fake-repo", + "fake-dir", + "fake-dir-renamed", + "fake-user-id", + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) + expect(spyRollback).toHaveBeenCalledWith("fake-repo", "fake-hash") + }) + + it("should return ConflictError if newPath is already an existing file/directory", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + + const actual = await GitFileSystemService.renameSinglePath( + "fake-repo", + "fake-dir", + "fake-empty-dir", + "fake-user-id", + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(ConflictError) + }) + + it("should return NotFoundError if the oldPath does not exist", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + + const actual = await GitFileSystemService.renameSinglePath( + "fake-repo", + "fake-nonexistent-dir", + "fake-new-dir", + "fake-user-id", + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(NotFoundError) + }) + }) + + describe("moveFiles", () => { + it("should move files to an existing directory successfully", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + // Note: This will not cause the actual file to be renamed + const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: mockGitMv, + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), + }), + }) + + const actual = await GitFileSystemService.moveFiles( + "fake-repo", + "", + "fake-dir", + "fake-user-id", + ["another-fake-file"], + "fake-message" + ) + + expect(actual._unsafeUnwrap()).toEqual("fake-new-hash") + expect(mockGitMv).toHaveBeenCalledWith( + "another-fake-file", + "fake-dir/another-fake-file" + ) + }) + + it("should move files to a new directory successfully", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + // Note: This will not cause the actual file to be renamed + const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: mockGitMv, + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockReturnValueOnce({ + commit: jest.fn().mockResolvedValueOnce({ commit: "fake-new-hash" }), + }), + }) + + const actual = await GitFileSystemService.moveFiles( + "fake-repo", + "", + "fake-new-dir", + "fake-user-id", + ["another-fake-file"], + "fake-message" + ) + + expect(actual._unsafeUnwrap()).toEqual("fake-new-hash") + expect(mockGitMv).toHaveBeenCalledWith( + "another-fake-file", + "fake-new-dir/another-fake-file" + ) + }) + + it("should rollback changes if an error occurred when committing", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + // Note: This will not cause the actual file to be renamed + const mockGitMv = jest.fn().mockResolvedValueOnce(undefined) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: mockGitMv, + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + checkIsRepo: jest.fn().mockResolvedValueOnce(true), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + remote: jest + .fn() + .mockResolvedValueOnce( + `git@github.com:${ISOMER_GITHUB_ORG_NAME}/fake-repo.git` + ), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + revparse: jest.fn().mockResolvedValueOnce(BRANCH_REF), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + add: jest.fn().mockReturnValueOnce({ + commit: jest.fn().mockRejectedValueOnce(new GitError()), + }), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + reset: jest.fn().mockReturnValueOnce({ + clean: jest.fn().mockResolvedValueOnce(undefined), + }), + }) + const spyRollback = jest.spyOn(GitFileSystemService, "rollback") + + const actual = await GitFileSystemService.moveFiles( + "fake-repo", + "", + "fake-dir", + "fake-user-id", + ["another-fake-file"], + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) + expect(spyRollback).toHaveBeenCalledWith("fake-repo", "fake-hash") + }) + + it("should rollback changes if an error occurred when moving files", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + mv: jest.fn().mockRejectedValueOnce(new GitError()), + }) + MockSimpleGit.cwd.mockReturnValueOnce({ + reset: jest.fn().mockReturnValueOnce({ + clean: jest.fn().mockResolvedValueOnce(undefined), + }), + }) + const spyRollback = jest.spyOn(GitFileSystemService, "rollback") + + const actual = await GitFileSystemService.moveFiles( + "fake-repo", + "", + "fake-dir", + "fake-user-id", + ["another-fake-file"], + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) + expect(spyRollback).toHaveBeenCalledWith("fake-repo", "fake-hash") + }) + + it("should return ConflictError if newPath is already an existing file/directory", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + + const actual = await GitFileSystemService.moveFiles( + "fake-repo", + "fake-dir", + "another-fake-dir", + "fake-user-id", + ["fake-file"], + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(ConflictError) + }) + + it("should return GitFileSystemError if the oldPath is not a directory", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + + const actual = await GitFileSystemService.moveFiles( + "fake-repo", + "another-fake-file", + "another-fake-dir", + "fake-user-id", + ["fake-file"], + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) + }) + + it("should return NotFoundError if the oldPath does not exist", async () => { + MockSimpleGit.cwd.mockReturnValueOnce({ + log: jest.fn().mockResolvedValueOnce({ + latest: { + hash: "fake-hash", + date: "fake-date", + message: "fake-message", + author_name: "fake-author", + author_email: "fake-email", + }, + }), + }) + + const actual = await GitFileSystemService.moveFiles( + "fake-repo", + "fake-nonexistent-dir", + "fake-new-dir", + "fake-user-id", + ["fake-file"], + "fake-message" + ) + + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(NotFoundError) + }) + }) + describe("getLatestCommitOfBranch", () => { it("should return the latest commit data from branch", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ diff --git a/src/services/db/__tests__/RepoService.spec.ts b/src/services/db/__tests__/RepoService.spec.ts index 15c3c968f..d5d73fa03 100644 --- a/src/services/db/__tests__/RepoService.spec.ts +++ b/src/services/db/__tests__/RepoService.spec.ts @@ -5,6 +5,7 @@ import { mockAccessToken, mockEmail, mockGithubId, + mockGithubSessionData, mockIsomerUserId, mockSiteName, mockUserWithSiteSessionData, @@ -36,6 +37,8 @@ const MockGitFileSystemService = { update: jest.fn(), delete: jest.fn(), getLatestCommitOfBranch: jest.fn(), + renameSinglePath: jest.fn(), + moveFiles: jest.fn(), } const RepoService = new _RepoService( @@ -296,6 +299,7 @@ describe("RepoService", () => { MockGitFileSystemService.update.mockResolvedValueOnce( okAsync(expectedSha) ) + MockGitFileSystemService.push.mockReturnValueOnce(undefined) const actual = await RepoService.update(mockUserWithSiteSessionData, { fileContent: "test content", @@ -330,6 +334,190 @@ describe("RepoService", () => { }) }) + describe("renameSinglePath", () => { + it("should rename using the local Git file system if the repo is whitelisted", async () => { + const expectedSha = "fake-commit-sha" + MockGitFileSystemService.renameSinglePath.mockResolvedValueOnce( + okAsync(expectedSha) + ) + MockGitFileSystemService.push.mockReturnValueOnce(undefined) + + const actual = await RepoService.renameSinglePath( + mockUserWithSiteSessionData, + mockGithubSessionData, + "fake-old-path", + "fake-new-path", + "fake-commit-message" + ) + + expect(actual).toEqual({ newSha: expectedSha }) + }) + + it("should rename file using GitHub directly if the repo is not whitelisted", async () => { + const expectedSha = "fake-commit-sha" + const fakeCommitMessage = "fake-commit-message" + const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData({ + githubId: mockGithubId, + accessToken: mockAccessToken, + isomerUserId: mockIsomerUserId, + email: mockEmail, + siteName: "not-whitelisted", + }) + const mockedTree = [ + { + type: "file", + path: "fake-path/old-fake-file.md", + sha: "fake-original-sha", + }, + { + type: "tree", + path: `fake-path`, + sha: "sha1", + }, + ] + const expectedMovedTree = [ + { + type: "file", + path: "fake-path/new-fake-file.md", + sha: "fake-original-sha", + }, + { + type: "file", + path: "fake-path/old-fake-file.md", + sha: null, + }, + ] + const gitHubServiceGetTree = jest.spyOn( + GitHubService.prototype, + "getTree" + ) + gitHubServiceGetTree.mockResolvedValueOnce(mockedTree) + const gitHubServiceUpdateTree = jest.spyOn( + GitHubService.prototype, + "updateTree" + ) + gitHubServiceUpdateTree.mockResolvedValueOnce(expectedSha) + const gitHubServiceUpdateRepoState = jest.spyOn( + GitHubService.prototype, + "updateRepoState" + ) + gitHubServiceUpdateRepoState.mockResolvedValueOnce(undefined) + + const actual = await RepoService.renameSinglePath( + sessionData, + mockGithubSessionData, + "fake-path/old-fake-file.md", + "fake-path/new-fake-file.md", + fakeCommitMessage + ) + + expect(actual).toEqual({ newSha: expectedSha }) + expect(gitHubServiceUpdateTree).toHaveBeenCalledWith( + sessionData, + mockGithubSessionData, + { gitTree: expectedMovedTree, message: fakeCommitMessage } + ) + }) + }) + + describe("moveFiles", () => { + it("should move files using the Git local file system if the repo is whitelisted", async () => { + const expectedSha = "fake-commit-sha" + MockGitFileSystemService.moveFiles.mockResolvedValueOnce( + okAsync(expectedSha) + ) + MockGitFileSystemService.push.mockReturnValueOnce(undefined) + + const actual = await RepoService.moveFiles( + mockUserWithSiteSessionData, + mockGithubSessionData, + "fake-old-path", + "fake-new-path", + ["fake-file1", "fake-file2"], + "fake-commit-message" + ) + + expect(actual).toEqual({ newSha: expectedSha }) + }) + + it("should move files using GitHub directly if the repo is not whitelisted", async () => { + const expectedSha = "fake-commit-sha" + const fakeCommitMessage = "fake-commit-message" + const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData({ + githubId: mockGithubId, + accessToken: mockAccessToken, + isomerUserId: mockIsomerUserId, + email: mockEmail, + siteName: "not-whitelisted", + }) + const mockedTree = [ + { + type: "file", + path: "fake-path/old-fake-file.md", + sha: "fake-original-sha", + }, + { + type: "file", + path: `fake-path/old-fake-file-two.md`, + sha: "fake-original-sha-two", + }, + ] + const expectedMovedTree = [ + { + type: "file", + path: "fake-new-path/old-fake-file.md", + sha: "fake-original-sha", + }, + { + type: "file", + path: "fake-path/old-fake-file.md", + sha: null, + }, + { + type: "file", + path: "fake-new-path/old-fake-file-two.md", + sha: "fake-original-sha-two", + }, + { + type: "file", + path: `fake-path/old-fake-file-two.md`, + sha: null, + }, + ] + const gitHubServiceGetTree = jest.spyOn( + GitHubService.prototype, + "getTree" + ) + gitHubServiceGetTree.mockResolvedValueOnce(mockedTree) + const gitHubServiceUpdateTree = jest.spyOn( + GitHubService.prototype, + "updateTree" + ) + gitHubServiceUpdateTree.mockResolvedValueOnce(expectedSha) + const gitHubServiceUpdateRepoState = jest.spyOn( + GitHubService.prototype, + "updateRepoState" + ) + gitHubServiceUpdateRepoState.mockResolvedValueOnce(undefined) + + const actual = await RepoService.moveFiles( + sessionData, + mockGithubSessionData, + "fake-path", + "fake-new-path", + ["old-fake-file.md", "old-fake-file-two.md"], + fakeCommitMessage + ) + + expect(actual).toEqual({ newSha: expectedSha }) + expect(gitHubServiceUpdateTree).toHaveBeenCalledWith( + sessionData, + mockGithubSessionData, + { gitTree: expectedMovedTree, message: fakeCommitMessage } + ) + }) + }) + describe("getLatestCommitOfBranch", () => { it("should read the latest commit data from the local Git file system if the repo is whitelisted", async () => { const expected: GitHubCommitData = { diff --git a/src/services/directoryServices/BaseDirectoryService.js b/src/services/directoryServices/BaseDirectoryService.js index cd831f2c3..8fba47d3c 100644 --- a/src/services/directoryServices/BaseDirectoryService.js +++ b/src/services/directoryServices/BaseDirectoryService.js @@ -1,15 +1,13 @@ const _ = require("lodash") -const { ConflictError } = require("@errors/ConflictError") - // Job is to deal with directory level operations to and from GitHub class BaseDirectoryService { - constructor({ gitHubService }) { - this.gitHubService = gitHubService + constructor({ repoService }) { + this.repoService = repoService } async list(sessionData, { directoryName }) { - const directoryData = await this.gitHubService.readDirectory(sessionData, { + const directoryData = await this.repoService.readDirectory(sessionData, { directoryName, }) @@ -27,61 +25,26 @@ class BaseDirectoryService { return _.compact(filesOrDirs) } + async delete(sessionData, githubSessionData, { directoryName, message }) { + await this.repoService.deleteDirectory(sessionData, { + directoryName, + message, + githubSessionData, + }) + } + async rename( sessionData, githubSessionData, { oldDirectoryName, newDirectoryName, message } ) { - const gitTree = await this.gitHubService.getTree( - sessionData, - githubSessionData, - { - isRecursive: true, - } - ) - - const newGitTree = [] - - gitTree.forEach((item) => { - if (item.path === newDirectoryName && item.type === "tree") { - throw new ConflictError("Target directory already exists") - } else if (item.path === oldDirectoryName && item.type === "tree") { - // Rename old subdirectory to new name - newGitTree.push({ - ...item, - path: newDirectoryName, - }) - } else if ( - item.path.startsWith(`${oldDirectoryName}/`) && - item.type !== "tree" - ) { - // Delete old files - newGitTree.push({ - ...item, - sha: null, - }) - } - }) - - const newCommitSha = await this.gitHubService.updateTree( + await this.repoService.renameSinglePath( sessionData, githubSessionData, - { - gitTree: newGitTree, - message, - } + oldDirectoryName, + newDirectoryName, + message ) - await this.gitHubService.updateRepoState(sessionData, { - commitSha: newCommitSha, - }) - } - - async delete(sessionData, githubSessionData, { directoryName, message }) { - await this.gitHubService.deleteDirectory(sessionData, { - directoryName, - message, - githubSessionData, - }) } // Move files which do not require modification of content @@ -90,62 +53,14 @@ class BaseDirectoryService { githubSessionData, { oldDirectoryName, newDirectoryName, targetFiles, message } ) { - const gitTree = await this.gitHubService.getTree( - sessionData, - githubSessionData, - { - isRecursive: true, - } - ) - const newGitTree = [] - gitTree.forEach((item) => { - if ( - item.path.startsWith(`${newDirectoryName}/`) && - item.type !== "tree" - ) { - const fileName = item.path - .split(`${newDirectoryName}/`) - .slice(1) - .join(`${newDirectoryName}/`) - if (targetFiles.includes(fileName)) { - // Conflicting file - throw new ConflictError("File already exists in target directory") - } - } - if ( - item.path.startsWith(`${oldDirectoryName}/`) && - item.type !== "tree" - ) { - const fileName = item.path - .split(`${oldDirectoryName}/`) - .slice(1) - .join(`${oldDirectoryName}/`) - if (targetFiles.includes(fileName)) { - // Add file to target directory - newGitTree.push({ - ...item, - path: `${newDirectoryName}/${fileName}`, - }) - // Delete old file - newGitTree.push({ - ...item, - sha: null, - }) - } - } - }) - - const newCommitSha = await this.gitHubService.updateTree( + await this.repoService.moveFiles( sessionData, githubSessionData, - { - gitTree: newGitTree, - message, - } + oldDirectoryName, + newDirectoryName, + targetFiles, + message ) - await this.gitHubService.updateRepoState(sessionData, { - commitSha: newCommitSha, - }) } } diff --git a/src/services/directoryServices/__tests__/BaseDirectoryService.spec.js b/src/services/directoryServices/__tests__/BaseDirectoryService.spec.js index 0e7391d12..b6df2fea6 100644 --- a/src/services/directoryServices/__tests__/BaseDirectoryService.spec.js +++ b/src/services/directoryServices/__tests__/BaseDirectoryService.spec.js @@ -12,53 +12,12 @@ describe("Base Directory Service", () => { const treeSha = "00000" const mockGithubSessionData = "mockData" - const mockedTree = [ - { - type: "tree", - path: "_normal", - }, - { - type: "tree", - path: `${directoryName}`, - }, - { - type: "tree", - path: `${directoryName}/${subcollectionName}`, - }, - { - type: "tree", - path: `_to-keep/${directoryName}/${subcollectionName}`, - }, - { - type: "file", - path: "_normal/file.md", - }, - { - type: "file", - path: `${directoryName}/file.md`, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file.md`, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file2.md`, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file3.md`, - }, - { - type: "file", - path: `_to-keep/${directoryName}/${subcollectionName}/file.md`, - }, - ] - const sessionData = { siteName, accessToken, currentCommitSha, treeSha } - const mockGithubService = { + const mockRepoService = { readDirectory: jest.fn(), + renameSinglePath: jest.fn(), + moveFiles: jest.fn(), getTree: jest.fn(), updateTree: jest.fn(), updateRepoState: jest.fn(), @@ -69,7 +28,7 @@ describe("Base Directory Service", () => { BaseDirectoryService, } = require("@services/directoryServices/BaseDirectoryService") const service = new BaseDirectoryService({ - gitHubService: mockGithubService, + repoService: mockRepoService, }) beforeEach(() => { @@ -97,58 +56,27 @@ describe("Base Directory Service", () => { ...item, extra: "extra", })) - mockGithubService.readDirectory.mockResolvedValueOnce(githubServiceResp) + mockRepoService.readDirectory.mockResolvedValueOnce(githubServiceResp) it("Listing directory contents filters and returns only relevant data", async () => { await expect( service.list(sessionData, { directoryName, }) ).resolves.toMatchObject(readDirResp) - expect(mockGithubService.readDirectory).toHaveBeenCalledWith( - sessionData, - { - directoryName, - } - ) + expect(mockRepoService.readDirectory).toHaveBeenCalledWith(sessionData, { + directoryName, + }) }) }) describe("Rename", () => { const renamedDir = "_renamed-dir" - const mockedRenamedTree = [ - { - type: "tree", - path: `${renamedDir}`, - }, - { - type: "file", - path: `${directoryName}/file.md`, - sha: null, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file.md`, - sha: null, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file2.md`, - sha: null, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file3.md`, - sha: null, - }, - ] - mockGithubService.getTree.mockResolvedValueOnce([ - ...mockedTree, - { - type: "tree", - path: renamedDir, - }, - ]) + it("Renaming a directory to one with an existing name throws an error", async () => { + mockRepoService.renameSinglePath.mockRejectedValueOnce( + new ConflictError() + ) + await expect( service.rename(sessionData, mockGithubSessionData, { oldDirectoryName: directoryName, @@ -156,17 +84,21 @@ describe("Base Directory Service", () => { message, }) ).rejects.toThrowError(ConflictError) - expect(mockGithubService.getTree).toHaveBeenCalledWith( + + expect(mockRepoService.renameSinglePath).toHaveBeenCalledWith( sessionData, mockGithubSessionData, - { - isRecursive: true, - } + directoryName, + renamedDir, + message ) }) - mockGithubService.getTree.mockResolvedValueOnce(mockedTree) - mockGithubService.updateTree.mockResolvedValueOnce(sha) + it("Renaming directories works correctly", async () => { + mockRepoService.renameSinglePath.mockResolvedValueOnce({ + newSha: sha, + }) + await expect( service.rename(sessionData, mockGithubSessionData, { oldDirectoryName: directoryName, @@ -174,63 +106,29 @@ describe("Base Directory Service", () => { message, }) ).resolves.not.toThrow() - expect(mockGithubService.getTree).toHaveBeenCalledWith( - sessionData, - mockGithubSessionData, - { - isRecursive: true, - } - ) - expect(mockGithubService.updateTree).toHaveBeenCalledWith( + + expect(mockRepoService.renameSinglePath).toHaveBeenCalledWith( sessionData, mockGithubSessionData, - { - gitTree: mockedRenamedTree, - message, - } - ) - expect(mockGithubService.updateRepoState).toHaveBeenCalledWith( - sessionData, - { - commitSha: sha, - } + directoryName, + renamedDir, + message ) }) }) describe("Delete", () => { - const mockedDeletedTree = [ - { - type: "file", - path: `${directoryName}/file.md`, - sha: null, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file.md`, - sha: null, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file2.md`, - sha: null, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file3.md`, - sha: null, - }, - ] - mockGithubService.getTree.mockResolvedValueOnce(mockedTree) - mockGithubService.updateTree.mockResolvedValueOnce(sha) it("Deleting directories works correctly", async () => { + mockRepoService.deleteDirectory.mockResolvedValueOnce(undefined) + await expect( service.delete(sessionData, mockGithubSessionData, { directoryName, message, }) ).resolves.not.toThrow() - expect(mockGithubService.deleteDirectory).toHaveBeenCalledWith( + + expect(mockRepoService.deleteDirectory).toHaveBeenCalledWith( sessionData, { directoryName, @@ -243,34 +141,10 @@ describe("Base Directory Service", () => { describe("Move Files", () => { const targetDir = "_target-dir" - const mockedMovedTree = [ - { - type: "file", - path: `${targetDir}/file.md`, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file.md`, - sha: null, - }, - { - type: "file", - path: `${targetDir}/file2.md`, - }, - { - type: "file", - path: `${directoryName}/${subcollectionName}/file2.md`, - sha: null, - }, - ] - mockGithubService.getTree.mockResolvedValueOnce([ - ...mockedTree, - { - type: "file", - path: `${targetDir}/file.md`, - }, - ]) + it("Moving files to a directory which has a file of the same name throws an error", async () => { + mockRepoService.moveFiles.mockRejectedValueOnce(new ConflictError()) + await expect( service.moveFiles(sessionData, mockGithubSessionData, { oldDirectoryName: `${directoryName}/${subcollectionName}`, @@ -279,17 +153,22 @@ describe("Base Directory Service", () => { message, }) ).rejects.toThrowError(ConflictError) - expect(mockGithubService.getTree).toHaveBeenCalledWith( + + expect(mockRepoService.moveFiles).toHaveBeenCalledWith( sessionData, mockGithubSessionData, - { - isRecursive: true, - } + `${directoryName}/${subcollectionName}`, + targetDir, + ["file.md", "file2.md"], + message ) }) - mockGithubService.getTree.mockResolvedValueOnce(mockedTree) - mockGithubService.updateTree.mockResolvedValueOnce(sha) + it("Moving files in directories works correctly", async () => { + mockRepoService.moveFiles.mockResolvedValueOnce({ + newSha: sha, + }) + await expect( service.moveFiles(sessionData, mockGithubSessionData, { oldDirectoryName: `${directoryName}/${subcollectionName}`, @@ -298,26 +177,14 @@ describe("Base Directory Service", () => { message, }) ).resolves.not.toThrow() - expect(mockGithubService.getTree).toHaveBeenCalledWith( - sessionData, - mockGithubSessionData, - { - isRecursive: true, - } - ) - expect(mockGithubService.updateTree).toHaveBeenCalledWith( + + expect(mockRepoService.moveFiles).toHaveBeenCalledWith( sessionData, mockGithubSessionData, - { - gitTree: mockedMovedTree, - message, - } - ) - expect(mockGithubService.updateRepoState).toHaveBeenCalledWith( - sessionData, - { - commitSha: sha, - } + `${directoryName}/${subcollectionName}`, + targetDir, + ["file.md", "file2.md"], + message ) }) }) diff --git a/src/services/fileServices/MdPageServices/MediaFileService.js b/src/services/fileServices/MdPageServices/MediaFileService.js index 56e24d6ec..86e0f128d 100644 --- a/src/services/fileServices/MdPageServices/MediaFileService.js +++ b/src/services/fileServices/MdPageServices/MediaFileService.js @@ -14,8 +14,8 @@ const { isMediaPathValid } = require("@validators/validators") const { getFileExt } = require("@root/utils/files") class MediaFileService { - constructor({ gitHubService }) { - this.gitHubService = gitHubService + constructor({ repoService }) { + this.repoService = repoService } mediaNameChecks({ directoryName, fileName }) { @@ -43,7 +43,7 @@ class MediaFileService { if (!sanitizedContent) { throw new MediaTypeError(`File extension is not within the approved list`) } - const { sha } = await this.gitHubService.create(sessionData, { + const { sha } = await this.repoService.create(sessionData, { content: sanitizedContent, fileName, directoryName, @@ -53,7 +53,7 @@ class MediaFileService { } async read(sessionData, { fileName, directoryName }) { - return this.gitHubService.readMediaFile(sessionData, { + return this.repoService.readMediaFile(sessionData, { fileName, directoryName, }) @@ -65,12 +65,12 @@ class MediaFileService { if (!sanitizedContent) { throw new MediaTypeError(`File extension is not within the approved list`) } - await this.gitHubService.delete(sessionData, { + await this.repoService.delete(sessionData, { sha, fileName, directoryName, }) - const { sha: newSha } = await this.gitHubService.create(sessionData, { + const { sha: newSha } = await this.repoService.create(sessionData, { content: sanitizedContent, fileName, directoryName, @@ -86,7 +86,7 @@ class MediaFileService { async delete(sessionData, { fileName, directoryName, sha }) { this.mediaNameChecks({ directoryName, fileName }) - return this.gitHubService.delete(sessionData, { + return this.repoService.delete(sessionData, { sha, fileName, directoryName, @@ -114,48 +114,18 @@ class MediaFileService { ) } - const gitTree = await this.gitHubService.getTree( + const { newSha: newCommitSha } = await this.repoService.renameSinglePath( sessionData, githubSessionData, - { - isRecursive: true, - } + `${directoryName}/${oldFileName}`, + `${directoryName}/${newFileName}`, + `Renamed ${oldFileName} to ${newFileName}` ) - const newGitTree = [] - gitTree.forEach((item) => { - if (item.path.startsWith(`${directoryName}/`) && item.type !== "tree") { - const fileName = item.path.split(`${directoryName}/`)[1] - if (fileName === oldFileName) { - // Delete old file - newGitTree.push({ - ...item, - sha: null, - }) - // Add file to target directory - newGitTree.push({ - ...item, - path: `${directoryName}/${newFileName}`, - }) - } - } - }) - - const newCommitSha = await this.gitHubService.updateTree( - sessionData, - githubSessionData, - { - gitTree: newGitTree, - message: `Renamed ${oldFileName} to ${newFileName}`, - } - ) - await this.gitHubService.updateRepoState(sessionData, { - commitSha: newCommitSha, - }) return { name: newFileName, oldSha: sha, - sha, + sha: newCommitSha, } } } diff --git a/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js b/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js index 9ccf570b3..ba890e8fa 100644 --- a/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js +++ b/src/services/fileServices/MdPageServices/__tests__/MediaFileService.spec.js @@ -19,7 +19,7 @@ describe("Media File Service", () => { const sessionData = { siteName, accessToken } - const mockGithubService = { + const mockRepoService = { create: jest.fn(), read: jest.fn(), update: jest.fn(), @@ -28,6 +28,7 @@ describe("Media File Service", () => { readMedia: jest.fn(), readMediaFile: jest.fn(), readDirectory: jest.fn(), + renameSinglePath: jest.fn(), getTree: jest.fn(), updateTree: jest.fn(), updateRepoState: jest.fn(), @@ -46,7 +47,7 @@ describe("Media File Service", () => { } = require("@services/fileServices/MdPageServices/MediaFileService") const service = new MediaFileService({ - gitHubService: mockGithubService, + repoService: mockRepoService, }) const { validateAndSanitizeFileUpload } = require("@utils/file-upload-utils") @@ -65,7 +66,7 @@ describe("Media File Service", () => { ).rejects.toThrowError(BadRequestError) }) - mockGithubService.create.mockResolvedValueOnce({ sha }) + mockRepoService.create.mockResolvedValueOnce({ sha }) it("Creating pages works correctly", async () => { await expect( service.create(sessionData, { @@ -78,7 +79,7 @@ describe("Media File Service", () => { content: mockContent, sha, }) - expect(mockGithubService.create).toHaveBeenCalledWith(sessionData, { + expect(mockRepoService.create).toHaveBeenCalledWith(sessionData, { content: mockSanitizedContent, fileName, directoryName, @@ -90,34 +91,6 @@ describe("Media File Service", () => { describe("Read", () => { // TODO: file tests when file handling is implemented - const imageDirResp = [ - { - name: imageName, - path: `${directoryName}/${imageName}`, - sha, - }, - { - name: "image2.png", - path: `${directoryName}/image2.png`, - sha: "image2sha", - }, - ] - const fileDirResp = [ - { - name: fileName, - path: `${directoryName}/${fileName}`, - sha, - }, - { - name: "file2.pdf", - path: `${directoryName}/file2.pdf`, - sha: "file2sha", - }, - ] - - mockGithubService.readMedia.mockResolvedValueOnce({ - content: mockContent, - }) it("Reading image files in public repos works correctly", async () => { const expectedResp = { mediaUrl: `https://raw.githubusercontent.com/${GITHUB_ORG_NAME}/${siteName}/staging/${directoryName}/${imageEncodedName}`, @@ -125,20 +98,17 @@ describe("Media File Service", () => { sha, } - mockGithubService.readMediaFile.mockResolvedValueOnce(expectedResp) + mockRepoService.readMediaFile.mockResolvedValueOnce(expectedResp) await expect( service.read(sessionData, { fileName: imageName, directoryName, }) ).resolves.toMatchObject(expectedResp) - expect(mockGithubService.readMediaFile).toHaveBeenCalledWith( - sessionData, - { - fileName: imageName, - directoryName, - } - ) + expect(mockRepoService.readMediaFile).toHaveBeenCalledWith(sessionData, { + fileName: imageName, + directoryName, + }) }) const svgName = "image.svg" @@ -149,7 +119,7 @@ describe("Media File Service", () => { sha, } - mockGithubService.readMediaFile.mockResolvedValueOnce(expectedResp) + mockRepoService.readMediaFile.mockResolvedValueOnce(expectedResp) await expect( service.read(sessionData, { fileName: svgName, @@ -165,7 +135,7 @@ describe("Media File Service", () => { sha, } - mockGithubService.readMediaFile.mockResolvedValueOnce(expectedResp) + mockRepoService.readMediaFile.mockResolvedValueOnce(expectedResp) await expect( service.read(sessionData, { @@ -174,19 +144,16 @@ describe("Media File Service", () => { mediaType: "files", }) ).resolves.toMatchObject(expectedResp) - expect(mockGithubService.readMediaFile).toHaveBeenCalledWith( - sessionData, - { - fileName, - directoryName, - } - ) + expect(mockRepoService.readMediaFile).toHaveBeenCalledWith(sessionData, { + fileName, + directoryName, + }) }) }) describe("Update", () => { const oldSha = "54321" - mockGithubService.create.mockResolvedValueOnce({ sha }) + mockRepoService.create.mockResolvedValueOnce({ sha }) it("Updating media file content works correctly", async () => { await expect( service.update(sessionData, { @@ -201,12 +168,12 @@ describe("Media File Service", () => { oldSha, newSha: sha, }) - expect(mockGithubService.delete).toHaveBeenCalledWith(sessionData, { + expect(mockRepoService.delete).toHaveBeenCalledWith(sessionData, { fileName, directoryName, sha: oldSha, }) - expect(mockGithubService.create).toHaveBeenCalledWith(sessionData, { + expect(mockRepoService.create).toHaveBeenCalledWith(sessionData, { content: mockSanitizedContent, fileName, directoryName, @@ -221,7 +188,7 @@ describe("Media File Service", () => { await expect( service.delete(sessionData, { fileName, directoryName, sha }) ).resolves.not.toThrow() - expect(mockGithubService.delete).toHaveBeenCalledWith(sessionData, { + expect(mockRepoService.delete).toHaveBeenCalledWith(sessionData, { fileName, directoryName, sha, @@ -231,33 +198,6 @@ describe("Media File Service", () => { describe("Rename", () => { const oldFileName = "test old file.pdf" - const treeSha = "treesha" - const mockedTree = [ - { - type: "file", - path: `${directoryName}/${oldFileName}`, - sha, - }, - { - type: "file", - path: `${directoryName}/file.md`, - sha: "sha1", - }, - ] - const mockedMovedTree = [ - { - type: "file", - path: `${directoryName}/${oldFileName}`, - sha: null, - }, - { - type: "file", - path: `${directoryName}/${fileName}`, - sha, - }, - ] - mockGithubService.getTree.mockResolvedValueOnce(mockedTree) - mockGithubService.updateTree.mockResolvedValueOnce(treeSha) it("rejects renaming to page names with special characters", async () => { await expect( @@ -270,6 +210,10 @@ describe("Media File Service", () => { ).rejects.toThrowError(BadRequestError) }) it("Renaming pages works correctly", async () => { + mockRepoService.renameSinglePath.mockResolvedValueOnce({ + newSha: sha, + }) + await expect( service.rename(sessionData, mockGithubSessionData, { oldFileName, @@ -283,26 +227,13 @@ describe("Media File Service", () => { oldSha: sha, sha, }) - expect(mockGithubService.getTree).toHaveBeenCalledWith( - sessionData, - mockGithubSessionData, - { - isRecursive: true, - } - ) - expect(mockGithubService.updateTree).toHaveBeenCalledWith( + + expect(mockRepoService.renameSinglePath).toHaveBeenCalledWith( sessionData, mockGithubSessionData, - { - gitTree: mockedMovedTree, - message: `Renamed ${oldFileName} to ${fileName}`, - } - ) - expect(mockGithubService.updateRepoState).toHaveBeenCalledWith( - sessionData, - { - commitSha: treeSha, - } + `${directoryName}/${oldFileName}`, + `${directoryName}/${fileName}`, + `Renamed ${oldFileName} to ${fileName}` ) }) })