Skip to content

LG-8509: update selected location alert to include address#7727

Merged
svalexander merged 9 commits intomainfrom
shannon/lg-8509-update-selected-location-alert
Feb 1, 2023
Merged

LG-8509: update selected location alert to include address#7727
svalexander merged 9 commits intomainfrom
shannon/lg-8509-update-selected-location-alert

Conversation

@svalexander
Copy link
Contributor

@svalexander svalexander commented Jan 30, 2023

🎫 Ticket

Lg-8509

🛠 Summary of changes

SelectedLocation is updated to include the po address instead of the po name.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Make sure arcgisSearchEnabled is true
  • Navigate to search using failing id
  • Select a location by clicking a select button
  • Verify that the alert displays the po address

👀 Screenshots

en: Screen Shot 2023-01-30 at 5 04 21 PM
es: Screen Shot 2023-01-30 at 5 05 11 PM
fr: Screen Shot 2023-01-30 at 5 05 51 PM

@svalexander svalexander requested review from a team, allis-green and sheldon-b and removed request for a team January 30, 2023 20:57
Comment on lines +41 to +43
const { streetAddress } = selectedLocation;
setSubmitEventMetadata({ selected_location: streetAddress });
onChange({ streetAddress });
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making sure: is this all we need (not the full address)? I believe this will only include 100 main and not 100 main, city, state etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, i'll add in the additional items

await userEvent.click(button);

await findByText('{"selected_location":"Baltimore"}');
await findByText('{"selected_location":"100 Main St E"}');
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting... was there some code that preferred "address" to "name" somewhere?

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 selected_location value should now be the address as that's what the alert will now expect

Copy link
Contributor

Choose a reason for hiding this comment

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

I see... so the analytics thing is dropping stringified text in there for us to assert against?

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 metadata, yes

@allthesignals
Copy link
Contributor

So, I think this looks good, but I was confused why we're still modifying in-person-location-step. Then I realized the test was probably handing data to a view that expected something else.

So, I think the real fix is to update the test (finally) to use the po search path, which we should do asap anyway...

I'll make a ticket, but maintaining both components is becoming a hassle

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I had trouble testing this locally (more related to address search generally than these specific changes, I think), but LGTM from looking at the code 👍

Comment on lines +42 to +43
setSubmitEventMetadata({ selected_location: `${streetAddress}, ${formattedCityStateZip}` });
onChange({ streetAddress, formattedCityStateZip });
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it allow some simplifying to concatenate the string once and use that as the value in InPersonPrepareStep ?

Suggested change
setSubmitEventMetadata({ selected_location: `${streetAddress}, ${formattedCityStateZip}` });
onChange({ streetAddress, formattedCityStateZip });
const selectedLocationAddress = `${streetAddress}, ${formattedCityStateZip}`;
setSubmitEventMetadata({ selected_location: selectedLocationAddress });
onChange({ selectedLocationAddress });

@svalexander svalexander merged commit df180a0 into main Feb 1, 2023
@svalexander svalexander deleted the shannon/lg-8509-update-selected-location-alert branch February 1, 2023 16:05
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.

3 participants