Add erroneous character checking to address search field#11224
Add erroneous character checking to address search field#11224WilliamBirdsall merged 1 commit intomainfrom
Conversation
|
Hey @aduth sorry to bother you on this one again, but is there any reason you can think of as to why the test in: |
I think that for this test to be valuable (i.e. to verify that it actually shows the invalid characters), we should stub i18n data for the tests so that it has the Example of where we've done this elsewhere: |
I'm all for adding this but wouldn't we run into the same issue with the error markup itself not rendering in the mock? |
Interestingly, I think the issue currently is that you have an invalid escape in your regular expression here (the
|
Ah! Must have been a typo when I switched the regex from ruby to JS. UGH thank you I cant believe that was it! |
94c2676 to
0d82aa0
Compare
|
Note: Going to create a new ticket for the more robust tests as this ticket is currently blocking other tickets from starting. |
|
I'd personally disagree with splitting tests to its own ticket. To me, test coverage is expected to be included for any change. It's also written in the program Definition of Done:
|
|
I had iterated on tests locally in an earlier version of the branch. In case it's helpful. diff --git a/app/javascript/packages/address-search/components/full-address-search-input.tsx b/app/javascript/packages/address-search/components/full-address-search-input.tsx
index 757498981e..27401a1194 100644
--- a/app/javascript/packages/address-search/components/full-address-search-input.tsx
+++ b/app/javascript/packages/address-search/components/full-address-search-input.tsx
@@ -6,3 +6,3 @@ import { SpinnerButtonRefHandle, SpinnerButton } from '@18f/identity-spinner-but
import { ValidatedField } from '@18f/identity-validated-field';
-import { t } from '@18f/identity-i18n';
+import { useI18n } from '@18f/identity-react-i18n';
import { useCallback, useEffect, useRef, useState } from 'react';
@@ -32,2 +32,3 @@ export default function FullAddressSearchInput({
}: FullAddressSearchInputProps) {
+ const { t } = useI18n();
const spinnerButtonRef = useRef<SpinnerButtonRefHandle>(null);
diff --git a/app/javascript/packages/address-search/components/full-address-search.spec.tsx b/app/javascript/packages/address-search/components/full-address-search.spec.tsx
index 82f0c5bcbe..85ab24cb21 100644
--- a/app/javascript/packages/address-search/components/full-address-search.spec.tsx
+++ b/app/javascript/packages/address-search/components/full-address-search.spec.tsx
@@ -8,2 +8,4 @@ import type { SetupServer } from 'msw/node';
import { SWRConfig } from 'swr';
+import { I18nContext } from '@18f/identity-react-i18n';
+import { I18n } from '@18f/identity-i18n';
import FullAddressSearch from './full-address-search';
@@ -137,13 +139,24 @@ describe('FullAddressSearch', () => {
const handleLocationsFound = sandbox.stub();
- const { findByText, findAllByText, findByLabelText } = render(
- <SWRConfig value={{ provider: () => new Map() }}>
- <FullAddressSearch
- usStatesTerritories={usStatesTerritories}
- onFoundLocations={handleLocationsFound}
- locationsURL={locationsURL}
- registerField={() => undefined}
- handleLocationSelect={undefined}
- disabled={false}
- />
- </SWRConfig>,
+ const { findByText, findByLabelText } = render(
+ <I18nContext.Provider
+ value={
+ new I18n({
+ strings: {
+ 'in_person_proofing.form.address.errors.unsupported_chars':
+ 'Our system cannot read the following characters: %{char_list}.',
+ },
+ })
+ }
+ >
+ <SWRConfig value={{ provider: () => new Map() }}>
+ <FullAddressSearch
+ usStatesTerritories={usStatesTerritories}
+ onFoundLocations={handleLocationsFound}
+ locationsURL={locationsURL}
+ registerField={() => undefined}
+ handleLocationSelect={undefined}
+ disabled={false}
+ />
+ </SWRConfig>
+ </I18nContext.Provider>,
);
@@ -170,8 +183,5 @@ describe('FullAddressSearch', () => {
- const errors = await findAllByText(
- 'in_person_proofing.form.address.errors.unsupported_chars',
- { exact: false },
- );
+ const error = await findByText('Our system cannot read the following characters: ,.');
- expect(errors).to.have.lengthOf(1);
+ expect(error).to.exist();
}); |
|
Can you add the pull request template to the original comment? |
|
Will do x2! Thanks! |
39cae4e to
7fdebbf
Compare
5673bbb to
7f0fff6
Compare
There was a problem hiding this comment.
Could we pull the pattern attribute off validatedAddressFieldRef.current rather than duplicating the regular expression? Or at least could we assign it once as a constant somewhere and reference it in the same place?
There was a problem hiding this comment.
Looks like both files would have access to validatedAddressFieldRef since this one creates it, and the other one just uses it. Can we arbitrarily set the pattern as a property on the ref in this file? Or is there a specific way of handling that? Not super familiar with ref uses.
There was a problem hiding this comment.
I was expecting that we could keep the pattern assignment in the JSX for the <TextInput /> element, and then retrieve it off the ref here. validatedAddressFieldRef.current should be the rendered HTMLInputElement.
There was a problem hiding this comment.
Oh! I was thinking about it the wrong way around. Great idea!
There was a problem hiding this comment.
Similar point as previous, maybe we could assign the regular expression to a constant and reference it in both places in this file.
7f0fff6 to
ec67914
Compare
ec67914 to
af24dd8
Compare
|
Looks like we need to have the regex itself in |
af24dd8 to
02a7f73
Compare
Should we be concerned that |
Its defined as null directly above that and then I think since its a test for the internal functionality of that file the element isn't created yet. That's how I'm reading it at least... |
02a7f73 to
49b346b
Compare
|
Typically |
That makes sense. But in the test it's only using |
Sounds like the test is lacking some test coverage for a scenario where those elements exist (i.e. the actual end-user behavior). But I do think it could make sense to use the optional chain operator to access the safely access address element properties like we're doing with other ref element references in that file. |
KeithNava
left a comment
There was a problem hiding this comment.
Testing steps passed for me! 🔥
So the test that's in the PR changes handles the actual end user behavior (checking for proper error message) while this test ( Over all we have coverage for end user and the internal hook logic so I ~ think ~ its solid enough? Note that zipcode also repeats the regex in the |
Oh wait are you saying we should also add extra tests for the upper part of the |
|
Yes, I think if the hook has specific behaviors for how it interacts with elements, it'd be reasonable to expect that the specs for that hook would test those behaviors. The overarching issue with what you're seeing is that the specs currently don't reflect real-world usage, hence the issues you're running into. |
|
Hey @aduth, I tried pairing with another Joy engineer to figure out how to get the Is there any way you could provide some guidance on what is needed? Thanks! |
|
I think if you could at least have test coverage somewhere demonstrating the expected behavior, that'd be fine, even if it's in the spec of the component using the hook. This is a case where the hook is so intrinsically tied to the component that it would be fine (and arguably there might not be much value in having the hook at all). I'm guessing the expected behavior here is that we don't make a network request? We could check that the SWR cache doesn't expand on the invalid submission. I actually think the repeated validation in the hook is unnecessary if we're also using native form validation. We can check the validity on the elements directly? Something like this: diff --git a/app/javascript/packages/address-search/components/full-address-search.spec.tsx b/app/javascript/packages/address-search/components/full-address-search.spec.tsx
index 5e1688923b..a3a7ff018d 100644
--- a/app/javascript/packages/address-search/components/full-address-search.spec.tsx
+++ b/app/javascript/packages/address-search/components/full-address-search.spec.tsx
@@ -139,2 +139,3 @@ describe('FullAddressSearch', () => {
const handleLocationsFound = sandbox.stub();
+ const locationCache = new Map();
const { findByText, findByLabelText } = render(
@@ -150,3 +151,3 @@ describe('FullAddressSearch', () => {
>
- <SWRConfig value={{ provider: () => new Map() }}>
+ <SWRConfig value={{ provider: () => locationCache }}>
<FullAddressSearch
@@ -172,3 +173,3 @@ describe('FullAddressSearch', () => {
);
- await userEvent.type(
+ await userEvent.selectOptions(
await findByLabelText('in_person_proofing.body.location.po_search.state_label'),
@@ -178,3 +179,3 @@ describe('FullAddressSearch', () => {
await findByLabelText('in_person_proofing.body.location.po_search.zipcode_label'),
- '10',
+ '00010',
);
@@ -189,2 +190,3 @@ describe('FullAddressSearch', () => {
expect(error).to.exist();
+ expect(locationCache.size).to.equal(1);
});
diff --git a/app/javascript/packages/address-search/hooks/use-validated-usps-locations.ts b/app/javascript/packages/address-search/hooks/use-validated-usps-locations.ts
index f782c564fa..cd95c3f98b 100644
--- a/app/javascript/packages/address-search/hooks/use-validated-usps-locations.ts
+++ b/app/javascript/packages/address-search/hooks/use-validated-usps-locations.ts
@@ -8,6 +8,6 @@ export default function useValidatedUspsLocations(locationsURL: string) {
const [locationQuery, setLocationQuery] = useState<LocationQuery | null>(null);
- const validatedAddressFieldRef = useRef<HTMLFormElement>(null);
- const validatedCityFieldRef = useRef<HTMLFormElement>(null);
- const validatedStateFieldRef = useRef<HTMLFormElement>(null);
- const validatedZipCodeFieldRef = useRef<HTMLFormElement>(null);
+ const validatedAddressFieldRef = useRef<HTMLInputElement>(null);
+ const validatedCityFieldRef = useRef<HTMLInputElement>(null);
+ const validatedStateFieldRef = useRef<HTMLInputElement>(null);
+ const validatedZipCodeFieldRef = useRef<HTMLInputElement>(null);
@@ -16,5 +16,2 @@ export default function useValidatedUspsLocations(locationsURL: string) {
const zipCodeIsValid = zipCode.length === 5 && !!zipCode.match(/\d{5}/);
- const addressReStr = "[A-Za-z0-9-' ./#]*";
- const addressRegex = new RegExp(addressReStr, 'g');
- const addressIsValid = address.replace(addressRegex, '') === '';
@@ -53,3 +50,10 @@ export default function useValidatedUspsLocations(locationsURL: string) {
- return formIsValid && zipCodeIsValid && addressIsValid;
+ const hasInvalidFields = [
+ validatedAddressFieldRef,
+ validatedCityFieldRef,
+ validatedStateFieldRef,
+ validatedZipCodeFieldRef,
+ ].some((fieldRef) => fieldRef.current?.validity?.valid === false);
+
+ return formIsValid && zipCodeIsValid && !hasInvalidFields;
}; |
Patched this into the branch and tested successfully. Was curious what the SWR bit was for that makes more sense now thanks! |
…h that matches other address field validations
49b346b to
6170345
Compare
|
@gina-yamada Would you mind taking a quick look at the last couple small changes? Added an if check in the erroneous characters function and patched with Andrew's patch above. Then ill merge! |
| ); | ||
|
|
||
| const error = await findByText( | ||
| 'Our system cannot read the following characters: , . Please try again using substitutes for those characters.', |
There was a problem hiding this comment.
I modified text here to prove test was working properly. Good
|
I tested same list as above and all behavior observed is what I'd expect after last update. LGTM |
|
merging! |
…h that matches other address field validations (#11224)




🎫 Ticket
Link to the relevant ticket:
LG-14071
🛠 Summary of changes
Added client side character checking to PO search address field so that it aligns with other address field validations such as the ones associated with the state ID form.
Also added a validation function server side in case someone disables JS and submits. (It will fail as it used to instead of showing the new unsupported characters message since this is not how users
shouldbe interacting with the form, but it's technically possible.)📜 Testing Plan
Provide a checklist of steps to confirm the changes.
(To see the dynamism of the error message you can also throw in a dollar sign and resubmit)