From d8d1bdd4616782b88afda1a701b38e2cc48d008b Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Tue, 3 Mar 2020 08:59:09 -0500 Subject: [PATCH] Merge pull request #2647 from overleaf/em-promisify-history-manager Promisify HistoryManager GitOrigin-RevId: cbf946a7d61f7f57bad03e4c83184c6decd91027 --- .../src/Features/History/HistoryManager.js | 353 +++++++--------- .../unit/src/History/HistoryManagerTests.js | 378 +++++++----------- 2 files changed, 281 insertions(+), 450 deletions(-) diff --git a/services/web/app/src/Features/History/HistoryManager.js b/services/web/app/src/Features/History/HistoryManager.js index 6721c98837..e064fa98f4 100644 --- a/services/web/app/src/Features/History/HistoryManager.js +++ b/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() - } - return request.post( - { - url: `${settings.apis.project_history.url}/project` - }, - function(error, res, body) { - if (error != null) { - return callback(error) - } - - if (res.statusCode >= 200 && res.statusCode < 300) { - let project - try { - project = JSON.parse(body) - } catch (error1) { - error = error1 - return callback(error) - } - - 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) - } - - return callback(null, { overleaf_id }) - } else { - error = new Error( - `project-history returned a non-success status code: ${ - res.statusCode - }` - ) - return callback(error) - } - } - ) - }, - - 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() - } 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) - } - } - } - 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 } +module.exports = { + initializeProject: callbackify(initializeProject), + flushProject: callbackify(flushProject), + resyncProject: callbackify(resyncProject), + injectUserDetails: callbackify(injectUserDetails), + promises: { + initializeProject, + flushProject, + resyncProject, + injectUserDetails } } -function __guard__(value, transform) { - return typeof value !== 'undefined' && value !== null - ? transform(value) - : undefined + +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 { overleaf_id: overleafId } + } catch (err) { + throw new OError({ + message: 'failed to initialize project history' + }).withCause(err) + } } -HistoryManager.promises = promisifyAll(HistoryManager, { without: '_userView' }) -module.exports = HistoryManager +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) + } +} + +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 } + }) + } +} + +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) + } + } + } + + 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 { + return user + } + }) + } + } + return data +} + +function _userView(user) { + const { _id, first_name: firstName, last_name: lastName, email } = user + return { first_name: firstName, last_name: lastName, email, id: _id } +} diff --git a/services/web/test/unit/src/History/HistoryManagerTests.js b/services/web/test/unit/src/History/HistoryManagerTests.js index 551408876f..b3a3cf4460 100644 --- a/services/web/test/unit/src/History/HistoryManagerTests.js +++ b/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] } - ] - }, - (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 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] } - ] - }, - (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]) }) }) 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] } - ] - }, - (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]) }) - 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]) }) }) })