LG-7720: IPP: PO Search SWR refactor and enhancements by allthesignals · Pull Request #7468 · 18F/identity-idp

allthesignals · 2022-12-11T20:15:44Z

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

Refactors much of the in-person-location component by:

Splitting out the locations view log
Implementing SWR to simplify state management

Feature work:

Avoid firing request on load

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:

Initial load, blank page:

No results found (locally):

After:

Initial load (shows pilots):

No results found (in dev env):

allthesignals · 2022-12-11T20:35:40Z

A lot of tension here around naming... switching around between: 1) In-Person-Locations to 2) PostOffices to 2) UspsLocations?

Most naming is some format of "In-Person Location" or some derivation of In-Person Proofing... so let's go with that? Although "Post Office" is most accurate, it's a specific kind of Post Office, one that performs ID proofing...

allthesignals · 2022-12-11T20:58:53Z

This Step component is more like an "InPersonLocationSelectionStep" rather than

allthesignals · 2022-12-12T07:01:37Z

app/javascript/packages/document-capture/components/address-search.tsx

Location should be renamed too?

allthesignals · 2022-12-12T07:08:06Z

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

Seems like the wrong name... it's in Address form before it becomes a query for in person locations...

app/javascript/packages/request/index.ts

app/javascript/packages/document-capture/components/in-person-locations.tsx

allthesignals · 2022-12-12T23:25:17Z

A little confused by this failure:

$ is-es5-safe 'public/packs?(-test)/js/*.js'
public/packs/js/39-c364e100.js SyntaxError: The keyword 'const' is reserved (2:135043)
    at Parser.pp$4.raise (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:2825:15)
    at Parser.pp$3.checkUnreserved (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:2752:12)
    at Parser.pp$3.parseIdent (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:2781:12)
    at Parser.pp$3.parseExprAtom (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:2198:21)
    at Parser.pp$3.parseExprSubscripts (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:2089:21)
    at Parser.pp$3.parseMaybeUnary (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:2066:19)
    at Parser.pp$3.parseExprOps (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:2010:21)
    at Parser.pp$3.parseMaybeConditional (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:1993:21)
    at Parser.pp$3.parseMaybeAssign (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:1968:21)
    at Parser.pp$3.parseExpression (/builds/lg/identity-idp/app/javascript/packages/es5-safe/node_modules/acorn/dist/acorn.js:1933:21) {
  pos: 135112,
  loc: Position { line: 2, column: 135043 },
  raisedAt: 135119
}
error Command failed with exit code 1.

It's a lint step...

Further: we need to duplicate/modify the IPP Rails test to get the full coverage needed (I think?)

NavaTim

Would you post desktop/mobile screenshots or an animated GIF to compare w/ the mockup?

app/javascript/packages/document-capture/components/address-search.tsx

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

NavaTim · 2022-12-13T01:45:25Z

$ is-es5-safe 'public/packs?(-test)/js/*.js'

@allthesignals Looks to me like a test for IE11 support.

A lot of tension here around naming... switching around between: 1) In-Person-Locations to 2) PostOffices to 2) UspsLocations?

Yeah, makes sense to consolidate the naming.

aduth · 2022-12-13T13:18:06Z

A little confused by this failure:

That check is to help prevent the introduction of non-IE11-compatible JavaScript. It's flagging const as being ES6 that would not run in IE11†. My guess is that swr may not explicitly support IE11.

At least for the syntax, we can add the package to our existing allowlist of dependencies to be processed by Babel.

I would also want to either test or check explicit support guarantees to see whether IE11 will work with it, in case it's relying on some other newer features.

FWIW, IE11 support will conclude at the end of the month, so it may not be an issue starting in January.

† Technically, const itself is compatible, though I'd suspect there's other syntax which would not be.

allthesignals · 2022-12-13T17:45:43Z

A little confused by this failure:

That check is to help prevent the introduction of non-IE11-compatible JavaScript. It's flagging const as being ES6 that would not run in IE11†. My guess is that swr may not explicitly support IE11.

At least for the syntax, we can add the package to our existing allowlist of dependencies to be processed by Babel.

I would also want to either test or check explicit support guarantees to see whether IE11 will work with it, in case it's relying on some other newer features.

FWIW, IE11 support will conclude at the end of the month, so it may not be an issue starting in January.

