diff --git a/services/web/app/src/Features/Analytics/AnalyticsController.js b/services/web/app/src/Features/Analytics/AnalyticsController.js index 67ab97f657..40bb834109 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsController.js +++ b/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) { - 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) { - 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) - }, + updateEditingSession, + recordEvent, } diff --git a/services/web/app/src/Features/Analytics/AnalyticsRouter.js b/services/web/app/src/Features/Analytics/AnalyticsRouter.js index 90707a3eb7..3027d5961f 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsRouter.js +++ b/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( diff --git a/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js b/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js index ec1ec0b6db..16a9ad666d 100644 --- a/services/web/test/unit/src/Analytics/AnalyticsControllerTests.js +++ b/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) - - this.AnalyticsManager.updateEditingSession - .calledWith('1234', 'a project id', 'XY', { editorType: 'abc' }) - .should.equal(true) - done() + await this.controller.updateEditingSession(this.req, this.res) + sinon.assert.calledWith( + this.AnalyticsManager.updateEditingSession, + '1234', + 'a project id', + '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 - .calledWith(this.req.session, this.req.params.event, this.expectedData) - .should.equal(true) + sinon.assert.calledWith( + this.AnalyticsManager.recordEventForSession, + 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 - .calledWith(this.req.session, this.req.params.event, this.expectedData) - .should.equal(true) + sinon.assert.calledWith( + this.AnalyticsManager.recordEventForSession, + this.req.session, + this.req.params.event, + this.expectedData + ) done() }) })