Skip to content

LG-10785: Move full address entry PO search into the @18f/identity-address-search package#9234

Merged
sheldon-b merged 7 commits intomainfrom
sbachstein/lg-10785-move-full-entry-address-form-into-address-search-package
Sep 20, 2023
Merged

LG-10785: Move full address entry PO search into the @18f/identity-address-search package#9234
sheldon-b merged 7 commits intomainfrom
sbachstein/lg-10785-move-full-entry-address-form-into-address-search-package

Conversation

@sheldon-b
Copy link
Contributor

@sheldon-b sheldon-b commented Sep 19, 2023

🎫 Ticket

LG-10785

🛠 Summary of changes

  • Move the full address entry components from the document-capture javascript package into the address-search package
  • Move the URLs for the address and location search endpoints into the InPerson context provider. This removes a dependency from full address search on single entry address search and allows tests to inject this dependency instead of importing it from the component
  • Pass usStatesTerritories down into the full address entry components instead of having the low level components depend on the InPerson context provider

🗺️ Next Steps

Once this PR is merged I'll publish a new version of the @18f/address-search package

Sheldon Bachstein added 6 commits September 19, 2023 11:08
@sheldon-b sheldon-b force-pushed the sbachstein/lg-10785-move-full-entry-address-form-into-address-search-package branch from 39c55cd to d96c43b Compare September 19, 2023 15:10
Comment on lines +100 to +101
locationsURL: new URL('/verify/in_person/usps_locations', window.location.href).toString(),
addressSearchURL: new URL('/api/addresses', window.location.href).toString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern for other URLs is for the backend to pass them to the javascript app. I didn't do that here because it's a larger refactor, but I'm leaning towards creating a ticket for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eg here and here

Comment on lines +42 to +43
locationsURL: '',
addressSearchURL: '',
Copy link
Contributor Author

@sheldon-b sheldon-b Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't good default values for these. I could move the logic for constructing the URLs to here, or follow the pattern for inPersonURL (which is for it to be optional and no default is defined). Or just leave it as-is here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure the best answer here. I'd like to hear opinions on this.

@sheldon-b sheldon-b marked this pull request as ready for review September 19, 2023 16:52
@@ -6,6 +6,7 @@ import type { LocationQuery, FormattedLocation } from '@18f/identity-address-sea
import FullAddressSearchInput from './full-address-search-input';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you shortened file name. Now component name is the same as the file name.


function FullAddressSearch({
usStatesTerritories,
registerField,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you decided to pass in usStatesTerritories as a prop. If we use this component in sites, I imagine we will need to write an issue to get usStateTerritories in sites (help center). (I did not look into how we are getting these- jumping into another meeting). I imagine the lift to do this in sites and pass it in is not large so am okay with passing it in- just calling it out as some additional work I think

Copy link
Contributor Author

@sheldon-b sheldon-b Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. For reference how this works in identity-idp is:

  1. US states and territories are defined in the Ruby app
  2. They're passed into the javascript application on the document capture page
  3. They're loaded into the InPerson context, per this PR
  4. They're passed down into the components in the PO search step, per this PR

As-is we'll have to redefine them in the identity-site repo

An alternative is that we could write a script to read from the Ruby file and write to a javascript file, and then publish the generated javascript file as part of an npm package. And then in the Ruby file add a file comment saying something like,

If you change this set of states/territories be sure to go run script bin/update-states-and-territories.sh and publish a new version of the npm package

I'm going to go ahead and merge this PR. If we do refactor it in some way I think it should be part of a different ticket. But let me know your thoughts about it

Copy link
Contributor

@gina-yamada gina-yamada Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a new ticket is a good idea, extra work and out of scope to get this working in identity-sites. I think both are fine. I don't think the list will change much over time just as long as we can think of a way to try to ensure both get updated in the future

@gina-yamada
Copy link
Contributor

I did test your branch. I tested with mock facilities, with no mock facilities, and hit PO (usps dev) and was successful. For all tests, in_person_full_address_entry_enabled in my application.yml file was set to true.

@gina-yamada gina-yamada self-requested a review September 19, 2023 21:25
Copy link
Contributor

@gina-yamada gina-yamada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing any concerns here. Manual testing all checks out and code change all make sense. I did have one comment about usStateTerritories- not a blocker just a thought/something to consider.

…ove-full-entry-address-form-into-address-search-package
@sheldon-b sheldon-b merged commit a4ffdb3 into main Sep 20, 2023
@sheldon-b sheldon-b deleted the sbachstein/lg-10785-move-full-entry-address-form-into-address-search-package branch September 20, 2023 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants