Merge pull request #4117 from overleaf/ab-geoiplookup-fix

Fix regression in GeoIpLookup + add tests

GitOrigin-RevId: 4bf0ecac7b6e5aaed0f56b0f0455d76923c094e1

services/web/app/src/infrastructure/GeoIpLookup.js

@@ -49,9 +49,8 @@ const EuroCountries = [
 _.each(EuroCountries, country => (currencyMappings[country] = 'EUR'))
 
 function getDetails(ip, callback) {
-  if (ip == null) {
+  if (!ip) {
-    const e = new Error('no ip passed')
+    return callback(new Error('no ip passed'))
-    return callback(e)
   }
   ip = ip.trim().split(' ')[0]
   const opts = {
@@ -78,12 +77,12 @@ function getCurrencyCode(ip, callback) {
       return callback(null, 'USD')
     }
     const countryCode =
-      ipDetails && ipDetails.countryCode
+      ipDetails && ipDetails.country_code
-        ? ipDetails.countryCode.toUpperCase()
+        ? ipDetails.country_code.toUpperCase()
         : undefined
     const currencyCode = currencyMappings[countryCode] || 'USD'
     logger.log({ ip, currencyCode, ipDetails }, 'got currencyCode for ip')
-    return callback(err, currencyCode, countryCode)
+    callback(err, currencyCode, countryCode)
   })
 }
 

services/web/test/unit/src/infrastructure/GeoIpLookupTest.js

@@ -0,0 +1,266 @@
+const SandboxedModule = require('sandboxed-module')
+const assert = require('assert')
+const path = require('path')
+const sinon = require('sinon')
+const modulePath = path.join(
+  __dirname,
+  '../../../../app/src/infrastructure/GeoIpLookup'
+)
+
+describe('GeoIpLookup', function () {
+  beforeEach(function () {
+    this.request = { get: sinon.stub() }
+    this.settings = {
+      apis: {
+        geoIpLookup: {
+          url: 'http://lookup.com',
+        },
+      },
+    }
+    this.GeoIpLookup = SandboxedModule.require(modulePath, {
+      requires: {
+        request: this.request,
+        'settings-sharelatex': this.settings,
+      },
+    })
+    this.ipAddress = '12.34.56.78'
+
+    this.stubbedResponse = {
+      ip: this.ipAddress,
+      country_code: 'GB',
+      country_name: 'United Kingdom',
+      region_code: 'H9',
+      region_name: 'London, City of',
+      city: 'London',
+      zipcode: 'SE16',
+      latitude: 51.0,
+      longitude: -0.0493,
+      metro_code: '',
+      area_code: '',
+    }
+  })
+
+  describe('getDetails', function () {
+    beforeEach(function () {
+      this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+    })
+
+    describe('callback', function () {
+      it('should request the details using the ip', function (done) {
+        this.GeoIpLookup.getDetails(this.ipAddress, err => {
+          this.request.get
+            .calledWith({
+              url: this.settings.apis.geoIpLookup.url + '/' + this.ipAddress,
+              timeout: 1000,
+              json: true,
+            })
+            .should.equal(true)
+          done()
+        })
+      })
+
+      it('should return the ip details', function (done) {
+        this.GeoIpLookup.getDetails(this.ipAddress, (err, returnedDetails) => {
+          assert.deepEqual(returnedDetails, this.stubbedResponse)
+          done()
+        })
+      })
+
+      it('should take the first ip in the string', function (done) {
+        this.GeoIpLookup.getDetails(
+          ` ${this.ipAddress} 123.123.123.123 234.234.234.234`,
+          err => {
+            this.request.get
+              .calledWith({
+                url: this.settings.apis.geoIpLookup.url + '/' + this.ipAddress,
+                timeout: 1000,
+                json: true,
+              })
+              .should.equal(true)
+            done()
+          }
+        )
+      })
+    })
+
+    describe('async', function () {
+      it('should request the details using the ip', async function () {
+        await this.GeoIpLookup.promises.getDetails(this.ipAddress)
+        this.request.get
+          .calledWith({
+            url: this.settings.apis.geoIpLookup.url + '/' + this.ipAddress,
+            timeout: 1000,
+            json: true,
+          })
+          .should.equal(true)
+      })
+
+      it('should return the ip details', async function () {
+        const returnedDetails = await this.GeoIpLookup.promises.getDetails(
+          this.ipAddress
+        )
+        assert.deepEqual(returnedDetails, this.stubbedResponse)
+      })
+
+      it('should take the first ip in the string', async function () {
+        await this.GeoIpLookup.promises.getDetails(
+          ` ${this.ipAddress} 123.123.123.123 234.234.234.234`
+        )
+        this.request.get
+          .calledWith({
+            url: this.settings.apis.geoIpLookup.url + '/' + this.ipAddress,
+            timeout: 1000,
+            json: true,
+          })
+          .should.equal(true)
+      })
+    })
+  })
+
+  describe('getCurrencyCode', function () {
+    describe('callback', function () {
+      it('should return GBP for GB country', function (done) {
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        this.GeoIpLookup.getCurrencyCode(
+          this.ipAddress,
+          (err, currencyCode) => {
+            currencyCode.should.equal('GBP')
+            done()
+          }
+        )
+      })
+
+      it('should return GBP for gb country', function (done) {
+        this.stubbedResponse.country_code = 'gb'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        this.GeoIpLookup.getCurrencyCode(
+          this.ipAddress,
+          (err, currencyCode) => {
+            currencyCode.should.equal('GBP')
+            done()
+          }
+        )
+      })
+
+      it('should return USD for US', function (done) {
+        this.stubbedResponse.country_code = 'US'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        this.GeoIpLookup.getCurrencyCode(
+          this.ipAddress,
+          (err, currencyCode) => {
+            currencyCode.should.equal('USD')
+            done()
+          }
+        )
+      })
+
+      it('should return EUR for DE', function (done) {
+        this.stubbedResponse.country_code = 'DE'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        this.GeoIpLookup.getCurrencyCode(
+          this.ipAddress,
+          (err, currencyCode) => {
+            currencyCode.should.equal('EUR')
+            done()
+          }
+        )
+      })
+
+      it('should default to USD if there is an error', function (done) {
+        this.request.get.callsArgWith(1, null, null, { error: true })
+        this.GeoIpLookup.getCurrencyCode(
+          this.ipAddress,
+          (err, currencyCode) => {
+            currencyCode.should.equal('USD')
+            done()
+          }
+        )
+      })
+
+      it('should default to USD if there are no details', function (done) {
+        this.request.get.callsArgWith(1, null, null, {})
+        this.GeoIpLookup.getCurrencyCode(
+          this.ipAddress,
+          (err, currencyCode) => {
+            currencyCode.should.equal('USD')
+            done()
+          }
+        )
+      })
+
+      it('should default to USD if there is no match for their country', function (done) {
+        this.stubbedResponse.country_code = 'Non existant'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        this.GeoIpLookup.getCurrencyCode(
+          this.ipAddress,
+          (err, currencyCode) => {
+            currencyCode.should.equal('USD')
+            done()
+          }
+        )
+      })
+    })
+
+    describe('async', function () {
+      it('should return GBP for GB country', async function () {
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        const currencyCode = await this.GeoIpLookup.promises.getCurrencyCode(
+          this.ipAddress
+        )
+        currencyCode.should.equal('GBP')
+      })
+
+      it('should return GBP for gb country', async function () {
+        this.stubbedResponse.country_code = 'gb'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        const currencyCode = await this.GeoIpLookup.promises.getCurrencyCode(
+          this.ipAddress
+        )
+        currencyCode.should.equal('GBP')
+      })
+
+      it('should return USD for US', async function () {
+        this.stubbedResponse.country_code = 'US'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        const currencyCode = await this.GeoIpLookup.promises.getCurrencyCode(
+          this.ipAddress
+        )
+        currencyCode.should.equal('USD')
+      })
+
+      it('should return EUR for DE', async function () {
+        this.stubbedResponse.country_code = 'DE'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        const currencyCode = await this.GeoIpLookup.promises.getCurrencyCode(
+          this.ipAddress
+        )
+        currencyCode.should.equal('EUR')
+      })
+
+      it('should default to USD if there is an error', async function () {
+        this.request.get.callsArgWith(1, null, null, { error: true })
+        const currencyCode = await this.GeoIpLookup.promises.getCurrencyCode(
+          this.ipAddress
+        )
+        currencyCode.should.equal('USD')
+      })
+
+      it('should default to USD if there are no details', async function () {
+        this.request.get.callsArgWith(1, null, null, {})
+        const currencyCode = await this.GeoIpLookup.promises.getCurrencyCode(
+          this.ipAddress
+        )
+        currencyCode.should.equal('USD')
+      })
+
+      it('should default to USD if there is no match for their country', async function () {
+        this.stubbedResponse.country_code = 'Non existant'
+        this.request.get.callsArgWith(1, null, null, this.stubbedResponse)
+        const currencyCode = await this.GeoIpLookup.promises.getCurrencyCode(
+          this.ipAddress
+        )
+        currencyCode.should.equal('USD')
+      })
+    })
+  })
+})