Merge pull request #17680 from overleaf/rh-promisify-templates-man

[web] Promisify TemplatesManager and TemplatesManagerTests

GitOrigin-RevId: 69ff0a5f34ceb7b4c23452bb00963eda35621f3f

services/web/app/src/Features/Templates/TemplatesManager.js

@@ -1,22 +1,25 @@
 const { Project } = require('../../models/Project')
-const OError = require('@overleaf/o-error')
 const ProjectDetailsHandler = require('../Project/ProjectDetailsHandler')
-const ProjectOptionsHandler = require('../Project/ProjectOptionsHandler')
-const ProjectRootDocManager = require('../Project/ProjectRootDocManager')
+const ProjectOptionsHandler =
+  require('../Project/ProjectOptionsHandler').promises
+const ProjectRootDocManager =
+  require('../Project/ProjectRootDocManager').promises
 const ProjectUploadManager = require('../Uploads/ProjectUploadManager')
-const async = require('async')
 const fs = require('fs')
 const util = require('util')
 const logger = require('@overleaf/logger')
-const { fetchJson, RequestFailedError } = require('@overleaf/fetch-utils')
-const request = require('request')
+const {
+  fetchJson,
+  fetchStreamWithResponse,
+  RequestFailedError,
+} = require('@overleaf/fetch-utils')
 const settings = require('@overleaf/settings')
 const crypto = require('crypto')
 const Errors = require('../Errors/Errors')
