LG-8932: Send USPS the state ID address instead of residential address#8098
LG-8932: Send USPS the state ID address instead of residential address#8098
Conversation
changelog: Upcoming Features, In-Person Proofing, Send USPS the state ID address instead of residential address based on a feature flag
eileen-nava
left a comment
eileen-nava
left a comment
There was a problem hiding this comment.
Hey, I started to review this then saw the build is failing. It looks like there’s a problem with the symbolize_keys method. Here's a link to one of the failing specs. Could you please get the build to pass and then I’ll return to this review? Thanks!
| @@ -45,17 +45,26 @@ def create_usps_enrollment(enrollment, pii) | |||
| # Use the enrollment's unique_id value if it exists, otherwise use the deprecated | |||
| # #usps_unique_id value in order to remain backwards-compatible. LG-7024 will remove this | |||
There was a problem hiding this comment.
This is not directly related to this PR, but should we add LG-7024 to our scheduled work for upcoming sprints?
There was a problem hiding this comment.
@eileen-nava I think that's a good idea, would recommend bringing it up in the Slack channel w/ David & Sumi.
sheldon-b
left a comment
sheldon-b
left a comment
There was a problem hiding this comment.
Couple minor comments but generally looks good! Will wait for the build to pass to approve
|
|
||
| private | ||
|
|
||
| SECONDARY_ID_CAPTURE_MAP = { |
There was a problem hiding this comment.
nit: this seems more descriptive to me
| SECONDARY_ID_CAPTURE_MAP = { | |
| SECONDARY_ID_ADDRESS_MAP = { |
There was a problem hiding this comment.
This constant name was based on the feature flag, but I think your refactor is more intuitive. I'll push up this change in a bit.
| transform_keys(&:to_s).freeze | ||
| end | ||
|
|
||
| context 'DAV enabled' do |
There was a problem hiding this comment.
I prefer spelling out acronyms so it's more comprehensible to people who aren't familiar with it
| context 'DAV enabled' do | |
| context 'double address verification is enabled' do |
There was a problem hiding this comment.
The context is provided by the outer describe block here. If anything, I would remove DAV altogether and just leave enabled. Also was thinking spelling this out again could be excessively verbose for the output in the case of a test failure.
|
|
||
| # If we're using secondary ID capture (aka double address verification), | ||
| # then send the state ID address to USPS. Otherwise send the residential address. | ||
| if enrollment.capture_secondary_id_enabled? |
There was a problem hiding this comment.
Oh, should we also check that the user actually entered two different addresses?
| if enrollment.capture_secondary_id_enabled? | |
| if enrollment.capture_secondary_id_enabled? && !pii.same_address_as_id |
There was a problem hiding this comment.
No, the state_id_* fields would be the only fields containing values in that case. This kind of transformation would still need to occur.
There was a problem hiding this comment.
The plan is to copy the user's state ID address into the address1, address2, etc fields if the user says that their residential address matches their state ID address, so those fields will be filled and this would be unnecessary
There was a problem hiding this comment.
It's my understanding that the copying is not implemented yet. When the copying is implemented, then we could potentially add that as an optimization on this code.
There was a problem hiding this comment.
It's not implemented, but these fields aren't empty either. The user just inputs their address twice no?
There was a problem hiding this comment.
Fair point, and the story for doing that is literally the next story we have planned to pull in.
…ley/lg-8932-send-state-id-details-to-usps
3 participants
🎫 Ticket
🛠 Summary of changes
If the Double Address Verification feature is enabled for a given in-person enrollment record, then send the state ID address to USPS instead of the residential address.
📜 Testing Plan
in_person_capture_secondary_id_enabled