From e681c6322fdd5ddfacac4a5588e02da306aed268 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 26 Oct 2021 14:31:24 +0100 Subject: [PATCH] Merge pull request #5479 from overleaf/bg-refresh-features-on-editor-load refresh user features on editor load when out of date GitOrigin-RevId: ef39b5626cfdc6ed611137a6f6eca3417d3ce73f --- .../src/Features/Project/ProjectController.js | 65 ++++++++++++++++++- .../Features/Subscription/FeaturesUpdater.js | 8 +++ .../Subscription/UserFeaturesUpdater.js | 5 ++ services/web/app/src/models/User.js | 3 + services/web/config/settings.defaults.js | 2 + .../src/Project/ProjectControllerTests.js | 17 +++++ .../Subscription/UserFeaturesUpdaterTests.js | 26 ++++++++ 7 files changed, 123 insertions(+), 3 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 7398bdab99..fdea927257 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -39,6 +39,7 @@ const AnalyticsManager = require('../Analytics/AnalyticsManager') const Modules = require('../../infrastructure/Modules') const SplitTestV2Handler = require('../SplitTests/SplitTestV2Handler') const { getNewLogsUIVariantForUser } = require('../Helpers/NewLogsUI') +const FeaturesUpdater = require('../Subscription/FeaturesUpdater') const _ssoAvailable = (affiliation, session, linkedInstitutionIds) => { if (!affiliation.institution) return false @@ -661,16 +662,21 @@ const ProjectController = { } else { User.findById( userId, - 'email first_name last_name referal_id signUpDate featureSwitches features refProviders alphaProgram betaProgram isAdmin ace', + 'email first_name last_name referal_id signUpDate featureSwitches features featuresEpoch refProviders alphaProgram betaProgram isAdmin ace', (err, user) => { // Handle case of deleted user if (user == null) { UserController.logout(req, res, next) return } - + if (err) { + return cb(err) + } logger.log({ projectId, userId }, 'got user') - cb(err, user) + if (FeaturesUpdater.featuresEpochIsCurrent(user)) { + return cb(null, user) + } + ProjectController._refreshFeatures(req, user, cb) } ) } @@ -893,6 +899,59 @@ const ProjectController = { ) }, + _refreshFeatures(req, user, callback) { + // If the feature refresh has failed in this session, don't retry + // it - require the user to log in again. + if (req.session.feature_refresh_failed) { + metrics.inc('features-refresh', 1, { + path: 'load-editor', + status: 'skipped', + }) + return callback(null, user) + } + // If the refresh takes too long then return the current + // features. Note that the user.features property may still be + // updated in the background after the callback is called. + callback = _.once(callback) + const refreshTimeoutHandler = setTimeout(() => { + req.session.feature_refresh_failed = { reason: 'timeout', at: new Date() } + metrics.inc('features-refresh', 1, { + path: 'load-editor', + status: 'timeout', + }) + callback(null, user) + }, 5000) + // try to refresh user features now + const timer = new metrics.Timer('features-refresh-on-load-editor') + FeaturesUpdater.refreshFeatures( + user._id, + 'load-editor', + (err, features) => { + clearTimeout(refreshTimeoutHandler) + timer.done() + if (err) { + // keep a record to prevent unneceary retries and leave + // the original features unmodified if the refresh failed + req.session.feature_refresh_failed = { + reason: 'error', + at: new Date(), + } + metrics.inc('features-refresh', 1, { + path: 'load-editor', + status: 'error', + }) + } else { + user.features = features + metrics.inc('features-refresh', 1, { + path: 'load-editor', + status: 'success', + }) + } + return callback(null, user) + } + ) + }, + _buildProjectList(allProjects, userId) { let project const { diff --git a/services/web/app/src/Features/Subscription/FeaturesUpdater.js b/services/web/app/src/Features/Subscription/FeaturesUpdater.js index 04aa3e91e8..323a041ca2 100644 --- a/services/web/app/src/Features/Subscription/FeaturesUpdater.js +++ b/services/web/app/src/Features/Subscription/FeaturesUpdater.js @@ -22,6 +22,13 @@ async function scheduleRefreshFeatures(userId, reason) { await queue.add({ userId, reason }) } +/* Check if user features refresh if needed, based on the global featuresEpoch setting */ +function featuresEpochIsCurrent(user) { + return Settings.featuresEpoch + ? user.featuresEpoch === Settings.featuresEpoch + : true +} + /** * Refresh features for the given user */ @@ -197,6 +204,7 @@ function _getMatchedFeatureSet(features) { } module.exports = { + featuresEpochIsCurrent, computeFeatures: callbackify(computeFeatures), refreshFeatures: callbackifyMultiResult(refreshFeatures, [ 'features', diff --git a/services/web/app/src/Features/Subscription/UserFeaturesUpdater.js b/services/web/app/src/Features/Subscription/UserFeaturesUpdater.js index f041b06137..026251b816 100644 --- a/services/web/app/src/Features/Subscription/UserFeaturesUpdater.js +++ b/services/web/app/src/Features/Subscription/UserFeaturesUpdater.js @@ -1,5 +1,6 @@ const { User } = require('../../models/User') const { promisifyAll } = require('../../util/promises') +const Settings = require('@overleaf/settings') function _featuresChanged(newFeatures, featuresBefore) { for (const feature in newFeatures) { @@ -15,6 +16,10 @@ module.exports = { const update = { featuresUpdatedAt: new Date(), } + // record the system-wide features epoch, if defined + if (Settings.featuresEpoch) { + update.featuresEpoch = Settings.featuresEpoch + } for (const key in features) { const value = features[key] update[`features.${key}`] = value diff --git a/services/web/app/src/models/User.js b/services/web/app/src/models/User.js index 924ff6862c..cbf0493bb2 100644 --- a/services/web/app/src/models/User.js +++ b/services/web/app/src/models/User.js @@ -135,6 +135,9 @@ const UserSchema = new Schema({ }, ], featuresUpdatedAt: { type: Date }, + featuresEpoch: { + type: String, + }, // when auto-merged from SL and must-reconfirm is set, we may end up using // `sharelatexHashedPassword` to recover accounts... sharelatexHashedPassword: String, diff --git a/services/web/config/settings.defaults.js b/services/web/config/settings.defaults.js index 43cf816139..4b35c05c45 100644 --- a/services/web/config/settings.defaults.js +++ b/services/web/config/settings.defaults.js @@ -296,6 +296,8 @@ module.exports = { trackChanges: true, }), + // featuresEpoch: 'YYYY-MM-DD', + features: { personal: defaultFeatures, }, diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index a747c72ee5..17405dbf25 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -105,6 +105,10 @@ describe('ProjectController', function () { this.Features = { hasFeature: sinon.stub(), } + this.FeaturesUpdater = { + featuresEpochIsCurrent: sinon.stub().returns(true), + refreshFeatures: sinon.stub().yields(null, this.user), + } this.BrandVariationsHandler = { getBrandVariationById: sinon .stub() @@ -167,6 +171,7 @@ describe('ProjectController', function () { '../Collaborators/CollaboratorsGetter': this.CollaboratorsGetter, './ProjectEntityHandler': this.ProjectEntityHandler, '../../infrastructure/Features': this.Features, + '../Subscription/FeaturesUpdater': this.FeaturesUpdater, '../Notifications/NotificationsBuilder': this.NotificationBuilder, '../User/UserGetter': this.UserGetter, '../BrandVariations/BrandVariationsHandler': this @@ -1083,6 +1088,18 @@ describe('ProjectController', function () { this.ProjectController.loadEditor(this.req, this.res) }) + it('should refresh the user features if the epoch is outdated', function (done) { + this.FeaturesUpdater.featuresEpochIsCurrent = sinon.stub().returns(false) + this.res.render = () => { + this.FeaturesUpdater.refreshFeatures.should.have.been.calledWith( + this.user._id, + 'load-editor' + ) + done() + } + this.ProjectController.loadEditor(this.req, this.res) + }) + describe('pdf caching feature flags', function () { /* eslint-disable mocha/no-identical-title */ function showNoVariant() { diff --git a/services/web/test/unit/src/Subscription/UserFeaturesUpdaterTests.js b/services/web/test/unit/src/Subscription/UserFeaturesUpdaterTests.js index 13da43e654..607226993d 100644 --- a/services/web/test/unit/src/Subscription/UserFeaturesUpdaterTests.js +++ b/services/web/test/unit/src/Subscription/UserFeaturesUpdaterTests.js @@ -31,6 +31,7 @@ describe('UserFeaturesUpdater', function () { '../../models/User': { User: this.User, }, + '@overleaf/settings': (this.Settings = {}), }, }) }) @@ -58,6 +59,31 @@ describe('UserFeaturesUpdater', function () { ) expect(updateArgs[1].featuresUpdatedAt instanceof Date).to.be.true features.should.deep.equal(update) + expect(updateArgs[1].featuresEpoch).to.be.undefined + done() + } + ) + }) + it('should set the featuresEpoch when present', function (done) { + const userId = '5208dd34438842e2db000005' + const update = { + versioning: true, + } + this.Settings.featuresEpoch = 'epoch-1' + this.UserFeaturesUpdater.updateFeatures( + userId, + update, + (err, features) => { + const updateArgs = this.User.findByIdAndUpdate.lastCall.args + expect(updateArgs[0]).to.deep.equal(userId) + assert.equal(err, null) + expect(Object.keys(updateArgs[1]).length).to.equal(3) + expect(updateArgs[1]['features.versioning']).to.equal( + update.versioning + ) + expect(updateArgs[1].featuresUpdatedAt instanceof Date).to.be.true + features.should.deep.equal(update) + expect(updateArgs[1].featuresEpoch).to.equal('epoch-1') done() } )