From 21514418e58a8fca1028bcb20174ca7a5c9810ba Mon Sep 17 00:00:00 2001 From: Jessica Lawshe <5312836+lawshe@users.noreply.github.com> Date: Tue, 24 Oct 2023 08:47:38 -0500 Subject: [PATCH] Merge pull request #14881 from overleaf/ab-beta-program-async-await [web] Convert BetaProgramController to async/await GitOrigin-RevId: 2423cb00b78b2f3fddb7bcd317d34ed4dbc12b95 --- .../BetaProgram/BetaProgramController.js | 100 ++++++------ .../BetaProgram/BetaProgramHandler.js | 14 +- .../BetaProgram/BetaProgramControllerTests.js | 153 ++++++++++-------- 3 files changed, 140 insertions(+), 127 deletions(-) diff --git a/services/web/app/src/Features/BetaProgram/BetaProgramController.js b/services/web/app/src/Features/BetaProgram/BetaProgramController.js index 02501d535a..0120f59803 100644 --- a/services/web/app/src/Features/BetaProgram/BetaProgramController.js +++ b/services/web/app/src/Features/BetaProgram/BetaProgramController.js @@ -1,61 +1,57 @@ const BetaProgramHandler = require('./BetaProgramHandler') const OError = require('@overleaf/o-error') const UserGetter = require('../User/UserGetter') -const Settings = require('@overleaf/settings') const logger = require('@overleaf/logger') const SessionManager = require('../Authentication/SessionManager') const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const { expressify } = require('../../util/promises') -const BetaProgramController = { - optIn(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - logger.debug({ userId }, 'user opting in to beta program') - if (userId == null) { - return next(new Error('no user id in session')) - } - BetaProgramHandler.optIn(userId, function (err) { - if (err) { - return next(err) - } - SplitTestHandler.sessionMaintenance(req, null, () => - res.redirect('/beta/participate') - ) - }) - }, - - optOut(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - logger.debug({ userId }, 'user opting out of beta program') - if (userId == null) { - return next(new Error('no user id in session')) - } - BetaProgramHandler.optOut(userId, function (err) { - if (err) { - return next(err) - } - SplitTestHandler.sessionMaintenance(req, null, () => - res.redirect('/beta/participate') - ) - }) - }, - - optInPage(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - logger.debug({ userId }, 'showing beta participation page for user') - UserGetter.getUser(userId, { betaProgram: 1 }, function (err, user) { - if (err) { - OError.tag(err, 'error fetching user', { - userId, - }) - return next(err) - } - res.render('beta_program/opt_in', { - title: 'sharelatex_beta_program', - user, - languages: Settings.languages, - }) - }) - }, +async function optIn(req, res) { + const userId = SessionManager.getLoggedInUserId(req.session) + await BetaProgramHandler.promises.optIn(userId) + try { + await SplitTestHandler.promises.sessionMaintenance(req, null) + } catch (error) { + logger.error( + { err: error }, + 'Failed to perform session maintenance after beta program opt in' + ) + } + res.redirect('/beta/participate') } -module.exports = BetaProgramController +async function optOut(req, res) { + const userId = SessionManager.getLoggedInUserId(req.session) + await BetaProgramHandler.promises.optOut(userId) + try { + await SplitTestHandler.promises.sessionMaintenance(req, null) + } catch (error) { + logger.error( + { err: error }, + 'Failed to perform session maintenance after beta program opt out' + ) + } + res.redirect('/beta/participate') +} + +async function optInPage(req, res) { + const userId = SessionManager.getLoggedInUserId(req.session) + let user + try { + user = await UserGetter.promises.getUser(userId, { betaProgram: 1 }) + } catch (error) { + throw OError.tag(error, 'error fetching user', { + userId, + }) + } + res.render('beta_program/opt_in', { + title: 'sharelatex_beta_program', + user, + }) +} + +module.exports = { + optIn: expressify(optIn), + optOut: expressify(optOut), + optInPage: expressify(optInPage), +} diff --git a/services/web/app/src/Features/BetaProgram/BetaProgramHandler.js b/services/web/app/src/Features/BetaProgram/BetaProgramHandler.js index 1e9e43e35f..d558de37df 100644 --- a/services/web/app/src/Features/BetaProgram/BetaProgramHandler.js +++ b/services/web/app/src/Features/BetaProgram/BetaProgramHandler.js @@ -17,15 +17,11 @@ async function optOut(userId) { AnalyticsManager.setUserPropertyForUser(userId, 'beta-program', false) } -const BetaProgramHandler = { +module.exports = { optIn: callbackify(optIn), - optOut: callbackify(optOut), + promises: { + optIn, + optOut, + }, } - -BetaProgramHandler.promises = { - optIn, - optOut, -} - -module.exports = BetaProgramHandler diff --git a/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js b/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js index 8db25d1b64..8a27f484c0 100644 --- a/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js +++ b/services/web/test/unit/src/BetaProgram/BetaProgramControllerTests.js @@ -1,6 +1,8 @@ const SandboxedModule = require('sandboxed-module') const path = require('path') const sinon = require('sinon') +const { expect } = require('chai') +const MockResponse = require('../helpers/MockResponse') const modulePath = path.join( __dirname, @@ -22,17 +24,23 @@ describe('BetaProgramController', function () { }, } this.SplitTestHandler = { - sessionMaintenance: sinon.stub().yields(), + promises: { + sessionMaintenance: sinon.stub(), + }, } this.BetaProgramController = SandboxedModule.require(modulePath, { requires: { '../SplitTests/SplitTestHandler': this.SplitTestHandler, './BetaProgramHandler': (this.BetaProgramHandler = { - optIn: sinon.stub(), - optOut: sinon.stub(), + promises: { + optIn: sinon.stub().resolves(), + optOut: sinon.stub().resolves(), + }, }), '../User/UserGetter': (this.UserGetter = { - getUser: sinon.stub(), + promises: { + getUser: sinon.stub().resolves(), + }, }), '@overleaf/settings': (this.settings = { languages: {}, @@ -43,23 +51,17 @@ describe('BetaProgramController', function () { }), }, }) - this.res = { - send: sinon.stub(), - redirect: sinon.stub(), - render: sinon.stub(), - } + this.res = new MockResponse() this.next = sinon.stub() }) describe('optIn', function () { - beforeEach(function () { - this.BetaProgramHandler.optIn.callsArgWith(1, null) - }) - - it("should redirect to '/beta/participate'", function () { - this.BetaProgramController.optIn(this.req, this.res, this.next) - this.res.redirect.callCount.should.equal(1) - this.res.redirect.firstCall.args[0].should.equal('/beta/participate') + it("should redirect to '/beta/participate'", function (done) { + this.res.callback = () => { + this.res.redirectedTo.should.equal('/beta/participate') + done() + } + this.BetaProgramController.optIn(this.req, this.res, done) }) it('should not call next with an error', function () { @@ -69,19 +71,22 @@ describe('BetaProgramController', function () { it('should call BetaProgramHandler.optIn', function () { this.BetaProgramController.optIn(this.req, this.res, this.next) - this.BetaProgramHandler.optIn.callCount.should.equal(1) + this.BetaProgramHandler.promises.optIn.callCount.should.equal(1) }) - it('should invoke the session maintenance', function () { - this.BetaProgramController.optIn(this.req, this.res, this.next) - this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( - this.req - ) + it('should invoke the session maintenance', function (done) { + this.res.callback = () => { + this.SplitTestHandler.promises.sessionMaintenance.should.have.been.calledWith( + this.req + ) + done() + } + this.BetaProgramController.optIn(this.req, this.res, done) }) describe('when BetaProgramHandler.opIn produces an error', function () { beforeEach(function () { - this.BetaProgramHandler.optIn.callsArgWith(1, new Error('woops')) + this.BetaProgramHandler.promises.optIn.throws(new Error('woops')) }) it("should not redirect to '/beta/participate'", function () { @@ -89,75 +94,89 @@ describe('BetaProgramController', function () { this.res.redirect.callCount.should.equal(0) }) - it('should produce an error', function () { - this.BetaProgramController.optIn(this.req, this.res, this.next) - this.next.callCount.should.equal(1) - this.next.firstCall.args[0].should.be.instanceof(Error) + it('should produce an error', function (done) { + this.BetaProgramController.optIn(this.req, this.res, err => { + expect(err).to.be.instanceof(Error) + done() + }) }) }) }) describe('optOut', function () { - beforeEach(function () { - this.BetaProgramHandler.optOut.callsArgWith(1, null) + it("should redirect to '/beta/participate'", function (done) { + this.res.callback = () => { + expect(this.res.redirectedTo).to.equal('/beta/participate') + done() + } + this.BetaProgramController.optOut(this.req, this.res, done) }) - it("should redirect to '/beta/participate'", function () { - this.BetaProgramController.optOut(this.req, this.res, this.next) - this.res.redirect.callCount.should.equal(1) - this.res.redirect.firstCall.args[0].should.equal('/beta/participate') + it('should not call next with an error', function (done) { + this.res.callback = () => { + this.next.callCount.should.equal(0) + done() + } + this.BetaProgramController.optOut(this.req, this.res, done) }) - it('should not call next with an error', function () { - this.BetaProgramController.optOut(this.req, this.res, this.next) - this.next.callCount.should.equal(0) + it('should call BetaProgramHandler.optOut', function (done) { + this.res.callback = () => { + this.BetaProgramHandler.promises.optOut.callCount.should.equal(1) + done() + } + this.BetaProgramController.optOut(this.req, this.res, done) }) - it('should call BetaProgramHandler.optOut', function () { - this.BetaProgramController.optOut(this.req, this.res, this.next) - this.BetaProgramHandler.optOut.callCount.should.equal(1) - }) - - it('should invoke the session maintenance', function () { - this.BetaProgramController.optOut(this.req, this.res, this.next) - this.SplitTestHandler.sessionMaintenance.should.have.been.calledWith( - this.req - ) + it('should invoke the session maintenance', function (done) { + this.res.callback = () => { + this.SplitTestHandler.promises.sessionMaintenance.should.have.been.calledWith( + this.req, + null + ) + done() + } + this.BetaProgramController.optOut(this.req, this.res, done) }) describe('when BetaProgramHandler.optOut produces an error', function () { beforeEach(function () { - this.BetaProgramHandler.optOut.callsArgWith(1, new Error('woops')) + this.BetaProgramHandler.promises.optOut.throws(new Error('woops')) }) - it("should not redirect to '/beta/participate'", function () { - this.BetaProgramController.optOut(this.req, this.res, this.next) - this.res.redirect.callCount.should.equal(0) + it("should not redirect to '/beta/participate'", function (done) { + this.BetaProgramController.optOut(this.req, this.res, error => { + expect(error).to.exist + expect(this.res.redirected).to.equal(false) + done() + }) }) - it('should produce an error', function () { - this.BetaProgramController.optOut(this.req, this.res, this.next) - this.next.callCount.should.equal(1) - this.next.firstCall.args[0].should.be.instanceof(Error) + it('should produce an error', function (done) { + this.BetaProgramController.optOut(this.req, this.res, error => { + expect(error).to.exist + done() + }) }) }) }) describe('optInPage', function () { beforeEach(function () { - this.UserGetter.getUser.yields(null, this.user) + this.UserGetter.promises.getUser.resolves(this.user) }) - it('should render the opt-in page', function () { - this.BetaProgramController.optInPage(this.req, this.res, this.next) - this.res.render.callCount.should.equal(1) - const { args } = this.res.render.firstCall - args[0].should.equal('beta_program/opt_in') + it('should render the opt-in page', function (done) { + this.res.callback = () => { + expect(this.res.renderedTemplate).to.equal('beta_program/opt_in') + done() + } + this.BetaProgramController.optInPage(this.req, this.res, done) }) describe('when UserGetter.getUser produces an error', function () { beforeEach(function () { - this.UserGetter.getUser.yields(new Error('woops')) + this.UserGetter.promises.getUser.throws(new Error('woops')) }) it('should not render the opt-in page', function () { @@ -165,10 +184,12 @@ describe('BetaProgramController', function () { this.res.render.callCount.should.equal(0) }) - it('should produce an error', function () { - this.BetaProgramController.optInPage(this.req, this.res, this.next) - this.next.callCount.should.equal(1) - this.next.firstCall.args[0].should.be.instanceof(Error) + it('should produce an error', function (done) { + this.BetaProgramController.optInPage(this.req, this.res, error => { + expect(error).to.exist + expect(error).to.be.instanceof(Error) + done() + }) }) }) })