LG-7832: Transliterate name, address, and city for USPS API (w/ FF)#7935
LG-7832: Transliterate name, address, and city for USPS API (w/ FF)#7935
Conversation
changelog: Improvements, In-Person Proofing, USPS API Transliteration
| stub_request_token | ||
| stub_request_enroll | ||
| allow(IdentityConfig.store).to receive(:usps_mock_fallback).and_return(usps_mock_fallback) | ||
| allow(UspsInPersonProofing::Transliterator).to receive(:new).and_return(transliterator) |
There was a problem hiding this comment.
in general I dislike stubbing .new, it's really broad. A rule I use is "don't stub what you don't own"
so a workaround would be that the enrollment helper has a memoized transliterator property that we stub in instead
There was a problem hiding this comment.
I tried using a memoized transliterator, and that didn't work well with tests at all. Really I prefer constructor injection (because I have had issues with both this approach and the property approach), but considered it out-of-scope to refactor for that here.
There was a problem hiding this comment.
I would still stub the transliterator method rather than .new even if we're not memoizing
There was a problem hiding this comment.
transliterator function stubbed. I generally prefer to avoid stubbing any part of the subject, but I'll make an exception here.
| end | ||
|
|
||
| def transliterator | ||
| Transliterator.new |
There was a problem hiding this comment.
this is stateless, so we could micro-optimize and save a few allocations by memoizing
or, we could probably just make transliterate a static method?
| Transliterator.new | |
| @transliterator ||= Transliterator.new |
There was a problem hiding this comment.
This is exactly what I tried before, and it caused the mock to be preserved between tests (and therefore the tests failed). I also don't think this micro-optimization has a high payoff.
There was a problem hiding this comment.
oh I just saw that there's a class << self around all of this, no wonder there was test pollution. It's probably time to maybe split this class up and maybe maybe it stateful (ex InPersonEnroller.new(user).create_enrollment vs generic Helper), but that's definitely outside the scope of this PR
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
spec/services/usps_in_person_proofing/enrollment_helper_spec.rb
Outdated
Show resolved
Hide resolved
| changed?: value != transliterated, | ||
| original: value, | ||
| transliterated: transliterated, | ||
| unsupported_chars: unsupported_chars, |
There was a problem hiding this comment.
Does it make more sense for unsupported_chars to be a boolean? Are we going to refer to the array values to update the characters in the transliterate/en.yml file?
There was a problem hiding this comment.
Does it make more sense for unsupported_chars to be a boolean?
No, we need to the list of characters to generate the error message required by the story.
Are we going to refer to the array values to update the characters in the transliterate/en.yml file?
No. Monitoring transliteration in-detail is out-of-scope, and the characters could in some cases amount to additional recorded PII.
…ttps://github.com/18F/identity-idp into tbradley/lg-7832-transliterate-usps-api-integration
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
couple last small comments and this LGTM
spec/services/usps_in_person_proofing/enrollment_helper_spec.rb
Outdated
Show resolved
Hide resolved
| let(:usps_ipp_transliteration_enabled) { true } | ||
|
|
||
| it 'creates usps enrollment while using transliteration' do | ||
| mock_proofer = double(UspsInPersonProofing::Mock::Proofer.class) |
There was a problem hiding this comment.
ditto for the .class here
| enrollment.update(unique_id: nil) | ||
| proofer = UspsInPersonProofing::Mock::Proofer.new | ||
| mock = double | ||
| mock_proofer = double(UspsInPersonProofing::Mock::Proofer.class) |
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
…ley/lg-7832-transliterate-usps-api-integration
…ley/lg-7832-transliterate-usps-api-integration
3 participants
🎫 Ticket
🛠 Summary of changes
📜 Testing Plan
Provide a checklist of steps to confirm the changes.