-const _ = require('lodash')
+const { pipeline } = require('stream/promises')
 
 const TemplatesManager = {
-  createProjectFromV1Template(
+  async createProjectFromV1Template(
     brandVariationId,
     compiler,
     mainFile,
@@ -24,153 +27,113 @@ const TemplatesManager = {
     templateName,
     templateVersionId,
     userId,
-    imageName,
-    _callback
+    imageName
   ) {
-    const callback = _.once(_callback)
     const zipUrl = `${settings.apis.v1.url}/api/v1/overleaf/templates/${templateVersionId}`
-    const zipReq = request(zipUrl, {
-      auth: {
+    const zipReq = await fetchStreamWithResponse(zipUrl, {
+      basicAuth: {
         user: settings.apis.v1.user,
-        pass: settings.apis.v1.pass,
+        password: settings.apis.v1.pass,
       },
-      timeout: settings.apis.v1.timeout,
-    })
-    zipReq.on('error', function (err) {
-      logger.warn({ err }, 'error getting zip from template API')
-      return callback(err)
+      signal: AbortSignal.timeout(settings.apis.v1.timeout),
     })
 
     const projectName = ProjectDetailsHandler.fixProjectName(templateName)
     const dumpPath = `${settings.path.dumpFolder}/${crypto.randomUUID()}`
     const writeStream = fs.createWriteStream(dumpPath)
-    const attributes = {
-      fromV1TemplateId: templateId,
-      fromV1TemplateVersionId: templateVersionId,
-    }
-    writeStream.on('close', function () {
-      if (zipReq.response.statusCode !== 200) {
+    try {
+      const attributes = {
+        fromV1TemplateId: templateId,
+        fromV1TemplateVersionId: templateVersionId,
+      }
+      await pipeline(zipReq.stream, writeStream)
+
+      if (zipReq.response.status !== 200) {
         logger.warn(
-          { uri: zipUrl, statusCode: zipReq.response.statusCode },
+          { uri: zipUrl, statusCode: zipReq.response.status },
           'non-success code getting zip from template API'
         )
-        return callback(new Error('get zip failed'))
+        throw new Error(`get zip failed: ${zipReq.response.status}`)
       }
-      ProjectUploadManager.createProjectFromZipArchiveWithName(
-        userId,
-        projectName,
-        dumpPath,
-        attributes,
-        function (err, project) {
-          if (err) {
-            OError.tag(err, 'problem building project from zip', {
-              zipReq,
-            })
-            return callback(err)
-          }
-          async.series(
-            [
-              cb => TemplatesManager._setCompiler(project._id, compiler, cb),
-              cb => TemplatesManager._setImage(project._id, imageName, cb),
-              cb => TemplatesManager._setMainFile(project._id, mainFile, cb),
-              cb =>
-                TemplatesManager._setBrandVariationId(
-                  project._id,
-                  brandVariationId,
-                  cb
-                ),
-            ],
-            function (err) {
-              if (err) {
-                return callback(err)
-              }
-              fs.unlink(dumpPath, function (err) {
-                if (err) {
-                  return logger.err({ err }, 'error unlinking template zip')
-                }
-              })
-              const update = {
-                fromV1TemplateId: templateId,
-                fromV1TemplateVersionId: templateVersionId,
-              }
-              Project.updateOne(
-                { _id: project._id },
-                update,
-                {},
-                function (err) {
-                  if (err) {
-                    return callback(err)
-                  }
-                  callback(null, project)
-                }
-              )
-            }
-          )
-        }
-      )
-    })
-    zipReq.pipe(writeStream)
+      const project =
+        await ProjectUploadManager.promises.createProjectFromZipArchiveWithName(
+          userId,
+          projectName,
+          dumpPath,
+          attributes
+        )
+
+      await TemplatesManager._setCompiler(project._id, compiler)
+      await TemplatesManager._setImage(project._id, imageName)
+      await TemplatesManager._setMainFile(project._id, mainFile)
+      await TemplatesManager._setBrandVariationId(project._id, brandVariationId)
+
+      const update = {
+        fromV1TemplateId: templateId,
+        fromV1TemplateVersionId: templateVersionId,
+      }
+      await Project.updateOne({ _id: project._id }, update, {})
+
+      return project
+    } finally {
+      await fs.promises.unlink(dumpPath)
+    }
   },
 
-  _setCompiler(projectId, compiler, callback) {
+  async _setCompiler(projectId, compiler) {
     if (compiler == null) {
-      return callback()
+      return
     }
-    ProjectOptionsHandler.setCompiler(projectId, compiler, callback)
+    await ProjectOptionsHandler.setCompiler(projectId, compiler)
   },
 
-  _setImage(projectId, imageName, callback) {
+  async _setImage(projectId, imageName) {
     if (!imageName) {
       imageName = 'wl_texlive:2018.1'
     }
-    ProjectOptionsHandler.setImageName(projectId, imageName, callback)
+
+    await ProjectOptionsHandler.setImageName(projectId, imageName)
   },
 
-  _setMainFile(projectId, mainFile, callback) {
+  async _setMainFile(projectId, mainFile) {
     if (mainFile == null) {
-      return callback()
+      return
     }
-    ProjectRootDocManager.setRootDocFromName(projectId, mainFile, callback)
+    await ProjectRootDocManager.setRootDocFromName(projectId, mainFile)
   },
 
-  _setBrandVariationId(projectId, brandVariationId, callback) {
+  async _setBrandVariationId(projectId, brandVariationId) {
     if (brandVariationId == null) {
-      return callback()
+      return
     }
-    ProjectOptionsHandler.setBrandVariationId(
-      projectId,
-      brandVariationId,
-      callback
-    )
+    await ProjectOptionsHandler.setBrandVariationId(projectId, brandVariationId)
   },
 
-  promises: {
-    async fetchFromV1(templateId) {
-      const url = new URL(
-        `/api/v2/templates/${templateId}`,
-        settings.apis.v1.url
-      )
+  async fetchFromV1(templateId) {
+    const url = new URL(`/api/v2/templates/${templateId}`, settings.apis.v1.url)
 
-      try {
-        return await fetchJson(url, {
-          basicAuth: {
-            user: settings.apis.v1.user,
-            password: settings.apis.v1.pass,
-          },
-          signal: AbortSignal.timeout(settings.apis.v1.timeout),
-        })
-      } catch (err) {
-        if (err instanceof RequestFailedError && err.response.status === 404) {
-          throw new Errors.NotFoundError()
-        } else {
-          throw err
-        }
+    try {
+      return await fetchJson(url, {
+        basicAuth: {
+          user: settings.apis.v1.user,
+          password: settings.apis.v1.pass,
+        },
+        signal: AbortSignal.timeout(settings.apis.v1.timeout),
+      })
+    } catch (err) {
+      if (err instanceof RequestFailedError && err.response.status === 404) {
+        throw new Errors.NotFoundError()
+      } else {
+        throw err
       }
-    },
+    }
   },
 }
 
-TemplatesManager.fetchFromV1 = util.callbackify(
-  TemplatesManager.promises.fetchFromV1
-)
-module.exports = TemplatesManager
+module.exports = {
+  promises: TemplatesManager,
+  createProjectFromV1Template: util.callbackify(
+    TemplatesManager.createProjectFromV1Template
+  ),
+  fetchFromV1: util.callbackify(TemplatesManager.fetchFromV1),
+}

services/web/test/unit/src/Templates/TemplatesManagerTests.js

@@ -11,9 +11,9 @@
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
 const SandboxedModule = require('sandboxed-module')
-const assert = require('assert')
 const sinon = require('sinon')
 const { RequestFailedError } = require('@overleaf/fetch-utils')
+const { ReadableString } = require('@overleaf/stream-utils')
 
 const modulePath = '../../../../app/src/Features/Templates/TemplatesManager'
 
@@ -30,6 +30,9 @@ describe('TemplatesManager', function () {
     this.user_id = 'user-id'
     this.dumpPath = `${this.dumpFolder}/${this.uuid}`
     this.callback = sinon.stub()
+    this.pipeline = sinon.stub().callsFake(async (stream, res) => {
+      if (res.callback) res.callback()
+    })
     this.request = sinon.stub().returns({
       pipe() {},
       on() {},
@@ -38,31 +41,50 @@ describe('TemplatesManager', function () {
       },
     })
     this.fs = {
+      promises: { unlink: sinon.stub() },
       unlink: sinon.stub(),
       createWriteStream: sinon.stub().returns({ on: sinon.stub().yields() }),
     }
     this.ProjectUploadManager = {
-      createProjectFromZipArchiveWithName: sinon
-        .stub()
-        .callsArgWith(4, null, { _id: this.project_id }),
+      promises: {
+        createProjectFromZipArchiveWithName: sinon
+          .stub()
+          .resolves({ _id: this.project_id }),
+      },
     }
     this.dumpFolder = 'dump/path'
     this.ProjectOptionsHandler = {
-      setCompiler: sinon.stub().callsArgWith(2),
-      setImageName: sinon.stub().callsArgWith(2),
-      setBrandVariationId: sinon.stub().callsArgWith(2),
+      promises: {
+        setCompiler: sinon.stub().resolves(),
+        setImageName: sinon.stub().resolves(),
+        setBrandVariationId: sinon.stub().resolves(),
+      },
     }
     this.uuid = '1234'
     this.ProjectRootDocManager = {
-      setRootDocFromName: sinon.stub().callsArgWith(2),
+      promises: {
+        setRootDocFromName: sinon.stub().resolves(),
+      },
     }
     this.ProjectDetailsHandler = {
       getProjectDescription: sinon.stub(),
       fixProjectName: sinon.stub().returns(this.templateName),
     }
-    this.Project = { updateOne: sinon.stub().callsArgWith(3, null) }
+    this.Project = { updateOne: sinon.stub().resolves() }
+    this.mockStream = new ReadableString('{}')
+    this.mockResponse = {
+      status: 200,
+      headers: new Headers({
+        'Content-Length': '2',
+        'Content-Type': 'application/json',
+      }),
+    }
     this.FetchUtils = {
       fetchJson: sinon.stub(),
+      fetchStreamWithResponse: sinon.stub().resolves({
+        stream: this.mockStream,
+        response: this.mockResponse,
+      }),
       RequestFailedError,
     }
     this.TemplatesManager = SandboxedModule.require(modulePath, {
@@ -85,6 +107,7 @@ describe('TemplatesManager', function () {
               url: (this.v1Url = 'http://overleaf.com'),
               user: 'overleaf',
               pass: 'password',
+              timeout: 10,
             },
           },
           overleaf: {
@@ -97,8 +120,9 @@ describe('TemplatesManager', function () {
         request: this.request,
         fs: this.fs,
         '../../models/Project': { Project: this.Project },
+        'stream/promises': { pipeline: this.pipeline },
       },
-    })
+    }).promises
     return (this.zipUrl =
       '%2Ftemplates%2F52fb86a81ae1e566597a25f6%2Fv%2F4%2Fzip&templateName=Moderncv%20Banking&compiler=pdflatex')
   })
@@ -114,13 +138,12 @@ describe('TemplatesManager', function () {
           this.templateName,
           this.templateVersionId,
           this.user_id,
-          this.imageName,
-          this.callback
+          this.imageName
         )
       })
 
       it('should fetch zip from v1 based on template id', function () {
-        return this.request.should.have.been.calledWith(
+        return this.FetchUtils.fetchStreamWithResponse.should.have.been.calledWith(
           `${this.v1Url}/api/v1/overleaf/templates/${this.templateVersionId}`
         )
       })
@@ -132,7 +155,7 @@ describe('TemplatesManager', function () {
       })
 
       it('should create project', function () {
-        return this.ProjectUploadManager.createProjectFromZipArchiveWithName.should.have.been.calledWithMatch(
+        return this.ProjectUploadManager.promises.createProjectFromZipArchiveWithName.should.have.been.calledWithMatch(
           this.user_id,
           this.templateName,
           this.dumpPath,
@@ -144,23 +167,25 @@ describe('TemplatesManager', function () {
       })
 
       it('should unlink file', function () {
-        return this.fs.unlink.should.have.been.calledWith(this.dumpPath)
+        return this.fs.promises.unlink.should.have.been.calledWith(
+          this.dumpPath
+        )
       })
 
       it('should set project options when passed', function () {
-        this.ProjectOptionsHandler.setCompiler.should.have.been.calledWithMatch(
+        this.ProjectOptionsHandler.promises.setCompiler.should.have.been.calledWithMatch(
           this.project_id,
           this.compiler
         )
-        this.ProjectOptionsHandler.setImageName.should.have.been.calledWithMatch(
+        this.ProjectOptionsHandler.promises.setImageName.should.have.been.calledWithMatch(
           this.project_id,
           this.imageName
         )
-        this.ProjectRootDocManager.setRootDocFromName.should.have.been.calledWithMatch(
+        this.ProjectRootDocManager.promises.setRootDocFromName.should.have.been.calledWithMatch(
           this.project_id,
           this.mainFile
         )
-        return this.ProjectOptionsHandler.setBrandVariationId.should.have.been.calledWithMatch(
+        return this.ProjectOptionsHandler.promises.setBrandVariationId.should.have.been.calledWithMatch(
           this.project_id,
           this.brandVariationId
         )
@@ -187,18 +212,21 @@ describe('TemplatesManager', function () {
           this.templateName,
           this.templateVersionId,
           this.user_id,
-          null,
-          this.callback
+          null
         )
       })
 
       it('should not set missing project options', function () {
-        this.ProjectOptionsHandler.setCompiler.called.should.equal(false)
-        this.ProjectRootDocManager.setRootDocFromName.called.should.equal(false)
-        this.ProjectOptionsHandler.setBrandVariationId.called.should.equal(
+        this.ProjectOptionsHandler.promises.setCompiler.called.should.equal(
+          false
+        )
+        this.ProjectRootDocManager.promises.setRootDocFromName.called.should.equal(
+          false
+        )
+        this.ProjectOptionsHandler.promises.setBrandVariationId.called.should.equal(
           false
         )
-        return this.ProjectOptionsHandler.setImageName.should.have.been.calledWithMatch(
+        return this.ProjectOptionsHandler.promises.setImageName.should.have.been.calledWithMatch(
           this.project_id,
           'wl_texlive:2018.1'
         )