[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

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,

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

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)
     })
   })
 })