Merge pull request #13161 from overleaf/jdt-async-await-contacts

Async-Await the Contact

GitOrigin-RevId: 00971c6880a53b65c68a1365aa9028a13209bfd0

services/web/app/src/Features/Contacts/ContactController.js

@@ -1,92 +1,60 @@
-/* eslint-disable
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let ContactsController
 const SessionManager = require('../Authentication/SessionManager')
 const ContactManager = require('./ContactManager')
 const UserGetter = require('../User/UserGetter')
-const logger = require('@overleaf/logger')
 const Modules = require('../../infrastructure/Modules')
+const { expressify } = require('../../util/promises')
 
-module.exports = ContactsController = {
-  getContacts(req, res, next) {
-    const userId = SessionManager.getLoggedInUserId(req.session)
-    return ContactManager.getContactIds(
-      userId,
-      { limit: 50 },
-      function (error, contactIds) {
-        if (error != null) {
-          return next(error)
-        }
-        return UserGetter.getUsers(
-          contactIds,
-          {
-            email: 1,
-            first_name: 1,
-            last_name: 1,
-            holdingAccount: 1,
-          },
-          function (error, contacts) {
-            if (error != null) {
-              return next(error)
-            }
-
-            // UserGetter.getUsers may not preserve order so put them back in order
-            const positions = {}
-            for (let i = 0; i < contactIds.length; i++) {
-              const contactId = contactIds[i]
-              positions[contactId] = i
-            }
-            contacts.sort(
-              (a, b) =>
-                positions[a._id != null ? a._id.toString() : undefined] -
-                positions[b._id != null ? b._id.toString() : undefined]
-            )
-
-            // Don't count holding accounts to discourage users from repeating mistakes (mistyped or wrong emails, etc)
-            contacts = contacts.filter(c => !c.holdingAccount)
-
-            contacts = contacts.map(ContactsController._formatContact)
-
-            return Modules.hooks.fire(
-              'getContacts',
-              userId,
-              contacts,
-              function (error, additionalContacts) {
-                if (error != null) {
-                  return next(error)
-                }
-                contacts = contacts.concat(
-                  ...Array.from(additionalContacts || [])
-                )
-                return res.json({
-                  contacts,
-                })
-              }
-            )
-          }
-        )
-      }
-    )
-  },
-
-  _formatContact(contact) {
-    return {
-      id: contact._id != null ? contact._id.toString() : undefined,
-      email: contact.email || '',
-      first_name: contact.first_name || '',
-      last_name: contact.last_name || '',
-      type: 'user',
-    }
-  },
+function _formatContact(contact) {
+  return {
+    id: contact._id?.toString(),
+    email: contact.email || '',
+    first_name: contact.first_name || '',
+    last_name: contact.last_name || '',
+    type: 'user',
+  }
+}
+
+async function getContacts(req, res) {
+  const userId = SessionManager.getLoggedInUserId(req.session)
+
+  const contactIds = await ContactManager.promises.getContactIds(userId, {
+    limit: 50,
+  })
+
+  let contacts = await UserGetter.promises.getUsers(contactIds, {
+    email: 1,
+    first_name: 1,
+    last_name: 1,
+    holdingAccount: 1,
+  })
+
+  // UserGetter.getUsers may not preserve order so put them back in order
+  const positions = {}
+  for (let i = 0; i < contactIds.length; i++) {
+    const contactId = contactIds[i]
+    positions[contactId] = i
+  }
+  contacts.sort(
+    (a, b) => positions[a._id?.toString()] - positions[b._id?.toString()]
+  )
+
+  // Don't count holding accounts to discourage users from repeating mistakes (mistyped or wrong emails, etc)
+  contacts = contacts.filter(c => !c.holdingAccount)
+
+  contacts = contacts.map(_formatContact)
+
+  const additionalContacts = await Modules.promises.hooks.fire(
+    'getContacts',
+    userId,
+    contacts
+  )
+
+  contacts = contacts.concat(...(additionalContacts || []))
+  return res.json({
+    contacts,
+  })
+}
+
+module.exports = {
+  getContacts: expressify(getContacts),
 }

services/web/app/src/Features/Contacts/ContactManager.js

@@ -1,90 +1,64 @@
-/* eslint-disable
-    n/handle-callback-err,
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let ContactManager
+const { callbackify } = require('util')
 const OError = require('@overleaf/o-error')
-const request = require('request')
+const fetch = require('node-fetch')
 const settings = require('@overleaf/settings')
 
-module.exports = ContactManager = {
-  getContactIds(userId, options, callback) {
-    if (options == null) {
-      options = { limits: 50 }
-    }
-    if (callback == null) {
-      callback = function () {}
-    }
-    const url = `${settings.apis.contacts.url}/user/${userId}/contacts`
-    return request.get(
-      {
-        url,
-        qs: options,
-        json: true,
-        jar: false,
-      },
-      function (error, res, data) {
-        if (error != null) {
-          return callback(error)
-        }
-        if (res.statusCode >= 200 && res.statusCode < 300) {
-          return callback(
-            null,
-            (data != null ? data.contact_ids : undefined) || []
-          )
-        } else {
-          error = new OError(
-            `contacts api responded with non-success code: ${res.statusCode}`,
-            { user_id: userId }
-          )
-          return callback(error)
-        }
-      }
-    )
-  },
+async function getContactIds(userId, options) {
+  options = options ?? { limit: 50 }
 
-  addContact(userId, contactId, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
-    const url = `${settings.apis.contacts.url}/user/${userId}/contacts`
-    return request.post(
+  const url = new URL(`${settings.apis.contacts.url}/user/${userId}/contacts`)
+
+  for (const [key, val] of Object.entries(options)) {
+    url.searchParams.set(key, val)
+  }
+
+  const response = await fetch(url.toString(), {
+    method: 'GET',
+    headers: { Accept: 'application/json' },
+  })
+
+  const body = await response.json()
+
+  if (!response.ok) {
+    throw new OError(
+      `contacts api responded with non-success code: ${response.statusCode}`,
+      { user_id: userId }
+    )
+  }
+
+  return body?.contact_ids || []
+}
+
+async function addContact(userId, contactId) {
+  const url = new URL(`${settings.apis.contacts.url}/user/${userId}/contacts`)
+  const response = await fetch(url.toString(), {
+    method: 'POST',
+    headers: {
+      'Content-Type': 'application/json',
+      Accept: 'application/json',
+    },
+    body: JSON.stringify({ contact_id: contactId }),
+  })
+
+  const body = await response.json()
+  if (!response.ok) {
+    throw new OError(
+      `contacts api responded with non-success code: ${response.statusCode}`,
       {
-        url,
-        json: {
-          contact_id: contactId,
-        },
-        jar: false,
-      },
-      function (error, res, data) {
-        if (error != null) {
-          return callback(error)
-        }
-        if (res.statusCode >= 200 && res.statusCode < 300) {
-          return callback(
-            null,
-            (data != null ? data.contact_ids : undefined) || []
-          )
-        } else {
-          error = new OError(
-            `contacts api responded with non-success code: ${res.statusCode}`,
-            {
-              user_id: userId,
-              contact_id: contactId,
-            }
-          )
-          return callback(error)
-        }
+        user_id: userId,
+        contact_id: contactId,
       }
     )
+  }
+
+  return body?.contact_ids || []
+}
+
+module.exports = {
+  getContactIds: callbackify(getContactIds),
+  addContact: callbackify(addContact),
+  promises: {
+    getContactIds,
+    addContact,
   },
 }

services/web/test/unit/src/Contact/ContactControllerTests.js

@@ -1,18 +1,5 @@
-/* eslint-disable
-    max-len,
-    no-dupe-keys,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const sinon = require('sinon')
-const { assert, expect } = require('chai')
+const { expect } = require('chai')
 const modulePath = '../../../../app/src/Features/Contacts/ContactController.js'
 const SandboxedModule = require('sandboxed-module')
 const MockResponse = require('../helpers/MockResponse')
@@ -22,14 +9,15 @@ describe('ContactController', function () {
     this.SessionManager = { getLoggedInUserId: sinon.stub() }
     this.ContactController = SandboxedModule.require(modulePath, {
       requires: {
-        '../User/UserGetter': (this.UserGetter = {}),
-        './ContactManager': (this.ContactManager = {}),
+        '../User/UserGetter': (this.UserGetter = { promises: {} }),
+        './ContactManager': (this.ContactManager = { promises: {} }),
         '../Authentication/SessionManager': (this.SessionManager = {}),
-        '../../infrastructure/Modules': (this.Modules = { hooks: {} }),
+        '../../infrastructure/Modules': (this.Modules = {
+          promises: { hooks: {} },
+        }),
       },
     })
 
-    this.next = sinon.stub()
     this.req = {}
     this.res = new MockResponse()
   })
@@ -63,63 +51,77 @@ describe('ContactController', function () {
         },
       ]
       this.SessionManager.getLoggedInUserId = sinon.stub().returns(this.user_id)
-      this.ContactManager.getContactIds = sinon
+      this.ContactManager.promises.getContactIds = sinon
         .stub()
-        .callsArgWith(2, null, this.contact_ids)
-      this.UserGetter.getUsers = sinon
-        .stub()
-        .callsArgWith(2, null, this.contacts)
-      this.Modules.hooks.fire = sinon.stub().callsArg(3)
-
-      return this.ContactController.getContacts(this.req, this.res, this.next)
+        .resolves(this.contact_ids)
+      this.UserGetter.promises.getUsers = sinon.stub().resolves(this.contacts)
+      this.Modules.promises.hooks.fire = sinon.stub()
     })
 
-    it('should look up the logged in user id', function () {
-      return this.SessionManager.getLoggedInUserId
+    it('should look up the logged in user id', async function () {
+      this.ContactController.getContacts(this.req, this.res)
+      this.SessionManager.getLoggedInUserId
         .calledWith(this.req.session)
         .should.equal(true)
     })
 
-    it('should get the users contact ids', function () {
-      return this.ContactManager.getContactIds
-        .calledWith(this.user_id, { limit: 50 })
-        .should.equal(true)
+    it('should get the users contact ids', async function () {
+      this.res.callback = () => {
+        expect(
+          this.ContactManager.promises.getContactIds
+        ).to.have.been.calledWith(this.user_id, { limit: 50 })
+      }
+      this.ContactController.getContacts(this.req, this.res)
     })
 
-    it('should populate the users contacts ids', function () {
-      return this.UserGetter.getUsers
-        .calledWith(this.contact_ids, {
-          email: 1,
-          first_name: 1,
-          last_name: 1,
-          holdingAccount: 1,
-        })
-        .should.equal(true)
+    it('should populate the users contacts ids', function (done) {
+      this.res.callback = () => {
+        expect(this.UserGetter.promises.getUsers).to.have.been.calledWith(
+          this.contact_ids,
+          {
+            email: 1,
+            first_name: 1,
+            last_name: 1,
+            holdingAccount: 1,
+          }
+        )
+        done()
+      }
+      this.ContactController.getContacts(this.req, this.res, done)
     })
 
-    it('should fire the getContact module hook', function () {
-      return this.Modules.hooks.fire
-        .calledWith('getContacts', this.user_id)
-        .should.equal(true)
+    it('should fire the getContact module hook', function (done) {
+      this.res.callback = () => {
+        expect(this.Modules.promises.hooks.fire).to.have.been.calledWith(
+          'getContacts',
+          this.user_id
+        )
+        done()
+      }
+      this.ContactController.getContacts(this.req, this.res, done)
     })
 
-    it('should return a formatted list of contacts in contact list order, without holding accounts', function () {
-      return this.res.json.args[0][0].contacts.should.deep.equal([
-        {
-          id: 'contact-1',
-          email: 'joe@example.com',
-          first_name: 'Joe',
-          last_name: 'Example',
-          type: 'user',
-        },
-        {
-          id: 'contact-3',
-          email: 'jim@example.com',
-          first_name: 'Jim',
-          last_name: 'Example',
-          type: 'user',
-        },
-      ])
+    it('should return a formatted list of contacts in contact list order, without holding accounts', function (done) {
+      this.res.callback = () => {
+        this.res.json.args[0][0].contacts.should.deep.equal([
+          {
+            id: 'contact-1',
+            email: 'joe@example.com',
+            first_name: 'Joe',
+            last_name: 'Example',
+            type: 'user',
+          },
+          {
+            id: 'contact-3',
+            email: 'jim@example.com',
+            first_name: 'Jim',
+            last_name: 'Example',
+            type: 'user',
+          },
+        ])
+        done()
+      }
+      this.ContactController.getContacts(this.req, this.res, done)
     })
   })
 })

services/web/test/unit/src/Contact/ContactManagerTests.js

@@ -1,158 +1,114 @@
-/* eslint-disable
-    max-len,
-    no-return-assign,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
+const { expect } = require('chai')
 const sinon = require('sinon')
 const modulePath = '../../../../app/src/Features/Contacts/ContactManager'
 const SandboxedModule = require('sandboxed-module')
 
 describe('ContactManager', function () {
   beforeEach(function () {
+    this.user_id = 'user-id-123'
+    this.contact_id = 'contact-id-123'
+    this.contact_ids = ['mock', 'contact_ids']
+    this.qs = new URLSearchParams({ limit: 42 })
+    this.fetch = sinon.stub()
     this.ContactManager = SandboxedModule.require(modulePath, {
       requires: {
-        request: (this.request = sinon.stub()),
+        'node-fetch': this.fetch,
         '@overleaf/settings': (this.settings = {
           apis: {
             contacts: {
-              url: 'contacts.sharelatex.com',
+              url: 'http://contacts.sharelatex.com',
             },
           },
         }),
       },
     })
-
-    this.user_id = 'user-id-123'
-    this.contact_id = 'contact-id-123'
-    return (this.callback = sinon.stub())
   })
 
   describe('getContacts', function () {
     describe('with a successful response code', function () {
-      beforeEach(function () {
-        this.request.get = sinon
-          .stub()
-          .callsArgWith(
-            1,
-            null,
-            { statusCode: 204 },
-            { contact_ids: (this.contact_ids = ['mock', 'contact_ids']) }
-          )
-        return this.ContactManager.getContactIds(
+      beforeEach(async function () {
+        this.response = {
+          ok: true,
+          json: sinon.stub().resolves({ contact_ids: this.contact_ids }),
+        }
+        this.fetch.resolves(this.response)
+
+        this.result = await this.ContactManager.promises.getContactIds(
           this.user_id,
-          { limit: 42 },
-          this.callback
+          { limit: 42 }
         )
       })
 
       it('should get the contacts from the contacts api', function () {
-        return this.request.get
-          .calledWith({
-            url: `${this.settings.apis.contacts.url}/user/${this.user_id}/contacts`,
-            qs: { limit: 42 },
-            json: true,
-            jar: false,
+        this.fetch.should.have.been.calledWithMatch(
+          `${this.settings.apis.contacts.url}/user/${this.user_id}/contacts?${this.qs}`,
+          sinon.match({
+            method: 'GET',
+            headers: { Accept: 'application/json' },
           })
-          .should.equal(true)
+        )
       })
 
-      it('should call the callback with the contatcs', function () {
-        return this.callback
-          .calledWith(null, this.contact_ids)
-          .should.equal(true)
+      it('should return the contacts', function () {
+        this.result.should.equal(this.contact_ids)
       })
     })
 
     describe('with a failed response code', function () {
-      beforeEach(function () {
-        this.request.get = sinon
-          .stub()
-          .callsArgWith(1, null, { statusCode: 500 }, null)
-        return this.ContactManager.getContactIds(
-          this.user_id,
-          { limit: 42 },
-          this.callback
-        )
+      beforeEach(async function () {
+        this.response = {
+          ok: false,
+          statusCode: 500,
+          json: sinon.stub().resolves({ contact_ids: this.contact_ids }),
+        }
+        this.fetch.resolves(this.response)
       })
 
-      it('should call the callback with an error', function () {
-        return this.callback
-          .calledWith(
-            sinon.match
-              .instanceOf(Error)
-              .and(
-                sinon.match.has(
-                  'message',
-                  'contacts api responded with non-success code: 500'
-                )
-              )
-          )
-          .should.equal(true)
+      it('should reject the promise', async function () {
+        await expect(
+          this.ContactManager.promises.getContactIds(this.user_id, {
+            limit: 42,
+          })
+        ).to.be.rejectedWith(
+          'contacts api responded with non-success code: 500'
+        )
       })
     })
   })
 
   describe('addContact', function () {
     describe('with a successful response code', function () {
-      beforeEach(function () {
-        this.request.post = sinon
-          .stub()
-          .callsArgWith(1, null, { statusCode: 200 }, null)
-        return this.ContactManager.addContact(
+      beforeEach(async function () {
+        this.response = {
+          ok: true,
+          json: sinon.stub().resolves({ contact_ids: this.contact_ids }),
+        }
+        this.fetch.resolves(this.response)
+
+        this.result = await this.ContactManager.promises.addContact(
           this.user_id,
-          this.contact_id,
-          this.callback
+          this.contact_id
         )
       })
 
       it('should add the contacts for the user in the contacts api', function () {
-        return this.request.post
-          .calledWith({
-            url: `${this.settings.apis.contacts.url}/user/${this.user_id}/contacts`,
-            json: {
-              contact_id: this.contact_id,
+        this.fetch.should.have.been.calledWithMatch(
+          `${this.settings.apis.contacts.url}/user/${this.user_id}/contacts`,
+          sinon.match({
+            method: 'POST',
+            headers: {
+              'Content-Type': 'application/json',
+              Accept: 'application/json',
             },
-            jar: false,
+            body: JSON.stringify({
+              contact_id: this.contact_id,
+            }),
           })
-          .should.equal(true)
-      })
-
-      it('should call the callback', function () {
-        return this.callback.called.should.equal(true)
-      })
-    })
-
-    describe('with a failed response code', function () {
-      beforeEach(function () {
-        this.request.post = sinon
-          .stub()
-          .callsArgWith(1, null, { statusCode: 500 }, null)
-        return this.ContactManager.addContact(
-          this.user_id,
-          this.contact_id,
-          this.callback
         )
       })
 
-      it('should call the callback with an error', function () {
-        return this.callback
-          .calledWith(
-            sinon.match
-              .instanceOf(Error)
-              .and(
-                sinon.match.has(
-                  'message',
-                  'contacts api responded with non-success code: 500'
-                )
-              )
-          )
-          .should.equal(true)
+      it('should call the callback', function () {
+        this.result.should.equal(this.contact_ids)
       })
     })
   })