From d7357b4d62cd0726b244837771a166d4d93ebd39 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Wed, 24 Jul 2024 14:38:51 +0100 Subject: [PATCH] Merge pull request #19400 from overleaf/dp-duplicate-file-folder-name Improvements to handling of file/folder upload conflicts GitOrigin-RevId: 526edf30dfbaec7ee1e03ffd156365f09be25e86 --- .../src/Features/Errors/ErrorController.js | 6 + .../web/app/src/Features/Errors/Errors.js | 3 + .../src/Features/History/RestoreManager.js | 4 +- .../ProjectEntityMongoUpdateHandler.js | 2 +- .../ThirdPartyDataStore/TpdsController.js | 5 +- .../Uploads/ProjectUploadController.js | 6 + .../web/frontend/extracted-translations.json | 2 + .../file-tree-create/error-message.jsx | 7 + .../file-tree-upload-conflicts.tsx | 120 ++++++++++-------- .../modes/file-tree-upload-doc.tsx | 108 +++++++++++----- .../util/is-name-unique-in-folder.ts | 24 +++- services/web/locales/en.json | 2 + .../unit/src/History/RestoreManagerTests.js | 4 +- .../ProjectEntityMongoUpdateHandlerTests.js | 4 +- 14 files changed, 203 insertions(+), 94 deletions(-) diff --git a/services/web/app/src/Features/Errors/ErrorController.js b/services/web/app/src/Features/Errors/ErrorController.js index 4b4bc9a5dd..a23b6521c7 100644 --- a/services/web/app/src/Features/Errors/ErrorController.js +++ b/services/web/app/src/Features/Errors/ErrorController.js @@ -65,6 +65,12 @@ async function handleError(error, req, res, next) { res.status(400) plainTextResponse(res, error.message) } + } else if (error instanceof Errors.DuplicateNameError) { + req.logger.setLevel('warn') + if (shouldSendErrorResponse) { + res.status(400) + plainTextResponse(res, error.message) + } } else if (error instanceof Errors.InvalidNameError) { req.logger.setLevel('warn') if (shouldSendErrorResponse) { diff --git a/services/web/app/src/Features/Errors/Errors.js b/services/web/app/src/Features/Errors/Errors.js index a047a8a731..ed504836fa 100644 --- a/services/web/app/src/Features/Errors/Errors.js +++ b/services/web/app/src/Features/Errors/Errors.js @@ -38,6 +38,8 @@ class ServiceNotConfiguredError extends BackwardCompatibleError {} class TooManyRequestsError extends BackwardCompatibleError {} +class DuplicateNameError extends OError {} + class InvalidNameError extends BackwardCompatibleError {} class UnsupportedFileTypeError extends BackwardCompatibleError {} @@ -270,6 +272,7 @@ module.exports = { ForbiddenError, ServiceNotConfiguredError, TooManyRequestsError, + DuplicateNameError, InvalidNameError, UnsupportedFileTypeError, FileTooLargeError, diff --git a/services/web/app/src/Features/History/RestoreManager.js b/services/web/app/src/Features/History/RestoreManager.js index 95a0a1c9f3..6655e00e3e 100644 --- a/services/web/app/src/Features/History/RestoreManager.js +++ b/services/web/app/src/Features/History/RestoreManager.js @@ -222,8 +222,8 @@ const RestoreManager = { try { return await addEntityWithName(basename) } catch (error) { - if (error instanceof Errors.InvalidNameError) { - // likely a duplicate name, so try with a prefix + if (error instanceof Errors.DuplicateNameError) { + // Duplicate name, so try with a prefix const date = moment(new Date()).format('Do MMM YY H:mm:ss') // Move extension to the end so the file type is preserved const extension = Path.extname(basename) diff --git a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js index cf543a3a4e..73f7d57ada 100644 --- a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js @@ -618,7 +618,7 @@ function _checkValidElementName(folder, name) { .concat(folder.folders || []) for (const element of elements) { if (element.name === name) { - throw new Errors.InvalidNameError('file already exists') + throw new Errors.DuplicateNameError('file already exists') } } } diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js index b4617a3d44..adcef05407 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js @@ -146,7 +146,10 @@ async function updateProjectContents(req, res, next) { try { await UpdateMerger.promises.mergeUpdate(null, projectId, path, req, source) } catch (error) { - if (error.constructor === Errors.InvalidNameError) { + if ( + error instanceof Errors.InvalidNameError || + error instanceof Errors.DuplicateNameError + ) { return res.sendStatus(422) } else { throw error diff --git a/services/web/app/src/Features/Uploads/ProjectUploadController.js b/services/web/app/src/Features/Uploads/ProjectUploadController.js index d0429b24e9..15f6f6004d 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadController.js +++ b/services/web/app/src/Features/Uploads/ProjectUploadController.js @@ -12,6 +12,7 @@ const { InvalidZipFileError } = require('./ArchiveErrors') const multer = require('multer') const { defaultsDeep } = require('lodash') const { expressify } = require('@overleaf/promise-utils') +const { DuplicateNameError } = require('../Errors/Errors') const upload = multer( defaultsDeep( @@ -107,6 +108,11 @@ async function uploadFile(req, res, next) { success: false, error: 'invalid_filename', }) + } else if (error instanceof DuplicateNameError) { + return res.status(422).json({ + success: false, + error: 'duplicate_file_name', + }) } else if (error.message === 'project_has_too_many_files') { return res.status(422).json({ success: false, diff --git a/services/web/frontend/extracted-translations.json b/services/web/frontend/extracted-translations.json index cb55a49dd3..69356c578b 100644 --- a/services/web/frontend/extracted-translations.json +++ b/services/web/frontend/extracted-translations.json @@ -428,6 +428,7 @@ "file_name_figure_modal": "", "file_name_in_this_project": "", "file_name_in_this_project_figure_modal": "", + "file_or_folder_name_already_exists": "", "file_outline": "", "file_size": "", "files_cannot_include_invalid_characters": "", @@ -1385,6 +1386,7 @@ "thanks_for_subscribing_you_help_sl": "", "thanks_settings_updated": "", "the_following_files_already_exist_in_this_project": "", + "the_following_files_and_folders_already_exist_in_this_project": "", "the_following_folder_already_exists_in_this_project": "", "the_following_folder_already_exists_in_this_project_plural": "", "the_original_text_has_changed": "", diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-create/error-message.jsx b/services/web/frontend/js/features/file-tree/components/file-tree-create/error-message.jsx index ea618d48a1..0c9ab381c6 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-create/error-message.jsx +++ b/services/web/frontend/js/features/file-tree/components/file-tree-create/error-message.jsx @@ -42,6 +42,13 @@ export default function ErrorMessage({ error }) { ) + case 'duplicate_file_name': + return ( + + {t('file_or_folder_name_already_exists')} + + ) + case 'rate-limit-hit': return ( diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-upload-conflicts.tsx b/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-upload-conflicts.tsx index e3ba6fa590..814e8a164b 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-upload-conflicts.tsx +++ b/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-upload-conflicts.tsx @@ -4,90 +4,110 @@ import { useProjectContext } from '@/shared/context/project-context' import { useCallback } from 'react' import { syncDelete } from '@/features/file-tree/util/sync-mutation' import { Button } from 'react-bootstrap' +import { TFunction } from 'i18next' -export function UploadConflicts({ +export type Conflict = { + entity: FileTreeEntity + type: 'file' | 'folder' +} + +const getConflictText = (conflicts: Conflict[], t: TFunction) => { + const hasFolderConflict = conflicts.some( + conflict => conflict.type === 'folder' + ) + + const hasFileConflict = conflicts.some(conflict => conflict.type === 'file') + + if (hasFolderConflict && hasFileConflict) { + return t('the_following_files_and_folders_already_exist_in_this_project') + } + + if (hasFolderConflict) { + return t('the_following_folder_already_exists_in_this_project', { + count: conflicts.length, + }) + } + + return t('the_following_files_already_exist_in_this_project') +} + +export function FileUploadConflicts({ cancel, conflicts, - folderConflicts, handleOverwrite, - setError, }: { cancel: () => void - conflicts: FileTreeEntity[] - folderConflicts: FileTreeEntity[] + conflicts: Conflict[] handleOverwrite: () => void - setError: (error: string) => void }) { const { t } = useTranslation() - // ensure that no uploads happen while there are folder conflicts - if (folderConflicts.length > 0) { - return ( - - ) - } + // Don't allow overwriting folders with files + const hasFolderConflict = conflicts.some( + conflict => conflict.type === 'folder' + ) return (
{conflicts.length > 0 && ( <> -

- {t('the_following_files_already_exist_in_this_project')} -

+

{getConflictText(conflicts, t)}

    {conflicts.map((conflict, index) => (
  • - {conflict.name} + {conflict.entity.name}
    • ))}
)} -

