fix internal server error on too long input for transaction create and update mutation #13608
Conversation
There was a problem hiding this comment.
Hey!
Thanks for creating the PR with the fix.
I think that the details about maximal length should be placed in each TransactionItem input description. It is the easiest way to get to know about this limitation.
Additionally, to fully apply size validation, this should be also applied on the webhook response received from PaymentApp.
See below to get more context:
saleor.payment.utils.handle_transaction_initialize_session
saleor.payment.utils.handle_transaction_process_session
saleor.plugins.webhook.tasks.handle_transaction_request_task
All the above functions accept the pspReference from payment Apps. We should have validation also there. If the size is too big we should create a failure TransactionEvent with the info about exceeding the size limit
|
Hey @ssuraliya do you intend to work on this PR further? 🙏 |
|
Hey @aniav , I have been busy with my full time work and will try to resolve this PR in the coming weekend. |
ssuraliya
force-pushed
the
12696-transaction-psp-reference
branch
from
251e505
to
b8a8445
Compare
|
Greeting @korycins, |
|
@korycins Please re-review when you have time. |
CHANGELOG.md
Outdated
| @@ -306,6 +306,8 @@ Shipping methods can be removed by the user after it has been assigned to a chec | |||
| - Add missing descriptions to payment module - #13546 by @devilsautumn | |||
| - Fix `NOTIFY_USER` allow to create webhook with only one event - #13584 by @Air-t | |||
| - Add Index for 'Created' field of the Order Model - #13682 by @ritanjandawn | |||
| - Fix transaction create mutation's psp_reference max length issue - #12696 by @ssuraliya | |||
There was a problem hiding this comment.
We would have to move this entry to the newest version section - 3.18.
There was a problem hiding this comment.
We already moved forward with releases so when you fix this please move this to 3.19 (or just the newest unreleased version in the CHANGELOG.md)
| transaction_data, | ||
| transaction_event_data |
There was a problem hiding this comment.
Maybe it's just a small nitpick - the first mutation uses transaction and transaction_event names, while this one has different ones. It would be nice to use the same pattern for consistency as I think these are the same objects.
| @@ -834,6 +834,12 @@ def parse_transaction_action_data( | |||
| logger.error(msg) | |||
| return None, msg | |||
|
|
|||
| if psp_reference: | |||
There was a problem hiding this comment.
you could make it as a single condition:
if psp_reference and len(psp_reference) > 512:
...
I want to merge this change because fix internal server error on too long input transaction create and update mutation
Fixes #12696
Impact
Pull Request Checklist