Merge pull request #3628 from LLK/revert-3618-hotfix/join-retry-session

Revert "[Develop] Hotfix/join retry session"

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.props.refreshSession();
-                    this.setState({
+                this.setState({step: this.state.step + 1});
-                        step: this.state.step + 1,
-                        waiting: false
-                    });
-                });
                 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({
+                this.setState({registrationError: {errorAllowsTryAgain: true}});
-                    registrationError: {errorAllowsTryAgain: true},
-                    waiting: false
-                });
                 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: () => (
+    refreshSession: () => {
-        dispatch(sessionActions.refreshSessionWithRetry())
+        dispatch(sessionActions.refreshSession());
-    )
+    }
 });
 
 // Allow incoming props to override redux-provided props. Used to mock in tests.

src/lib/session.js

@@ -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)
-);

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(sessionActions.refreshSession());
-            dispatch(module.exports.setRegistrationOpen(false))
+        dispatch(module.exports.setRegistrationOpen(false));
-        );
     }
 });
 

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) => (
+    api({
-        requestSession(resolve, reject).then(body => {
+        host: '',
-            handleSessionResponse(dispatch, body);
+        uri: '/session/'
-        }, err => {
+    }, (err, body) => {
-            dispatch(module.exports.setSessionError(err));
+        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 => {
+        // get the permissions from the updated session
-    dispatch(module.exports.setStatus(module.exports.Status.FETCHING));
+        dispatch(permissionsActions.storePermissions(body.permissions));
-    return new Promise((resolve, reject) => (
+        if (typeof body.user !== 'undefined') {
-        requestSessionWithRetry(resolve, reject, 4, 7500)
+            dispatch(messageCountActions.getCount(body.user.username));
-    )).then(body => {
+        }
-        handleSessionResponse(dispatch, body);
+        return;
-    }, err => {
-        dispatch(module.exports.setSessionError(err));
     });
 });

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
+            refreshSession: jest.fn()
-                done(); // ensures that joinFlowInstance.props.refreshSessionWithRetry() was called
-            }))
         };
         const joinFlowInstance = getJoinFlowWrapper(props).instance();
         joinFlowInstance.handleRegistrationResponse(null, responseBodySuccess, {statusCode: 200});
-    });
+        expect(joinFlowInstance.state.registrationError).toEqual(null);
-
+        expect(joinFlowInstance.props.refreshSession).toHaveBeenCalled();
-    test('handleRegistrationResponse advances to next step when passed body with success', () => {
+        expect(joinFlowInstance.state.step).toEqual(1);
-        const props = {
+        expect(joinFlowInstance.state.waiting).toBeFalsy();
-            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();
-            }
-        );
     });
 
     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
         });

test/unit/lib/session.test.js

@@ -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();
-        });
-    });
-
-});