- {t('do_you_want_to_overwrite_them')} -

+ {!hasFolderConflict && ( +

+ {t('do_you_want_to_overwrite_them')} +

+ )}

  - + {!hasFolderConflict && ( + + )}

) } -function FolderUploadConflicts({ +export function FolderUploadConflicts({ cancel, handleOverwrite, - folderConflicts, + conflicts, setError, }: { cancel: () => void handleOverwrite: () => void - folderConflicts: FileTreeEntity[] + conflicts: Conflict[] setError: (error: string) => void }) { const { t } = useTranslation() const { _id: projectId } = useProjectContext() + // Don't allow overwriting files with a folder + const hasFileConflict = conflicts.some(conflict => conflict.type === 'file') + const deleteAndRetry = useCallback(async () => { // TODO: confirm deletion? try { await Promise.all( - folderConflicts.map( - entity => syncDelete(projectId, 'folder', entity._id) // TODO: might be a file! + conflicts.map(conflict => + syncDelete(projectId, 'folder', conflict.entity._id) ) ) @@ -95,40 +115,40 @@ function FolderUploadConflicts({ } catch (error: any) { setError(error.message) } - }, [setError, folderConflicts, handleOverwrite, projectId]) + }, [setError, conflicts, handleOverwrite, projectId]) return (
-

- {t('the_following_folder_already_exists_in_this_project', { - count: folderConflicts.length, - })} -

+

{getConflictText(conflicts, t)}

    - {folderConflicts.map((entity, index) => ( + {conflicts.map((conflict, index) => (
  • - {entity.name} + {conflict.entity.name}
    • ))}
-

- {t('overwriting_the_original_folder')} -
- {t('do_you_want_to_overwrite_it', { - count: folderConflicts.length, - })} -

+ {!hasFileConflict && ( +

+ {t('overwriting_the_original_folder')} +
+ {t('do_you_want_to_overwrite_it', { + count: conflicts.length, + })} +

+ )}

  - + {!hasFileConflict && ( + + )}

) diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-create/modes/file-tree-upload-doc.tsx b/services/web/frontend/js/features/file-tree/components/file-tree-create/modes/file-tree-upload-doc.tsx index 82c6060085..7ec8277923 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-create/modes/file-tree-upload-doc.tsx +++ b/services/web/frontend/js/features/file-tree/components/file-tree-create/modes/file-tree-upload-doc.tsx @@ -12,10 +12,16 @@ import { refreshProjectMetadata } from '../../../util/api' import ErrorMessage from '../error-message' import { debugConsole } from '@/utils/debugging' import { isAcceptableFile } from '@/features/file-tree/util/is-acceptable-file' -import { findByNameInFolder } from '@/features/file-tree/util/is-name-unique-in-folder' +import { + findFileByNameInFolder, + findFolderByNameInFolder, +} from '@/features/file-tree/util/is-name-unique-in-folder' import { useFileTreeData } from '@/shared/context/file-tree-data-context' -import { FileTreeEntity } from '../../../../../../../types/file-tree-entity' -import { UploadConflicts } from '@/features/file-tree/components/file-tree-create/file-tree-upload-conflicts' +import { + Conflict, + FileUploadConflicts, + FolderUploadConflicts, +} from '@/features/file-tree/components/file-tree-create/file-tree-upload-conflicts' import getMeta from '@/utils/meta' export default function FileTreeUploadDoc() { @@ -26,8 +32,8 @@ export default function FileTreeUploadDoc() { const [error, setError] = useState() - const [conflicts, setConflicts] = useState([]) - const [folderConflicts, setFolderConflicts] = useState([]) + const [conflicts, setConflicts] = useState([]) + const [folderConflicts, setFolderConflicts] = useState([]) const [overwrite, setOverwrite] = useState(false) const maxNumberOfFiles = 180 @@ -35,46 +41,79 @@ export default function FileTreeUploadDoc() { // calculate conflicts const buildConflicts = (files: Record) => { - const conflicts = new Set() + const conflicts: Conflict[] = [] for (const file of Object.values(files)) { const { name, relativePath } = file.meta if (!relativePath) { const targetFolderId = file.meta.targetFolderId ?? parentFolderId - const duplicate = findByNameInFolder(fileTreeData, targetFolderId, name) - if (duplicate) { - conflicts.add(duplicate) + const duplicateFile = findFileByNameInFolder( + fileTreeData, + targetFolderId, + name + ) + if (duplicateFile) { + conflicts.push({ + entity: duplicateFile, + type: 'file', + }) + } + + const duplicateFolder = findFolderByNameInFolder( + fileTreeData, + targetFolderId, + name + ) + if (duplicateFolder) { + conflicts.push({ + entity: duplicateFolder, + type: 'folder', + }) } } } - return [...conflicts] + return conflicts } const buildFolderConflicts = (files: Record) => { - const conflicts = new Set() + const conflicts: Conflict[] = [] for (const file of Object.values(files)) { const { relativePath } = file.meta if (relativePath) { const [rootName] = relativePath.replace(/^\//, '').split('/') - if (!conflicts.has(rootName)) { - const targetFolderId = file.meta.targetFolderId ?? parentFolderId - const duplicateEntity = findByNameInFolder( - fileTreeData, - targetFolderId, - rootName - ) - if (duplicateEntity) { - conflicts.add(duplicateEntity) - } + + const targetFolderId = file.meta.targetFolderId ?? parentFolderId + const duplicateFile = findFileByNameInFolder( + fileTreeData, + targetFolderId, + rootName + ) + if (duplicateFile) { + conflicts.push({ + entity: duplicateFile, + type: 'file', + }) + } + + const duplicateFolder = findFolderByNameInFolder( + fileTreeData, + targetFolderId, + rootName + ) + if (duplicateFolder) { + conflicts.push({ + entity: duplicateFolder, + type: 'folder', + }) } } } - return [...conflicts] + return conflicts } const buildEndpoint = (projectId: string, targetFolderId: string) => { @@ -214,25 +253,32 @@ export default function FileTreeUploadDoc() { uppy.upload() }, [uppy]) - // whether to show a message about conflicting files - const showConflicts = - !overwrite && (conflicts.length > 0 || folderConflicts.length > 0) + const showFolderUploadConflicts = !overwrite && folderConflicts.length > 0 + const showFileUploadConfilcts = + !overwrite && !showFolderUploadConflicts && conflicts.length > 0 + const showDashboard = !showFileUploadConfilcts && !showFolderUploadConflicts return ( <> {error && ( )} - - {showConflicts ? ( - - ) : ( + )} + {showFileUploadConfilcts && ( + + )} + {showDashboard && ( entity.name === name) || - tree.fileRefs.find(entity => entity.name === name) || - tree.folders.find(entity => entity.name === name) + tree.fileRefs.find(entity => entity.name === name) ) } + +export function findFolderByNameInFolder( + tree: Folder, + parentFolderId: string, + name: string +): Folder | undefined { + if (tree._id !== parentFolderId) { + tree = findInTree(tree, parentFolderId).entity + } + + return tree.folders.find(entity => entity.name === name) +} diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 33ff0fe34b..34b4fa965d 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -634,6 +634,7 @@ "file_name_figure_modal": "File name", "file_name_in_this_project": "File Name In This Project", "file_name_in_this_project_figure_modal": "File name in this project", + "file_or_folder_name_already_exists": "A file or folder with this name already exists", "file_outline": "File outline", "file_size": "File size", "file_too_large": "File too large", @@ -1990,6 +1991,7 @@ "thanks_settings_updated": "Thanks, your settings have been updated.", "the_file_supplied_is_of_an_unsupported_type ": "The link to open this content on Overleaf pointed to the wrong kind of file. Valid file types are .tex documents and .zip files. If this keeps happening for links on a particular site, please report this to them.", "the_following_files_already_exist_in_this_project": "The following files already exist in this project:", + "the_following_files_and_folders_already_exist_in_this_project": "The following files and folders already exist in this project:", "the_following_folder_already_exists_in_this_project": "The following folder already exists in this project:", "the_following_folder_already_exists_in_this_project_plural": "The following folders already exist in this project:", "the_original_text_has_changed": "The original text has changed, so this suggestion can’t be applied", diff --git a/services/web/test/unit/src/History/RestoreManagerTests.js b/services/web/test/unit/src/History/RestoreManagerTests.js index 96d3156ac3..a1c3fa26df 100644 --- a/services/web/test/unit/src/History/RestoreManagerTests.js +++ b/services/web/test/unit/src/History/RestoreManagerTests.js @@ -180,9 +180,9 @@ describe('RestoreManager', function () { }) }) - describe('with an invalid name', function () { + describe('with a duplicate name', function () { beforeEach(async function () { - this.addEntityWithName.rejects(new Errors.InvalidNameError()) + this.addEntityWithName.rejects(new Errors.DuplicateNameError()) this.addEntityWithName .onSecondCall() .resolves((this.entity = 'mock-entity')) diff --git a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js index 8645c6f9fe..1464ed3463 100644 --- a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js @@ -856,7 +856,7 @@ describe('ProjectEntityMongoUpdateHandler', function () { 'doc', this.folder.name ) - ).to.be.rejectedWith(Errors.InvalidNameError) + ).to.be.rejectedWith(Errors.DuplicateNameError) }) }) }) @@ -990,7 +990,7 @@ describe('ProjectEntityMongoUpdateHandler', function () { file, 'file' ) - ).to.be.rejectedWith(Errors.InvalidNameError) + ).to.be.rejectedWith(Errors.DuplicateNameError) }) }) })