-
Notifications
You must be signed in to change notification settings - Fork 9
Rewrite init_checkout for robustness and logging #861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
9bc3f41
11e2be6
a22bbfd
9acd540
04984f9
2dd0f37
81bffed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| from helloasso_python.api.paiements_api import PaiementsApi | ||
| from helloasso_python.api_client import ApiClient | ||
| from helloasso_python.configuration import Configuration | ||
| from helloasso_python.exceptions import UnauthorizedException | ||
| from helloasso_python.exceptions import BadRequestException, UnauthorizedException | ||
| from helloasso_python.models.hello_asso_api_v5_models_carts_checkout_payer import ( | ||
| HelloAssoApiV5ModelsCartsCheckoutPayer, | ||
| ) | ||
|
|
@@ -19,7 +19,6 @@ | |
|
|
||
| from app.core.payment import cruds_payment, models_payment, schemas_payment | ||
| from app.core.payment.types_payment import HelloAssoConfig | ||
| from app.core.users import schemas_users | ||
| from app.core.utils import security | ||
| from app.types.exceptions import ( | ||
| MissingHelloAssoCheckoutIdError, | ||
|
|
@@ -130,7 +129,7 @@ async def init_checkout( | |
| checkout_amount: int, | ||
| checkout_name: str, | ||
| db: AsyncSession, | ||
| payer_user: schemas_users.CoreUser | None = None, | ||
| payer_user: schemas_payment.PayerUser, | ||
| redirection_uri: str | None = None, | ||
| ) -> schemas_payment.Checkout: | ||
| """ | ||
|
|
@@ -152,27 +151,24 @@ async def init_checkout( | |
| This method use HelloAsso API. It may raise exceptions if HA checkout initialization fails. | ||
| Exceptions can be imported from `helloasso_python` package. | ||
| """ | ||
| configuration = self.get_hello_asso_configuration() | ||
|
|
||
| redirection_uri = redirection_uri or self._redirection_uri | ||
| if not redirection_uri: | ||
| raise UnsetRedirectionUriError | ||
|
|
||
| # We want to ensure that any error is logged, even if modules tries to try/except this method | ||
| # Thus we catch any exception and log it, then reraise it | ||
| exception_start = f"Payment: failed to init a checkout with HA for module {module} and name {checkout_name} for payer {payer_user.firstname} {payer_user.name}" | ||
| try: | ||
| payer: HelloAssoApiV5ModelsCartsCheckoutPayer | None = None | ||
| if payer_user: | ||
| payer = HelloAssoApiV5ModelsCartsCheckoutPayer( | ||
| firstName=payer_user.firstname, | ||
| lastName=payer_user.name, | ||
| email=payer_user.email, | ||
| dateOfBirth=payer_user.birthday, | ||
| ) | ||
| # We want to ensure that any error is logged | ||
| # Thus we catch any exception and log it, then reraise it | ||
|
|
||
| checkout_model_id = uuid.uuid4() | ||
| secret = security.generate_token(nbytes=12) | ||
|
|
||
| payer = HelloAssoApiV5ModelsCartsCheckoutPayer( | ||
| firstName=payer_user.firstname, | ||
| lastName=payer_user.name, | ||
| email=payer_user.email, | ||
| dateOfBirth=payer_user.birthday, | ||
| ) | ||
| init_checkout_body = HelloAssoApiV5ModelsCartsInitCheckoutBody( | ||
| total_amount=checkout_amount, | ||
| initial_amount=checkout_amount, | ||
|
|
@@ -189,69 +185,62 @@ async def init_checkout( | |
| ) | ||
|
|
||
| response: HelloAssoApiV5ModelsCartsInitCheckoutResponse | ||
| configuration = self.get_hello_asso_configuration() | ||
| with ApiClient(configuration) as api_client: | ||
| checkout_api = CheckoutApi(api_client) | ||
| try: | ||
| response = checkout_api.organizations_organization_slug_checkout_intents_post( | ||
| self._helloasso_slug, | ||
| init_checkout_body, | ||
| ) | ||
| except UnauthorizedException: | ||
| # We know that HelloAsso may refuse some payer infos, like using the firstname "test" | ||
| # Even when prefilling the payer infos,the user will be able to edit them on the payment page, | ||
| # so we can safely retry without the payer infos | ||
| if not payer_user: | ||
| hyperion_error_logger.exception( | ||
| f"Payment: failed to init a checkout with HA for module {module} and name {checkout_name} (no payer info provided).", | ||
| ) | ||
| else: | ||
| payer_user_name = f"{payer_user.firstname} {payer_user.name}" | ||
| hyperion_error_logger.warning( | ||
| f"Payment: failed to init a checkout with HA for module {module} and name {checkout_name}. Retrying without payer infos for {payer_user_name}", | ||
| ) | ||
|
|
||
| init_checkout_body.payer = None | ||
| try: | ||
| response = checkout_api.organizations_organization_slug_checkout_intents_post( | ||
| self._helloasso_slug, | ||
| init_checkout_body, | ||
| ) | ||
| except UnauthorizedException: | ||
| # HelloAsso returned a 401 unauthorized again | ||
| hyperion_error_logger.exception( | ||
| f"Payment: failed to init a checkout with HA for module {module} and name {checkout_name}, with and without payer {payer_user_name} infos", | ||
| ) | ||
|
|
||
| if response and response.id: | ||
| checkout_model = models_payment.Checkout( | ||
| id=checkout_model_id, | ||
| module=module, | ||
| name=checkout_name, | ||
| amount=checkout_amount, | ||
| hello_asso_checkout_id=response.id, | ||
| secret=secret, | ||
| ) | ||
|
|
||
| await cruds_payment.create_checkout(db=db, checkout=checkout_model) | ||
|
|
||
| return schemas_payment.Checkout( | ||
| id=checkout_model_id, | ||
| payment_url=response.redirect_url, | ||
| ) | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you remove the comment explaining why we retry without payer infos? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well spotted, it's more explicative with this line + the previous comment |
||
| hyperion_error_logger.warning( | ||
| f"{exception_start}: retrying without payer infos", | ||
| ) | ||
| init_checkout_body.payer = None | ||
| response = checkout_api.organizations_organization_slug_checkout_intents_post( | ||
| self._helloasso_slug, | ||
| init_checkout_body, | ||
| ) | ||
| if response.id is None: | ||
| raise MissingHelloAssoCheckoutIdError() # noqa: TRY301 | ||
|
|
||
| checkout_model = models_payment.Checkout( | ||
| id=checkout_model_id, | ||
| module=module, | ||
| name=checkout_name, | ||
| amount=checkout_amount, | ||
| hello_asso_checkout_id=response.id, | ||
| secret=secret, | ||
| ) | ||
| await cruds_payment.create_checkout(db=db, checkout=checkout_model) | ||
| return schemas_payment.Checkout( | ||
| id=checkout_model_id, | ||
| payment_url=response.redirect_url, | ||
| ) | ||
| raise MissingHelloAssoCheckoutIdError() # noqa: TRY301 | ||
|
|
||
| except Exception: | ||
| # Different from a 401 unauthorized | ||
| payer_user_name = "" | ||
| if payer_user: | ||
| payer_user_name = f"{payer_user.firstname} {payer_user.name}" | ||
| except UnauthorizedException as e: | ||
| hyperion_error_logger.exception( | ||
| f"Payment: failed to init a checkout with HA for module {module} and name {checkout_name} with payer {payer_user_name} infos", | ||
| f"{exception_start}: unauthorized for headers {e.headers}.", | ||
| ) | ||
| raise | ||
| except BadRequestException as e: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should go in the previous except block There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, now we have 2 instead of 3
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 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? |
||
| # We know that HelloAsso may refuse some payer infos, | ||
| # e.g. >18 years old, valid email, and firstname and name without some characters and patterns. | ||
| # See https://dev.helloasso.com/docs/int%C3%A9grer-le-paiement-sur-votre-site#contr%C3%B4le-des-champs | ||
| hyperion_error_logger.exception( | ||
| f"{exception_start}: bad request for reason {e.data or e.body}.", | ||
| ) | ||
| raise | ||
| except MissingHelloAssoCheckoutIdError: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idem. If the purpose is to log this, I think it would be simpler to do it directly |
||
| hyperion_error_logger.exception( | ||
| f"{exception_start}: no checkout id returned.", | ||
| ) | ||
| raise | ||
| except Exception: | ||
| hyperion_error_logger.exception(f"{exception_start}: unknown exception.") | ||
| raise | ||
|
|
||
| async def get_checkout( | ||
| self, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,11 @@ | |
| from fastapi.responses import FileResponse | ||
| from sqlalchemy.ext.asyncio import AsyncSession | ||
|
|
||
| from app.core.groups import cruds_groups, schemas_groups | ||
| from app.core.groups import cruds_groups | ||
| from app.core.groups.groups_type import GroupType | ||
| from app.core.memberships import cruds_memberships, schemas_memberships | ||
| from app.core.payment.payment_tool import PaymentTool | ||
| from app.core.payment.schemas_payment import PayerUser | ||
| from app.core.payment.types_payment import HelloAssoConfigName | ||
| from app.core.users import cruds_users, models_users, schemas_users | ||
| from app.core.users.cruds_users import get_user_by_id, get_users | ||
|
|
@@ -2619,36 +2620,24 @@ async def get_payment_url( | |
| status_code=403, | ||
| detail="Please give an amount in cents, greater than 1€.", | ||
| ) | ||
| user_schema = schemas_users.CoreUser( | ||
| account_type=user.account_type, | ||
| school_id=user.school_id, | ||
| email=user.email, | ||
| birthday=user.birthday, | ||
| promo=user.promo, | ||
| floor=user.floor, | ||
| phone=user.phone, | ||
| created_on=user.created_on, | ||
| groups=[ | ||
| schemas_groups.CoreGroupSimple( | ||
| id=group.id, | ||
| name=group.name, | ||
| description=group.description, | ||
| ) | ||
| for group in user.groups | ||
| ], | ||
| id=user.id, | ||
| name=user.name, | ||
| firstname=user.firstname, | ||
| nickname=user.nickname, | ||
| ) | ||
| checkout = await payment_tool.init_checkout( | ||
| module=module.root, | ||
| checkout_amount=amount, | ||
| checkout_name="Chaine de rentrée", | ||
| payer_user=user_schema, | ||
| db=db, | ||
| ) | ||
|
|
||
| try: | ||
| checkout = await payment_tool.init_checkout( | ||
| module=module.root, | ||
| checkout_amount=amount, | ||
| checkout_name="Chaine de rentrée", | ||
| payer_user=PayerUser( | ||
| firstname=user.firstname, | ||
| name=user.name, | ||
| email=user.email, | ||
| birthday=user.birthday, | ||
| ), | ||
| db=db, | ||
| ) | ||
| except Exception: | ||
| raise HTTPException(status_code=502, detail="Cannot init the checkout") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you return a 502? |
||
| hyperion_error_logger.info(f"CDR: Logging Checkout id {checkout.id}") | ||
|
|
||
| cruds_cdr.create_checkout( | ||
| db=db, | ||
| checkout=models_cdr.Checkout( | ||
|
|
@@ -2657,7 +2646,6 @@ async def get_payment_url( | |
| checkout_id=checkout.id, | ||
| ), | ||
| ) | ||
|
|
||
| return schemas_cdr.PaymentUrl( | ||
| url=checkout.payment_url, | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, 502 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you try/except this block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init_checkoutmay result in an exceptionHTTPExceptionto return something gracefully (currently the client has no idea what happens when initiating a checkout still fails)