From 218a4538c16f45eb73c96550b94707dec272ede5 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Wed, 29 May 2024 12:07:40 +0200 Subject: [PATCH] [web] support for reverting binary files (#18033) * [web] revert binary file * use addEntityWithName if file was deleted * todo comments * only show Revert file in ui even if deleted * use _revertBinaryFile function * emit new ids when reverting * format:fix * await emitToRoom calls * use EditorController.upsertFile * remove _revertBinaryFile function * binary file check * mock importFile method in tests * move findElementByPath stub * debug ci error * resolve with empty object as file * fix tests * remove await before expect() * format:fix * test when binary file exists and when it does not * use "file-revert" for source * [web] revert existing file without ranges support (#18107) * [web] revert existing file without ranges support * ignore document_updated_externally if file-revert * fix test GitOrigin-RevId: a5e0c83a7635bc7d934dec9debe916bdd4beb51e --- .../src/Features/History/RestoreManager.js | 53 ++++++++---- .../features/history/services/types/shared.ts | 2 +- .../context/editor-manager-context.tsx | 3 + .../frontend/js/ide/editor/EditorManager.js | 3 + .../unit/src/History/RestoreManagerTests.js | 85 ++++++++++++++----- 5 files changed, 106 insertions(+), 40 deletions(-) diff --git a/services/web/app/src/Features/History/RestoreManager.js b/services/web/app/src/Features/History/RestoreManager.js index dbde81a452..a2401a5c6e 100644 --- a/services/web/app/src/Features/History/RestoreManager.js +++ b/services/web/app/src/Features/History/RestoreManager.js @@ -8,6 +8,7 @@ const moment = require('moment') const { callbackifyAll } = require('@overleaf/promise-utils') const { fetchJson } = require('@overleaf/fetch-utils') const ProjectLocator = require('../Project/ProjectLocator') +const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandler') const RestoreManager = { async restoreFileFromV2(userId, projectId, version, pathname) { @@ -42,6 +43,7 @@ const RestoreManager = { }, async revertFile(userId, projectId, version, pathname) { + const source = 'file-revert' const fsPath = await RestoreManager._writeFileVersionToDisk( projectId, version, @@ -50,34 +52,53 @@ const RestoreManager = { const basename = Path.basename(pathname) let dirname = Path.dirname(pathname) if (dirname === '.') { - // no directory - dirname = '' + // root directory + dirname = '/' } const parentFolderId = await RestoreManager._findOrCreateFolder( projectId, dirname ) - let fileExists = true - try { - // TODO: Is there a better way of doing this? - await ProjectLocator.promises.findElementByPath({ - projectId, + const file = await ProjectLocator.promises + .findElementByPath({ + project_id: projectId, path: pathname, }) - } catch (error) { - fileExists = false - } - if (fileExists) { - throw new Errors.InvalidError('File already exists') - } + .catch(() => null) const importInfo = await FileSystemImportManager.promises.importFile( fsPath, pathname ) - if (importInfo.type !== 'doc') { - // TODO: Handle binary files - throw new Errors.InvalidError('File is not editable') + if (importInfo.type === 'file') { + const newFile = await EditorController.promises.upsertFile( + projectId, + parentFolderId, + basename, + fsPath, + file?.element?.linkedFileData, + source, + userId + ) + + return { + _id: newFile._id, + type: importInfo.type, + } + } + + if (file) { + await DocumentUpdaterHandler.promises.setDocument( + projectId, + file.element._id, + userId, + importInfo.lines, + source + ) + return { + _id: file.element._id, + type: importInfo.type, + } } const ranges = await RestoreManager._getRangesFromHistory( diff --git a/services/web/frontend/js/features/history/services/types/shared.ts b/services/web/frontend/js/features/history/services/types/shared.ts index 47785cd139..90962db115 100644 --- a/services/web/frontend/js/features/history/services/types/shared.ts +++ b/services/web/frontend/js/features/history/services/types/shared.ts @@ -12,7 +12,7 @@ export interface Meta { start_ts: number end_ts: number type?: 'external' // TODO - source?: 'git-bridge' // TODO + source?: 'git-bridge' | 'file-revert' // TODO origin?: { kind: | 'dropbox' diff --git a/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx b/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx index bc2ce5a673..f90e7c1961 100644 --- a/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx +++ b/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx @@ -283,6 +283,9 @@ export const EditorManagerProvider: FC = ({ children }) => { ) { return } + if (update.meta.source === 'file-revert') { + return + } showGenericMessageModal( t('document_updated_externally'), t('document_updated_externally_detail') diff --git a/services/web/frontend/js/ide/editor/EditorManager.js b/services/web/frontend/js/ide/editor/EditorManager.js index 49ae7a4517..2f3abb7b24 100644 --- a/services/web/frontend/js/ide/editor/EditorManager.js +++ b/services/web/frontend/js/ide/editor/EditorManager.js @@ -442,6 +442,9 @@ export default EditorManager = (function () { ) { return } + if (update?.meta?.source === 'file-revert') { + return + } return this.ide.showGenericMessageModal( 'Document Updated Externally', 'This document was just updated externally. Any recent changes you have made may have been overwritten. To see previous versions please look in the history.' diff --git a/services/web/test/unit/src/History/RestoreManagerTests.js b/services/web/test/unit/src/History/RestoreManagerTests.js index ec7ed71075..96d198a4f0 100644 --- a/services/web/test/unit/src/History/RestoreManagerTests.js +++ b/services/web/test/unit/src/History/RestoreManagerTests.js @@ -23,6 +23,8 @@ describe('RestoreManager', function () { promises: {}, }), '../Project/ProjectLocator': (this.ProjectLocator = { promises: {} }), + '../DocumentUpdater/DocumentUpdaterHandler': + (this.DocumentUpdaterHandler = { promises: {} }), }, }) this.user_id = 'mock-user-id' @@ -217,41 +219,78 @@ describe('RestoreManager', function () { describe('with an existing file in the current project', function () { beforeEach(function () { this.pathname = 'foo.tex' - this.ProjectLocator.promises.findElementByPath = sinon.stub().resolves() + this.FileSystemImportManager.promises.importFile = sinon + .stub() + .resolves({ type: 'doc' }) + this.ProjectLocator.promises.findElementByPath = sinon + .stub() + .resolves({ type: 'doc', element: { _id: 'mock-file-id' } }) + this.FileSystemImportManager.promises.importFile = sinon + .stub() + .resolves({ type: 'doc', lines: ['foo', 'bar', 'baz'] }) + this.DocumentUpdaterHandler.promises.setDocument = sinon + .stub() + .resolves() }) - it('should reject', function () { - expect( - this.RestoreManager.promises.revertFile( - this.user_id, - this.project_id, - this.version, - this.pathname - ) + it('should call setDocument in document updater and revert file', async function () { + const revertRes = await this.RestoreManager.promises.revertFile( + this.user_id, + this.project_id, + this.version, + this.pathname ) - .to.eventually.be.rejectedWith('File already exists') - .and.be.instanceOf(Errors.InvalidError) + + expect( + this.DocumentUpdaterHandler.promises.setDocument + ).to.have.been.calledWith( + this.project_id, + 'mock-file-id', + this.user_id, + ['foo', 'bar', 'baz'], + 'file-revert' + ) + expect(revertRes).to.deep.equal({ _id: 'mock-file-id', type: 'doc' }) }) }) describe('when reverting a binary file', function () { - beforeEach(function () { + beforeEach(async function () { this.pathname = 'foo.png' this.FileSystemImportManager.promises.importFile = sinon .stub() - .resolves({ type: 'binary' }) + .resolves({ type: 'file' }) + this.EditorController.promises.upsertFile = sinon + .stub() + .resolves({ _id: 'mock-file-id', type: 'file' }) }) - it('should reject', function () { - expect( - this.RestoreManager.promises.revertFile( - this.user_id, - this.project_id, - this.version, - this.pathname - ) + + it('should return the created entity if file exists', async function () { + this.ProjectLocator.promises.findElementByPath = sinon + .stub() + .resolves({ type: 'file' }) + + const revertRes = await this.RestoreManager.promises.revertFile( + this.user_id, + this.project_id, + this.version, + this.pathname ) - .to.eventually.be.rejectedWith('File is not editable') - .and.be.instanceOf(Errors.InvalidError) + + expect(revertRes).to.deep.equal({ _id: 'mock-file-id', type: 'file' }) + }) + + it('should return the created entity if file does not exists', async function () { + this.ProjectLocator.promises.findElementByPath = sinon.stub().rejects() + + const revertRes = await this.RestoreManager.promises.revertFile( + this.user_id, + this.project_id, + this.version, + this.pathname + ) + + expect(revertRes).to.deep.equal({ _id: 'mock-file-id', type: 'file' }) }) })