Translate Post Office search results#10882
Conversation
There was a problem hiding this comment.
just realized this whole thing is repeated in two controllers, what if we made EnrollmentHelper.localized_location or some other helper that could be reused?
There was a problem hiding this comment.
Yeah, I thought about that and I felt I was already deep enough in the rabbit hole to open a PR. I'm on board with it.
There was a problem hiding this comment.
Forgot that slice/merge doesn't work because location is a PostOffice rather than a hash. I've moved it to the EnrollmentHelper and switched to calling the attributes as methods rather than #[] to indicate that in a1ecb7aa25.
There was a problem hiding this comment.
yay big fan of methods
There was a problem hiding this comment.
oooo or what if we had PostOffice#localized which returns another instance of PostOffice (maybe with a localized?: true snuck in there to make sure we can't double-localize
There was a problem hiding this comment.
I experimented with something along those lines, but it felt less consistent with how it's handled in the ready_to_verify presenter/view. I think this would be preferable if the presenter used PostOffice as well.
mitchellhenke
force-pushed
the
mitchellhenke/localized-usps-search
branch
2 times, most recently
from
33c82c1 to
342dd44
Compare
changelog: Internal, Bug Fixes, Translate Post Office search results
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
mitchellhenke
force-pushed
the
mitchellhenke/localized-usps-search
branch
from
74ee1f5 to
07738cf
Compare
eileen-nava
requested review from
a team and
rutvigupta-design
and removed request for
a team
eileen-nava
left a comment
eileen-nava
left a comment
There was a problem hiding this comment.
I think this looks good from an engineering perspective. I tagged @18F/identity-joy-designers to take a look.
@mitchellhenke please don't merge the PR until someone from Joy design has okayed the change. (To be clear, I don't expect any issues, but I'd rather err on the side of checking with design.) Thanks!
| return unless selected_location_details | ||
| hours = selected_location_details["#{prefix}_hours"] | ||
| return localized_hours(hours) if hours | ||
| UspsInPersonProofing::EnrollmentHelper.localized_hours(hours) if hours |
There was a problem hiding this comment.
👏🏻 Nice refactor, I like the idea of moving this to the EnrollmentHelper.
| is_enhanced_ipp = resolved_authn_context_result.enhanced_ipp? | ||
| response = proofer.request_facilities(candidate, is_enhanced_ipp) | ||
| if response.length > 0 | ||
| locations = proofer.request_facilities(candidate, is_enhanced_ipp) |
There was a problem hiding this comment.
There was a problem hiding this comment.
@eileen-nava the results LGTM but is there something strange going on in the H3 with all the languages? I don't think "lkn, lkn" in French, "kn, lkn" in Spanish, and "poj, ojpoj" should be there?
There was a problem hiding this comment.
I think those are test values for the address/city fields in the search form
There was a problem hiding this comment.
Ahh, okay that makes sense, thanks for clarifying! This LGTM then!
There was a problem hiding this comment.
@rutvigupta-design Mitchell is correct, I typed in nonsense values for a test address. Thanks for reviewing the PR! 🙏🏻
This reverts commit b2306c7
5 participants
🛠 Summary of changes
Re-uses existing hours translation functionality in the
ReadyToVerifyPresenterfor the translation of content for the Post Office search results controllers.