Skip to content

LG-8510: Don't display the same post office multiple times#7774

Merged
eileen-nava merged 2 commits intomainfrom
em/8510-deduplicate-post-offices
Feb 6, 2023
Merged

LG-8510: Don't display the same post office multiple times#7774
eileen-nava merged 2 commits intomainfrom
em/8510-deduplicate-post-offices

Conversation

@eileen-nava
Copy link
Contributor

@eileen-nava eileen-nava commented Feb 6, 2023

🎫 Ticket

Deduplicate Post Offices with the same street address for display

🛠 Summary of changes

Previously, if the USPS API returned the same post office multiple times, our search feature would display duplicate post offices. This bug was obvious when the user searched for 3775 INDUSTRIAL BLVD WEST SACRAMENTO CA 95799.

Now, our search feature will only return each post office one time.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Navigate to the post office search page
  • Search for 3775 INDUSTRIAL BLVD WEST SACRAMENTO CA 95799
  • Observe that no duplicate post offices are returned

👀 Screenshots

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

Before:
SacramentoDuplicates.mov
After:
SacramentoNoDuplicates.mov

@eileen-nava eileen-nava marked this pull request as ready for review February 6, 2023 16:09
@eileen-nava eileen-nava requested review from a team and svalexander February 6, 2023 16:12
@eileen-nava eileen-nava changed the title LG-8510 Don't display the same post office multiple times LG-8510: Don't display the same post office multiple times Feb 6, 2023
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

def dedupe_facilities(facilities)
facilities.uniq do |facility|
[facility.address, facility.city, facility.state, facility.zip_code_5]
end
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 be worth throwing on a sort here too just in case?

Suggested change
end
end.sort_by { |facility| facility.distance }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I don’t think that change is necessary. I have yet to see any instances of post offices being returned out of order, and I wouldn’t expect the dedupe_facilities method to throw off order. If you disagree, please let me know. 🙏🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't checked the API but figured maybe it would be a good check to have just in case. Fine to ship without! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the distance-based ordering represented as a guarantee in the API documentation, so I would normally be inclined to hedge my bets by ordering it. That said, I don't think the current ordering is truly at risk of changing any time soon. Maybe we could get updated API docs that state the ordering as a guarantee.

@eileen-nava eileen-nava merged commit 9f2f1bd into main Feb 6, 2023
@eileen-nava eileen-nava deleted the em/8510-deduplicate-post-offices branch February 6, 2023 19:18
@aduth aduth mentioned this pull request Feb 9, 2023
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