Merge pull request #7106 from overleaf/ab-analytics-controller-async

Convert AnalyticsController to async/await

GitOrigin-RevId: a38194b2970a15b06fe6a3d95048681d7927bfc3

services/web/app/src/Features/Analytics/AnalyticsController.js

@@ -4,7 +4,48 @@ const SessionManager = require('../Authentication/SessionManager')
 const GeoIpLookup = require('../../infrastructure/GeoIpLookup')
 const Features = require('../../infrastructure/Features')
 
-const getSegmentation = req => {
+async function updateEditingSession(req, res, next) {
+  if (!Features.hasFeature('analytics')) {
+    return res.sendStatus(202)
+  }
+  const userId = SessionManager.getLoggedInUserId(req.session)
+  const { projectId } = req.params
+  const segmentation = _getSegmentation(req)
+  let countryCode = null
+
+  if (userId) {
+    try {
+      const geoDetails = await GeoIpLookup.promises.getDetails(req.ip)
+      if (geoDetails && geoDetails.country_code) {
+        countryCode = geoDetails.country_code
+      }
+      AnalyticsManager.updateEditingSession(
+        userId,
+        projectId,
+        countryCode,
+        segmentation
+      )
+    } catch (error) {
+      metrics.inc('analytics_geo_ip_lookup_errors')
+    }
+  }
+  res.sendStatus(202)
+}
+
+function recordEvent(req, res, next) {
+  if (!Features.hasFeature('analytics')) {
+    return res.sendStatus(202)
+  }
+  delete req.body._csrf
+  AnalyticsManager.recordEventForSession(
+    req.session,
+    req.params.event,
+    req.body
+  )
+  res.sendStatus(202)
+}
+
+function _getSegmentation(req) {
   const segmentation = req.body ? req.body.segmentation : null
   const cleanedSegmentation = {}
   if (
@@ -19,43 +60,6 @@ const getSegmentation = req => {
 }
 
 module.exports = {
-  updateEditingSession(req, res, next) {
+  updateEditingSession,
-    if (!Features.hasFeature('analytics')) {
+  recordEvent,
-      return res.sendStatus(202)
-    }
-    const userId = SessionManager.getLoggedInUserId(req.session)
-    const { projectId } = req.params
-    const segmentation = getSegmentation(req)
-    let countryCode = null
-
-    if (userId) {
-      GeoIpLookup.getDetails(req.ip, function (err, geoDetails) {
-        if (err) {
-          metrics.inc('analytics_geo_ip_lookup_errors')
-        } else if (geoDetails && geoDetails.country_code) {
-          countryCode = geoDetails.country_code
-        }
-        AnalyticsManager.updateEditingSession(
-          userId,
-          projectId,
-          countryCode,
-          segmentation
-        )
-      })
-    }
-    res.sendStatus(202)
-  },
-
-  recordEvent(req, res, next) {
-    if (!Features.hasFeature('analytics')) {
-      return res.sendStatus(202)
-    }
-    delete req.body._csrf
-    AnalyticsManager.recordEventForSession(
-      req.session,
-      req.params.event,
-      req.body
-    )
-    res.sendStatus(202)
-  },
 }

services/web/app/src/Features/Analytics/AnalyticsRouter.js

@@ -2,6 +2,7 @@ const AuthenticationController = require('./../Authentication/AuthenticationCont
 const AnalyticsController = require('./AnalyticsController')
 const AnalyticsProxy = require('./AnalyticsProxy')
 const RateLimiterMiddleware = require('./../Security/RateLimiterMiddleware')
+const { expressify } = require('../../util/promises')
 
 module.exports = {
   apply(webRouter, privateApiRouter, publicApiRouter) {
@@ -23,7 +24,7 @@ module.exports = {
         maxRequests: 20,
         timeInterval: 60,
       }),
-      AnalyticsController.updateEditingSession
+      expressify(AnalyticsController.updateEditingSession)
     )
 
     publicApiRouter.use(

services/web/test/unit/src/Analytics/AnalyticsControllerTests.js

@@ -25,7 +25,9 @@ describe('AnalyticsController', function () {
         '../Authentication/SessionManager': this.SessionManager,
         '../../infrastructure/Features': this.Features,
         '../../infrastructure/GeoIpLookup': (this.GeoIpLookup = {
-          getDetails: sinon.stub(),
+          promises: {
+            getDetails: sinon.stub().resolves(),
+          },
         }),
       },
     })
@@ -49,19 +51,21 @@ describe('AnalyticsController', function () {
           },
         },
       }
-      this.GeoIpLookup.getDetails = sinon
+      this.GeoIpLookup.promises.getDetails = sinon
         .stub()
-        .callsArgWith(1, null, { country_code: 'XY' })
+        .resolves({ country_code: 'XY' })
     })
 
-    it('delegates to the AnalyticsManager', function (done) {
+    it('delegates to the AnalyticsManager', async function () {
       this.SessionManager.getLoggedInUserId.returns('1234')
-      this.controller.updateEditingSession(this.req, this.res)
+      await this.controller.updateEditingSession(this.req, this.res)
-
+      sinon.assert.calledWith(
-      this.AnalyticsManager.updateEditingSession
+        this.AnalyticsManager.updateEditingSession,
-        .calledWith('1234', 'a project id', 'XY', { editorType: 'abc' })
+        '1234',
-        .should.equal(true)
+        'a project id',
-      done()
+        'XY',
+        { editorType: 'abc' }
+      )
     })
   })
 
@@ -86,17 +90,23 @@ describe('AnalyticsController', function () {
 
     it('should use the session', function (done) {
       this.controller.recordEvent(this.req, this.res)
-      this.AnalyticsManager.recordEventForSession
+      sinon.assert.calledWith(
-        .calledWith(this.req.session, this.req.params.event, this.expectedData)
+        this.AnalyticsManager.recordEventForSession,
-        .should.equal(true)
+        this.req.session,
+        this.req.params.event,
+        this.expectedData
+      )
       done()
     })
 
     it('should remove the CSRF token before sending to the manager', function (done) {
       this.controller.recordEvent(this.req, this.res)
-      this.AnalyticsManager.recordEventForSession
+      sinon.assert.calledWith(
-        .calledWith(this.req.session, this.req.params.event, this.expectedData)
+        this.AnalyticsManager.recordEventForSession,
-        .should.equal(true)
+        this.req.session,
+        this.req.params.event,
+        this.expectedData
+      )
       done()
     })
   })