Merge pull request #2647 from overleaf/em-promisify-history-manager

Promisify HistoryManager

GitOrigin-RevId: cbf946a7d61f7f57bad03e4c83184c6decd91027

services/web/app/src/Features/History/HistoryManager.js

@@ -1,224 +1,143 @@
-/* eslint-disable
-    camelcase,
-    handle-callback-err,
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS103: Rewrite code to no longer use __guard__
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const request = require('request')
+const { callbackify } = require('util')
+const request = require('request-promise-native')
 const settings = require('settings-sharelatex')
-const async = require('async')
+const OError = require('@overleaf/o-error')
 const UserGetter = require('../User/UserGetter')
-const { promisifyAll } = require('../../util/promises')
 
-const HistoryManager = {
-  initializeProject(callback) {
-    if (callback == null) {
-      callback = function(error, history_id) {}
-    }
-    if (
-      !(settings.apis.project_history != null
-        ? settings.apis.project_history.initializeHistoryForNewProjects
-        : undefined)
-    ) {
-      return callback()
+module.exports = {
+  initializeProject: callbackify(initializeProject),
+  flushProject: callbackify(flushProject),
+  resyncProject: callbackify(resyncProject),
+  injectUserDetails: callbackify(injectUserDetails),
+  promises: {
+    initializeProject,
+    flushProject,
+    resyncProject,
+    injectUserDetails
+  }
+}
+
+async function initializeProject() {
+  if (
+    !(
+      settings.apis.project_history &&
+      settings.apis.project_history.initializeHistoryForNewProjects
+    )
+  ) {
+    return
+  }
+  try {
+    const body = await request.post({
+      url: `${settings.apis.project_history.url}/project`
+    })
+    const project = JSON.parse(body)
+    const overleafId = project && project.project && project.project.id
+    if (!overleafId) {
+      throw new Error('project-history did not provide an id', project)
     }
-    return request.post(
-      {
-        url: `${settings.apis.project_history.url}/project`
-      },
-      function(error, res, body) {
-        if (error != null) {
-          return callback(error)
-        }
+    return { overleaf_id: overleafId }
+  } catch (err) {
+    throw new OError({
+      message: 'failed to initialize project history'
+    }).withCause(err)
+  }
+}
 
-        if (res.statusCode >= 200 && res.statusCode < 300) {
-          let project
-          try {
-            project = JSON.parse(body)
-          } catch (error1) {
-            error = error1
-            return callback(error)
-          }
+async function flushProject(projectId) {
+  try {
+    await request.post({
+      url: `${settings.apis.project_history.url}/project/${projectId}/flush`
+    })
+  } catch (err) {
+    throw new OError({
+      message: 'failed to flush project to project history',
+      info: { projectId }
+    }).withCause(err)
+  }
+}
 
-          const overleaf_id = __guard__(
-            project != null ? project.project : undefined,
-            x => x.id
-          )
-          if (!overleaf_id) {
-            error = new Error('project-history did not provide an id', project)
-            return callback(error)
-          }
+async function resyncProject(projectId) {
+  try {
+    await request.post({
+      url: `${settings.apis.project_history.url}/project/${projectId}/resync`
+    })
+  } catch (err) {
+    throw new OError({
+      message: 'failed to resync project history',
+      info: { projectId }
+    })
+  }
+}
 
-          return callback(null, { overleaf_id })
-        } else {
-          error = new Error(
-            `project-history returned a non-success status code: ${
-              res.statusCode
-            }`
-          )
-          return callback(error)
-        }
+async function injectUserDetails(data) {
+  // data can be either:
+  // {
+  // 	diff: [{
+  // 		i: "foo",
+  // 		meta: {
+  // 			users: ["user_id", v1_user_id, ...]
+  // 			...
+  // 		}
+  // 	}, ...]
+  // }
+  // or
+  // {
+  // 	updates: [{
+  // 		pathnames: ["main.tex"]
+  // 		meta: {
+  // 			users: ["user_id", v1_user_id, ...]
+  // 			...
+  // 		},
+  // 		...
+  // 	}, ...]
+  // }
+  // Either way, the top level key points to an array of objects with a meta.users property
+  // that we need to replace user_ids with populated user objects.
+  // Note that some entries in the users arrays may be v1 ids returned by the v1 history
+  // service. v1 ids will be `numbers`
+  let userIds = new Set()
+  let v1UserIds = new Set()
+  for (const entry of data.diff || data.updates || []) {
+    for (const user of (entry.meta && entry.meta.users) || []) {
+      if (typeof user === 'string') {
+        userIds.add(user)
+      } else if (typeof user === 'number') {
+        v1UserIds.add(user)
       }
-    )
-  },
-
-  flushProject(project_id, callback) {
-    if (callback == null) {
-      callback = function(error) {}
     }
-    return request.post(
-      {
-        url: `${settings.apis.project_history.url}/project/${project_id}/flush`
-      },
-      function(error, res, body) {
-        if (error != null) {
-          return callback(error)
-        }
-        if (res.statusCode >= 200 && res.statusCode < 300) {
-          return callback()
-        } else {
-          error = new Error(
-            `project-history returned a non-success status code: ${
-              res.statusCode
-            }`
-          )
-          return callback(error)
-        }
-      }
-    )
-  },
+  }
 
-  resyncProject(project_id, callback) {
-    if (callback == null) {
-      callback = function(error) {}
-    }
-    return request.post(
-      {
-        url: `${settings.apis.project_history.url}/project/${project_id}/resync`
-      },
-      function(error, res, body) {
-        if (error != null) {
-          return callback(error)
-        }
-        if (res.statusCode >= 200 && res.statusCode < 300) {
-          return callback()
+  userIds = Array.from(userIds)
+  v1UserIds = Array.from(v1UserIds)
+  const projection = { first_name: 1, last_name: 1, email: 1 }
+  const usersArray = await UserGetter.promises.getUsers(userIds, projection)
+  const users = {}
+  for (const user of usersArray) {
+    users[user._id.toString()] = _userView(user)
+  }
+  projection.overleaf = 1
+  const v1IdentifiedUsersArray = await UserGetter.promises.getUsersByV1Ids(
+    v1UserIds,
+    projection
+  )
+  for (const user of v1IdentifiedUsersArray) {
+    users[user.overleaf.id] = _userView(user)
+  }
+  for (const entry of data.diff || data.updates || []) {
+    if (entry.meta != null) {
+      entry.meta.users = ((entry.meta && entry.meta.users) || []).map(user => {
+        if (typeof user === 'string' || typeof user === 'number') {
+          return users[user]
         } else {
-          error = new Error(
-            `project-history returned a non-success status code: ${
-              res.statusCode
-            }`
-          )
-          return callback(error)
-        }
-      }
-    )
-  },
-
-  injectUserDetails(data, callback) {
-    // data can be either:
-    // {
-    // 	diff: [{
-    // 		i: "foo",
-    // 		meta: {
-    // 			users: ["user_id", v1_user_id, ...]
-    // 			...
-    // 		}
-    // 	}, ...]
-    // }
-    // or
-    // {
-    // 	updates: [{
-    // 		pathnames: ["main.tex"]
-    // 		meta: {
-    // 			users: ["user_id", v1_user_id, ...]
-    // 			...
-    // 		},
-    // 		...
-    // 	}, ...]
-    // }
-    // Either way, the top level key points to an array of objects with a meta.users property
-    // that we need to replace user_ids with populated user objects.
-    // Note that some entries in the users arrays may be v1 ids returned by the v1 history
-    // service. v1 ids will be `numbers`
-    let entry, user
-    if (callback == null) {
-      callback = function(error, data_with_users) {}
-    }
-    let user_ids = new Set()
-    let v1_user_ids = new Set()
-    for (entry of Array.from(data.diff || data.updates || [])) {
-      for (user of Array.from(
-        (entry.meta != null ? entry.meta.users : undefined) || []
-      )) {
-        if (typeof user === 'string') {
-          user_ids.add(user)
-        } else if (typeof user === 'number') {
-          v1_user_ids.add(user)
+          return user
         }
-      }
+      })
     }
-    user_ids = Array.from(user_ids)
-    v1_user_ids = Array.from(v1_user_ids)
-    const projection = { first_name: 1, last_name: 1, email: 1 }
-    return UserGetter.getUsers(user_ids, projection, function(
-      error,
-      users_array
-    ) {
-      if (error != null) {
-        return callback(error)
-      }
-      const v1_query = { 'overleaf.id': v1_user_ids }
-      const users = {}
-      for (user of Array.from(users_array || [])) {
-        users[user._id.toString()] = HistoryManager._userView(user)
-      }
-      projection.overleaf = 1
-      UserGetter.getUsersByV1Ids(
-        v1_user_ids,
-        projection,
-        (error, v1_identified_users_array) => {
-          for (user of Array.from(v1_identified_users_array || [])) {
-            users[user.overleaf.id] = HistoryManager._userView(user)
-          }
-          for (entry of Array.from(data.diff || data.updates || [])) {
-            if (entry.meta != null) {
-              entry.meta.users = (
-                (entry.meta != null ? entry.meta.users : undefined) || []
-              ).map(function(user) {
-                if (typeof user === 'string' || typeof user === 'number') {
-                  return users[user]
-                } else {
-                  return user
-                }
-              })
-            }
-          }
-          return callback(null, data)
-        }
-      )
-    })
-  },
-
-  _userView(user) {
-    const { _id, first_name, last_name, email } = user
-    return { first_name, last_name, email, id: _id }
   }
-}
-function __guard__(value, transform) {
-  return typeof value !== 'undefined' && value !== null
-    ? transform(value)
-    : undefined
+  return data
 }
 
-HistoryManager.promises = promisifyAll(HistoryManager, { without: '_userView' })
-module.exports = HistoryManager
+function _userView(user) {
+  const { _id, first_name: firstName, last_name: lastName, email } = user
+  return { first_name: firstName, last_name: lastName, email, id: _id }
+}

services/web/test/unit/src/History/HistoryManagerTests.js

@@ -1,45 +1,45 @@
-/* eslint-disable
-    max-len,
-    no-return-assign,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const chai = require('chai')
-chai.should()
-const { expect } = chai
+const { expect } = require('chai')
 const sinon = require('sinon')
-const modulePath = '../../../../app/src/Features/History/HistoryManager'
 const SandboxedModule = require('sandboxed-module')
 
+const MODULE_PATH = '../../../../app/src/Features/History/HistoryManager'
+
 describe('HistoryManager', function() {
   beforeEach(function() {
-    this.callback = sinon.stub()
     this.user_id = 'user-id-123'
     this.AuthenticationController = {
       getLoggedInUserId: sinon.stub().returns(this.user_id)
     }
-    this.HistoryManager = SandboxedModule.require(modulePath, {
+    this.request = {
+      post: sinon.stub()
+    }
+    this.settings = {
+      apis: {
+        trackchanges: {
+          enabled: false,
+          url: 'http://trackchanges.example.com'
+        },
+        project_history: {
+          url: 'http://project_history.example.com'
+        }
+      }
+    }
+
+    this.UserGetter = {
+      promises: {
+        getUsersByV1Ids: sinon.stub(),
+        getUsers: sinon.stub()
+      }
+    }
+
+    this.HistoryManager = SandboxedModule.require(MODULE_PATH, {
       globals: {
         console: console
       },
       requires: {
-        request: (this.request = sinon.stub()),
-        'settings-sharelatex': (this.settings = {}),
-        '../User/UserGetter': (this.UserGetter = {})
-      }
-    })
-    return (this.settings.apis = {
-      trackchanges: {
-        enabled: false,
-        url: 'http://trackchanges.example.com'
-      },
-      project_history: {
-        url: 'http://project_history.example.com'
+        'request-promise-native': this.request,
+        'settings-sharelatex': this.settings,
+        '../User/UserGetter': this.UserGetter
       }
     })
   })
@@ -47,101 +47,53 @@ describe('HistoryManager', function() {
   describe('initializeProject', function() {
     describe('with project history enabled', function() {
       beforeEach(function() {
-        return (this.settings.apis.project_history.initializeHistoryForNewProjects = true)
+        this.settings.apis.project_history.initializeHistoryForNewProjects = true
       })
 
       describe('project history returns a successful response', function() {
-        beforeEach(function() {
+        beforeEach(async function() {
           this.overleaf_id = 1234
-          this.res = { statusCode: 200 }
-          this.body = JSON.stringify({ project: { id: this.overleaf_id } })
-          this.request.post = sinon
-            .stub()
-            .callsArgWith(1, null, this.res, this.body)
-
-          return this.HistoryManager.initializeProject(this.callback)
+          this.request.post.resolves(
+            JSON.stringify({ project: { id: this.overleaf_id } })
+          )
+          this.result = await this.HistoryManager.promises.initializeProject()
         })
 
         it('should call the project history api', function() {
-          return this.request.post
+          this.request.post
             .calledWith({
               url: `${this.settings.apis.project_history.url}/project`
             })
             .should.equal(true)
         })
 
-        it('should return the callback with the overleaf id', function() {
-          return this.callback
-            .calledWithExactly(null, { overleaf_id: this.overleaf_id })
-            .should.equal(true)
+        it('should return the overleaf id', function() {
+          expect(this.result).to.deep.equal({ overleaf_id: this.overleaf_id })
         })
       })
 
       describe('project history returns a response without the project id', function() {
-        beforeEach(function() {
-          this.res = { statusCode: 200 }
-          this.body = JSON.stringify({ project: {} })
-          this.request.post = sinon
-            .stub()
-            .callsArgWith(1, null, this.res, this.body)
-
-          return this.HistoryManager.initializeProject(this.callback)
-        })
-
-        it('should return the callback with an error', function() {
-          return this.callback
-            .calledWith(
-              sinon.match.has(
-                'message',
-                'project-history did not provide an id'
-              )
-            )
-            .should.equal(true)
-        })
-      })
-
-      describe('project history returns a unsuccessful response', function() {
-        beforeEach(function() {
-          this.res = { statusCode: 404 }
-          this.request.post = sinon.stub().callsArgWith(1, null, this.res)
-
-          return this.HistoryManager.initializeProject(this.callback)
-        })
-
-        it('should return the callback with an error', function() {
-          return this.callback
-            .calledWith(
-              sinon.match.has(
-                'message',
-                'project-history returned a non-success status code: 404'
-              )
-            )
-            .should.equal(true)
+        it('should throw an error', async function() {
+          this.request.post.resolves(JSON.stringify({ project: {} }))
+          await expect(this.HistoryManager.promises.initializeProject()).to.be
+            .rejected
         })
       })
 
       describe('project history errors', function() {
-        beforeEach(function() {
-          this.error = sinon.stub()
-          this.request.post = sinon.stub().callsArgWith(1, this.error)
-
-          return this.HistoryManager.initializeProject(this.callback)
-        })
-
-        it('should return the callback with the error', function() {
-          return this.callback.calledWithExactly(this.error).should.equal(true)
+        it('should propagate the error', async function() {
+          this.request.post.rejects(new Error('problem connecting'))
+          await expect(this.HistoryManager.promises.initializeProject()).to.be
+            .rejected
         })
       })
     })
 
     describe('with project history disabled', function() {
-      beforeEach(function() {
+      it('should return without errors', async function() {
         this.settings.apis.project_history.initializeHistoryForNewProjects = false
-        return this.HistoryManager.initializeProject(this.callback)
-      })
-
-      it('should return the callback', function() {
-        return this.callback.calledWithExactly().should.equal(true)
+        await expect(this.HistoryManager.promises.initializeProject()).to.be
+          .fulfilled
       })
     })
   })
@@ -173,160 +125,120 @@ describe('HistoryManager', function() {
         last_name: 'Doe',
         email: 'john@example.com'
       }
-      this.UserGetter.getUsersByV1Ids = sinon.stub().yields(null, [this.user1])
-      return (this.UserGetter.getUsers = sinon
-        .stub()
-        .yields(null, [this.user1, this.user2]))
+      this.UserGetter.promises.getUsersByV1Ids.resolves([this.user1])
+      this.UserGetter.promises.getUsers.resolves([this.user1, this.user2])
     })
 
     describe('with a diff', function() {
-      it('should turn user_ids into user objects', function(done) {
-        return this.HistoryManager.injectUserDetails(
-          {
-            diff: [
-              {
-                i: 'foo',
-                meta: {
-                  users: [this.user_id1]
-                }
-              },
-              {
-                i: 'bar',
-                meta: {
-                  users: [this.user_id2]
-                }
+      it('should turn user_ids into user objects', async function() {
+        const diff = await this.HistoryManager.promises.injectUserDetails({
+          diff: [
+            {
+              i: 'foo',
+              meta: {
+                users: [this.user_id1]
               }
-            ]
-          },
-          (error, diff) => {
-            expect(error).to.be.null
-            expect(diff.diff[0].meta.users).to.deep.equal([this.user1_view])
-            expect(diff.diff[1].meta.users).to.deep.equal([this.user2_view])
-            return done()
-          }
-        )
+            },
+            {
+              i: 'bar',
+              meta: {
+                users: [this.user_id2]
+              }
+            }
+          ]
+        })
+        expect(diff.diff[0].meta.users).to.deep.equal([this.user1_view])
+        expect(diff.diff[1].meta.users).to.deep.equal([this.user2_view])
       })
 
-      it('should handle v1 user ids', function(done) {
-        return this.HistoryManager.injectUserDetails(
-          {
-            diff: [
-              {
-                i: 'foo',
-                meta: {
-                  users: [5011]
-                }
-              },
-              {
-                i: 'bar',
-                meta: {
-                  users: [this.user_id2]
-                }
+      it('should handle v1 user ids', async function() {
+        const diff = await this.HistoryManager.promises.injectUserDetails({
+          diff: [
+            {
+              i: 'foo',
+              meta: {
+                users: [5011]
+              }
+            },
+            {
+              i: 'bar',
+              meta: {
+                users: [this.user_id2]
               }
-            ]
-          },
-          (error, diff) => {
-            expect(error).to.be.null
-            expect(diff.diff[0].meta.users).to.deep.equal([this.user1_view])
-            expect(diff.diff[1].meta.users).to.deep.equal([this.user2_view])
-            return done()
-          }
-        )
+            }
+          ]
+        })
+        expect(diff.diff[0].meta.users).to.deep.equal([this.user1_view])
+        expect(diff.diff[1].meta.users).to.deep.equal([this.user2_view])
       })
 
-      it('should leave user objects', function(done) {
-        return this.HistoryManager.injectUserDetails(
-          {
-            diff: [
-              {
-                i: 'foo',
-                meta: {
-                  users: [this.user1_view]
-                }
-              },
-              {
-                i: 'bar',
-                meta: {
-                  users: [this.user_id2]
-                }
+      it('should leave user objects', async function() {
+        const diff = await this.HistoryManager.promises.injectUserDetails({
+          diff: [
+            {
+              i: 'foo',
+              meta: {
+                users: [this.user1_view]
+              }
+            },
+            {
+              i: 'bar',
+              meta: {
+                users: [this.user_id2]
               }
-            ]
-          },
-          (error, diff) => {
-            expect(error).to.be.null
-            expect(diff.diff[0].meta.users).to.deep.equal([this.user1_view])
-            expect(diff.diff[1].meta.users).to.deep.equal([this.user2_view])
-            return done()
-          }
-        )
+            }
+          ]
+        })
+        expect(diff.diff[0].meta.users).to.deep.equal([this.user1_view])
+        expect(diff.diff[1].meta.users).to.deep.equal([this.user2_view])
       })
     })
 
     describe('with a list of updates', function() {
-      it('should turn user_ids into user objects', function(done) {
-        return this.HistoryManager.injectUserDetails(
-          {
-            updates: [
-              {
-                fromV: 5,
-                toV: 8,
-                meta: {
-                  users: [this.user_id1]
-                }
-              },
-              {
-                fromV: 4,
-                toV: 5,
-                meta: {
-                  users: [this.user_id2]
-                }
+      it('should turn user_ids into user objects', async function() {
+        const updates = await this.HistoryManager.promises.injectUserDetails({
+          updates: [
+            {
+              fromV: 5,
+              toV: 8,
+              meta: {
+                users: [this.user_id1]
+              }
+            },
+            {
+              fromV: 4,
+              toV: 5,
+              meta: {
+                users: [this.user_id2]
               }
-            ]
-          },
-          (error, updates) => {
-            expect(error).to.be.null
-            expect(updates.updates[0].meta.users).to.deep.equal([
-              this.user1_view
-            ])
-            expect(updates.updates[1].meta.users).to.deep.equal([
-              this.user2_view
-            ])
-            return done()
-          }
-        )
+            }
+          ]
+        })
+        expect(updates.updates[0].meta.users).to.deep.equal([this.user1_view])
+        expect(updates.updates[1].meta.users).to.deep.equal([this.user2_view])
       })
 
-      it('should leave user objects', function(done) {
-        return this.HistoryManager.injectUserDetails(
-          {
-            updates: [
-              {
-                fromV: 5,
-                toV: 8,
-                meta: {
-                  users: [this.user1_view]
-                }
-              },
-              {
-                fromV: 4,
-                toV: 5,
-                meta: {
-                  users: [this.user_id2]
-                }
+      it('should leave user objects', async function() {
+        const updates = await this.HistoryManager.promises.injectUserDetails({
+          updates: [
+            {
+              fromV: 5,
+              toV: 8,
+              meta: {
+                users: [this.user1_view]
               }
-            ]
-          },
-          (error, updates) => {
-            expect(error).to.be.null
-            expect(updates.updates[0].meta.users).to.deep.equal([
-              this.user1_view
-            ])
-            expect(updates.updates[1].meta.users).to.deep.equal([
-              this.user2_view
-            ])
-            return done()
-          }
-        )
+            },
+            {
+              fromV: 4,
+              toV: 5,
+              meta: {
+                users: [this.user_id2]
+              }
+            }
+          ]
+        })
+        expect(updates.updates[0].meta.users).to.deep.equal([this.user1_view])
+        expect(updates.updates[1].meta.users).to.deep.equal([this.user2_view])
       })
     })
   })