Rewrite init_checkout for robustness and logging #861
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #861 +/- ##
==========================================
- Coverage 85.46% 85.41% -0.05%
==========================================
Files 187 187
Lines 14320 14335 +15
==========================================
+ Hits 12238 12244 +6
- Misses 2082 2091 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| db=db, | ||
| ) | ||
| except Exception: | ||
| raise HTTPException(status_code=502, detail="Cannot init the checkout") |
There was a problem hiding this comment.
Why do you try/except this block?
There was a problem hiding this comment.
- 5xx to indicate the client that it is not a client-side error (it's on HelloAsso's side at best or Hyperion's side at worst)
- Try-except because
init_checkoutmay result in an exception- we need to raise a
HTTPExceptionto return something gracefully (currently the client has no idea what happens when initiating a checkout still fails)
| hyperion_error_logger.error( | ||
| f"Payment: failed to init a checkout with HA for module {module} and name {checkout_name}. No checkout id returned", | ||
| except BadRequestException: | ||
| # In this case only, we retry |
There was a problem hiding this comment.
Why did you remove the comment explaining why we retry without payer infos?
There was a problem hiding this comment.
Well spotted, it's more explicative with this line + the previous comment
| f"{exception_start}: unauthorized for headers {e.headers}.", | ||
| ) | ||
| raise | ||
| except BadRequestException as e: |
There was a problem hiding this comment.
It seems we have multiple try/except chained, it's kind of strange. You catch a BadRequestException then raise it then catch it a second time only to log it.
There was a problem hiding this comment.
I think this should go in the previous except block
There was a problem hiding this comment.
multiple try/except chained
Well, now we have 2 instead of 3
You catch a BadRequestException then raise it then catch it a second time only to log it.
What is wrong with this? We catch it if it happens with payer info, in the case we retry without, and if it happens again, we log it cleanly.
I think this should go in the previous except block
I tried to have all the logging part put together at the end of the method instead of spread out across the method, so that the code be more readable. Perhaps it's not good practice, what do you think?
| f"{exception_start}: bad request for reason {e.data or e.body}.", | ||
| ) | ||
| raise | ||
| except MissingHelloAssoCheckoutIdError: |
There was a problem hiding this comment.
Idem. If the purpose is to log this, I think it would be simpler to do it directly
| db=db, | ||
| ) | ||
| except Exception: | ||
| raise HTTPException(status_code=502, detail="Cannot init the checkout") |
There was a problem hiding this comment.
Why do you return a 502?
Description
Related Issues
Supersedes these two PRs:
Changes Made
PaymentTool.init_checkout()method to handle at once all the known possible exceptions, with cleaner separation in paragraphs and a bit less nesting (no more try in a try in a try).PayerUserpayment schema to easily pass the 4 desired pieces of informationinit_checkoutAlso fixes the bug introduced in the last release that makes it impossible to retry when it failed due to issues in the payer user's infomation.
Type of Change
Impact & Scope
Testing
Documentation