From 1dbe89f2bcc90f8fac4a24326b7900e4aa53aac3 Mon Sep 17 00:00:00 2001 From: Benjamin Wheeler Date: Thu, 9 Jan 2020 08:44:27 -0500 Subject: [PATCH] Revert "[Develop] Hotfix/join retry session" --- src/components/join-flow/join-flow.jsx | 27 ++--- src/lib/session.js | 69 ------------ src/redux/navigation.js | 5 +- src/redux/session.js | 91 +++++++--------- test/unit/components/join-flow.test.jsx | 41 +++----- test/unit/lib/session.test.js | 133 ------------------------ 6 files changed, 63 insertions(+), 303 deletions(-) delete mode 100644 src/lib/session.js delete mode 100644 test/unit/lib/session.test.js diff --git a/src/components/join-flow/join-flow.jsx b/src/components/join-flow/join-flow.jsx index c23705144..fd6760da6 100644 --- a/src/components/join-flow/join-flow.jsx +++ b/src/components/join-flow/join-flow.jsx @@ -121,16 +121,13 @@ class JoinFlow extends React.Component { // * "birth_month": ["Ensure this value is greater than or equal to 1."] handleRegistrationResponse (err, body, res) { this.setState({ - numAttempts: this.state.numAttempts + 1 + numAttempts: this.state.numAttempts + 1, + waiting: false }, () => { const success = this.registrationIsSuccessful(err, body, res); if (success) { - this.props.refreshSessionWithRetry().then(() => { - this.setState({ - step: this.state.step + 1, - waiting: false - }); - }); + this.props.refreshSession(); + this.setState({step: this.state.step + 1}); return; } // now we know something went wrong -- either an actual error (client-side @@ -138,10 +135,7 @@ class JoinFlow extends React.Component { // if an actual error, prompt user to try again. if (err || res.statusCode !== 200) { - this.setState({ - registrationError: {errorAllowsTryAgain: true}, - waiting: false - }); + this.setState({registrationError: {errorAllowsTryAgain: true}}); return; } @@ -170,8 +164,7 @@ class JoinFlow extends React.Component { registrationError: { errorAllowsTryAgain: false, errorMsg: errorMsg - }, - waiting: false + } }); }); } @@ -290,15 +283,15 @@ JoinFlow.propTypes = { createProjectOnComplete: PropTypes.bool, intl: intlShape, onCompleteRegistration: PropTypes.func, - refreshSessionWithRetry: PropTypes.func + refreshSession: PropTypes.func }; const IntlJoinFlow = injectIntl(JoinFlow); const mapDispatchToProps = dispatch => ({ - refreshSessionWithRetry: () => ( - dispatch(sessionActions.refreshSessionWithRetry()) - ) + refreshSession: () => { + dispatch(sessionActions.refreshSession()); + } }); // Allow incoming props to override redux-provided props. Used to mock in tests. diff --git a/src/lib/session.js b/src/lib/session.js deleted file mode 100644 index 22490a010..000000000 --- a/src/lib/session.js +++ /dev/null @@ -1,69 +0,0 @@ -const api = require('./api'); - -module.exports = {}; - -/* -requestSessionWithRetry() - -Retries the session api call until it has either reached the limit of number of -retries, or received a successful response with that contains a 'user' field in -its body. - -Each time it retries, it will double its previous waiting time, and subtract -that time from the totalDelayMS parameter. - -example of what this might look like: - -1st call: -receives params: retriesLeft=3 and totalDelayMS=3500 -performs api call -delay until next call: 3500 / (2^3 - 1) = 500ms -next call params: retriesLeft=2, totalDelayMS=3000 - -2nd call: -receives params: retriesLeft=2 and totalDelayMS=3000 -performs api call -delay until next call: 3000 / (2^2 - 1) = 1000ms -next call: retriesLeft=1, totalDelayMS=2000 - -3rd call: -receives params: retriesLeft=1, totalDelayMS=2000 -performs api call -delay until next call: 2000 / (2^1 - 1) = 2000ms -next call: retriesLeft=0, totalDelayMS=0 - -4th call: -receives params: retriesLeft=0, totalDelayMS=0 -performs api call -returns the response, even if it is undefined or empty - -total api calls: 4 -total delay time: 3500ms -*/ -module.exports.requestSessionWithRetry = (resolve, reject, retriesLeft, totalDelayMS) => { - api({ - host: '', - uri: '/session/' - }, (err, body, response) => { - if (err || (response && response.statusCode === 404)) { - return reject(err); - } - if (typeof body === 'undefined' || body === null || !body.user) { - if (retriesLeft < 1) { - return resolve(body); - } - const nextTimeout = totalDelayMS / (Math.pow(2, retriesLeft) - 1); - return setTimeout( - module.exports.requestSessionWithRetry.bind( - null, resolve, reject, retriesLeft - 1, totalDelayMS - nextTimeout - ), - nextTimeout - ); - } - return resolve(body); - }); -}; - -module.exports.requestSession = (resolve, reject) => ( - module.exports.requestSessionWithRetry(resolve, reject, 0, 0) -); diff --git a/src/redux/navigation.js b/src/redux/navigation.js index 5c2c37623..8191ce5d6 100644 --- a/src/redux/navigation.js +++ b/src/redux/navigation.js @@ -110,9 +110,8 @@ module.exports.handleCompleteRegistration = createProject => (dispatch => { // to be logged in before we try creating a project due to replication lag. window.location = '/'; } else { - dispatch(sessionActions.refreshSessionWithRetry()).then( - dispatch(module.exports.setRegistrationOpen(false)) - ); + dispatch(sessionActions.refreshSession()); + dispatch(module.exports.setRegistrationOpen(false)); } }); diff --git a/src/redux/session.js b/src/redux/session.js index a0d951a07..d5701cb1d 100644 --- a/src/redux/session.js +++ b/src/redux/session.js @@ -1,7 +1,7 @@ const keyMirror = require('keymirror'); const defaults = require('lodash.defaults'); -const {requestSession, requestSessionWithRetry} = require('../lib/session'); +const api = require('../lib/api'); const messageCountActions = require('./message-count.js'); const permissionsActions = require('./permissions.js'); @@ -61,60 +61,45 @@ module.exports.setStatus = status => ({ status: status }); -const handleSessionResponse = (dispatch, body) => { - if (typeof body === 'undefined') return dispatch(module.exports.setSessionError('No session content')); - if ( - body.user && - body.user.banned && - banWhitelistPaths.indexOf(window.location.pathname) === -1 - ) { - window.location = '/accounts/banned-response/'; - return; - } else if ( - body.flags && - body.flags.must_complete_registration && - window.location.pathname !== '/classes/complete_registration' - ) { - window.location = '/classes/complete_registration'; - return; - } else if ( - body.flags && - body.flags.must_reset_password && - !body.flags.must_complete_registration && - window.location.pathname !== '/classes/student_password_reset/' - ) { - window.location = '/classes/student_password_reset/'; - return; - } - dispatch(module.exports.setSession(body)); - dispatch(module.exports.setStatus(module.exports.Status.FETCHED)); - - // get the permissions from the updated session - dispatch(permissionsActions.storePermissions(body.permissions)); - if (typeof body.user !== 'undefined') { - dispatch(messageCountActions.getCount(body.user.username)); - } - return; -}; - module.exports.refreshSession = () => (dispatch => { dispatch(module.exports.setStatus(module.exports.Status.FETCHING)); - return new Promise((resolve, reject) => ( - requestSession(resolve, reject).then(body => { - handleSessionResponse(dispatch, body); - }, err => { - dispatch(module.exports.setSessionError(err)); - }) - )); -}); + api({ + host: '', + uri: '/session/' + }, (err, body) => { + if (err) return dispatch(module.exports.setSessionError(err)); + if (typeof body === 'undefined') return dispatch(module.exports.setSessionError('No session content')); + if ( + body.user && + body.user.banned && + banWhitelistPaths.indexOf(window.location.pathname) === -1 + ) { + window.location = '/accounts/banned-response/'; + return; + } else if ( + body.flags && + body.flags.must_complete_registration && + window.location.pathname !== '/classes/complete_registration' + ) { + window.location = '/classes/complete_registration'; + return; + } else if ( + body.flags && + body.flags.must_reset_password && + !body.flags.must_complete_registration && + window.location.pathname !== '/classes/student_password_reset/' + ) { + window.location = '/classes/student_password_reset/'; + return; + } + dispatch(module.exports.setSession(body)); + dispatch(module.exports.setStatus(module.exports.Status.FETCHED)); -module.exports.refreshSessionWithRetry = () => (dispatch => { - dispatch(module.exports.setStatus(module.exports.Status.FETCHING)); - return new Promise((resolve, reject) => ( - requestSessionWithRetry(resolve, reject, 4, 7500) - )).then(body => { - handleSessionResponse(dispatch, body); - }, err => { - dispatch(module.exports.setSessionError(err)); + // get the permissions from the updated session + dispatch(permissionsActions.storePermissions(body.permissions)); + if (typeof body.user !== 'undefined') { + dispatch(messageCountActions.getCount(body.user.username)); + } + return; }); }); diff --git a/test/unit/components/join-flow.test.jsx b/test/unit/components/join-flow.test.jsx index 8093c8108..91d4b188d 100644 --- a/test/unit/components/join-flow.test.jsx +++ b/test/unit/components/join-flow.test.jsx @@ -41,7 +41,7 @@ describe('JoinFlow', () => { beforeEach(() => { store = mockStore({sessionActions: { - refreshSessionWithRetry: jest.fn() + refreshSession: jest.fn() }}); }); @@ -283,31 +283,16 @@ describe('JoinFlow', () => { expect(success).toEqual(false); }); - test('handleRegistrationResponse calls refreshSessionWithRetry() when passed body with success', done => { + test('handleRegistrationResponse when passed body with success', () => { const props = { - refreshSessionWithRetry: () => (new Promise(() => { // eslint-disable-line no-undef - done(); // ensures that joinFlowInstance.props.refreshSessionWithRetry() was called - })) + refreshSession: jest.fn() }; const joinFlowInstance = getJoinFlowWrapper(props).instance(); joinFlowInstance.handleRegistrationResponse(null, responseBodySuccess, {statusCode: 200}); - }); - - test('handleRegistrationResponse advances to next step when passed body with success', () => { - const props = { - refreshSessionWithRetry: () => (new Promise(resolve => { // eslint-disable-line no-undef - resolve(); - })) - }; - const joinFlowInstance = getJoinFlowWrapper(props).instance(); - joinFlowInstance.handleRegistrationResponse(null, responseBodySuccess, {statusCode: 200}); - process.nextTick( - () => { - expect(joinFlowInstance.state.registrationError).toEqual(null); - expect(joinFlowInstance.state.step).toEqual(1); - expect(joinFlowInstance.state.waiting).toBeFalsy(); - } - ); + expect(joinFlowInstance.state.registrationError).toEqual(null); + expect(joinFlowInstance.props.refreshSession).toHaveBeenCalled(); + expect(joinFlowInstance.state.step).toEqual(1); + expect(joinFlowInstance.state.waiting).toBeFalsy(); }); test('handleRegistrationResponse when passed body with preset server error', () => { @@ -321,7 +306,7 @@ describe('JoinFlow', () => { test('handleRegistrationResponse with failure response, with error fields missing', () => { const props = { - refreshSessionWithRetry: jest.fn() + refreshSession: jest.fn() }; const joinFlowInstance = getJoinFlowWrapper(props).instance(); const responseErr = null; @@ -335,7 +320,7 @@ describe('JoinFlow', () => { statusCode: 200 }; joinFlowInstance.handleRegistrationResponse(responseErr, responseBody, responseObj); - expect(joinFlowInstance.props.refreshSessionWithRetry).not.toHaveBeenCalled(); + expect(joinFlowInstance.props.refreshSession).not.toHaveBeenCalled(); expect(joinFlowInstance.state.registrationError).toEqual({ errorAllowsTryAgain: false, errorMsg: null @@ -354,7 +339,7 @@ describe('JoinFlow', () => { test('handleRegistrationResponse with failure response, with no text explanation', () => { const props = { - refreshSessionWithRetry: jest.fn() + refreshSession: jest.fn() }; const joinFlowInstance = getJoinFlowWrapper(props).instance(); const responseErr = null; @@ -367,7 +352,7 @@ describe('JoinFlow', () => { statusCode: 200 }; joinFlowInstance.handleRegistrationResponse(responseErr, responseBody, responseObj); - expect(joinFlowInstance.props.refreshSessionWithRetry).not.toHaveBeenCalled(); + expect(joinFlowInstance.props.refreshSession).not.toHaveBeenCalled(); expect(joinFlowInstance.state.registrationError).toEqual({ errorAllowsTryAgain: false, errorMsg: null @@ -384,11 +369,11 @@ describe('JoinFlow', () => { test('handleRegistrationResponse when passed status 400', () => { const props = { - refreshSessionWithRetry: jest.fn() + refreshSession: jest.fn() }; const joinFlowInstance = getJoinFlowWrapper(props).instance(); joinFlowInstance.handleRegistrationResponse({}, responseBodyMultipleErrs, {statusCode: 400}); - expect(joinFlowInstance.props.refreshSessionWithRetry).not.toHaveBeenCalled(); + expect(joinFlowInstance.props.refreshSession).not.toHaveBeenCalled(); expect(joinFlowInstance.state.registrationError).toEqual({ errorAllowsTryAgain: true }); diff --git a/test/unit/lib/session.test.js b/test/unit/lib/session.test.js deleted file mode 100644 index 324cf6f1f..000000000 --- a/test/unit/lib/session.test.js +++ /dev/null @@ -1,133 +0,0 @@ -describe('session library', () => { - // respond to session requests with empty session object - let sessionNoUser = jest.fn((opts, callback) => { - callback(null, {}, {statusCode: 200}); - }); - // respond to session requests with session object that indicates - // successfully logged-in user - let sessionYesUser = jest.fn((opts, callback) => { - callback(null, {user: {username: 'test_username'}}, {statusCode: 200}); - }); - // respond to first two requests with empty session object; after that, - // respond with user in object - let sessionNoThenYes = jest.fn((opts, callback) => { - if (sessionNoThenYes.mock.calls.length <= 2) { - callback(null, {}, {statusCode: 200}); - } else { - callback(null, {user: {username: 'test_username'}}, {statusCode: 200}); - } - }); - // respond to session requests with response code 404, indicating no session - // found for that user - let sessionNotFound = jest.fn((opts, callback) => { - callback(null, null, {statusCode: 404}); - }); - // respond to session requests with response code 503, indicating connection failure - let sessionConnectFailure = jest.fn((opts, callback) => { - callback(null, null, {statusCode: 503}); - }); - - // by changing whichMockAPIRequest, we can simulate different api responses - let whichMockAPIRequest = null; - let mockAPIRequest = (opts, callback) => { - whichMockAPIRequest(opts, callback); - }; - - // mock lib/api.js, and include our mocked version in lib/session.js - jest.mock('../../../src/lib/api', () => { - return mockAPIRequest; - }); - const sessionLib = require('../../../src/lib/session'); // eslint-disable-line global-require - - afterEach(() => { - jest.clearAllMocks(); - }); - - test('requestSession can call api 1 time, when session found', done => { - whichMockAPIRequest = sessionYesUser; - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSession(resolve, reject); - }).then(body => { - expect(sessionYesUser).toHaveBeenCalledTimes(1); - expect(body).toEqual({user: {username: 'test_username'}}); - done(); - }); - }); - - test('requestSession can call api 1 time, when session not found', done => { - whichMockAPIRequest = sessionNoUser; - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSession(resolve, reject); - }).then(body => { - expect(sessionNoUser).toHaveBeenCalledTimes(1); - expect(body).toEqual({}); - done(); - }); - }); - - test('requestSessionWithRetry can call api once', done => { - whichMockAPIRequest = sessionNoUser; - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionWithRetry(resolve, reject, 0, 0); - }).then(() => { - expect(sessionNoUser).toHaveBeenCalledTimes(1); - done(); - }); - }); - - test('requestSessionWithRetry can call api multiple times', done => { - whichMockAPIRequest = sessionNoUser; - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionWithRetry(resolve, reject, 2, 0); - }).then(() => { - expect(sessionNoUser).toHaveBeenCalledTimes(3); - done(); - }); - }); - - test('requestSessionWithRetry respects total delay time param within a reasonable tolerance', done => { - whichMockAPIRequest = sessionNoUser; - const startTime = new Date().getTime(); - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionWithRetry(resolve, reject, 2, 2500); - }).then(() => { - const endTime = new Date().getTime(); - expect(endTime - startTime > 2000).toBeTruthy(); - expect(endTime - startTime < 3000).toBeTruthy(); - done(); - }); - }); - - test('requestSessionWithRetry will retry if no user found, then stop when user is found', done => { - whichMockAPIRequest = sessionNoThenYes; - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionWithRetry(resolve, reject, 4, 3000); - }).then(body => { - expect(body).toEqual({user: {username: 'test_username'}}); - expect(sessionNoThenYes).toHaveBeenCalledTimes(3); - done(); - }); - }); - - test('requestSessionWithRetry handles session not found as immediate error', done => { - whichMockAPIRequest = sessionNotFound; - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionWithRetry(resolve, reject, 2, 0); - }).then(() => {}, err => { - expect(err).toBeFalsy(); - done(); - }); - }); - - test('requestSessionWithRetry handles connection failure by retrying', done => { - whichMockAPIRequest = sessionConnectFailure; - new Promise((resolve, reject) => { // eslint-disable-line no-undef - sessionLib.requestSessionWithRetry(resolve, reject, 2, 0); - }).then(body => { - expect(sessionConnectFailure).toHaveBeenCalledTimes(3); - expect(body).toBeFalsy(); - done(); - }); - }); - -});