From 45d44d2fe6213336d5187603781e7549428d2814 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe <5312836+lawshe@users.noreply.github.com> Date: Tue, 23 Jan 2024 08:45:37 -0600 Subject: [PATCH] Merge pull request #16444 from overleaf/jel-v1-api [web] Begin promisifying InstitutionsAPI GitOrigin-RevId: aa615e22d3fc8aa77255a26dcdab0e2ce28d93c0 --- .../Features/Institutions/InstitutionsAPI.js | 493 +++++++++++------- .../src/Institutions/InstitutionsAPITests.js | 125 +++-- 2 files changed, 391 insertions(+), 227 deletions(-) diff --git a/services/web/app/src/Features/Institutions/InstitutionsAPI.js b/services/web/app/src/Features/Institutions/InstitutionsAPI.js index 3e379ad410..12745ac239 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsAPI.js +++ b/services/web/app/src/Features/Institutions/InstitutionsAPI.js @@ -1,3 +1,4 @@ +const { callbackify } = require('util') const OError = require('@overleaf/o-error') const logger = require('@overleaf/logger') const metrics = require('@overleaf/metrics') @@ -9,210 +10,306 @@ const { V1ConnectionError, InvalidInstitutionalEmailError, } = require('../Errors/Errors') +const { fetchJson, fetchNothing } = require('@overleaf/fetch-utils') + +function _makeRequestOptions(options) { + const requestOptions = { + method: options.method, + basicAuth: { user: settings.apis.v1.user, password: settings.apis.v1.pass }, + signal: AbortSignal.timeout(settings.apis.v1.timeout), + } + + if (options.body) { + requestOptions.json = options.body + } + + return requestOptions +} + +function _responseErrorHandling(options, error) { + const status = error.response.status + + if (status >= 500) { + throw new V1ConnectionError({ + message: 'error getting affiliations from v1', + info: { + status, + body: error.body, + }, + }) + } + + let errorBody + + try { + if (error.body) { + errorBody = JSON.parse(error.body) + } + } catch (e) {} + + let errorMessage + if (errorBody?.errors) { + errorMessage = `${status}: ${errorBody.errors}` + } else { + errorMessage = `${options.defaultErrorMessage}: ${status}` + } + + throw new OError(errorMessage, { status }) +} + +async function _affiliationRequestFetchJson(options) { + if (!settings.apis.v1.url) { + return + } // service is not configured + + const url = `${settings.apis.v1.url}${options.path}` + + const requestOptions = _makeRequestOptions(options) + + try { + return await fetchJson(url, requestOptions) + } catch (error) { + _responseErrorHandling(options, error) + } +} + +async function _affiliationRequestFetchNothing(options) { + if (!settings.apis.v1.url) { + return + } // service is not configured + + const url = `${settings.apis.v1.url}${options.path}` + + const requestOptions = _makeRequestOptions(options) + + try { + await fetchNothing(url, requestOptions) + } catch (error) { + _responseErrorHandling(options, error) + } +} + +async function _affiliationRequestFetchNothing404Ok(options) { + try { + await _affiliationRequestFetchNothing(options) + } catch (error) { + const status = error.info?.status + if (status !== 404) { + throw error + } + } +} + +function getInstitutionAffiliations(institutionId, callback) { + makeAffiliationRequest( + { + method: 'GET', + path: `/api/v2/institutions/${institutionId.toString()}/affiliations`, + defaultErrorMessage: "Couldn't get institution affiliations", + }, + (error, body) => callback(error, body || []) + ) +} + +function getConfirmedInstitutionAffiliations(institutionId, callback) { + makeAffiliationRequest( + { + method: 'GET', + path: `/api/v2/institutions/${institutionId.toString()}/confirmed_affiliations`, + defaultErrorMessage: "Couldn't get institution affiliations", + }, + (error, body) => callback(error, body || []) + ) +} + +function getInstitutionAffiliationsCounts(institutionId, callback) { + makeAffiliationRequest( + { + method: 'GET', + path: `/api/v2/institutions/${institutionId.toString()}/affiliations_counts`, + defaultErrorMessage: "Couldn't get institution counts", + }, + (error, body) => callback(error, body || []) + ) +} + +function getLicencesForAnalytics(lag, queryDate, callback) { + makeAffiliationRequest( + { + method: 'GET', + path: `/api/v2/institutions/institutions_licences`, + body: { query_date: queryDate, lag }, + defaultErrorMessage: 'Could not get institutions licences', + }, + callback + ) +} + +function getUserAffiliations(userId, callback) { + makeAffiliationRequest( + { + method: 'GET', + path: `/api/v2/users/${userId.toString()}/affiliations`, + defaultErrorMessage: "Couldn't get user affiliations", + }, + (error, body) => callback(error, body || []) + ) +} + +async function getUsersNeedingReconfirmationsLapsedProcessed() { + return await _affiliationRequestFetchJson({ + method: 'GET', + path: '/api/v2/institutions/need_reconfirmation_lapsed_processed', + defaultErrorMessage: + 'Could not get users that need reconfirmations lapsed processed', + }) +} + +async function addAffiliation(userId, email, affiliationOptions) { + const { + university, + department, + role, + confirmedAt, + entitlement, + rejectIfBlocklisted, + } = affiliationOptions + + try { + await _affiliationRequestFetchNothing({ + method: 'POST', + path: `/api/v2/users/${userId.toString()}/affiliations`, + body: { + email, + university, + department, + role, + confirmedAt, + entitlement, + rejectIfBlocklisted, + }, + defaultErrorMessage: "Couldn't create affiliation", + }) + } catch (error) { + if (error.info?.status === 422) { + throw new InvalidInstitutionalEmailError(error.message).withCause(error) + } + throw error + } + + if (!university) { + return + } + + // have notifications delete any ip matcher notifications for this university + try { + await NotificationsBuilder.promises + .ipMatcherAffiliation(userId) + .read(university.id) + } catch (err) { + // log and ignore error + logger.err({ err }, 'Something went wrong marking ip notifications read') + } +} + +async function removeAffiliation(userId, email) { + await _affiliationRequestFetchNothing404Ok({ + method: 'POST', + path: `/api/v2/users/${userId.toString()}/affiliations/remove`, + body: { email }, + defaultErrorMessage: "Couldn't remove affiliation", + }) +} + +function endorseAffiliation(userId, email, role, department, callback) { + makeAffiliationRequest( + { + method: 'POST', + path: `/api/v2/users/${userId.toString()}/affiliations/endorse`, + body: { email, role, department }, + defaultErrorMessage: "Couldn't endorse affiliation", + }, + callback + ) +} + +function deleteAffiliations(userId, callback) { + makeAffiliationRequest( + { + method: 'DELETE', + path: `/api/v2/users/${userId.toString()}/affiliations`, + defaultErrorMessage: "Couldn't delete affiliations", + }, + callback + ) +} + +function addEntitlement(userId, email, callback) { + makeAffiliationRequest( + { + method: 'POST', + path: `/api/v2/users/${userId}/affiliations/add_entitlement`, + body: { email }, + defaultErrorMessage: "Couldn't add entitlement", + }, + callback + ) +} + +function removeEntitlement(userId, email, callback) { + makeAffiliationRequest( + { + method: 'POST', + path: `/api/v2/users/${userId}/affiliations/remove_entitlement`, + body: { email }, + defaultErrorMessage: "Couldn't remove entitlement", + extraSuccessStatusCodes: [404], + }, + callback + ) +} + +function sendUsersWithReconfirmationsLapsedProcessed(users, callback) { + makeAffiliationRequest( + { + method: 'POST', + path: '/api/v2/institutions/reconfirmation_lapsed_processed', + body: { users }, + defaultErrorMessage: + 'Could not update reconfirmation_lapsed_processed_at', + }, + (error, body) => callback(error, body || []) + ) +} const InstitutionsAPI = { - getInstitutionAffiliations(institutionId, callback) { - makeAffiliationRequest( - { - method: 'GET', - path: `/api/v2/institutions/${institutionId.toString()}/affiliations`, - defaultErrorMessage: "Couldn't get institution affiliations", - }, - (error, body) => callback(error, body || []) - ) - }, + getInstitutionAffiliations, - getConfirmedInstitutionAffiliations(institutionId, callback) { - makeAffiliationRequest( - { - method: 'GET', - path: `/api/v2/institutions/${institutionId.toString()}/confirmed_affiliations`, - defaultErrorMessage: "Couldn't get institution affiliations", - }, - (error, body) => callback(error, body || []) - ) - }, + getConfirmedInstitutionAffiliations, - getInstitutionAffiliationsCounts(institutionId, callback) { - makeAffiliationRequest( - { - method: 'GET', - path: `/api/v2/institutions/${institutionId.toString()}/affiliations_counts`, - defaultErrorMessage: "Couldn't get institution counts", - }, - (error, body) => callback(error, body || []) - ) - }, + getInstitutionAffiliationsCounts, - getLicencesForAnalytics(lag, queryDate, callback) { - makeAffiliationRequest( - { - method: 'GET', - path: `/api/v2/institutions/institutions_licences`, - body: { query_date: queryDate, lag }, - defaultErrorMessage: 'Could not get institutions licences', - }, - callback - ) - }, + getLicencesForAnalytics, - getUserAffiliations(userId, callback) { - makeAffiliationRequest( - { - method: 'GET', - path: `/api/v2/users/${userId.toString()}/affiliations`, - defaultErrorMessage: "Couldn't get user affiliations", - }, - (error, body) => callback(error, body || []) - ) - }, + getUserAffiliations, - getUsersNeedingReconfirmationsLapsedProcessed(callback) { - makeAffiliationRequest( - { - method: 'GET', - path: '/api/v2/institutions/need_reconfirmation_lapsed_processed', - defaultErrorMessage: - 'Could not get users that need reconfirmations lapsed processed', - }, - (error, body) => callback(error, body || []) - ) - }, + getUsersNeedingReconfirmationsLapsedProcessed: callbackify( + getUsersNeedingReconfirmationsLapsedProcessed + ), - addAffiliation(userId, email, affiliationOptions, callback) { - if (!callback) { - // affiliationOptions is optional - callback = affiliationOptions - affiliationOptions = {} - } + addAffiliation: callbackify(addAffiliation), - const { - university, - department, - role, - confirmedAt, - entitlement, - rejectIfBlocklisted, - } = affiliationOptions - makeAffiliationRequest( - { - method: 'POST', - path: `/api/v2/users/${userId.toString()}/affiliations`, - body: { - email, - university, - department, - role, - confirmedAt, - entitlement, - rejectIfBlocklisted, - }, - defaultErrorMessage: "Couldn't create affiliation", - }, - function (error, body) { - if (error) { - if (error.info && error.info.statusCode === 422) { - return callback( - new InvalidInstitutionalEmailError(error.message).withCause(error) - ) - } - return callback(error) - } - if (!university) { - return callback(null, body) - } + removeAffiliation: callbackify(removeAffiliation), - // have notifications delete any ip matcher notifications for this university - NotificationsBuilder.ipMatcherAffiliation(userId).read( - university.id, - function (err) { - if (err) { - // log and ignore error - logger.err( - { err }, - 'Something went wrong marking ip notifications read' - ) - } - callback(null, body) - } - ) - } - ) - }, + endorseAffiliation, - removeAffiliation(userId, email, callback) { - makeAffiliationRequest( - { - method: 'POST', - path: `/api/v2/users/${userId.toString()}/affiliations/remove`, - body: { email }, - extraSuccessStatusCodes: [404], // `Not Found` responses are considered successful - defaultErrorMessage: "Couldn't remove affiliation", - }, - callback - ) - }, + deleteAffiliations, - endorseAffiliation(userId, email, role, department, callback) { - makeAffiliationRequest( - { - method: 'POST', - path: `/api/v2/users/${userId.toString()}/affiliations/endorse`, - body: { email, role, department }, - defaultErrorMessage: "Couldn't endorse affiliation", - }, - callback - ) - }, + addEntitlement, - deleteAffiliations(userId, callback) { - makeAffiliationRequest( - { - method: 'DELETE', - path: `/api/v2/users/${userId.toString()}/affiliations`, - defaultErrorMessage: "Couldn't delete affiliations", - }, - callback - ) - }, + removeEntitlement, - addEntitlement(userId, email, callback) { - makeAffiliationRequest( - { - method: 'POST', - path: `/api/v2/users/${userId}/affiliations/add_entitlement`, - body: { email }, - defaultErrorMessage: "Couldn't add entitlement", - }, - callback - ) - }, - - removeEntitlement(userId, email, callback) { - makeAffiliationRequest( - { - method: 'POST', - path: `/api/v2/users/${userId}/affiliations/remove_entitlement`, - body: { email }, - defaultErrorMessage: "Couldn't remove entitlement", - extraSuccessStatusCodes: [404], - }, - callback - ) - }, - - sendUsersWithReconfirmationsLapsedProcessed(users, callback) { - makeAffiliationRequest( - { - method: 'POST', - path: '/api/v2/institutions/reconfirmation_lapsed_processed', - body: { users }, - defaultErrorMessage: - 'Could not update reconfirmation_lapsed_processed_at', - }, - (error, body) => callback(error, body || []) - ) - }, + sendUsersWithReconfirmationsLapsedProcessed, } function makeAffiliationRequest(options, callback) { @@ -280,8 +377,6 @@ function makeAffiliationRequest(options, callback) { 'getInstitutionAffiliations', 'getConfirmedInstitutionAffiliations', 'getUserAffiliations', - 'addAffiliation', - 'removeAffiliation', ].map(method => metrics.timeAsyncMethod( InstitutionsAPI, @@ -291,5 +386,17 @@ function makeAffiliationRequest(options, callback) { ) ) -InstitutionsAPI.promises = promisifyAll(InstitutionsAPI) +InstitutionsAPI.promises = promisifyAll(InstitutionsAPI, { + without: [ + 'addAffiliation', + 'removeAffiliation', + 'getUsersNeedingReconfirmationsLapsedProcessed', + ], +}) + +InstitutionsAPI.promises.addAffiliation = addAffiliation +InstitutionsAPI.promises.removeAffiliation = removeAffiliation +InstitutionsAPI.promises.getUsersNeedingReconfirmationsLapsedProcessed = + getUsersNeedingReconfirmationsLapsedProcessed + module.exports = InstitutionsAPI diff --git a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js index 010168d364..bf0a106a69 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js +++ b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js @@ -10,10 +10,13 @@ const Errors = require('../../../../app/src/Features/Errors/Errors') describe('InstitutionsAPI', function () { beforeEach(function () { - this.settings = { apis: { v1: { url: 'v1.url', user: '', pass: '' } } } + this.settings = { + apis: { v1: { url: 'v1.url', user: '', pass: '', timeout: 5000 } }, + } this.request = sinon.stub() + this.fetchNothing = sinon.stub() this.ipMatcherNotification = { - read: (this.markAsReadIpMatcher = sinon.stub().callsArgWith(1, null)), + read: (this.markAsReadIpMatcher = sinon.stub().resolves()), } this.InstitutionsAPI = SandboxedModule.require(modulePath, { requires: { @@ -22,10 +25,16 @@ describe('InstitutionsAPI', function () { }, '@overleaf/settings': this.settings, requestretry: this.request, + '@overleaf/fetch-utils': { + fetchNothing: this.fetchNothing, + fetchJson: (this.fetchJson = sinon.stub()), + }, '../Notifications/NotificationsBuilder': { - ipMatcherAffiliation: sinon - .stub() - .returns(this.ipMatcherNotification), + promises: { + ipMatcherAffiliation: sinon + .stub() + .returns(this.ipMatcherNotification), + }, }, }, }) @@ -162,14 +171,14 @@ describe('InstitutionsAPI', function () { describe('getUsersNeedingReconfirmationsLapsedProcessed', function () { it('get the list of users', function (done) { - this.request.callsArgWith(1, null, { statusCode: 200 }) + this.fetchJson.resolves({ statusCode: 200 }) this.InstitutionsAPI.getUsersNeedingReconfirmationsLapsedProcessed( error => { expect(error).not.to.exist - this.request.calledOnce.should.equal(true) - const requestOptions = this.request.lastCall.args[0] + this.fetchJson.calledOnce.should.equal(true) + const requestOptions = this.fetchJson.lastCall.args[1] const expectedUrl = `v1.url/api/v2/institutions/need_reconfirmation_lapsed_processed` - requestOptions.url.should.equal(expectedUrl) + this.fetchJson.lastCall.args[0].should.equal(expectedUrl) requestOptions.method.should.equal('GET') done() } @@ -177,7 +186,7 @@ describe('InstitutionsAPI', function () { }) it('handle error', function (done) { - this.request.callsArgWith(1, null, { statusCode: 500 }) + this.fetchJson.throws({ info: { statusCode: 500 } }) this.InstitutionsAPI.getUsersNeedingReconfirmationsLapsedProcessed( error => { expect(error).to.exist @@ -189,14 +198,14 @@ describe('InstitutionsAPI', function () { describe('addAffiliation', function () { beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 201 }) + this.fetchNothing.resolves({ status: 201 }) }) it('add affiliation', function (done) { const affiliationOptions = { university: { id: 1 }, - role: 'Prof', department: 'Math', + role: 'Prof', confirmedAt: new Date(), entitlement: true, } @@ -206,38 +215,86 @@ describe('InstitutionsAPI', function () { affiliationOptions, err => { expect(err).not.to.exist - this.request.calledOnce.should.equal(true) - const requestOptions = this.request.lastCall.args[0] + this.fetchNothing.calledOnce.should.equal(true) + const requestOptions = this.fetchNothing.lastCall.args[1] const expectedUrl = `v1.url/api/v2/users/${this.stubbedUser._id}/affiliations` - requestOptions.url.should.equal(expectedUrl) + expect(this.fetchNothing.lastCall.args[0]).to.equal(expectedUrl) requestOptions.method.should.equal('POST') - requestOptions.maxAttempts.should.equal(0) - const { body } = requestOptions - Object.keys(body).length.should.equal(7) - body.email.should.equal(this.newEmail) - body.university.should.equal(affiliationOptions.university) - body.department.should.equal(affiliationOptions.department) - body.role.should.equal(affiliationOptions.role) - body.confirmedAt.should.equal(affiliationOptions.confirmedAt) - body.entitlement.should.equal(affiliationOptions.entitlement) + const { json } = requestOptions + Object.keys(json).length.should.equal(7) + expect(json).to.deep.equal( + Object.assign( + { email: this.newEmail, rejectIfBlocklisted: undefined }, + affiliationOptions + ) + ) this.markAsReadIpMatcher.calledOnce.should.equal(true) done() } ) }) - it('handle error', function (done) { - const body = { errors: 'affiliation error message' } - this.request.callsArgWith(1, null, { statusCode: 422 }, body) + it('handles 422 error', function (done) { + const messageFromApi = 'affiliation error message' + const body = JSON.stringify({ errors: messageFromApi }) + this.fetchNothing.throws({ response: { status: 422 }, body }) this.InstitutionsAPI.addAffiliation( this.stubbedUser._id, this.newEmail, {}, err => { expect(err).to.exist + expect(err).to.be.instanceOf(Errors.InvalidInstitutionalEmailError) err.message.should.have.string(422) - err.message.should.have.string(body.errors) + err.message.should.have.string(messageFromApi) + done() + } + ) + }) + + it('handles 500 error', function (done) { + const body = { errors: 'affiliation error message' } + this.fetchNothing.throws({ response: { status: 500 }, body }) + this.InstitutionsAPI.addAffiliation( + this.stubbedUser._id, + this.newEmail, + {}, + err => { + expect(err).to.be.instanceOf(Errors.V1ConnectionError) + expect(err.message).to.equal('error getting affiliations from v1') + expect(err.info).to.deep.equal({ status: 500, body }) + done() + } + ) + }) + + it('uses default error message when no error body in response', function (done) { + this.fetchNothing.throws({ response: { status: 429 } }) + this.InstitutionsAPI.addAffiliation( + this.stubbedUser._id, + this.newEmail, + {}, + err => { + expect(err).to.exist + expect(err.message).to.equal("Couldn't create affiliation: 429") + done() + } + ) + }) + + it('does not try to mark IP matcher notifications as read if no university passed', function (done) { + const affiliationOptions = { + confirmedAt: new Date(), + } + + this.InstitutionsAPI.addAffiliation( + this.stubbedUser._id, + this.newEmail, + affiliationOptions, + err => { + expect(err).not.to.exist + expect(this.markAsReadIpMatcher.callCount).to.equal(0) done() } ) @@ -246,7 +303,7 @@ describe('InstitutionsAPI', function () { describe('removeAffiliation', function () { beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 404 }) + this.fetchNothing.throws({ response: { status: 404 } }) }) it('remove affiliation', function (done) { @@ -255,19 +312,19 @@ describe('InstitutionsAPI', function () { this.newEmail, err => { expect(err).not.to.exist - this.request.calledOnce.should.equal(true) - const requestOptions = this.request.lastCall.args[0] + this.fetchNothing.calledOnce.should.equal(true) + const requestOptions = this.fetchNothing.lastCall.args[1] const expectedUrl = `v1.url/api/v2/users/${this.stubbedUser._id}/affiliations/remove` - requestOptions.url.should.equal(expectedUrl) + this.fetchNothing.lastCall.args[0].should.equal(expectedUrl) requestOptions.method.should.equal('POST') - expect(requestOptions.body).to.deep.equal({ email: this.newEmail }) + expect(requestOptions.json).to.deep.equal({ email: this.newEmail }) done() } ) }) it('handle error', function (done) { - this.request.callsArgWith(1, null, { statusCode: 500 }) + this.fetchNothing.throws({ response: { status: 500 } }) this.InstitutionsAPI.removeAffiliation( this.stubbedUser._id, this.newEmail,