From 4b6f038a827c57351e1908539adc2ed87d0cf387 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Wed, 30 Oct 2019 15:42:37 +0000 Subject: [PATCH] Merge pull request #2307 from overleaf/spd-project-page-without-v1 Add additional error handling to enable /projects to load without V1 GitOrigin-RevId: 710ab2f07f191aa60ffdd71e2f54bc7c5db0c430 --- .../Features/Institutions/InstitutionsAPI.js | 18 +++++++- .../src/Features/Project/ProjectController.js | 23 ++++++++-- .../Subscription/LimitationsManager.js | 7 +++- .../Subscription/V1SubscriptionManager.js | 19 ++++++--- services/web/app/src/Features/V1/V1Api.js | 12 +++++- .../src/Institutions/InstitutionsAPITests.js | 12 +++--- .../src/Project/ProjectControllerTests.js | 42 +++++++++++++++++++ 7 files changed, 114 insertions(+), 19 deletions(-) diff --git a/services/web/app/src/Features/Institutions/InstitutionsAPI.js b/services/web/app/src/Features/Institutions/InstitutionsAPI.js index b7fd4e8288..5dbdb60ed8 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsAPI.js +++ b/services/web/app/src/Features/Institutions/InstitutionsAPI.js @@ -17,6 +17,7 @@ const settings = require('settings-sharelatex') const request = require('request') const { promisifyAll } = require('../../util/promises') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') +const { V1ConnectionError } = require('../Errors/Errors') const InstitutionsAPI = { getInstitutionAffiliations(institutionId, callback) { @@ -205,7 +206,22 @@ var makeAffiliationRequest = function(requestOptions, callback) { }, function(error, response, body) { if (error != null) { - return callback(error) + return callback( + new V1ConnectionError('error getting affiliations from v1').withCause( + error + ) + ) + } + if (response && response.statusCode >= 500) { + return callback( + new V1ConnectionError({ + message: 'error getting affiliations from v1', + info: { + status: response.statusCode, + body: body + } + }) + ) } let isSuccess = response.statusCode >= 200 && response.statusCode < 300 if (!isSuccess) { diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index bff7efb576..56db8b5031 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -364,6 +364,7 @@ const ProjectController = { const timer = new metrics.Timer('project-list') const userId = AuthenticationController.getLoggedInUserId(req) const currentUser = AuthenticationController.getSessionUser(req) + let noV1Connection = false async.parallel( { tags(cb) { @@ -389,7 +390,8 @@ const ProjectController = { projects = [] } if (error != null && error instanceof V1ConnectionError) { - return cb(null, { projects: [], tags: [], noConnection: true }) + noV1Connection = true + return cb(null, null) } cb(error, projects[0]) }) @@ -399,6 +401,7 @@ const ProjectController = { currentUser, (error, hasPaidSubscription) => { if (error != null && error instanceof V1ConnectionError) { + noV1Connection = true return cb(null, true) } cb(error, hasPaidSubscription) @@ -416,7 +419,13 @@ const ProjectController = { if (!Features.hasFeature('affiliations')) { return cb(null, null) } - getUserAffiliations(userId, cb) + getUserAffiliations(userId, (error, affiliations) => { + if (error && error instanceof V1ConnectionError) { + noV1Connection = true + return cb(null, []) + } + cb(error, affiliations) + }) } }, (err, results) => { @@ -424,6 +433,10 @@ const ProjectController = { logger.warn({ err }, 'error getting data for project list page') return next(err) } + if (noV1Connection) { + results.v1Projects = results.v1Projects || { projects: [], tags: [] } + results.v1Projects.noConnection = true + } const { notifications, user, userAffiliations } = results const v1Tags = (results.v1Projects != null ? results.v1Projects.tags : undefined) || @@ -995,8 +1008,10 @@ const ProjectController = { v1ProjectData = {} } const warnings = [] - if (v1ProjectData.noConnection && Settings.overleaf) { - warnings.push('No V1 Connection') + if (v1ProjectData.noConnection) { + warnings.push( + 'Error accessing Overleaf V1. Some of your projects or features may be missing.' + ) } if (v1ProjectData.hasHiddenV1Projects) { warnings.push( diff --git a/services/web/app/src/Features/Subscription/LimitationsManager.js b/services/web/app/src/Features/Subscription/LimitationsManager.js index b5cdffd5ce..ab4fed34b0 100644 --- a/services/web/app/src/Features/Subscription/LimitationsManager.js +++ b/services/web/app/src/Features/Subscription/LimitationsManager.js @@ -21,6 +21,7 @@ const Settings = require('settings-sharelatex') const CollaboratorsGetter = require('../Collaborators/CollaboratorsGetter') const CollaboratorsInvitesHandler = require('../Collaborators/CollaboratorsInviteHandler') const V1SubscriptionManager = require('./V1SubscriptionManager') +const { V1ConnectionError } = require('../Errors/Errors') module.exports = LimitationsManager = { allowedNumberOfCollaboratorsInProject(project_id, callback) { @@ -107,7 +108,11 @@ module.exports = LimitationsManager = { } return this.userHasV1Subscription(user, (err, hasV1Subscription) => { if (err != null) { - return callback(err) + return callback( + new V1ConnectionError( + 'error getting subscription from v1' + ).withCause(err) + ) } logger.log( { diff --git a/services/web/app/src/Features/Subscription/V1SubscriptionManager.js b/services/web/app/src/Features/Subscription/V1SubscriptionManager.js index 801f327b05..4ec8320813 100644 --- a/services/web/app/src/Features/Subscription/V1SubscriptionManager.js +++ b/services/web/app/src/Features/Subscription/V1SubscriptionManager.js @@ -180,11 +180,20 @@ module.exports = V1SubscriptionManager = { }, function(error, response, body) { if (error != null) { - // Specially handle no connection err, so warning can be shown - if (error.code === 'ECONNREFUSED') { - error = new V1ConnectionError('No V1 connection') - } - return callback(error) + return callback( + new V1ConnectionError('no v1 connection').withCause(error) + ) + } + if (response && response.statusCode >= 500) { + return callback( + new V1ConnectionError({ + message: 'error from v1', + info: { + status: response.statusCode, + body: body + } + }) + ) } if (response.statusCode >= 200 && response.statusCode < 300) { return callback(null, body, v1Id) diff --git a/services/web/app/src/Features/V1/V1Api.js b/services/web/app/src/Features/V1/V1Api.js index e0516ce9ec..2ad5677af1 100644 --- a/services/web/app/src/Features/V1/V1Api.js +++ b/services/web/app/src/Features/V1/V1Api.js @@ -61,7 +61,17 @@ module.exports = V1Api = { _responseHandler(options, error, response, body, callback) { if (error != null) { - return callback(error, response, body) + return callback( + new Errors.V1ConnectionError('error from V1 API').withCause(error) + ) + } + if (response && response.statusCode >= 500) { + return callback( + new Errors.V1ConnectionError({ + message: 'error from V1 API', + info: { status: response.statusCode, body: body } + }) + ) } if ( (response.statusCode >= 200 && response.statusCode < 300) || diff --git a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js index 6c64995b81..d90c794eb1 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsAPITests.js +++ b/services/web/test/unit/src/Institutions/InstitutionsAPITests.js @@ -21,6 +21,7 @@ const modulePath = path.join( '../../../../app/src/Features/Institutions/InstitutionsAPI' ) ;({ expect } = require('chai')) +const Errors = require('../../../../app/src/Features/Errors/Errors') describe('InstitutionsAPI', function() { beforeEach(function() { @@ -46,7 +47,8 @@ describe('InstitutionsAPI', function() { }, '../../../../../app/src/Features/V1/V1Api': { request: sinon.stub() - } + }, + '../Errors/Errors': Errors } }) @@ -158,9 +160,7 @@ describe('InstitutionsAPI', function() { return this.InstitutionsAPI.getUserAffiliations( this.stubbedUser._id, err => { - should.exist(err) - err.message.should.have.string(503) - err.message.should.have.string(body.errors) + expect(err).to.be.instanceof(Errors.V1ConnectionError) return done() } ) @@ -299,9 +299,7 @@ describe('InstitutionsAPI', function() { return this.InstitutionsAPI.deleteAffiliations( this.stubbedUser._id, err => { - should.exist(err) - err.message.should.have.string(518) - err.message.should.have.string(body.errors) + expect(err).to.be.instanceof(Errors.V1ConnectionError) return done() } ) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index e16c903de8..93412563fb 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -520,6 +520,48 @@ describe('ProjectController', function() { this.ProjectController.projectListPage(this.req, this.res) }) + describe('when there is a v1 connection error', function() { + beforeEach(function() { + this.Features.hasFeature = sinon + .stub() + .withArgs('overleaf-integration') + .returns(true) + this.connectionWarning = + 'Error accessing Overleaf V1. Some of your projects or features may be missing.' + }) + + it('should show a warning when there is an error getting v1 projects', function(done) { + this.Modules.hooks.fire + .withArgs('findAllV1Projects', this.user._id) + .yields(new Errors.V1ConnectionError('error')) + this.res.render = (pageName, opts) => { + expect(opts.warnings).to.contain(this.connectionWarning) + done() + } + this.ProjectController.projectListPage(this.req, this.res) + }) + + it('should show a warning when there is an error getting subscriptions from v1', function(done) { + this.LimitationsManager.hasPaidSubscription.yields( + new Errors.V1ConnectionError('error') + ) + this.res.render = (pageName, opts) => { + expect(opts.warnings).to.contain(this.connectionWarning) + done() + } + this.ProjectController.projectListPage(this.req, this.res) + }) + + it('should show a warning when there is an error getting affiliations from v1', function(done) { + this.getUserAffiliations.yields(new Errors.V1ConnectionError('error')) + this.res.render = (pageName, opts) => { + expect(opts.warnings).to.contain(this.connectionWarning) + done() + } + this.ProjectController.projectListPage(this.req, this.res) + }) + }) + describe('front widget', function(done) { beforeEach(function() { this.settings.overleaf = {