From 039491f86607f824ed5ea32117dbf88267f05bdc Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Tue, 16 Dec 2025 10:30:48 +0100 Subject: [PATCH] [web] Promisify NotificationsHandler (#28805) * Replace request by fetch * Promisify NotificationsHandler.js * Replace to `fetchNothing` when we don't consume the body. I also checked that these endpoints don't return anything: https://github.com/overleaf/internal/blob/de2d0ed8cab15c4347ddecf227073a6d4e06120f/services/notifications/app.js * Make URLs safer * Ignore (but log) failing `fetchNothing`s on DELETE endpoints * Defensively add `userId.toString()` to prevent bugs (!) * Update NotificationsHandler unit tests * Throw fetch errors only if we did not get a response GitOrigin-RevId: 03700ad29658c0e3af4e476b25a964030e9c97f1 --- .../Notifications/NotificationsBuilder.mjs | 6 +- .../Notifications/NotificationsHandler.mjs | 268 ++++++++++-------- .../NotificationsHandler.test.mjs | 82 +++--- 3 files changed, 191 insertions(+), 165 deletions(-) diff --git a/services/web/app/src/Features/Notifications/NotificationsBuilder.mjs b/services/web/app/src/Features/Notifications/NotificationsBuilder.mjs index cc1cba8886..c74c720df9 100644 --- a/services/web/app/src/Features/Notifications/NotificationsBuilder.mjs +++ b/services/web/app/src/Features/Notifications/NotificationsBuilder.mjs @@ -57,7 +57,7 @@ function featuresUpgradedByAffiliation(affiliation, user) { async create() { const messageOpts = { institutionName: affiliation.institutionName } return await NotificationsHandler.promises.createNotification( - user._id, + user._id.toString(), this.key, 'notification_features_upgraded_by_affiliation', messageOpts, @@ -80,7 +80,7 @@ function redundantPersonalSubscription(affiliation, user) { async create() { const messageOpts = { institutionName: affiliation.institutionName } return await NotificationsHandler.promises.createNotification( - user._id, + user._id.toString(), this.key, 'notification_personal_subscription_not_required_due_to_affiliation', messageOpts, @@ -108,7 +108,7 @@ function projectInvite(invite, project, sendingUser, user) { token: invite.token, } return await NotificationsHandler.promises.createNotification( - user._id, + user._id.toString(), this.key, 'notification_project_invite', messageOpts, diff --git a/services/web/app/src/Features/Notifications/NotificationsHandler.mjs b/services/web/app/src/Features/Notifications/NotificationsHandler.mjs index 7c0ce9cac6..7d7441b329 100644 --- a/services/web/app/src/Features/Notifications/NotificationsHandler.mjs +++ b/services/web/app/src/Features/Notifications/NotificationsHandler.mjs @@ -1,142 +1,168 @@ +import path from 'node:path' import settings from '@overleaf/settings' -import request from 'request' +import { + fetchJson, + fetchNothing, + RequestFailedError, +} from '@overleaf/fetch-utils' import logger from '@overleaf/logger' import _ from 'lodash' -import { promisifyAll } from '@overleaf/promise-utils' +import { callbackifyAll } from '@overleaf/promise-utils' +import OError from '@overleaf/o-error' const notificationsApi = _.get(settings, ['apis', 'notifications', 'url']) const oneSecond = 1000 -const makeRequest = function (opts, callback) { - if (notificationsApi) { - request(opts, callback) - } else { - callback(null, { statusCode: 200 }) +async function getUserNotifications(userId) { + if (!notificationsApi) return [] + + const url = new URL(notificationsApi) + url.pathname = path.posix.join('user', userId.toString()) + + try { + const body = await fetchJson(url, { + signal: AbortSignal.timeout(oneSecond), + }) + return body || [] + } catch (err) { + logger.err({ err, userId }, 'something went wrong getting notifications') + return [] } } -const NotificationsHandler = { - getUserNotifications(userId, callback) { - const opts = { - uri: `${notificationsApi}/user/${userId}`, - json: true, - timeout: oneSecond, - method: 'GET', - } - makeRequest(opts, function (err, res, unreadNotifications) { - const statusCode = res ? res.statusCode : 500 - if (err || statusCode !== 200) { - logger.err( - { err, statusCode }, - 'something went wrong getting notifications' - ) - callback(null, []) - } else { - if (unreadNotifications == null) { - unreadNotifications = [] - } - callback(null, unreadNotifications) - } - }) - }, +async function createNotification( + userId, + key, + templateKey, + messageOpts, + expiryDateTime, + forceCreate = true +) { + if (!notificationsApi) return - createNotification( - userId, + const payload = { key, - templateKey, messageOpts, - expiryDateTime, + templateKey, forceCreate, - callback - ) { - if (!callback) { - callback = forceCreate - forceCreate = true - } - const payload = { - key, - messageOpts, - templateKey, - forceCreate, - } - if (expiryDateTime) { - payload.expires = expiryDateTime - } - const opts = { - uri: `${notificationsApi}/user/${userId}`, - timeout: oneSecond, + } + if (expiryDateTime) { + payload.expires = expiryDateTime + } + const url = new URL(notificationsApi) + url.pathname = path.posix.join('user', userId.toString()) + try { + return await fetchNothing(url, { method: 'POST', json: payload, - } - makeRequest(opts, callback) - }, - - markAsReadWithKey(userId, key, callback) { - const opts = { - uri: `${notificationsApi}/user/${userId}`, - method: 'DELETE', - timeout: oneSecond, - json: { - key, - }, - } - makeRequest(opts, callback) - }, - - markAsRead(userId, notificationId, callback) { - const opts = { - method: 'DELETE', - uri: `${notificationsApi}/user/${userId}/notification/${notificationId}`, - timeout: oneSecond, - } - makeRequest(opts, callback) - }, - - // removes notification by key, without regard for user_id, - // should not be exposed to user via ui/router - markAsReadByKeyOnly(key, callback) { - const opts = { - uri: `${notificationsApi}/key/${key}`, - method: 'DELETE', - timeout: oneSecond, - } - makeRequest(opts, callback) - }, - - previewMarkAsReadByKeyOnlyBulk(key, callback) { - const opts = { - uri: `${notificationsApi}/key/${key}/count`, - method: 'GET', - timeout: 10 * oneSecond, - json: true, - } - makeRequest(opts, (err, res, body) => { - if (err) return callback(err) - if (res.statusCode !== 200) { - return callback( - new Error('cannot preview bulk delete notification: ' + key) - ) - } - callback(null, (body && body.count) || 0) + signal: AbortSignal.timeout(oneSecond), }) - }, - - markAsReadByKeyOnlyBulk(key, callback) { - const opts = { - uri: `${notificationsApi}/key/${key}/bulk`, - method: 'DELETE', - timeout: 10 * oneSecond, - json: true, + } catch (err) { + // keep the behavior from `request` + if (!(err instanceof RequestFailedError)) { + throw OError.tag(err, 'Failed create notification') } - makeRequest(opts, (err, res, body) => { - if (err) return callback(err) - if (res.statusCode !== 200) { - return callback(new Error('cannot bulk delete notification: ' + key)) - } - callback(null, (body && body.count) || 0) - }) - }, + } +} + +async function markAsReadWithKey(userId, key) { + if (!notificationsApi) return + + const url = new URL(notificationsApi) + url.pathname = path.posix.join('user', userId.toString()) + try { + await fetchNothing(url, { + method: 'DELETE', + json: { key }, + signal: AbortSignal.timeout(oneSecond), + }) + } catch (err) { + // keep the behavior from `request` + if (!(err instanceof RequestFailedError)) { + throw OError.tag(err, 'markAsReadWithKey failed') + } + } +} + +async function markAsRead(userId, notificationId) { + if (!notificationsApi) return + + const url = new URL(notificationsApi) + url.pathname = path.posix.join( + 'user', + userId.toString(), + 'notification', + notificationId + ) + try { + await fetchNothing(url, { + method: 'DELETE', + signal: AbortSignal.timeout(oneSecond), + }) + } catch (err) { + // keep the behavior from `request` + if (!(err instanceof RequestFailedError)) { + throw OError.tag(err, 'markAsRead failed') + } + } +} + +// removes notification by key, without regard for user_id, +// should not be exposed to user via ui/router +async function markAsReadByKeyOnly(key) { + if (!notificationsApi) return + + const url = new URL(notificationsApi) + url.pathname = path.posix.join('key', key) + try { + await fetchNothing(url, { + method: 'DELETE', + signal: AbortSignal.timeout(oneSecond), + }) + } catch (err) { + // keep the behavior from `request` + if (!(err instanceof RequestFailedError)) { + throw OError.tag(err, 'markAsReadByKeyOnly failed') + } + } +} + +async function previewMarkAsReadByKeyOnlyBulk(key) { + if (!notificationsApi) return 0 + + const url = new URL(notificationsApi) + url.pathname = path.posix.join('key', key, 'count') + const body = await fetchJson(url, { + signal: AbortSignal.timeout(10 * oneSecond), + }) + return body?.count || 0 +} + +async function markAsReadByKeyOnlyBulk(key) { + if (!notificationsApi) return 0 + + const url = new URL(notificationsApi) + url.pathname = path.posix.join('key', key, 'bulk') + const body = await fetchJson(url, { + method: 'DELETE', + signal: AbortSignal.timeout(10 * oneSecond), + }) + return body?.count || 0 +} + +const promises = { + getUserNotifications, + createNotification, + markAsReadWithKey, + markAsRead, + markAsReadByKeyOnly, + previewMarkAsReadByKeyOnlyBulk, + markAsReadByKeyOnlyBulk, +} + +const NotificationsHandler = { + ...callbackifyAll(promises), + promises, } -NotificationsHandler.promises = promisifyAll(NotificationsHandler) export default NotificationsHandler diff --git a/services/web/test/unit/src/Notifications/NotificationsHandler.test.mjs b/services/web/test/unit/src/Notifications/NotificationsHandler.test.mjs index 56156cd389..d7b7c2efa2 100644 --- a/services/web/test/unit/src/Notifications/NotificationsHandler.test.mjs +++ b/services/web/test/unit/src/Notifications/NotificationsHandler.test.mjs @@ -9,10 +9,13 @@ const modulePath = path.join( describe('NotificationsHandler', function () { const userId = '123nd3ijdks' const notificationId = '123njdskj9jlk' - const notificationUrl = 'notification.overleaf.testing' + const notificationUrl = 'http://notification.overleaf.testing' beforeEach(async function (ctx) { - ctx.request = sinon.stub().callsArgWith(1) + ctx.FetchUtils = { + fetchJson: sinon.stub().resolves(), + fetchNothing: sinon.stub().resolves(), + } vi.doMock('@overleaf/settings', () => ({ default: { @@ -20,9 +23,7 @@ describe('NotificationsHandler', function () { }, })) - vi.doMock('request', () => ({ - default: ctx.request, - })) + vi.doMock('@overleaf/fetch-utils', () => ctx.FetchUtils) ctx.handler = (await import(modulePath)).default }) @@ -30,26 +31,19 @@ describe('NotificationsHandler', function () { describe('getUserNotifications', function () { it('should get unread notifications', async function (ctx) { const stubbedNotifications = [{ _id: notificationId, user_id: userId }] - ctx.request.callsArgWith( - 1, - null, - { statusCode: 200 }, - stubbedNotifications - ) + ctx.FetchUtils.fetchJson.resolves(stubbedNotifications) const unreadNotifications = await ctx.handler.promises.getUserNotifications(userId) stubbedNotifications.should.deep.equal(unreadNotifications) - const getOpts = { - uri: `${notificationUrl}/user/${userId}`, - json: true, - timeout: 1000, - method: 'GET', - } - ctx.request.calledWith(getOpts).should.equal(true) + ctx.FetchUtils.fetchJson + .calledWith( + sinon.match(u => u.href === `${notificationUrl}/user/${userId}`) + ) + .should.equal(true) }) it('should return empty arrays if there are no notifications', async function (ctx) { - ctx.request.callsArgWith(1, null, { statusCode: 200 }, null) + ctx.FetchUtils.fetchJson.resolves(null) const unreadNotifications = await ctx.handler.promises.getUserNotifications(userId) unreadNotifications.length.should.equal(0) @@ -63,15 +57,15 @@ describe('NotificationsHandler', function () { it('should send a delete request when a delete has been received to mark a notification', async function (ctx) { await ctx.handler.promises.markAsReadWithKey(userId, ctx.key) - const opts = { - uri: `${notificationUrl}/user/${userId}`, - json: { - key: ctx.key, - }, - timeout: 1000, - method: 'DELETE', - } - ctx.request.calledWith(opts).should.equal(true) + ctx.FetchUtils.fetchNothing + .calledWith( + sinon.match(u => u.href === `${notificationUrl}/user/${userId}`), + sinon.match({ + method: 'DELETE', + json: { key: ctx.key }, + }) + ) + .should.equal(true) }) }) @@ -91,16 +85,17 @@ describe('NotificationsHandler', function () { ctx.messageOpts, ctx.expiry ) - const args = ctx.request.args[0][0] - args.uri.should.equal(`${notificationUrl}/user/${userId}`) - args.timeout.should.equal(1000) + const [url, opts] = ctx.FetchUtils.fetchNothing.getCall(0).args + url.href.should.equal(`${notificationUrl}/user/${userId}`) + opts.method.should.equal('POST') + const expectedJson = { key: ctx.key, templateKey: ctx.templateKey, messageOpts: ctx.messageOpts, forceCreate: true, } - assert.deepEqual(args.json, expectedJson) + assert.deepEqual(opts.json, expectedJson) }) describe('when expiry date is supplied', function () { @@ -120,9 +115,10 @@ describe('NotificationsHandler', function () { ctx.expiry ) - const args = ctx.request.args[0][0] - args.uri.should.equal(`${notificationUrl}/user/${userId}`) - args.timeout.should.equal(1000) + const [url, args] = ctx.FetchUtils.fetchNothing.getCall(0).args + url.href.should.equal(`${notificationUrl}/user/${userId}`) + args.method.should.equal('POST') + const expectedJson = { key: ctx.key, templateKey: ctx.templateKey, @@ -142,12 +138,16 @@ describe('NotificationsHandler', function () { it('should send a delete request when a delete has been received to mark a notification', async function (ctx) { await ctx.handler.promises.markAsReadByKeyOnly(ctx.key) - const opts = { - uri: `${notificationUrl}/key/${ctx.key}`, - timeout: 1000, - method: 'DELETE', - } - ctx.request.calledWith(opts).should.equal(true) + ctx.FetchUtils.fetchNothing + .calledWith( + sinon.match( + u => u.href === `${notificationUrl}/key/some%20key%20here` + ), + sinon.match({ + method: 'DELETE', + }) + ) + .should.equal(true) }) }) })