Merge pull request #3485 from benjiwheeler/join-flow-cache-ignores-api-failures

username step and email step caches ignore api failures

src/components/join-flow/email-step.jsx

@@ -83,7 +83,10 @@ class EmailStep extends React.Component {
         // email is not in our cache
         return validate.validateEmailRemotely(email).then(
             remoteResult => {
-                this.emailRemoteCache[email] = remoteResult;
+                // cache result, if it successfully heard back from server
+                if (remoteResult.requestSucceeded) {
+                    this.emailRemoteCache[email] = remoteResult;
+                }
                 return remoteResult;
             }
         );

src/components/join-flow/username-step.jsx

@@ -56,7 +56,10 @@ class UsernameStep extends React.Component {
         // username is not in our cache
         return validate.validateUsernameRemotely(username).then(
             remoteResult => {
-                this.usernameRemoteCache[username] = remoteResult;
+                // cache result, if it successfully heard back from server
+                if (remoteResult.requestSucceeded) {
+                    this.usernameRemoteCache[username] = remoteResult;
+                }
                 return remoteResult;
             }
         );

src/lib/validate.js

@@ -21,21 +21,21 @@ module.exports.validateUsernameRemotely = username => (
             uri: `/accounts/checkusername/${username}/`
         }, (err, body, res) => {
             if (err || res.statusCode !== 200) {
-                resolve({valid: false, errMsgId: 'general.error'});
+                resolve({requestSucceeded: false, valid: false, errMsgId: 'general.error'});
             }
             switch (body.msg) {
             case 'valid username':
-                resolve({valid: true});
+                resolve({requestSucceeded: true, valid: true});
                 break;
             case 'username exists':
-                resolve({valid: false, errMsgId: 'registration.validationUsernameExists'});
+                resolve({requestSucceeded: true, valid: false, errMsgId: 'registration.validationUsernameExists'});
                 break;
             case 'bad username': // i.e., vulgar
-                resolve({valid: false, errMsgId: 'registration.validationUsernameNotAllowed'});
+                resolve({requestSucceeded: true, valid: false, errMsgId: 'registration.validationUsernameNotAllowed'});
                 break;
             case 'invalid username':
             default:
-                resolve({valid: false, errMsgId: 'registration.validationUsernameNotAllowed'});
+                resolve({requestSucceeded: true, valid: false, errMsgId: 'registration.validationUsernameNotAllowed'});
             }
         });
     })
@@ -86,16 +86,16 @@ module.exports.validateEmailRemotely = email => (
             uri: '/accounts/check_email/'
         }, (err, body, res) => {
             if (err || res.statusCode !== 200 || !body || body.length < 1 || !body[0].msg) {
-                resolve({valid: false, errMsgId: 'general.apiError'});
+                resolve({requestSucceeded: false, valid: false, errMsgId: 'general.apiError'});
             }
             switch (body[0].msg) {
             case 'valid email':
-                resolve({valid: true});
+                resolve({requestSucceeded: true, valid: true});
                 break;
             case 'Scratch is not allowed to send email to this address.': // e.g., bad TLD or block-listed
             case 'Enter a valid email address.':
             default:
-                resolve({valid: false, errMsgId: 'registration.validationEmailInvalid'});
+                resolve({requestSucceeded: true, valid: false, errMsgId: 'registration.validationEmailInvalid'});
                 break;
             }
         });

test/unit/components/email-step.test.jsx

