From 3342d672c2d126a4d25f30fa307d862c581f602d Mon Sep 17 00:00:00 2001 From: Christopher Hoskin Date: Thu, 16 May 2024 18:39:31 +0100 Subject: [PATCH] Merge pull request #18397 from overleaf/em-revert-download-all-link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert "Merge pull request #18190 from overleaf/ar-add-download-all-l… GitOrigin-RevId: 681eb2734636d76558e682dc85083bfcaa6b7d2d --- services/clsi/app/js/CompileController.js | 44 ++-------- services/clsi/app/js/CompileManager.js | 16 ++-- services/clsi/app/js/OutputCacheManager.js | 8 +- .../test/unit/js/CompileControllerTests.js | 88 +++++-------------- .../clsi/test/unit/js/CompileManagerTests.js | 5 +- .../src/Features/Compile/CompileController.js | 32 ++----- .../web/frontend/extracted-translations.json | 1 - .../pdf-preview/components/pdf-file-list.jsx | 16 ---- .../js/features/pdf-preview/util/file-list.js | 14 +-- services/web/locales/en.json | 1 - 10 files changed, 50 insertions(+), 175 deletions(-) diff --git a/services/clsi/app/js/CompileController.js b/services/clsi/app/js/CompileController.js index cc99cf1e54..2c893226d8 100644 --- a/services/clsi/app/js/CompileController.js +++ b/services/clsi/app/js/CompileController.js @@ -12,17 +12,6 @@ function timeSinceLastSuccessfulCompile() { return Date.now() - lastSuccessfulCompileTimestamp } -function addUrlToOutputFile(outputFile, projectId, userId) { - return { - url: - `${Settings.apis.clsi.url}/project/${projectId}` + - (userId != null ? `/user/${userId}` : '') + - (outputFile.build != null ? `/build/${outputFile.build}` : '') + - `/output/${outputFile.path}`, - ...outputFile, - } -} - function compile(req, res, next) { const timer = new Metrics.Timer('compile-request') RequestParser.parse(req.body, function (error, request) { @@ -41,7 +30,7 @@ function compile(req, res, next) { return next(error) } CompileManager.doCompileWithLock(request, (error, result) => { - let { buildId, outputFiles, stats, timings } = result || {} + let { outputFiles, stats, timings } = result || {} let code, status if (outputFiles == null) { outputFiles = [] @@ -109,7 +98,6 @@ function compile(req, res, next) { if (error) { outputFiles = error.outputFiles || [] - buildId = error.buildId } timer.done() @@ -120,28 +108,14 @@ function compile(req, res, next) { stats, timings, outputUrlPrefix: Settings.apis.clsi.outputUrlPrefix, - outputFiles: - outputFiles.length === 0 - ? [] - : outputFiles - .map(file => - addUrlToOutputFile( - file, - request.project_id, - request.user_id - ) - ) - .concat( - addUrlToOutputFile( - { - build: buildId, - path: 'output.zip', - type: 'zip', - }, - request.project_id, - request.user_id - ) - ), + outputFiles: outputFiles.map(file => ({ + url: + `${Settings.apis.clsi.url}/project/${request.project_id}` + + (request.user_id != null ? `/user/${request.user_id}` : '') + + (file.build != null ? `/build/${file.build}` : '') + + `/output/${file.path}`, + ...file, + })), }, }) }) diff --git a/services/clsi/app/js/CompileManager.js b/services/clsi/app/js/CompileManager.js index 9d1981263d..2a685d3e86 100644 --- a/services/clsi/app/js/CompileManager.js +++ b/services/clsi/app/js/CompileManager.js @@ -208,7 +208,7 @@ async function doCompile(request) { Metrics.inc('compiles-timeout', 1, request.metricsOpts) } - const { outputFiles, allEntries, buildId } = await _saveOutputFiles({ + const { outputFiles, allEntries } = await _saveOutputFiles({ request, compileDir, resourceList, @@ -216,7 +216,7 @@ async function doCompile(request) { timings, }) error.outputFiles = outputFiles // return output files so user can check logs - error.buildId = buildId + // Clear project if this compile was abruptly terminated if (error.terminated || error.timedout) { await clearProjectWithListing( @@ -280,7 +280,7 @@ async function doCompile(request) { // Emit compile time. timings.compile = ts - const { outputFiles, buildId } = await _saveOutputFiles({ + const { outputFiles } = await _saveOutputFiles({ request, compileDir, resourceList, @@ -296,7 +296,7 @@ async function doCompile(request) { emitPdfStats(stats, timings, request) } - return { outputFiles, stats, timings, buildId } + return { outputFiles, stats, timings } } async function _saveOutputFiles({ @@ -316,24 +316,20 @@ async function _saveOutputFiles({ let { outputFiles, allEntries } = await OutputFileFinder.promises.findOutputFiles(resourceList, compileDir) - let buildId - try { - const saveResult = await OutputCacheManager.promises.saveOutputFiles( + outputFiles = await OutputCacheManager.promises.saveOutputFiles( { request, stats, timings }, outputFiles, compileDir, outputDir ) - buildId = saveResult.buildId - outputFiles = saveResult.outputFiles } catch (err) { const { project_id: projectId, user_id: userId } = request logger.err({ projectId, userId, err }, 'failed to save output files') } timings.output = timer.done() - return { outputFiles, allEntries, buildId } + return { outputFiles, allEntries } } async function stopCompile(projectId, userId) { diff --git a/services/clsi/app/js/OutputCacheManager.js b/services/clsi/app/js/OutputCacheManager.js index cad5302000..ca80dcc2ae 100644 --- a/services/clsi/app/js/OutputCacheManager.js +++ b/services/clsi/app/js/OutputCacheManager.js @@ -164,7 +164,7 @@ module.exports = OutputCacheManager = { outputDir, stats, (err, outputFiles) => { - if (err) return callback(err, { outputFiles, buildId }) + if (err) return callback(err, outputFiles) const enablePdfCaching = request.enablePdfCaching const enablePdfCachingDark = @@ -173,7 +173,7 @@ module.exports = OutputCacheManager = { !Settings.enablePdfCaching || (!enablePdfCaching && !enablePdfCachingDark) ) { - return callback(null, { outputFiles, buildId }) + return callback(null, outputFiles) } OutputCacheManager.saveStreamsInContentDir( @@ -191,9 +191,9 @@ module.exports = OutputCacheManager = { { err, outputDir, stats, timings }, 'pdf caching failed' ) - return callback(null, { outputFiles, buildId }) + return callback(null, outputFiles) } - callback(err, { outputFiles, buildId }) + callback(err, outputFiles) } ) } diff --git a/services/clsi/test/unit/js/CompileControllerTests.js b/services/clsi/test/unit/js/CompileControllerTests.js index eb5a1134b6..041e03bbaf 100644 --- a/services/clsi/test/unit/js/CompileControllerTests.js +++ b/services/clsi/test/unit/js/CompileControllerTests.js @@ -50,7 +50,6 @@ function tryImageNameValidation(method, imageNameField) { describe('CompileController', function () { beforeEach(function () { - this.buildId = 'build-id-123' this.CompileController = SandboxedModule.require(modulePath, { requires: { './CompileManager': (this.CompileManager = {}), @@ -119,7 +118,6 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, - buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -142,32 +140,21 @@ describe('CompileController', function () { it('should return the JSON response', function () { this.res.status.calledWith(200).should.equal(true) - console.log(this.res.send.args[0][0].compile) - sinon.assert.calledWith( - this.res.send, - sinon.match.has( - 'compile', - sinon.match({ + this.res.send + .calledWith({ + compile: { status: 'success', error: null, stats: this.stats, timings: this.timings, outputUrlPrefix: '/zone/b', - outputFiles: [ - ...this.output_files.map(file => ({ - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, - ...file, - })), - { - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${this.buildId}/output/output.zip`, - build: this.buildId, - path: 'output.zip', - type: 'zip', - }, - ], - }) - ) - ) + outputFiles: this.output_files.map(file => ({ + url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, + ...file, + })), + }, + }) + .should.equal(true) }) }) @@ -178,7 +165,6 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, - buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -193,18 +179,10 @@ describe('CompileController', function () { stats: this.stats, timings: this.timings, outputUrlPrefix: '', - outputFiles: [ - ...this.output_files.map(file => ({ - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, - ...file, - })), - { - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${this.buildId}/output/output.zip`, - build: this.buildId, - path: 'output.zip', - type: 'zip', - }, - ], + outputFiles: this.output_files.map(file => ({ + url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, + ...file, + })), }, }) .should.equal(true) @@ -229,7 +207,6 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, - buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -244,18 +221,10 @@ describe('CompileController', function () { stats: this.stats, timings: this.timings, outputUrlPrefix: '/zone/b', - outputFiles: [ - ...this.output_files.map(file => ({ - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, - ...file, - })), - { - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${this.buildId}/output/output.zip`, - build: this.buildId, - path: 'output.zip', - type: 'zip', - }, - ], + outputFiles: this.output_files.map(file => ({ + url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, + ...file, + })), }, }) .should.equal(true) @@ -281,7 +250,6 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, - buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -296,18 +264,10 @@ describe('CompileController', function () { stats: this.stats, timings: this.timings, outputUrlPrefix: '/zone/b', - outputFiles: [ - ...this.output_files.map(file => ({ - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, - ...file, - })), - { - url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${this.buildId}/output/output.zip`, - build: this.buildId, - path: 'output.zip', - type: 'zip', - }, - ], + outputFiles: this.output_files.map(file => ({ + url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, + ...file, + })), }, }) .should.equal(true) @@ -316,11 +276,9 @@ describe('CompileController', function () { describe('with an error', function () { beforeEach(function () { - const error = new Error((this.message = 'error message')) - error.buildId = this.buildId this.CompileManager.doCompileWithLock = sinon .stub() - .callsArgWith(1, error, null) + .callsArgWith(1, new Error((this.message = 'error message')), null) this.CompileController.compile(this.req, this.res) }) diff --git a/services/clsi/test/unit/js/CompileManagerTests.js b/services/clsi/test/unit/js/CompileManagerTests.js index 6f5b5baf64..6aa416afae 100644 --- a/services/clsi/test/unit/js/CompileManagerTests.js +++ b/services/clsi/test/unit/js/CompileManagerTests.js @@ -35,7 +35,6 @@ describe('CompileManager', function () { build: 1234, }, ] - this.buildId = 'build-id-123' this.commandOutput = 'Dummy output' this.compileBaseDir = '/compile/dir' this.outputBaseDir = '/output/dir' @@ -62,9 +61,7 @@ describe('CompileManager', function () { } this.OutputCacheManager = { promises: { - saveOutputFiles: sinon - .stub() - .resolves({ outputFiles: this.buildFiles, buildId: this.buildId }), + saveOutputFiles: sinon.stub().resolves(this.buildFiles), }, } this.Settings = { diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 61d41a3128..dbab01a8f4 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -398,13 +398,6 @@ module.exports = CompileController = { if (error) { return next(error) } - - const qs = {} - - if (req.params.file === 'output.zip') { - qs.files = req.query.files - } - const url = CompileController._getFileUrl( projectId, userId, @@ -415,7 +408,7 @@ module.exports = CompileController = { projectId, 'output-file', url, - qs, + {}, req, res, next @@ -580,20 +573,10 @@ module.exports = CompileController = { return next(err) } url = new URL(`${Settings.apis.clsi.url}${url}`) - - const params = new URLSearchParams(persistenceOptions.qs) - - for (const [key, value] of Object.entries(qs)) { - if (Array.isArray(value)) { - for (const v of value) { - params.append(key, v) - } - continue - } - params.append(key, value) - } - - url.search = params.toString() + url.search = new URLSearchParams({ + ...persistenceOptions.qs, + ...qs, + }).toString() const timer = new Metrics.Timer( 'proxy_to_clsi', 1, @@ -621,10 +604,7 @@ module.exports = CompileController = { }) for (const key of ['Content-Length', 'Content-Type']) { - const headerValue = response.headers.get(key) - if (headerValue) { - res.setHeader(key, headerValue) - } + res.setHeader(key, response.headers.get(key)) } res.writeHead(response.status) return pipeline(stream, res) diff --git a/services/web/frontend/extracted-translations.json b/services/web/frontend/extracted-translations.json index 83e3f1e76b..a4bab14ac3 100644 --- a/services/web/frontend/extracted-translations.json +++ b/services/web/frontend/extracted-translations.json @@ -304,7 +304,6 @@ "doing_this_will_verify_affiliation_and_allow_log_in_2": "", "done": "", "download": "", - "download_all": "", "download_metadata": "", "download_pdf": "", "download_zip_file": "", diff --git a/services/web/frontend/js/features/pdf-preview/components/pdf-file-list.jsx b/services/web/frontend/js/features/pdf-preview/components/pdf-file-list.jsx index 0bc0d41b5d..7e2764e769 100644 --- a/services/web/frontend/js/features/pdf-preview/components/pdf-file-list.jsx +++ b/services/web/frontend/js/features/pdf-preview/components/pdf-file-list.jsx @@ -33,18 +33,6 @@ function PdfFileList({ fileList }) { {file.path} ))} - - {fileList.other.length + fileList.top.length > 0 && fileList.archive && ( - - - {t('download_all')} ({fileList.other.length + fileList.top.length}) - - - )} ) } @@ -60,10 +48,6 @@ PdfFileList.propTypes = { fileList: PropTypes.shape({ top: FilesArray, other: FilesArray, - archive: PropTypes.shape({ - path: PropTypes.string.isRequired, - url: PropTypes.string.isRequired, - }), }), } diff --git a/services/web/frontend/js/features/pdf-preview/util/file-list.js b/services/web/frontend/js/features/pdf-preview/util/file-list.js index c82078b4d9..c9edfa828d 100644 --- a/services/web/frontend/js/features/pdf-preview/util/file-list.js +++ b/services/web/frontend/js/features/pdf-preview/util/file-list.js @@ -1,5 +1,5 @@ const topFileTypes = ['bbl', 'gls', 'ind'] -const ignoreFiles = ['output.fls', 'output.fdb_latexmk', 'output.zip'] +const ignoreFiles = ['output.fls', 'output.fdb_latexmk'] export const buildFileList = (outputFiles, clsiServerId, compileGroup) => { const files = { top: [], other: [] } @@ -18,8 +18,6 @@ export const buildFileList = (outputFiles, clsiServerId, compileGroup) => { const allFiles = [] - let outputArchiveFile - // filter out ignored files and set some properties for (const file of outputFiles.values()) { if (!ignoreFiles.includes(file.path)) { @@ -30,8 +28,6 @@ export const buildFileList = (outputFiles, clsiServerId, compileGroup) => { } allFiles.push(file) - } else if (file.type === 'zip') { - outputArchiveFile = file } } @@ -56,14 +52,6 @@ export const buildFileList = (outputFiles, clsiServerId, compileGroup) => { files.other.push(file) } } - - if (outputArchiveFile) { - allFiles.forEach( - file => file.type !== 'pdf' && params.append('files', file.path) - ) - outputArchiveFile.url += `?${params.toString()}` - files.archive = outputArchiveFile - } } return files diff --git a/services/web/locales/en.json b/services/web/locales/en.json index f8016f1c05..bb2eac43b5 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -430,7 +430,6 @@ "done": "Done", "dont_have_account": "Don’t have an account?", "download": "Download", - "download_all": "Download all", "download_metadata": "Download Overleaf metadata", "download_pdf": "Download PDF", "download_zip_file": "Download .zip file",