LG-8788: Refactor address search controller#7820
Conversation
eileen-nava
left a comment
eileen-nava
left a comment
There was a problem hiding this comment.
This definitely slims down the addresses method. Good stuff! I had some questions about the analytics method you used in report_errors(error).
| Faraday::TimeoutError => :unprocessable_entity, | ||
| }[error.class] || :internal_server_error | ||
|
|
||
| analytics.idv_in_person_locations_request_failure( |
There was a problem hiding this comment.
What was the motivation for changing the analytics method from idv_arcgis_request_failure to idv_in_person_locations_request_failure?
Since this is the address search controller, and idv_arcgis_request_failure is documented as Tracks if request to get address candidates from ArcGIS fails, I think this should stay as idv_arcgis_request_failure.
Let me know if I'm missing something.
There was a problem hiding this comment.
Also, it might be helpful to add an expect statement to address_search_controller_spec.rb that tests if the analytics method is called when there's an error.
There was a problem hiding this comment.
@eileen-nava thank you! this is completely wrong. I will add a test for it because there wasn't one.
| @geocoder ||= IdentityConfig.store.arcgis_mock_fallback ? | ||
| ArcgisApi::Mock::Geocoder.new : ArcgisApi::Geocoder.new | ||
| end | ||
|
|
There was a problem hiding this comment.
my pr refactors this a bit. i'm currently working through a merge conflict with my pr and the usps controller error handling refactor. Can we pause a moment before merging this so I can take a look and figure out how this works with my changes?
There was a problem hiding this comment.
@svalexander sure. Feel free to copy the approach if you want. We should keep it consistent with how we changed things in USPS. Open to different approaches!
|
Since Shannon's PR #7763 was merged, @allthesignals, I think you can merge this once you fix the merge conflict. |
|
@mitchellhenke FYSA, I tagged you as a reviewer because you updated the HTTP codes that are returned for certain IPP API failures. I did this mostly to keep you in the loop, and it's definitely optional for you to look at this. |
JackRyan1989
left a comment
JackRyan1989
left a comment
There was a problem hiding this comment.
Tests pass locally. Looks good to me!
If I read it right, it should still return 4XX unless there was an unhandled exception? |
Correct. |
* LG-8887: In-Person Proofing: Add Login.gov logo above the Barcode (#7956) * WIP commit of half-baked changes for showing logo These are not yet functional * changelog: User-Facing Improvements, In-Person Proofing, Display Login.gov logo above barcode in email * Include asset_url * Revert "Include asset_url" This reverts commit a4612d2. * Revert "changelog: User-Facing Improvements, In-Person Proofing, Display Login.gov logo above barcode in email" This reverts commit 2f5326c. * Revert "WIP commit of half-baked changes for showing logo" This reverts commit c8d3af5. * WIP * Display miniature logo inside mailer and component view * Add a little padding * Drop skip_pipeline; simplify styles * Add image assertion * Allow alternative extensions --------- Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> * LG-8788: Refactor address search controller (#7820) * changelog: Internal, In-Person Proofing, refactor some error reporting (analytics * Add server error to address search controller * refactor --------- Co-authored-by: Eileen McFarland <eileenmcfarland@navapbc.com> * Revert "Remove send link step (#7929)" (#8019) This reverts commit 8340d0a. This caused errors in prod from the send_link route, which shouldn't be happening. Possibly because of the 50/50 state. Reverting pending further research. * Correct analytics name send_link to link_sent (#8022) * Correct analytics name for link sent event to 'IdV: doc auth link_sent submitted' from 'IdV: doc auth send_link submitted' [skip changelog] * Add email support test case to mailer shared examples (#8017) * Add email support test case to mailer shared examples changelog: Internal, Automated Testing, Enhance mailer tests to check for email client support features * Link support reference For future recall * Fix spec flake in GpoConfirmationUploader (#8024) changelog: Internal, Tests, Fix test that flaked due to clock drift * Clean up spec to stop stubbing .new * Revert "Merge pull request #8016 from 18F/stages-rc-2023-03-17-revert" This reverts commit 4c8419f, reversing changes made to 3741fd7. --------- Co-authored-by: Matt Gardner <wilburnforce@gmail.com> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Eileen McFarland <eileenmcfarland@navapbc.com> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
5 participants
🎫 Ticket
🛠 Summary of changes
Refactor the address_search controller to work similarly to USPS.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.