@@ -4,9 +4,24 @@ const JoinFlowStep = require('../../../src/components/join-flow/join-flow-step.j
 const FormikInput = require('../../../src/components/formik-forms/formik-input.jsx');
 const FormikCheckbox = require('../../../src/components/formik-forms/formik-checkbox.jsx');
 
-const mockedValidateEmailRemotely = jest.fn(() => (
+const requestSuccessResponse = {
+    requestSucceeded: true,
+    valid: false,
+    errMsgId: 'registration.validationEmailInvalid'
+};
+const requestFailureResponse = {
+    requestSucceeded: false,
+    valid: false,
+    errMsgId: 'general.error'
+};
+// mockedValidateEmailRemotely will return a promise resolving with remoteRequestResponse.
+// Using remoteRequestResponse, rather than using requestSuccessResponse directly,
+// lets us change where remoteRequestResponse points later, without actually changing
+// mockedValidateEmailRemotely.
+let remoteRequestResponse = requestSuccessResponse;
+let mockedValidateEmailRemotely = jest.fn(() => (
     /* eslint-disable no-undef */
-    Promise.resolve({valid: false, errMsgId: 'registration.validationEmailInvalid'})
+    Promise.resolve(remoteRequestResponse)
     /* eslint-enable no-undef */
 ));
 
@@ -179,6 +194,13 @@ describe('EmailStep test', () => {
         const val = emailStepWrapper.instance().validateEmail();
         expect(val).toBe('general.required');
     });
+});
+
+describe('validateEmailRemotelyWithCache test with successful requests', () => {
+
+    afterEach(() => {
+        jest.clearAllMocks();
+    });
 
     test('validateEmailRemotelyWithCache calls validate.validateEmailRemotely', done => {
         const intlWrapper = shallowWithIntl(
@@ -242,3 +264,79 @@ describe('EmailStep test', () => {
             });
     });
 });
+
+
+describe('validateEmailRemotelyWithCache test with failing requests', () => {
+
+    beforeEach(() => {
+        // needs to be wrapped inside beforeEach, because if not, it gets run as this
+        // test file is loaded, and goes into effect before any of the earlier tests run!
+        remoteRequestResponse = requestFailureResponse;
+    });
+
+    afterEach(() => {
+        jest.clearAllMocks();
+    });
+
+    test('validateEmailRemotelyWithCache calls validate.validateEmailRemotely', done => {
+        const wrapper = shallowWithIntl(
+            <EmailStep />);
+        const instance = wrapper.dive().instance();
+
+        instance.validateEmailRemotelyWithCache('some-email@some-domain.com')
+            .then(response => {
+                expect(mockedValidateEmailRemotely).toHaveBeenCalled();
+                expect(response.valid).toBe(false);
+                expect(response.errMsgId).toBe('general.error');
+                done();
+            });
+    });
+
+    test('validateEmailRemotelyWithCache, called twice with different data, makes two remote requests', done => {
+        const wrapper = shallowWithIntl(
+            <EmailStep />
+        );
+        const instance = wrapper.dive().instance();
+
+        instance.validateEmailRemotelyWithCache('some-email@some-domain.com')
+            .then(response => {
+                expect(mockedValidateEmailRemotely).toHaveBeenCalledTimes(1);
+                expect(response.valid).toBe(false);
+                expect(response.errMsgId).toBe('general.error');
+            })
+            .then(() => {
+                // make the same request a second time
+                instance.validateEmailRemotelyWithCache('different-email@some-domain.org')
+                    .then(response => {
+                        expect(mockedValidateEmailRemotely).toHaveBeenCalledTimes(2);
+                        expect(response.valid).toBe(false);
+                        expect(response.errMsgId).toBe('general.error');
+                        done();
+                    });
+            });
+    });
+
+    test('validateEmailRemotelyWithCache, called 2x w/same data, makes 2 requests, since 1st not stored', done => {
+        const wrapper = shallowWithIntl(
+            <EmailStep />
+        );
+        const instance = wrapper.dive().instance();
+
+        instance.validateEmailRemotelyWithCache('some-email@some-domain.com')
+            .then(response => {
+                expect(mockedValidateEmailRemotely).toHaveBeenCalledTimes(1);
+                expect(response.valid).toBe(false);
+                expect(response.errMsgId).toBe('general.error');
+            })
+            .then(() => {
+                // make the same request a second time
+                instance.validateEmailRemotelyWithCache('some-email@some-domain.com')
+                    .then(response => {
+                        expect(mockedValidateEmailRemotely).toHaveBeenCalledTimes(2);
+                        expect(response.valid).toBe(false);
+                        expect(response.errMsgId).toBe('general.error');
+                        done();
+                    });
+            });
+    });
+});

test/unit/components/username-step.test.jsx

 const React = require('react');
 const {shallowWithIntl} = require('../../helpers/intl-helpers.jsx');
 
-const mockedValidateUsernameRemotely = jest.fn(() => (
+const requestSuccessResponse = {
+    requestSucceeded: true,
+    valid: false,
+    errMsgId: 'registration.validationUsernameNotAllowed'
+};
+const requestFailureResponse = {
+    requestSucceeded: false,
+    valid: false,
+    errMsgId: 'general.error'
+};
+// mockedValidateUsernameRemotely will return a promise resolving with remoteRequestResponse.
+// Using remoteRequestResponse, rather than using requestSuccessResponse directly,
+// lets us change where remoteRequestResponse points later, without actually changing
+// mockedValidateUsernameRemotely.
+let remoteRequestResponse = requestSuccessResponse;
+let mockedValidateUsernameRemotely = jest.fn(() => (
     /* eslint-disable no-undef */
-    Promise.resolve({valid: false, errMsgId: 'registration.validationUsernameNotAllowed'})
+    Promise.resolve(remoteRequestResponse)
     /* eslint-enable no-undef */
 ));
 
@@ -17,11 +32,7 @@ jest.mock('../../../src/lib/validate.js', () => (
 // must come after validation mocks, so validate.js will be mocked before it is required
 const UsernameStep = require('../../../src/components/join-flow/username-step.jsx');
 
-describe('UsernameStep test', () => {
-
-    afterEach(() => {
-        jest.clearAllMocks();
-    });
+describe('UsernameStep tests', () => {
 
     test('send correct props to formik', () => {
         const wrapper = shallowWithIntl(<UsernameStep />);
@@ -54,14 +65,22 @@ describe('UsernameStep test', () => {
         expect(mockedOnNextStep).toHaveBeenCalledWith(formData);
     });
 
+});
+
+describe('validateUsernameRemotelyWithCache test with successful requests', () => {
+
+    afterEach(() => {
+        jest.clearAllMocks();
+    });
+
     test('validateUsernameRemotelyWithCache calls validate.validateUsernameRemotely', done => {
-        const wrapper = shallowWithIntl(
-            <UsernameStep />);
+        const wrapper = shallowWithIntl(<UsernameStep />);
         const instance = wrapper.dive().instance();
 
         instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
             .then(response => {
                 expect(mockedValidateUsernameRemotely).toHaveBeenCalled();
+                expect(response.requestSucceeded).toBe(true);
                 expect(response.valid).toBe(false);
                 expect(response.errMsgId).toBe('registration.validationUsernameNotAllowed');
                 done();
@@ -77,6 +96,7 @@ describe('UsernameStep test', () => {
         instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
             .then(response => {
                 expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(1);
+                expect(response.requestSucceeded).toBe(true);
                 expect(response.valid).toBe(false);
                 expect(response.errMsgId).toBe('registration.validationUsernameNotAllowed');
             })
@@ -85,6 +105,7 @@ describe('UsernameStep test', () => {
                 instance.validateUsernameRemotelyWithCache('secondDifferent66')
                     .then(response => {
                         expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(2);
+                        expect(response.requestSucceeded).toBe(true);
                         expect(response.valid).toBe(false);
                         expect(response.errMsgId).toBe('registration.validationUsernameNotAllowed');
                         done();
@@ -101,6 +122,7 @@ describe('UsernameStep test', () => {
         instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
             .then(response => {
                 expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(1);
+                expect(response.requestSucceeded).toBe(true);
                 expect(response.valid).toBe(false);
                 expect(response.errMsgId).toBe('registration.validationUsernameNotAllowed');
             })
@@ -109,6 +131,7 @@ describe('UsernameStep test', () => {
                 instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
                     .then(response => {
                         expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(1);
+                        expect(response.requestSucceeded).toBe(true);
                         expect(response.valid).toBe(false);
                         expect(response.errMsgId).toBe('registration.validationUsernameNotAllowed');
                         done();
@@ -116,3 +139,82 @@ describe('UsernameStep test', () => {
             });
     });
 });
+
+describe('validateUsernameRemotelyWithCache test with failing requests', () => {
+
+    beforeEach(() => {
+        // needs to be wrapped inside beforeEach, because if not, it gets run as this
+        // test file is loaded, and goes into effect before any of the earlier tests run!
+        remoteRequestResponse = requestFailureResponse;
+    });
+
+    afterEach(() => {
+        jest.clearAllMocks();
+    });
+
+    test('validateUsernameRemotelyWithCache calls validate.validateUsernameRemotely', done => {
+        const wrapper = shallowWithIntl(<UsernameStep />);
+        const instance = wrapper.dive().instance();
+
+        instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
+            .then(response => {
+                expect(mockedValidateUsernameRemotely).toHaveBeenCalled();
+                expect(response.requestSucceeded).toBe(false);
+                expect(response.valid).toBe(false);
+                expect(response.errMsgId).toBe('general.error');
+                done();
+            });
+    });
+
+    test('validateUsernameRemotelyWithCache, called twice with different data, makes two remote requests', done => {
+        const wrapper = shallowWithIntl(
+            <UsernameStep />
+        );
+        const instance = wrapper.dive().instance();
+
+        instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
+            .then(response => {
+                expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(1);
+                expect(response.requestSucceeded).toBe(false);
+                expect(response.valid).toBe(false);
+                expect(response.errMsgId).toBe('general.error');
+            })
+            .then(() => {
+                // make the same request a second time
+                instance.validateUsernameRemotelyWithCache('secondDifferent66')
+                    .then(response => {
+                        expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(2);
+                        expect(response.requestSucceeded).toBe(false);
+                        expect(response.valid).toBe(false);
+                        expect(response.errMsgId).toBe('general.error');
+                        done();
+                    });
+            });
+    });
+
+    test('validateUsernameRemotelyWithCache, called 2x w/same data, makes 2 requests, since 1st not stored', done => {
+        const wrapper = shallowWithIntl(
+            <UsernameStep />
+        );
+        const instance = wrapper.dive().instance();
+
+        instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
+            .then(response => {
+                expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(1);
+                expect(response.requestSucceeded).toBe(false);
+                expect(response.valid).toBe(false);
+                expect(response.errMsgId).toBe('general.error');
+            })
+            .then(() => {
+                // make the same request a second time
+                instance.validateUsernameRemotelyWithCache('newUniqueUsername55')
+                    .then(response => {
+                        expect(mockedValidateUsernameRemotely).toHaveBeenCalledTimes(2);
+                        expect(response.requestSucceeded).toBe(false);
+                        expect(response.valid).toBe(false);
+                        expect(response.errMsgId).toBe('general.error');
+                        done();
+                    });
+            });
+    });
+});