From 03bb99aeaae1ce222d5f379749ce349321e054db Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Thu, 8 Feb 2024 11:14:17 +0000 Subject: [PATCH] Merge pull request #16657 from overleaf/dp-mongoose-callback-project-update-handler Promisify ProjectUpdateHandler and ProjectUpdateHandlerTests GitOrigin-RevId: 312cbe71d431cf50932ab7d5501529d87f7827f2 --- .../Project/ProjectEntityUpdateHandler.js | 22 +++- .../Features/Project/ProjectUpdateHandler.js | 76 +++-------- .../ProjectEntityUpdateHandlerTests.js | 16 ++- .../src/Project/ProjectUpdateHandlerTests.js | 124 +++++++----------- 4 files changed, 92 insertions(+), 146 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index e91f59fbad..9e7b52460a 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js @@ -204,11 +204,11 @@ const ProjectEntityUpdateHandler = { return callback(null, { rev }) } // Don't need to block for marking as updated - ProjectUpdateHandler.markAsUpdated( - projectId, - lastUpdatedAt, - lastUpdatedBy - ) + ProjectUpdateHandler.promises + .markAsUpdated(projectId, lastUpdatedAt, lastUpdatedBy) + .catch(error => { + logger.error({ error }, 'failed to mark project as updated') + }) TpdsUpdateSender.addDoc( { projectId, @@ -557,7 +557,11 @@ const ProjectEntityUpdateHandler = { if (error != null) { return callback(error) } - ProjectUpdateHandler.markAsUpdated(projectId, new Date(), userId) + ProjectUpdateHandler.promises + .markAsUpdated(projectId, new Date(), userId) + .catch(error => { + logger.error({ error }, 'failed to mark project as updated') + }) callback(null, fileRef, folderId) } ) @@ -616,7 +620,11 @@ const ProjectEntityUpdateHandler = { if (err != null) { return callback(err) } - ProjectUpdateHandler.markAsUpdated(projectId, new Date(), userId) + ProjectUpdateHandler.promises + .markAsUpdated(projectId, new Date(), userId) + .catch(error => { + logger.error({ error }, 'failed to mark project as updated') + }) DocumentUpdaterHandler.updateProjectStructure( projectId, diff --git a/services/web/app/src/Features/Project/ProjectUpdateHandler.js b/services/web/app/src/Features/Project/ProjectUpdateHandler.js index 6ada85c683..ad9b914bc9 100644 --- a/services/web/app/src/Features/Project/ProjectUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectUpdateHandler.js @@ -1,24 +1,9 @@ -/* eslint-disable - n/handle-callback-err, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const { Project } = require('../../models/Project') -const logger = require('@overleaf/logger') -const { promisifyAll } = require('@overleaf/promise-utils') +const { callbackify } = require('util') const ProjectUpdateHandler = { - markAsUpdated(projectId, lastUpdatedAt, lastUpdatedBy, callback) { - if (callback == null) { - callback = function () {} - } - if (lastUpdatedAt == null) { + async markAsUpdated(projectId, lastUpdatedAt, lastUpdatedBy) { + if (!lastUpdatedAt) { lastUpdatedAt = new Date() } @@ -31,59 +16,32 @@ const ProjectUpdateHandler = { lastUpdated: lastUpdatedAt || new Date().getTime(), lastUpdatedBy, } - Project.updateOne(conditions, update, {}, callback) + await Project.updateOne(conditions, update, {}).exec() }, - // like markAsUpdated but allows lastUpdatedAt to be reset to earlier time - resetUpdated(projectId, lastUpdatedAt, lastUpdatedBy, callback) { - if (callback == null) { - callback = function () {} - } - if (lastUpdatedAt == null) { - lastUpdatedAt = new Date() - } - - const conditions = { - _id: projectId, - } - - const update = { - lastUpdated: lastUpdatedAt || new Date().getTime(), - lastUpdatedBy, - } - Project.updateOne(conditions, update, {}, callback) - }, - - markAsOpened(projectId, callback) { + async markAsOpened(projectId) { const conditions = { _id: projectId } const update = { lastOpened: Date.now() } - Project.updateOne(conditions, update, {}, function (err) { - if (callback != null) { - return callback() - } - }) + await Project.updateOne(conditions, update, {}).exec() }, - markAsInactive(projectId, callback) { + async markAsInactive(projectId) { const conditions = { _id: projectId } const update = { active: false } - Project.updateOne(conditions, update, {}, function (err) { - if (callback != null) { - return callback() - } - }) + await Project.updateOne(conditions, update, {}).exec() }, - markAsActive(projectId, callback) { + async markAsActive(projectId) { const conditions = { _id: projectId } const update = { active: true } - Project.updateOne(conditions, update, {}, function (err) { - if (callback != null) { - return callback() - } - }) + await Project.updateOne(conditions, update, {}).exec() }, } -ProjectUpdateHandler.promises = promisifyAll(ProjectUpdateHandler) -module.exports = ProjectUpdateHandler +module.exports = { + markAsUpdated: callbackify(ProjectUpdateHandler.markAsUpdated), + markAsOpened: callbackify(ProjectUpdateHandler.markAsOpened), + markAsInactive: callbackify(ProjectUpdateHandler.markAsInactive), + markAsActive: callbackify(ProjectUpdateHandler.markAsActive), + promises: ProjectUpdateHandler, +} diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index 1cc3765557..bec76521ce 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js @@ -103,7 +103,9 @@ describe('ProjectEntityUpdateHandler', function () { findElementByPath: sinon.stub(), } this.ProjectUpdater = { - markAsUpdated: sinon.stub(), + promises: { + markAsUpdated: sinon.stub().resolves(), + }, } this.ProjectEntityHandler = { getDoc: sinon.stub(), @@ -244,7 +246,7 @@ describe('ProjectEntityUpdateHandler', function () { it('should mark the project as updated', function () { sinon.assert.calledWith( - this.ProjectUpdater.markAsUpdated, + this.ProjectUpdater.promises.markAsUpdated, projectId, this.lastUpdatedAt, this.lastUpdatedBy @@ -283,7 +285,7 @@ describe('ProjectEntityUpdateHandler', function () { }) it('should not mark the project as updated', function () { - this.ProjectUpdater.markAsUpdated.called.should.equal(false) + this.ProjectUpdater.promises.markAsUpdated.called.should.equal(false) }) it('should not send the doc the to the TPDS', function () { @@ -326,7 +328,7 @@ describe('ProjectEntityUpdateHandler', function () { }) it('should not mark the project as updated', function () { - this.ProjectUpdater.markAsUpdated.called.should.equal(false) + this.ProjectUpdater.promises.markAsUpdated.called.should.equal(false) }) it('should not send the doc the to the TPDS', function () { @@ -373,7 +375,7 @@ describe('ProjectEntityUpdateHandler', function () { }) it('should not mark the project as updated', function () { - this.ProjectUpdater.markAsUpdated.called.should.equal(false) + this.ProjectUpdater.promises.markAsUpdated.called.should.equal(false) }) it('should not send the doc the to the TPDS', function () { @@ -658,7 +660,7 @@ describe('ProjectEntityUpdateHandler', function () { }) it('should mark the project as updated', function () { - const args = this.ProjectUpdater.markAsUpdated.args[0] + const args = this.ProjectUpdater.promises.markAsUpdated.args[0] args[0].should.equal(projectId) args[1].should.exist args[2].should.equal(userId) @@ -1066,7 +1068,7 @@ describe('ProjectEntityUpdateHandler', function () { }) it('should mark the project as updated', function () { - const args = this.ProjectUpdater.markAsUpdated.args[0] + const args = this.ProjectUpdater.promises.markAsUpdated.args[0] args[0].should.equal(projectId) args[1].should.exist args[2].should.equal(userId) diff --git a/services/web/test/unit/src/Project/ProjectUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectUpdateHandlerTests.js index dbda832330..4984f1489d 100644 --- a/services/web/test/unit/src/Project/ProjectUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectUpdateHandlerTests.js @@ -1,16 +1,3 @@ -/* eslint-disable - n/handle-callback-err, - max-len, - no-return-assign, - no-unused-vars, -*/ -// 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 sinon = require('sinon') const modulePath = '../../../../app/src/Features/Project/ProjectUpdateHandler.js' @@ -27,98 +14,89 @@ describe('ProjectUpdateHandler', function () { }) beforeEach(function () { - let Project - this.ProjectModel = Project = class Project {} - this.ProjectModel.updateOne = sinon.stub().callsArg(3) - return (this.handler = SandboxedModule.require(modulePath, { + this.ProjectModel = class Project {} + this.ProjectModel.updateOne = sinon.stub().returns({ + exec: sinon.stub(), + }) + this.handler = SandboxedModule.require(modulePath, { requires: { '../../models/Project': { Project: this.ProjectModel }, }, - })) + }) }) describe('marking a project as recently updated', function () { beforeEach(function () { this.project_id = 'project_id' this.lastUpdatedAt = 987654321 - return (this.lastUpdatedBy = 'fake-last-updater-id') + this.lastUpdatedBy = 'fake-last-updater-id' }) - it('should send an update to mongo', function (done) { - return this.handler.markAsUpdated( + it('should send an update to mongo', async function () { + await this.handler.promises.markAsUpdated( this.project_id, this.lastUpdatedAt, - this.lastUpdatedBy, - err => { - sinon.assert.calledWith( - this.ProjectModel.updateOne, - { - _id: this.project_id, - lastUpdated: { $lt: this.lastUpdatedAt }, - }, - { - lastUpdated: this.lastUpdatedAt, - lastUpdatedBy: this.lastUpdatedBy, - } - ) - return done() + this.lastUpdatedBy + ) + + sinon.assert.calledWith( + this.ProjectModel.updateOne, + { + _id: this.project_id, + lastUpdated: { $lt: this.lastUpdatedAt }, + }, + { + lastUpdated: this.lastUpdatedAt, + lastUpdatedBy: this.lastUpdatedBy, } ) }) - it('should set smart fallbacks', function (done) { - return this.handler.markAsUpdated(this.project_id, null, null, err => { - sinon.assert.calledWithMatch( - this.ProjectModel.updateOne, - { - _id: this.project_id, - lastUpdated: { $lt: this.fakeTime }, - }, - { - lastUpdated: this.fakeTime, - lastUpdatedBy: null, - } - ) - return done() - }) + it('should set smart fallbacks', async function () { + await this.handler.promises.markAsUpdated(this.project_id, null, null) + sinon.assert.calledWithMatch( + this.ProjectModel.updateOne, + { + _id: this.project_id, + lastUpdated: { $lt: this.fakeTime }, + }, + { + lastUpdated: this.fakeTime, + lastUpdatedBy: null, + } + ) }) }) describe('markAsOpened', function () { - it('should send an update to mongo', function (done) { + it('should send an update to mongo', async function () { const projectId = 'project_id' - return this.handler.markAsOpened(projectId, err => { - const args = this.ProjectModel.updateOne.args[0] - args[0]._id.should.equal(projectId) - const date = args[1].lastOpened + '' - const now = Date.now() + '' - date.substring(0, 5).should.equal(now.substring(0, 5)) - return done() - }) + await this.handler.promises.markAsOpened(projectId) + const args = this.ProjectModel.updateOne.args[0] + args[0]._id.should.equal(projectId) + const date = args[1].lastOpened + '' + const now = Date.now() + '' + date.substring(0, 5).should.equal(now.substring(0, 5)) }) }) describe('markAsInactive', function () { - it('should send an update to mongo', function (done) { + it('should send an update to mongo', async function () { const projectId = 'project_id' - return this.handler.markAsInactive(projectId, err => { - const args = this.ProjectModel.updateOne.args[0] - args[0]._id.should.equal(projectId) - args[1].active.should.equal(false) - return done() - }) + await this.handler.promises.markAsInactive(projectId) + const args = this.ProjectModel.updateOne.args[0] + args[0]._id.should.equal(projectId) + args[1].active.should.equal(false) }) }) describe('markAsActive', function () { - it('should send an update to mongo', function (done) { + it('should send an update to mongo', async function () { const projectId = 'project_id' - return this.handler.markAsActive(projectId, err => { - const args = this.ProjectModel.updateOne.args[0] - args[0]._id.should.equal(projectId) - args[1].active.should.equal(true) - return done() - }) + await this.handler.promises.markAsActive(projectId) + const args = this.ProjectModel.updateOne.args[0] + args[0]._id.should.equal(projectId) + args[1].active.should.equal(true) }) }) })