From 16c4f6219eace070d939bfd0b427ff8e6d2b8747 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 5 Sep 2023 08:41:34 -0400 Subject: [PATCH] Merge pull request #14470 from overleaf/em-promisify-document-controller Promisify DocumentController GitOrigin-RevId: f9ae24fc396cbcd27148ec4add641a0907bcf014 --- .../Features/Documents/DocumentController.js | 128 +++++++---------- .../src/Documents/DocumentControllerTests.js | 131 +++++++++++------- 2 files changed, 129 insertions(+), 130 deletions(-) diff --git a/services/web/app/src/Features/Documents/DocumentController.js b/services/web/app/src/Features/Documents/DocumentController.js index 57daba7497..a5652f5091 100644 --- a/services/web/app/src/Features/Documents/DocumentController.js +++ b/services/web/app/src/Features/Documents/DocumentController.js @@ -1,104 +1,76 @@ const ProjectGetter = require('../Project/ProjectGetter') -const OError = require('@overleaf/o-error') const ProjectLocator = require('../Project/ProjectLocator') const ProjectEntityHandler = require('../Project/ProjectEntityHandler') const ProjectEntityUpdateHandler = require('../Project/ProjectEntityUpdateHandler') const logger = require('@overleaf/logger') const _ = require('lodash') const { plainTextResponse } = require('../../infrastructure/Response') +const { expressify } = require('../../util/promises') -function getDocument(req, res, next) { +async function getDocument(req, res) { const { Project_id: projectId, doc_id: docId } = req.params const plain = req.query.plain === 'true' const peek = req.query.peek === 'true' - ProjectGetter.getProject( + const project = await ProjectGetter.promises.getProject(projectId, { + rootFolder: true, + overleaf: true, + }) + if (!project) { + return res.sendStatus(404) + } + + const { path } = await ProjectLocator.promises.findElement({ + project, + element_id: docId, + type: 'doc', + }) + + const { lines, version, ranges } = await ProjectEntityHandler.promises.getDoc( projectId, - { rootFolder: true, overleaf: true }, - (error, project) => { - if (error) { - return next(error) - } - if (!project) { - return res.sendStatus(404) - } - ProjectLocator.findElement( - { project, element_id: docId, type: 'doc' }, - (error, doc, path) => { - if (error) { - OError.tag(error, 'error finding element for getDocument', { - docId, - projectId, - }) - return next(error) - } - ProjectEntityHandler.getDoc( - projectId, - docId, - { peek }, - (error, lines, rev, version, ranges) => { - if (error) { - OError.tag( - error, - 'error finding doc contents for getDocument', - { - docId, - projectId, - } - ) - return next(error) - } - if (plain) { - plainTextResponse(res, lines.join('\n')) - } else { - const projectHistoryId = _.get(project, 'overleaf.history.id') - - // all projects are now migrated to Full Project History, keeping the field - // for API compatibility - const projectHistoryType = 'project-history' - - res.json({ - lines, - version, - ranges, - pathname: path.fileSystem, - projectHistoryId, - projectHistoryType, - }) - } - } - ) - } - ) - } + docId, + { peek } ) + + if (plain) { + plainTextResponse(res, lines.join('\n')) + } else { + const projectHistoryId = _.get(project, 'overleaf.history.id') + + // all projects are now migrated to Full Project History, keeping the field + // for API compatibility + const projectHistoryType = 'project-history' + + res.json({ + lines, + version, + ranges, + pathname: path.fileSystem, + projectHistoryId, + projectHistoryType, + }) + } } -function setDocument(req, res, next) { +async function setDocument(req, res) { const { Project_id: projectId, doc_id: docId } = req.params const { lines, version, ranges, lastUpdatedAt, lastUpdatedBy } = req.body - ProjectEntityUpdateHandler.updateDocLines( + const result = await ProjectEntityUpdateHandler.promises.updateDocLines( projectId, docId, lines, version, ranges, lastUpdatedAt, - lastUpdatedBy, - (error, result) => { - if (error) { - OError.tag(error, 'error finding element for getDocument', { - docId, - projectId, - }) - return next(error) - } - logger.debug( - { docId, projectId }, - 'finished receiving set document request from api (docupdater)' - ) - res.json(result) - } + lastUpdatedBy ) + logger.debug( + { docId, projectId }, + 'finished receiving set document request from api (docupdater)' + ) + res.json(result) } -module.exports = { getDocument, setDocument } +module.exports = { + getDocument: expressify(getDocument), + setDocument: expressify(setDocument), +} diff --git a/services/web/test/unit/src/Documents/DocumentControllerTests.js b/services/web/test/unit/src/Documents/DocumentControllerTests.js index 041dae0389..dbcfbb91c0 100644 --- a/services/web/test/unit/src/Documents/DocumentControllerTests.js +++ b/services/web/test/unit/src/Documents/DocumentControllerTests.js @@ -1,27 +1,18 @@ const sinon = require('sinon') -const modulePath = - '../../../../app/src/Features/Documents/DocumentController.js' const SandboxedModule = require('sandboxed-module') const MockRequest = require('../helpers/MockRequest') const MockResponse = require('../helpers/MockResponse') const Errors = require('../../../../app/src/Features/Errors/Errors') +const MODULE_PATH = + '../../../../app/src/Features/Documents/DocumentController.js' + describe('DocumentController', function () { beforeEach(function () { - this.DocumentController = SandboxedModule.require(modulePath, { - requires: { - '../Project/ProjectGetter': (this.ProjectGetter = {}), - '../Project/ProjectLocator': (this.ProjectLocator = {}), - '../Project/ProjectEntityHandler': (this.ProjectEntityHandler = {}), - '../Project/ProjectEntityUpdateHandler': - (this.ProjectEntityUpdateHandler = {}), - }, - }) this.res = new MockResponse() this.req = new MockRequest() this.next = sinon.stub() - this.project_id = 'project-id-123' - this.doc_id = 'doc-id-123' + this.doc = { _id: 'doc-id-123' } this.doc_lines = ['one', 'two', 'three'] this.version = 42 this.ranges = { mock: 'ranges' } @@ -29,40 +20,68 @@ describe('DocumentController', function () { this.lastUpdatedAt = new Date().getTime() this.lastUpdatedBy = 'fake-last-updater-id' this.rev = 5 + this.project = { + _id: 'project-id-123', + overleaf: { + history: { + id: 1234, + display: true, + }, + }, + } + + this.ProjectGetter = { + promises: { + getProject: sinon.stub().resolves(this.project), + }, + } + this.ProjectLocator = { + promises: { + findElement: sinon + .stub() + .resolves({ element: this.doc, path: { fileSystem: this.pathname } }), + }, + } + this.ProjectEntityHandler = { + promises: { + getDoc: sinon.stub().resolves({ + lines: this.doc_lines, + rev: this.rev, + version: this.version, + ranges: this.ranges, + }), + }, + } + this.ProjectEntityUpdateHandler = { + promises: { + updateDocLines: sinon.stub().resolves(), + }, + } + + this.DocumentController = SandboxedModule.require(MODULE_PATH, { + requires: { + '../Project/ProjectGetter': this.ProjectGetter, + '../Project/ProjectLocator': this.ProjectLocator, + '../Project/ProjectEntityHandler': this.ProjectEntityHandler, + '../Project/ProjectEntityUpdateHandler': + this.ProjectEntityUpdateHandler, + }, + }) }) describe('getDocument', function () { beforeEach(function () { this.req.params = { - Project_id: this.project_id, - doc_id: this.doc_id, + Project_id: this.project._id, + doc_id: this.doc._id, } }) describe('when project exists with project history enabled', function () { - beforeEach(function () { - this.doc = { _id: this.doc_id } - this.projectHistoryId = 1234 - this.projectHistoryDisplay = true - this.projectHistoryType = 'project-history' - this.project = { - _id: this.project_id, - overleaf: { - history: { - id: this.projectHistoryId, - display: this.projectHistoryDisplay, - }, - }, + beforeEach(function (done) { + this.res.callback = err => { + done(err) } - this.ProjectGetter.getProject = sinon - .stub() - .callsArgWith(2, null, this.project) - this.ProjectLocator.findElement = sinon - .stub() - .callsArgWith(1, null, this.doc, { fileSystem: this.pathname }) - this.ProjectEntityHandler.getDoc = sinon - .stub() - .yields(null, this.doc_lines, this.rev, this.version, this.ranges) this.DocumentController.getDocument(this.req, this.res, this.next) }) @@ -74,16 +93,19 @@ describe('DocumentController', function () { version: this.version, ranges: this.ranges, pathname: this.pathname, - projectHistoryId: this.projectHistoryId, - projectHistoryType: this.projectHistoryType, + projectHistoryId: this.project.overleaf.history.id, + projectHistoryType: 'project-history', }) ) }) }) describe('when the project does not exist', function () { - beforeEach(function () { - this.ProjectGetter.getProject = sinon.stub().callsArgWith(2, null, null) + beforeEach(function (done) { + this.ProjectGetter.promises.getProject.resolves(null) + this.res.callback = err => { + done(err) + } this.DocumentController.getDocument(this.req, this.res, this.next) }) @@ -96,14 +118,13 @@ describe('DocumentController', function () { describe('setDocument', function () { beforeEach(function () { this.req.params = { - Project_id: this.project_id, - doc_id: this.doc_id, + Project_id: this.project._id, + doc_id: this.doc._id, } }) describe('when the document exists', function () { - beforeEach(function () { - this.ProjectEntityUpdateHandler.updateDocLines = sinon.stub().yields() + beforeEach(function (done) { this.req.body = { lines: this.doc_lines, version: this.version, @@ -111,14 +132,17 @@ describe('DocumentController', function () { lastUpdatedAt: this.lastUpdatedAt, lastUpdatedBy: this.lastUpdatedBy, } + this.res.callback = err => { + done(err) + } this.DocumentController.setDocument(this.req, this.res, this.next) }) it('should update the document in Mongo', function () { sinon.assert.calledWith( - this.ProjectEntityUpdateHandler.updateDocLines, - this.project_id, - this.doc_id, + this.ProjectEntityUpdateHandler.promises.updateDocLines, + this.project._id, + this.doc._id, this.doc_lines, this.version, this.ranges, @@ -133,11 +157,14 @@ describe('DocumentController', function () { }) describe("when the document doesn't exist", function () { - beforeEach(function () { - this.ProjectEntityUpdateHandler.updateDocLines = sinon - .stub() - .yields(new Errors.NotFoundError('document does not exist')) + beforeEach(function (done) { + this.ProjectEntityUpdateHandler.promises.updateDocLines.rejects( + new Errors.NotFoundError('document does not exist') + ) this.req.body = { lines: this.doc_lines } + this.next.callsFake(() => { + done() + }) this.DocumentController.setDocument(this.req, this.res, this.next) })