YC-1188 - Update unpaid PF transactions status in cron task, refactor c… #1018
Conversation
…onfirm transaction code
There was a problem hiding this comment.
@monodo @rbovard Dispo pour review! J'ai mis quelques commentaires pour expliquer certains choix que j'ai fait pour cette feature. A noter que j'ai déplacé quelques bouts de codes dans les modèles pour rendre le code plus propre et moins couplé aux vues, donc il y a moins de changement qu'il n'y parait.
| @@ -1308,6 +1308,39 @@ def get_last_prolongation_transaction(self): | |||
| .first() | |||
| ) | |||
|
|
|||
| def set_prolongation_requested_and_notify(self, prolongation_date): | |||
There was a problem hiding this comment.
J'ai profité de l'occasion pour déplacer dans le modèle cette fonction qui sert à setter la date de prolongation . Aussi, j'ai enlevé l'argument request, qui était utilisé auparavant pour construire l'URL absolue de la soumission, parce qu'il est possible qu'une prolongation soit trigger depuis le cronjob, où on ne peut pas avoir le contexte d'une requête HTTP.
| @@ -74,6 +74,8 @@ class Transaction(models.Model): | |||
| default=TYPE_SUBMISSION, | |||
| ) | |||
|
|
|||
| extra_data = models.JSONField(_("Données supplémentaires"), default=dict) | |||
There was a problem hiding this comment.
Utilisé pour stocker la date de prolongation, quand il y a un paiement requis. Elle était avant renvoyée par paramètre dans l'URL au retour de PostFinance, mais il est nécéssaire de la stocker avant de déclencher le paiement, parce qu'il est possible que le paiement soit validé par le cronjob (donc pas de retour depuis PF)
| @@ -2639,22 +2605,36 @@ def get(self, request, *args, **kwargs): | |||
| return redirect(reverse_lazy("submissions:submissions_list")) | |||
|
|
|||
|
|
|||
| @method_decorator(login_required, name="dispatch") | |||
| class ConfirmTransactionCallback(View): | |||
There was a problem hiding this comment.
J'ai enlevé la nécéssité d'être authentifié pour accéder aux views de confirmation de paiement. Cela permet de valider le paiement sur un autre device (p. ex sur mobile) que celui qui a été utilisé pour remplir la demande. Vu que l'on vérifie que le paiement a été effectué sur PF avant de le valider (ligne 2636), cela ne pose pas de problème de sécurité
| or not transaction.status == transaction.STATUS_UNPAID | ||
| ): | ||
| raise PermissionDenied | ||
| if transaction.has_been_confirmed: |
There was a problem hiding this comment.
Petites améliorations dans le cas où la transaction est déjà confirmée, ou déjà échouée.
There was a problem hiding this comment.
Thanks a lot for the work @danickfort
Is it possible to document somewhere how to setup this cron job?
I also noticed that only the transaction is updated but not the related submission (it should be not be anymore in STATUS_DRAFT). And the confirmation and notification emails are not sent. I think you "forgot" to continue the original workflow after the transaction is confirmed.
I've also made some remarks directly in the code.
Don't hesitate to contact me to see it in live.
|
|
||
| class Command(BaseCommand): | ||
| help = gettext( | ||
| "Update the status of transactions that are pending and not older than %s hours." |
There was a problem hiding this comment.
| "Update the status of transactions that are pending and not older than %s hours." | |
| "Update the status of transactions that are pending and not older than %s minutes." |
| @@ -771,6 +771,9 @@ def show_toolbar(request): | |||
| os.getenv("PAYMENT_PROCESSING_TEST_ENVIRONMENT", "true").lower() == "true" or DEBUG | |||
| ) | |||
| PAYMENT_CURRENCY = os.getenv("PAYMENT_CURRENCY") | |||
| PAYMENT_PENDING_TRANSACTION_MAX_AGE_MINS = int( | |||
| os.getenv("PAYMENT_PENDING_TRANSACTION_MAX_AGE_MINS", 30) | |||
There was a problem hiding this comment.
Why not use the same value as in env.example?
| # Get all unpaid transactions that are not older than the specified time | ||
| transactions_to_update = PostFinanceTransaction.objects.filter( | ||
| status=Transaction.STATUS_UNPAID, | ||
| authorization_timeout_on__gte=timezone.now() |
There was a problem hiding this comment.
There is an older issue here, for a reason I don't get, the value of this attribute in DB is truncated to the day, the time is not stored (2024-06-12 --> 2024-06-12 00:00:00.000 +0200). So with the default value of 30 minutes, it will never work.
If we use the value as provided in env.example, it works because it's 24 hours and not 30 minutes.
But it's not clean, is there a way to store the timeout value with the hours?
…onfirm transaction code