Move the geolocation out of login, to the editingSession action

services/web/app/coffee/Features/Analytics/AnalyticsController.coffee

@@ -1,16 +1,18 @@
 AnalyticsManager = require "./AnalyticsManager"
 Errors = require "../Errors/Errors"
 AuthenticationController = require("../Authentication/AuthenticationController")
+GeoIpLookup = require '../../infrastructure/GeoIpLookup'
 
 module.exports = AnalyticsController =
 	updateEditingSession: (req, res, next) ->
 		userId    = AuthenticationController.getLoggedInUserId(req)
 		projectId = req.params.projectId
-		countryCode = req.session.countryCode || null
 
 		if userId?
-			AnalyticsManager.updateEditingSession userId, projectId, countryCode,  {}, (error) ->
-				respondWith(error, res, next)
+			GeoIpLookup.getDetails req.ip, (err, geoDetails) ->
+				countryCode = geoDetails?.country_code || null
+				AnalyticsManager.updateEditingSession userId, projectId, countryCode,  {}, (error) ->
+					respondWith(error, res, next)
 		else
 			res.send 204
 

services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee

@@ -46,6 +46,11 @@ module.exports =
 	updateEditingSession: (userId, projectId, countryCode, segmentation = {}, callback = (error) ->) ->
 		if userId+"" == settings.smokeTest?.userId+""
 			return callback()
+		query =
+			userId: userId
+			projectId: projectId
+		if countryCode
+			query.countryCode = countryCode
 		opts =
 			body:
 				segmentation: segmentation
@@ -53,10 +58,7 @@ module.exports =
 			method: "PUT"
 			timeout: 1000
 			url: "/editingSession"
-			qs:
-				userId: userId
-				projectId: projectId
-				countryCode: countryCode || undefined
+			qs: query
 			maxAttempts: 20
 			retryDelay: 5000
 		if settings.overleaf?

services/web/app/coffee/Features/Authentication/AuthenticationController.coffee

@@ -12,7 +12,6 @@ UserHandler = require("../User/UserHandler")
 UserSessionsManager = require("../User/UserSessionsManager")
 Analytics = require "../Analytics/AnalyticsManager"
 passport = require 'passport'
-GeoIpLookup = require '../../infrastructure/GeoIpLookup'
 
 module.exports = AuthenticationController =
 
@@ -48,17 +47,12 @@ module.exports = AuthenticationController =
 					req.session[key] = value
 				# copy to the old `session.user` location, for backward-comptability
 				req.session.user = req.session.passport.user
-				# try to geolocate the user to a country-code
-				GeoIpLookup.getDetails req.ip, (err, geoDetails) ->
+				req.session.save (err) ->
 					if err?
-						logger.err {err, ip: req.ip}, "error geolocating session, continuing"
-					req.session.countryCode = geoDetails?.country_code || null
-					req.session.save (err) ->
-						if err?
-							logger.err {user_id: user._id}, "error saving regenerated session after login"
-							return callback(err)
-						UserSessionsManager.trackSession(user, req.sessionID, () ->)
-						callback(err)
+						logger.err {user_id: user._id}, "error saving regenerated session after login"
+						return callback(err)
+					UserSessionsManager.trackSession(user, req.sessionID, () ->)
+					callback(err)
 
 	passportLogin: (req, res, next) ->
 		# This function is middleware which wraps the passport.authenticate middleware,

services/web/test/unit/coffee/Analytics/AnalyticsControllerTests.coffee

@@ -22,6 +22,8 @@ describe 'AnalyticsController', ->
 			"../Authentication/AuthenticationController":@AuthenticationController
 			"logger-sharelatex":
 				log:->
+			'../../infrastructure/GeoIpLookup': @GeoIpLookup =
+				getDetails: sinon.stub()
 
 		@res =
 			send:->
@@ -31,7 +33,8 @@ describe 'AnalyticsController', ->
 			@req =
 				params:
 					projectId: "a project id"
-				session: {countryCode: 'US'}
+			@GeoIpLookup.getDetails = sinon.stub()
+				.callsArgWith(1, null, {country_code: 'XY'})
 
 		it "delegates to the AnalyticsManager", (done) ->
 			@AuthenticationController.getLoggedInUserId.returns("1234")
@@ -40,7 +43,7 @@ describe 'AnalyticsController', ->
 			@AnalyticsManager.updateEditingSession.calledWith(
 				"1234",
 				"a project id",
-				'US',
+				'XY',
 				{}
 			).should.equal true
 			done()

services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee

@@ -28,8 +28,6 @@ describe "AuthenticationController", ->
 				trackSession: sinon.stub()
 				untrackSession: sinon.stub()
 				revokeAllUserSessions: sinon.stub().callsArgWith(1, null)
-			"../../infrastructure/GeoIpLookup": @GeoIpLookup =
-				getDetails: sinon.stub().callsArgWith(1, null, {})
 		@user =
 			_id: ObjectId()
 			email: @email = "USER@example.com"
@@ -174,8 +172,6 @@ describe "AuthenticationController", ->
 			@req.session.save = sinon.stub().callsArgWith(0, null)
 			@req.sessionStore = {generate: sinon.stub()}
 			@UserSessionsManager.trackSession = sinon.stub()
-			@GeoIpLookup.getDetails = sinon.stub()
-				.callsArgWith(1, null, @geoDetails = {country_code: 'US'})
 			@call = (callback) =>
 				@AuthenticationController.afterLoginSessionSetup @req, @user, callback
 
@@ -194,13 +190,6 @@ describe "AuthenticationController", ->
 				@UserSessionsManager.trackSession.callCount.should.equal 1
 				done()
 
-		it 'should call GeoIpLookup.getDetails', (done) ->
-			@call (err) =>
-				@GeoIpLookup.getDetails.callCount.should.equal 1
-				@req.session.countryCode.should.exist
-				@req.session.countryCode.should.equal @geoDetails.country_code
-				done()
-
 		it 'should call req.session.save', (done) ->
 			@call (err) =>
 				@req.session.save.callCount.should.equal 1
@@ -222,39 +211,6 @@ describe "AuthenticationController", ->
 					@UserSessionsManager.trackSession.callCount.should.equal 0
 					done()
 
-		describe 'when GeoIpLookup produces an error', ->
-			beforeEach ->
-				@GeoIpLookup.getDetails = sinon.stub()
-					.callsArgWith(1, new Error('woops'))
-
-			it 'should not produce an error', (done) ->
-				@call (err) =>
-					expect(err).to.not.exist
-					done()
-
-			it 'should still call req.session.save', (done) ->
-				@call (err) =>
-					@req.session.save.callCount.should.equal 1
-					expect(@req.session.countryCode).to.equal null
-					done()
-
-		describe 'when GeoIpLookup produces no data', ->
-			beforeEach ->
-				@GeoIpLookup.getDetails = sinon.stub()
-					.callsArgWith(1, null, undefined)
-
-			it 'should not produce an error', (done) ->
-				@call (err) =>
-					expect(err).to.not.exist
-					done()
-
-			it 'should still call req.session.save', (done) ->
-				@call (err) =>
-					@req.session.save.callCount.should.equal 1
-					expect(@req.session.countryCode).to.equal null
-					done()
-
-
 	describe 'getSessionUser', ->
 
 		it 'should get the user object from session', ->