† Technically, const itself is compatible, though I'd suspect there's other syntax which would not be.

allthesignals · 2022-12-14T01:02:53Z

app/javascript/packages/document-capture/components/address-search.tsx

        return;
      }
-      const addressCandidates = await request(ADDRESS_SEARCH_URL, {
+      const addressCandidates = await request<Location>(ADDRESS_SEARCH_URL, {


I changed the request library so that responses need to be typed

allthesignals · 2022-12-14T01:03:11Z

app/javascript/packages/document-capture/components/address-search.tsx

+      <>
+        <br />
+        {addressQuery.address}
+      </>


Todo: implement copy!

Todo: implement copy!

Just to clarify, are you planning to do that here, or in a follow-up pull request?

@aduth this one, just added.

allthesignals · 2022-12-14T01:03:33Z

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

+const transformKeys = (location: object, predicate = snakeCase) => {
  const sendObject = {};
  Object.keys(location).forEach((key) => {
-    sendObject[snakeCase(key)] = location[key];
+    sendObject[predicate(key)] = location[key];
  });
  return sendObject;


Refactor here improves the naming a bit so it's more generic

allthesignals · 2022-12-14T01:05:24Z

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

+const requestUspsLocations = async (address: LocationQuery): Promise<FormattedLocation[]> => {
+  const response = await request<PostOffice[]>(LOCATIONS_URL, {
+    method: 'post',
+    json: { address: transformKeys(address) },
+  });
+
+  return formatLocation(response);
+};


This is an extraction of "fetcher" logic from the previous hook for useEffect. This follows the pattern for SWR.

allthesignals · 2022-12-14T01:08:22Z

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

-      const selectedLocation = locationData[id];
+      const selectedLocation = locationResults![id]!;


This is not great because locationResults can potentially be null!

allthesignals · 2022-12-14T01:12:27Z

Hey @NavaTim — I've shortened this PR a little bit so that it's doing less... updating the PR description accordingly...

…r; prevent auto load

allthesignals · 2022-12-14T15:32:34Z

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

  city: string;
  state: string;
  zipCode: string;
+  address: string;


Added so that we can use the full address in messages

allthesignals · 2022-12-14T15:33:59Z

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

-      {arcgisSearchEnabled && (
-        <AddressSearch onAddressFound={handleFoundAddress} registerField={registerField} />


This is redundant due to this PR, so removed it.

allthesignals · 2022-12-14T15:34:41Z

config/locales/in_person_proofing/fr.yml

            poste américain proche.
-          search_button: Chercher
+          search_button: Rechercher
+          none_found:


Awaiting corrected french translation here cc @kellular (lemme know if i'm wrong)

allthesignals · 2022-12-14T16:17:30Z

config/locales/in_person_proofing/fr.yml

CC @kellular sorry, not sure how to read the translation doc for this... sounds like we're still trying to determine?

allthesignals · 2022-12-14T16:25:16Z

app/javascript/packages/document-capture/components/address-search.tsx

This only existed in order to display on screen

aduth

I found this difficult to test locally since I don't have credentials for the address geocoder. Is there a ticket where we plan to stub out a fake geocoder service? I assume once this becomes more generally available in the application we'd want the flow to be minimally functional for a default application configuration.

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

app/javascript/packages/document-capture/components/in-person-locations.tsx

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx

NavaTim

The screenshots differ significantly from the mockup, particularly the bolded result text and the address showing after the button. Was the latter temporary?

NavaTim · 2022-12-14T19:12:10Z

config/locales/in_person_proofing/fr.yml

          address_search_hint: 'Exemple : 1960 W Chelsea Ave Allentown PA 18104'
          address_search_label: Entrez une adresse pour trouver un bureau de poste près de chez vous.
-          none_found:
+          none_found: Placeholder %{address}


Best to actually translate this before merging (or mark as draft for now) since this is what a user would see in production.

I guess if there's an outstanding story it may be fine since this is behind a FF.

Let me do some cleanup on the screenshots and styling. Thanks for checking that. One of those screenshots shows intermediate progress on this and needs updated.

Updated screenshots.

allthesignals · 2022-12-14T22:38:33Z

I found this difficult to test locally since I don't have credentials for the address geocoder. Is there a ticket where we plan to stub out a fake geocoder service? I assume once this becomes more generally available in the application we'd want the flow to be minimally functional for a default application configuration.

Ticket created.

allthesignals added 3 commits December 9, 2022 16:43

Permit generic type for request library

c2f51c4

Install SWR

956b041

Refactor: use SWR; peel LocationResults into new component;

dde0cb5

allthesignals changed the title ~~Wmg/7720 swr integration~~ IPP: PO Search SWR refactor and enhancements Dec 11, 2022

allthesignals commented Dec 12, 2022

View reviewed changes

aduth reviewed Dec 12, 2022

View reviewed changes

app/javascript/packages/request/index.ts Outdated Show resolved Hide resolved

allthesignals commented Dec 12, 2022

View reviewed changes

app/javascript/packages/document-capture/components/in-person-locations.tsx Outdated Show resolved Hide resolved

allthesignals requested review from a team and NavaTim December 12, 2022 23:23

NavaTim reviewed Dec 13, 2022

View reviewed changes

app/javascript/packages/document-capture/components/address-search.tsx Outdated Show resolved Hide resolved

...vascript/packages/document-capture/components/in-person-location-post-office-search-step.tsx Outdated Show resolved Hide resolved

allthesignals and others added 5 commits December 13, 2022 19:52

Exclude SWR from ie11 check

395d797

Consider ESM file extensions in Webpack file handling

e27c30a

Clear comments and unused variables

af80882

Specify type for request now

0565404

Merge branch 'main' into wmg/7720-swr-integration-only

9f7a41c

allthesignals force-pushed the wmg/7720-swr-integration branch from 1082f35 to 9f7a41c Compare December 14, 2022 01:01

allthesignals commented Dec 14, 2022

View reviewed changes

allthesignals requested a review from NavaTim December 14, 2022 01:12

allthesignals changed the title ~~IPP: PO Search SWR refactor and enhancements~~ LG-7720: IPP: PO Search SWR refactor and enhancements Dec 14, 2022

allthesignals added 2 commits December 14, 2022 10:28

Add remaining translations; get full address string from arcgis;

745ee80

changelog: Upcoming Features, In-Person Proofing, use SWR and refacto…

c0485ed

…r; prevent auto load

allthesignals commented Dec 14, 2022

View reviewed changes

Lint

c751036

allthesignals force-pushed the wmg/7720-swr-integration branch from a3c6a0b to c751036 Compare December 14, 2022 15:51

Merge branch 'main' into wmg/7720-swr-integration-only

45e5dbb

allthesignals commented Dec 14, 2022

View reviewed changes

Add placeholder french translation

caeb20a

allthesignals force-pushed the wmg/7720-swr-integration branch from b6b02f6 to caeb20a Compare December 14, 2022 16:31

allthesignals requested a review from aduth December 14, 2022 16:48

aduth approved these changes Dec 14, 2022

View reviewed changes

NavaTim reviewed Dec 14, 2022

View reviewed changes

allthesignals added 5 commits December 14, 2022 18:03

Drop the default transformKeys predicate and require consumer to specify

5b462f7

Rename prop to be more truthful

01c10cb

Merge branch 'main' into wmg/7720-swr-integration-only

38e27e0

Wrap back button in PageFooter

05be876

Include formatting and divider

1f87724

allthesignals merged commit d5aeb2a into main Dec 15, 2022

allthesignals deleted the wmg/7720-swr-integration branch December 15, 2022 19:07

aduth mentioned this pull request Dec 19, 2022

Deploy RC 236 to Prod #7509

Merged

aduth mentioned this pull request Sep 11, 2023

Deploy RC 313 to Prod #9188

Merged

		const selectedLocation = locationData[id];
		const selectedLocation = locationResults![id]!;

		{arcgisSearchEnabled && (
		<AddressSearch onAddressFound={handleFoundAddress} registerField={registerField} />

Conversation

allthesignals commented Dec 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

👀 Screenshots

Uh oh!

allthesignals commented Dec 11, 2022

Uh oh!

allthesignals commented Dec 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

allthesignals commented Dec 12, 2022

Uh oh!

NavaTim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NavaTim commented Dec 13, 2022

Uh oh!

aduth commented Dec 13, 2022

Uh oh!

allthesignals commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allthesignals commented Dec 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NavaTim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allthesignals commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

allthesignals commented Dec 11, 2022 •

edited

Loading