feat(payments): rework form validation to handle focus & blur independently

packages/fxa-payments-server/.nsprc

@@ -1,3 +1,5 @@
 {
-  "exceptions": []
+  "exceptions": [
+    "https://npmjs.com/advisories/1184"
+  ]
 }

packages/fxa-payments-server/src/App.test.tsx

@@ -10,6 +10,17 @@ import { AppContext } from './lib/AppContext';
 // describe('App', () => {});
 
 describe('App/AppErrorBoundary', () => {
+  beforeEach(() => {
+    // HACK: Swallow the exception thrown by BadComponent - it bubbles up
+    // unnecesarily to jest and makes noise.
+    jest.spyOn(console, 'error');
+    global.console.error.mockImplementation(() => {});
+  });
+
+  afterEach(() => {
+    global.console.error.mockRestore();
+  });
+
   it('renders children that do not cause exceptions', () => {
     const GoodComponent = () => <p data-testid="good-component">Hi</p>;
     const { queryByTestId } = render(

packages/fxa-payments-server/src/components/PaymentForm/index.test.tsx

@@ -6,6 +6,7 @@ import { Omit } from '../../lib/types';
 
 import {
   mockStripeElementOnChangeFns,
+  mockStripeElementOnBlurFns,
   elementChangeResponse,
 } from '../../lib/test-utils';
 
@@ -103,15 +104,18 @@ it('renders error tooltips for invalid stripe elements', () => {
   act(() => {
     for (const [testid, errorMessage] of Object.entries(mockErrors)) {
       mockStripeElementOnChangeFns[testid](
-        elementChangeResponse({ errorMessage })
+        elementChangeResponse({ errorMessage, value: 'foo' })
+      );
+      mockStripeElementOnBlurFns[testid](
+        elementChangeResponse({ errorMessage, value: 'foo' })
       );
     }
   });
 
   for (const [testid, expectedMessage] of Object.entries(mockErrors)) {
     const el = getByTestId(testid);
     const tooltipEl = (el.parentNode as ParentNode).querySelector('.tooltip');
-    expect(tooltipEl).toBeDefined();
+    expect(tooltipEl).not.toBeNull();
     expect((tooltipEl as Node).textContent).toEqual(expectedMessage);
   }
 });

packages/fxa-payments-server/src/components/PaymentForm/index.tsx

@@ -155,12 +155,16 @@ export const PaymentForm = ({
         required
         autoFocus
         spellCheck={false}
-        onValidate={value => {
-          let error = null;
+        onValidate={(value, focused) => {
+          let valid = true;
           if (value !== null && !value) {
-            error = 'Please enter your name';
+            valid = false;
           }
-          return { value, error };
+          return {
+            value,
+            valid,
+            error: !valid && !focused ? 'Please enter your name' : null,
+          };
         }}
       />
 
@@ -199,15 +203,22 @@ export const PaymentForm = ({
           required
           data-testid="zip"
           placeholder="12345"
-          onValidate={value => {
+          onValidate={(value, focused) => {
+            let valid = true;
             let error = null;
             value = ('' + value).substr(0, 5);
             if (!value) {
+              valid = false;
               error = 'Zip code is required';
             } else if (value.length !== 5) {
+              valid = false;
               error = 'Zip code is too short';
             }
-            return { value, error };
+            return {
+              value,
+              valid,
+              error: !focused ? error : null,
+            };
           }}
         />
       </FieldGroup>

packages/fxa-payments-server/src/components/fields.test.tsx

@@ -203,25 +203,26 @@ describe('Input', () => {
           fieldType: 'input',
           required: true,
           value: '',
-          valid: true,
+          valid: false,
           error: 'This field is required',
         },
         'input-2': {
           fieldType: 'input',
           required: true,
           value: '',
-          valid: true,
+          valid: false,
           error: 'This field is required',
         },
       },
     });
   });
 
-  it('accepts a function to validate on blur', () => {
-    const validate = jest.fn((value: string) => {
+  it('accepts a function to validate on both change and blur', () => {
+    const validate = jest.fn((value: string, focused: boolean) => {
       return {
         value: `bar ${value}`,
-        error: 'bad thing',
+        valid: false,
+        error: focused ? null : 'bad thing',
       };
     });
 
@@ -238,9 +239,10 @@ describe('Input', () => {
     );
 
     fireEvent.change(getByTestId('testInput'), { target: { value: 'xyzzy' } });
-    fireEvent.blur(getByTestId('testInput'));
 
-    expect(validate).toBeCalledWith('xyzzy');
+    expect(validate.mock.calls.length).toBe(1);
+    expect(validate.mock.calls[0][0]).toBe('xyzzy');
+    expect(validate.mock.calls[0][1]).toBe(true);
 
     expect(validatorStateRef.current).toEqual({
       error: null,
@@ -250,7 +252,28 @@ describe('Input', () => {
           required: false,
           // validate function can alter value
           value: 'bar xyzzy',
-          // validate function can set error and valid state
+          // validate function can set valid state
+          valid: false,
+          // validate function can set error message separately from valid state
+          error: null,
+        },
+      },
+    });
+
+    fireEvent.blur(getByTestId('testInput'));
+
+    expect(validate.mock.calls.length).toBe(2);
+    expect(validate.mock.calls[1][0]).toBe('bar xyzzy');
+    expect(validate.mock.calls[1][1]).toBe(false);
+
+    expect(validatorStateRef.current).toEqual({
+      error: null,
+      fields: {
+        foo: {
+          fieldType: 'input',
+          required: false,
+          // validate function can alter value, which happens twice since we validated twice
+          value: 'bar bar xyzzy',
           valid: false,
           error: 'bad thing',
         },
@@ -259,7 +282,11 @@ describe('Input', () => {
   });
 
   it('adds an "invalid" class name when invalid', () => {
-    const validate = (value: string) => ({ value: 'bar', error: 'bad thing' });
+    const validate = (value: string) => ({
+      value: 'bar',
+      valid: false,
+      error: 'bad thing',
+    });
     const { container, getByTestId } = render(
       <TestForm>
         <Input
@@ -271,6 +298,7 @@ describe('Input', () => {
       </TestForm>
     );
     fireEvent.change(getByTestId('testInput'), { target: { value: 'xyzzy' } });
+    fireEvent.blur(getByTestId('testInput'), { target: { value: 'xyzzy' } });
     expect(container.querySelector('input.invalid')).not.toBeNull();
   });
 });
@@ -319,7 +347,9 @@ describe('StripeElement', () => {
       </TestForm>
     );
     fireEvent.click(getByTestId('mockStripe'));
-    expect(validatorStateRef.current.fields['input-1'].valid).toEqual(null);
+    expect(validatorStateRef.current.fields['input-1'].valid).toEqual(
+      undefined
+    );
   });
 
   it('does nothing if value is not yet complete', () => {
@@ -334,7 +364,9 @@ describe('StripeElement', () => {
       </TestForm>
     );
     fireEvent.click(getByTestId('mockStripe'));
-    expect(validatorStateRef.current.fields['input-1'].valid).toEqual(null);
+    expect(validatorStateRef.current.fields['input-1'].valid).toEqual(
+      undefined
+    );
   });
 
   it('handles error result from contained stripe element', () => {
@@ -349,6 +381,9 @@ describe('StripeElement', () => {
     );
 
     fireEvent.click(getByTestId('mockStripe'));
+    expect(container.querySelector('aside.tooltip')).toBeNull();
+
+    fireEvent.blur(getByTestId('mockStripe'));
 
     const tooltipEl = container.querySelector('aside.tooltip');
     expect(tooltipEl).not.toBeNull();
@@ -387,20 +422,31 @@ describe('StripeElement', () => {
       </TestForm>
     );
 
+    fireEvent.click(getByTestId('mockStripe'));
+    expect(container.querySelector('aside.tooltip')).toBeNull();
+
     fireEvent.blur(getByTestId('mockStripe'));
 
     const tooltipEl = container.querySelector('aside.tooltip');
     expect(tooltipEl).not.toBeNull();
-    expect((tooltipEl as Element).textContent).toContain('Frobnitz is required');
+    expect((tooltipEl as Element).textContent).toContain(
+      'Frobnitz is required'
+    );
   });
 
   it('supports a custom onValidate function', () => {
-    const MockStripeElement = buildMockStripeElement(
-      { value: 'foo', error: 'not this', complete: true }
-    );
+    const MockStripeElement = buildMockStripeElement({
+      value: 'foo',
+      error: 'not this',
+      complete: true,
+    });
     const validatorStateRef = mkValidatorStateRef();
     const expectedError = 'My hovercraft is full of eels';
-    const onValidate = jest.fn(value => ({ value, error: expectedError }));
+    const onValidate = jest.fn(value => ({
+      value,
+      valid: false,
+      error: expectedError,
+    }));
     const { container, getByTestId } = render(
       <TestForm validatorStateRef={validatorStateRef}>
         <StripeElement
@@ -431,6 +477,7 @@ describe('StripeElement', () => {
     );
 
     fireEvent.click(getByTestId('mockStripe'));
+    fireEvent.blur(getByTestId('mockStripe'));
 
     const tooltipEl = container.querySelector('aside.tooltip');
     expect(tooltipEl).not.toBeNull();