LG-9008: Remove code that supports pre-launch IPP functionality#11440
LG-9008: Remove code that supports pre-launch IPP functionality#11440eileen-nava merged 4 commits intomainfrom
Conversation
| {t('in_person_proofing.body.location.po_search.results_description', { | ||
| address, | ||
| count: locations?.length, | ||
| })} |
There was a problem hiding this comment.
This PR doesn't include a database change, a change to anything stored in redis, a change to a job, a change to a route, or a change to a feature flag.
Is there another 50/50 state scenario that I am missing? Or, let me know if you do think it falls under one of the scenarios covered in 50/50 state documentation. Since our team has hit multiple 50/50 state errors recently, I definitely agree with being cautious about it.
There was a problem hiding this comment.
It's not listed on that handbook article, but another scenario is one where we have JavaScript that requests from an API endpoint and assumes the response will take a particular shape. If a user is running an older version of the JavaScript, it may fall out of sync with the newer instances handling the API responses.
I think in this case it might be okay since it'll treat the absence of isPilot in the response as the same as if it had previously returned false, and the behavior would be the same?
Probably worth documenting this as an additional 50/50 scenario.
There was a problem hiding this comment.
Feel free to resolve this. I feel comfortable moving forward. (I didn't want to resolve it if you wanted to keep it as reminder to update docs.)
There was a problem hiding this comment.
Thanks for noting the additional 50/50 state scenario, @aduth. Could you, or another dev who has witnessed this type of 50-50 state bug, please take a look at this documentation PR? (I would be curious to hear what remediation steps you suggest.) Thanks!
There was a problem hiding this comment.
I think in this case it might be okay since it'll treat the absence of isPilot in the response as the same as if it had previously returned false, and the behavior would be the same?
I agree with the above.
I created a branch that still uses isPilot in in-person-locations.tsx, but does not have that property present in the response from the backend. This was meant to reproduce the 50/50 state scenario discussed in this thread. I included a video of manually testing that here. I didn't hit any bugs in that manual test.
That, along with what's discussed above, makes me think we'll be okay when this deploys. (Let me know if anyone disagrees or thinks I missed something. If I don't hear any objections, I'm going to move forward.)
| zip_code_4: string; | ||
| zip_code_5: string; | ||
| is_pilot: boolean; | ||
| } |
There was a problem hiding this comment.
One other thought I had- do you think we should version up for address-search (app/javascript/packages/address-search/package.json) because we are changing the interface? If you agree- we should also update app/javascript/packages/address-search/CHANGELOG.md
There was a problem hiding this comment.
I agree, that's a good idea. 👍🏻 I will get to that tomorrow. Thanks for raising the topic.
There was a problem hiding this comment.
@gina-yamada I added commit 4389c9a to version up the address search package. Unfortunately, I don't have access to the 18F npm organization. 😬
I updated the documentation to reflect the new process for requesting access to the 18F npm organization. I also requested access. I think I'm blocked on publishing this new minor version of @18f/identity-address-search until I have npm org access.
There was a problem hiding this comment.
If getting access will take a bit of time, add some time on my calendar. In that session, you can merge this in and I can publish it for you. Ideally, we should do it back to back so we know there is no drift. (I want to make sure I have access to publish before you merge it in.)
There was a problem hiding this comment.
Thanks, Gina! I booked time on your calendar for tomorrow.
WilliamBirdsall
left a comment
WilliamBirdsall
left a comment
There was a problem hiding this comment.
LGTM, but needs the changelog message to pass automated checks :)
…ace to remove obsolete is_pilot field
|
Gina and I paired to publish the new npm package. 🎉 (cc: @KeithNava for visibility) |
5 participants
🎫 Ticket
LG-9008: Remove code that supports pre-launch IPP functionality
🛠 Summary of changes
📜 Testing Plan