From dfd1652c35a0fb0c1de422eff10dcb638ae5dd92 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Fri, 17 May 2024 10:45:19 -0400 Subject: [PATCH] Merge pull request #18375 from overleaf/em-promisify-chat-api-handler Promisify ChatApiHandler GitOrigin-RevId: 83cedb14b5e2b187fb2cb02fcbf888ada5a599b1 --- .../app/src/Features/Chat/ChatApiHandler.js | 278 +++++++----------- .../test/unit/src/Chat/ChatApiHandlerTests.js | 135 ++++----- 2 files changed, 162 insertions(+), 251 deletions(-) diff --git a/services/web/app/src/Features/Chat/ChatApiHandler.js b/services/web/app/src/Features/Chat/ChatApiHandler.js index fbd12f8488..16edb81fea 100644 --- a/services/web/app/src/Features/Chat/ChatApiHandler.js +++ b/services/web/app/src/Features/Chat/ChatApiHandler.js @@ -1,190 +1,120 @@ -/* eslint-disable - n/handle-callback-err, - max-len, -*/ -// 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 ChatApiHandler -const OError = require('@overleaf/o-error') -const request = require('request') +// @ts-check + +const { fetchJson, fetchNothing } = require('@overleaf/fetch-utils') const settings = require('@overleaf/settings') -const { promisify } = require('util') +const { callbackify } = require('util') -function getThreads(projectId, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( +async function getThreads(projectId) { + return await fetchJson(chatApiUrl(`/project/${projectId}/threads`)) +} + +async function destroyProject(projectId) { + await fetchNothing(chatApiUrl(`/project/${projectId}`), { method: 'DELETE' }) +} + +async function sendGlobalMessage(projectId, userId, content) { + const message = await fetchJson( + chatApiUrl(`/project/${projectId}/messages`), { - url: `${settings.apis.chat.internal_url}/project/${projectId}/threads`, - method: 'GET', - json: true, - }, - callback + method: 'POST', + json: { user_id: userId, content }, + } + ) + return message +} + +async function getGlobalMessages(projectId, limit, before) { + const url = chatApiUrl(`/project/${projectId}/messages`) + if (limit != null) { + url.searchParams.set('limit', limit) + } + if (before != null) { + url.searchParams.set('before', before) + } + + return await fetchJson(url) +} + +async function sendComment(projectId, threadId, userId, content) { + const comment = await fetchJson( + chatApiUrl(`/project/${projectId}/thread/${threadId}/messages`), + { + method: 'POST', + json: { user_id: userId, content }, + } + ) + return comment +} + +async function resolveThread(projectId, threadId, userId) { + await fetchNothing( + chatApiUrl(`/project/${projectId}/thread/${threadId}/resolve`), + { + method: 'POST', + json: { user_id: userId }, + } ) } -function destroyProject(projectId, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}`, - method: 'DELETE', - }, - callback +async function reopenThread(projectId, threadId) { + await fetchNothing( + chatApiUrl(`/project/${projectId}/thread/${threadId}/reopen`), + { method: 'POST' } ) } -module.exports = ChatApiHandler = { - _apiRequest(opts, callback) { - if (callback == null) { - callback = function () {} +async function deleteThread(projectId, threadId) { + await fetchNothing(chatApiUrl(`/project/${projectId}/thread/${threadId}`), { + method: 'DELETE', + }) +} + +async function editMessage(projectId, threadId, messageId, userId, content) { + await fetchNothing( + chatApiUrl( + `/project/${projectId}/thread/${threadId}/messages/${messageId}/edit` + ), + { + method: 'POST', + json: { content, userId }, } - return request(opts, function (error, response, data) { - if (error != null) { - return callback(error) - } - if (response.statusCode >= 200 && response.statusCode < 300) { - return callback(null, data) - } else { - error = new OError( - `chat api returned non-success code: ${response.statusCode}`, - opts - ) - error.statusCode = response.statusCode - return callback(error) - } - }) - }, + ) +} - sendGlobalMessage(projectId, userId, content, callback) { - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/messages`, - method: 'POST', - json: { user_id: userId, content }, - }, - callback - ) - }, +async function deleteMessage(projectId, threadId, messageId) { + await fetchNothing( + chatApiUrl( + `/project/${projectId}/thread/${threadId}/messages/${messageId}` + ), + { method: 'DELETE' } + ) +} - getGlobalMessages(projectId, limit, before, callback) { - const qs = {} - if (limit != null) { - qs.limit = limit - } - if (before != null) { - qs.before = before - } - - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/messages`, - method: 'GET', - qs, - json: true, - }, - callback - ) - }, - - sendComment(projectId, threadId, userId, content, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/thread/${threadId}/messages`, - method: 'POST', - json: { user_id: userId, content }, - }, - callback - ) - }, - - resolveThread(projectId, threadId, userId, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/thread/${threadId}/resolve`, - method: 'POST', - json: { user_id: userId }, - }, - callback - ) - }, - - reopenThread(projectId, threadId, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/thread/${threadId}/reopen`, - method: 'POST', - }, - callback - ) - }, - - deleteThread(projectId, threadId, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/thread/${threadId}`, - method: 'DELETE', - }, - callback - ) - }, - - editMessage(projectId, threadId, messageId, userId, content, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/thread/${threadId}/messages/${messageId}/edit`, - method: 'POST', - json: { - content, - userId, - }, - }, - callback - ) - }, - - deleteMessage(projectId, threadId, messageId, callback) { - if (callback == null) { - callback = function () {} - } - return ChatApiHandler._apiRequest( - { - url: `${settings.apis.chat.internal_url}/project/${projectId}/thread/${threadId}/messages/${messageId}`, - method: 'DELETE', - }, - callback - ) - }, - - getThreads, - destroyProject, +function chatApiUrl(path) { + return new URL(path, settings.apis.chat.internal_url) +} +module.exports = { + getThreads: callbackify(getThreads), + destroyProject: callbackify(destroyProject), + sendGlobalMessage: callbackify(sendGlobalMessage), + getGlobalMessages: callbackify(getGlobalMessages), + sendComment: callbackify(sendComment), + resolveThread: callbackify(resolveThread), + reopenThread: callbackify(reopenThread), + deleteThread: callbackify(deleteThread), + editMessage: callbackify(editMessage), + deleteMessage: callbackify(deleteMessage), promises: { - getThreads: promisify(getThreads), - destroyProject: promisify(destroyProject), + getThreads, + destroyProject, + sendGlobalMessage, + getGlobalMessages, + sendComment, + resolveThread, + reopenThread, + deleteThread, + editMessage, + deleteMessage, }, } diff --git a/services/web/test/unit/src/Chat/ChatApiHandlerTests.js b/services/web/test/unit/src/Chat/ChatApiHandlerTests.js index 88f2006adc..560e9cf023 100644 --- a/services/web/test/unit/src/Chat/ChatApiHandlerTests.js +++ b/services/web/test/unit/src/Chat/ChatApiHandlerTests.js @@ -1,93 +1,83 @@ -/* eslint-disable - max-len, - 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 SandboxedModule = require('sandboxed-module') -const assert = require('assert') const path = require('path') const sinon = require('sinon') -const modulePath = path.join( +const { expect } = require('chai') +const { RequestFailedError } = require('@overleaf/fetch-utils') + +const MODULE_PATH = path.join( __dirname, '../../../../app/src/Features/Chat/ChatApiHandler' ) -const { expect } = require('chai') describe('ChatApiHandler', function () { beforeEach(function () { this.settings = { apis: { chat: { - internal_url: 'chat.overleaf.env', + internal_url: 'http://chat.overleaf.env', }, }, } - this.request = sinon.stub() - this.ChatApiHandler = SandboxedModule.require(modulePath, { + this.FetchUtils = { + fetchJson: sinon.stub(), + fetchNothing: sinon.stub().resolves(), + } + this.ChatApiHandler = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/settings': this.settings, - request: this.request, + '@overleaf/fetch-utils': this.FetchUtils, }, }) this.project_id = '3213213kl12j' this.user_id = '2k3jlkjs9' this.content = 'my message here' - return (this.callback = sinon.stub()) }) describe('sendGlobalMessage', function () { describe('successfully', function () { - beforeEach(function () { + beforeEach(async function () { this.message = { mock: 'message' } - this.request.callsArgWith(1, null, { statusCode: 200 }, this.message) - return this.ChatApiHandler.sendGlobalMessage( + this.FetchUtils.fetchJson.resolves(this.message) + this.result = await this.ChatApiHandler.promises.sendGlobalMessage( this.project_id, this.user_id, - this.content, - this.callback + this.content ) }) it('should post the data to the chat api', function () { - return this.request - .calledWith({ - url: `${this.settings.apis.chat.internal_url}/project/${this.project_id}/messages`, + this.FetchUtils.fetchJson.should.have.been.calledWith( + sinon.match( + url => + url.toString() === + `${this.settings.apis.chat.internal_url}/project/${this.project_id}/messages` + ), + { method: 'POST', json: { content: this.content, user_id: this.user_id, }, - }) - .should.equal(true) + } + ) }) it('should return the message from the post', function () { - return this.callback.calledWith(null, this.message).should.equal(true) + expect(this.result).to.deep.equal(this.message) }) }) describe('with a non-success status code', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 500 }) - return this.ChatApiHandler.sendGlobalMessage( - this.project_id, - this.user_id, - this.content, - this.callback - ) - }) - - it('should return an error', function () { - expect(this.callback).to.have.been.calledWith( - sinon.match.instanceOf(Error).and(sinon.match.has('statusCode', 500)) - ) + beforeEach(async function () { + this.error = new RequestFailedError('some-url', {}, { status: 500 }) + this.FetchUtils.fetchJson.rejects(this.error) + await expect( + this.ChatApiHandler.promises.sendGlobalMessage( + this.project_id, + this.user_id, + this.content + ) + ).to.be.rejectedWith(this.error) }) }) }) @@ -96,54 +86,45 @@ describe('ChatApiHandler', function () { beforeEach(function () { this.messages = [{ mock: 'message' }] this.limit = 30 - return (this.before = '1234') + this.before = '1234' }) describe('successfully', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 200 }, this.messages) - return this.ChatApiHandler.getGlobalMessages( + beforeEach(async function () { + this.FetchUtils.fetchJson.resolves(this.messages) + this.result = await this.ChatApiHandler.promises.getGlobalMessages( this.project_id, this.limit, - this.before, - this.callback + this.before ) }) it('should make get request for room to chat api', function () { - return this.request - .calledWith({ - method: 'GET', - url: `${this.settings.apis.chat.internal_url}/project/${this.project_id}/messages`, - qs: { - limit: this.limit, - before: this.before, - }, - json: true, - }) - .should.equal(true) + this.FetchUtils.fetchJson.should.have.been.calledWith( + sinon.match( + url => + url.toString() === + `${this.settings.apis.chat.internal_url}/project/${this.project_id}/messages?limit=${this.limit}&before=${this.before}` + ) + ) }) it('should return the messages from the request', function () { - return this.callback.calledWith(null, this.messages).should.equal(true) + expect(this.result).to.deep.equal(this.messages) }) }) describe('with failure error code', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 500 }, null) - return this.ChatApiHandler.getGlobalMessages( - this.project_id, - this.limit, - this.before, - this.callback - ) - }) - - it('should return an error', function () { - expect(this.callback).to.have.been.calledWith( - sinon.match.instanceOf(Error).and(sinon.match.has('statusCode', 500)) - ) + beforeEach(async function () { + this.error = new RequestFailedError('some-url', {}, { status: 500 }) + this.FetchUtils.fetchJson.rejects(this.error) + await expect( + this.ChatApiHandler.getGlobalMessages( + this.project_id, + this.limit, + this.before + ) + ).to.be.rejectedWith(this.error) }) }) })