feat: adding extra information to payment processor response#8
feat: adding extra information to payment processor response#8andrey-canon wants to merge 1 commit intomainfrom
Conversation
This will add user basic information to the payment processor response and also will associate the basket if it's possible.
| except Exception as exc: | ||
| error = exc.__class__.__name__ | ||
| finally: | ||
| verification_response.update({ |
There was a problem hiding this comment.
I test this in queries and its working nice.
| if status == PaymentStatus.FAILURE: | ||
| return redirect(reverse('payment_error')) | ||
| if status == PaymentStatus.PENDING: | ||
| elif status == PaymentStatus.PENDING: |
There was a problem hiding this comment.
Why is it better elif than the previous if ?, this is only curiosity of the change
There was a problem hiding this comment.
in this case it doesn't make any difference, probably pylint errors since that elif is after a return statement , but this is part of a testing that I made so I shouldn't have included
| check_status = self._get_check_status(request) | ||
|
|
||
| try: | ||
| status = PaymentStatus.PENDING |
There was a problem hiding this comment.
Why did you remove this default?
Is there not a case where the default, if check_status status is false, verify_status does not return a status?
There was a problem hiding this comment.
I didn't remove that, I consider this is as part of the initial conditions so I moved that line here
| verification_response.get('merchantTransactionId')): | ||
| transaction_id = verification_response['merchantTransactionId'] | ||
|
|
||
| if verification_response and 'merchantMemo' in verification_response: |
There was a problem hiding this comment.
I think is better check if isinstance(verification_response, dict) . Maybe this avoid if by x or y reason the
ecommerce-hyperpay/hyperpay/views.py
Line 126 in 48b02da
doesn't return a dict for response...
There was a problem hiding this comment.
That could be possible if someone makes a big change in the _verify_status method and that will require multiple code changes, one of them could be your suggestion, however the current implementation should be according to the current code and not suppositions, the method returns this that is a dictionary, if that isn't the case the method will fail here so I consider isinstance(verification_response, dict) unnecessary or the response is a dict or the code fail before
| basket_id = OrderNumberGenerator().basket_id(verification_response['merchantMemo']) | ||
| basket = self._get_basket(basket_id) | ||
|
|
||
| transaction_id = verification_response['id'] |
There was a problem hiding this comment.
I don't understand why not all transaction_id are with the id? I mean this line is not inside and if block, and it's the last one that changes.
This also happens in the current main implementation.
The only reason is that something else failed before this line and transaction_id was set to verification_response['merchantxxxx']
There was a problem hiding this comment.
Actully the correct basket submitted are set with the id that send processor.
And the failed to save the merchantxxx in the transaction_id.
There was a problem hiding this comment.
@andrey-canon what do you think in that behavior is ok that good transactions keep the id(processor sent) and the others keep the 'merchantxxxx' in transaction_id??
There was a problem hiding this comment.
personally I prefer to keep the same transaction id for all the operations, it's easier to filter error and success cases, however that is out of the scope of this pr and I would like to keep changes at minimum
omar-nelc
left a comment
omar-nelc
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks @andrey-canon!
3 participants
Description
This will add user basic information to the payment processor response and also will associate the basket if it's possible.
Check last records in https://edunext.ecommerce.stage.nelp.gov.sa/admin/payment/paymentprocessorresponse/