From 0f05623e16cfedb5c2d9e21ea1690c309c980027 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Mon, 19 Feb 2024 09:43:28 +0000 Subject: [PATCH] Merge pull request #17084 from overleaf/dp-mongoose-callback-system-message-manager Promisify SystemMessageManager and SystemMessageManagerTests GitOrigin-RevId: b8fafdfdba817160c1b18cf7eb0270a27adf114c --- libraries/promise-utils/index.js | 7 ++- .../test/unit/PromiseUtilsTests.js | 28 +++++++++ .../SystemMessages/SystemMessageController.js | 30 +++++----- .../SystemMessages/SystemMessageManager.js | 57 +++++++------------ .../SystemMessageManagerTests.js | 34 ++++------- 5 files changed, 76 insertions(+), 80 deletions(-) diff --git a/libraries/promise-utils/index.js b/libraries/promise-utils/index.js index 58bdc81639..5437a36997 100644 --- a/libraries/promise-utils/index.js +++ b/libraries/promise-utils/index.js @@ -143,13 +143,18 @@ function promisifyMultiResult(fn, resultNames) { * * @param {Object} module - The module to callbackify * @param {Object} opts - Options + * @param {Array} opts.without - Array of method names to exclude from + * being callbackified * @param {Object} opts.multiResult - Spec of methods to be callbackified with * callbackifyMultiResult() */ function callbackifyAll(module, opts = {}) { - const { multiResult = {} } = opts + const { without = [], multiResult = {} } = opts const callbacks = {} for (const propName of Object.getOwnPropertyNames(module)) { + if (without.includes(propName)) { + continue + } const propValue = module[propName] if (typeof propValue === 'function') { if (propValue.constructor.name === 'AsyncFunction') { diff --git a/libraries/promise-utils/test/unit/PromiseUtilsTests.js b/libraries/promise-utils/test/unit/PromiseUtilsTests.js index bfd7beb78d..c91121d757 100644 --- a/libraries/promise-utils/test/unit/PromiseUtilsTests.js +++ b/libraries/promise-utils/test/unit/PromiseUtilsTests.js @@ -295,4 +295,32 @@ describe('callbackifyAll', function () { }) }) }) + + describe('without option', function () { + before(function () { + this.module = { + async asyncAdd(a, b) { + return a + b + }, + async asyncArithmetic(a, b) { + return { sum: a + b, product: a * b } + }, + } + this.callbackified = callbackifyAll(this.module, { + without: ['asyncAdd'], + }) + }) + + it('does not callbackify excluded functions', function () { + expect(this.callbackified.asyncAdd).not.to.exist + }) + + it('callbackifies other functions', async function () { + this.callbackified.asyncArithmetic(5, 6, (err, { sum, product }) => { + expect(err).not.to.exist + expect(sum).to.equal(11) + expect(product).to.equal(30) + }) + }) + }) }) diff --git a/services/web/app/src/Features/SystemMessages/SystemMessageController.js b/services/web/app/src/Features/SystemMessages/SystemMessageController.js index 2c22e97850..41339aaf6f 100644 --- a/services/web/app/src/Features/SystemMessages/SystemMessageController.js +++ b/services/web/app/src/Features/SystemMessages/SystemMessageController.js @@ -8,23 +8,19 @@ const ProjectController = { // gracefully handle requests from anonymous users return res.json([]) } - SystemMessageManager.getMessages((err, messages) => { - if (err) { - next(err) - } else { - if (!Settings.siteIsOpen) { - // Override all messages with notice for admins when site is closed. - messages = [ - { - content: - 'SITE IS CLOSED TO PUBLIC. OPEN ONLY FOR SITE ADMINS. DO NOT EDIT PROJECTS.', - _id: 'protected', // prevents hiding message in frontend - }, - ] - } - res.json(messages || []) - } - }) + let messages = SystemMessageManager.getMessages() + + if (!Settings.siteIsOpen) { + // Override all messages with notice for admins when site is closed. + messages = [ + { + content: + 'SITE IS CLOSED TO PUBLIC. OPEN ONLY FOR SITE ADMINS. DO NOT EDIT PROJECTS.', + _id: 'protected', // prevents hiding message in frontend + }, + ] + } + res.json(messages || []) }, } diff --git a/services/web/app/src/Features/SystemMessages/SystemMessageManager.js b/services/web/app/src/Features/SystemMessages/SystemMessageManager.js index 196074677a..81df2bec5f 100644 --- a/services/web/app/src/Features/SystemMessages/SystemMessageManager.js +++ b/services/web/app/src/Features/SystemMessages/SystemMessageManager.js @@ -1,55 +1,30 @@ -/* eslint-disable - n/handle-callback-err, -*/ -// 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 - */ -let SystemMessageManager const { SystemMessage } = require('../../models/SystemMessage') const { addRequiredCleanupHandlerBeforeDrainingConnections, } = require('../../infrastructure/GracefulShutdown') +const { callbackifyAll } = require('@overleaf/promise-utils') -module.exports = SystemMessageManager = { - getMessages(callback) { - if (callback == null) { - callback = function () {} - } - callback(null, this._cachedMessages) +const SystemMessageManager = { + getMessages() { + return this._cachedMessages }, - getMessagesFromDB(callback) { - if (callback == null) { - callback = function () {} - } - SystemMessage.find({}, callback) + async getMessagesFromDB() { + return await SystemMessage.find({}).exec() }, - clearMessages(callback) { - if (callback == null) { - callback = function () {} - } - SystemMessage.deleteMany({}, callback) + async clearMessages() { + await SystemMessage.deleteMany({}).exec() }, - createMessage(content, callback) { - if (callback == null) { - callback = function () {} - } + async createMessage(content) { const message = new SystemMessage({ content }) - message.save(callback) + await message.save() }, - refreshCache() { - this.getMessagesFromDB((error, messages) => { - if (!error) { - this._cachedMessages = messages - } - }) + async refreshCache() { + const messages = await this.getMessagesFromDB() + this._cachedMessages = messages }, } @@ -66,3 +41,9 @@ addRequiredCleanupHandlerBeforeDrainingConnections( clearInterval(intervalHandle) } ) + +module.exports = { + getMessages: SystemMessageManager.getMessages.bind(SystemMessageManager), + ...callbackifyAll(SystemMessageManager, { without: ['getMessages'] }), + promises: SystemMessageManager, +} diff --git a/services/web/test/unit/src/SystemMessages/SystemMessageManagerTests.js b/services/web/test/unit/src/SystemMessages/SystemMessageManagerTests.js index cd98273ab3..8d542c3096 100644 --- a/services/web/test/unit/src/SystemMessages/SystemMessageManagerTests.js +++ b/services/web/test/unit/src/SystemMessages/SystemMessageManagerTests.js @@ -1,17 +1,4 @@ -/* eslint-disable - 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 SandboxedModule = require('sandboxed-module') -const assert = require('assert') const sinon = require('sinon') const modulePath = require('path').join( __dirname, @@ -22,14 +9,15 @@ describe('SystemMessageManager', function () { beforeEach(function () { this.messages = ['messages-stub'] this.SystemMessage = { - find: sinon.stub().yields(null, this.messages), + find: sinon.stub().returns({ + exec: sinon.stub().resolves(this.messages), + }), } this.SystemMessageManager = SandboxedModule.require(modulePath, { requires: { '../../models/SystemMessage': { SystemMessage: this.SystemMessage }, }, }) - return (this.callback = sinon.stub()) }) it('should look the messages up in the database on import', function () { @@ -39,26 +27,24 @@ describe('SystemMessageManager', function () { describe('getMessage', function () { beforeEach(function () { this.SystemMessageManager._cachedMessages = this.messages - return this.SystemMessageManager.getMessages(this.callback) + this.result = this.SystemMessageManager.getMessages() }) it('should return the messages', function () { - return this.callback.calledWith(null, this.messages).should.equal(true) + this.result.should.equal(this.messages) }) }) describe('clearMessages', function () { beforeEach(function () { - this.SystemMessage.deleteMany = sinon.stub().callsArg(1) - return this.SystemMessageManager.clearMessages(this.callback) + this.SystemMessage.deleteMany = sinon.stub().returns({ + exec: sinon.stub().resolves(), + }) + this.SystemMessageManager.promises.clearMessages() }) it('should remove the messages from the database', function () { - return this.SystemMessage.deleteMany.calledWith({}).should.equal(true) - }) - - it('should return the callback', function () { - return this.callback.called.should.equal(true) + this.SystemMessage.deleteMany.calledWith({}).should.equal(true) }) }) })