diff --git a/src/components/join-flow/email-step.jsx b/src/components/join-flow/email-step.jsx index c70093c74..ed13f9808 100644 --- a/src/components/join-flow/email-step.jsx +++ b/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; } ); diff --git a/src/components/join-flow/username-step.jsx b/src/components/join-flow/username-step.jsx index 3610a09e7..aef6d5617 100644 --- a/src/components/join-flow/username-step.jsx +++ b/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; } ); diff --git a/src/lib/validate.js b/src/lib/validate.js index 65efcc03e..ab75b29b0 100644 --- a/src/lib/validate.js +++ b/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; } }); diff --git a/test/unit/components/email-step.test.jsx b/test/unit/components/email-step.test.jsx index f74eebc9c..65dc7ca91 100644 --- a/test/unit/components/email-step.test.jsx +++ b/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( + ); + 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( + + ); + 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( + + ); + 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(); + }); + }); + }); +}); diff --git a/test/unit/components/username-step.test.jsx b/test/unit/components/username-step.test.jsx index c64045a12..f08b8b682 100644 --- a/test/unit/components/username-step.test.jsx +++ b/test/unit/components/username-step.test.jsx @@ -1,9 +1,24 @@ 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(); @@ -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( - ); + const wrapper = shallowWithIntl(); 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(); + 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( + + ); + 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( + + ); + 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(); + }); + }); + }); +});