Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 46 additions & 17 deletions hyperpay/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,10 @@
from django.utils.translation import ugettext_lazy as _
from django.views.decorators.csrf import csrf_exempt
from django.views.generic import View
from oscar.apps.partner import strategy
from oscar.core.loading import get_class, get_model

from ecommerce.extensions.checkout.mixins import EdxOrderPlacementMixin
from ecommerce.extensions.checkout.utils import get_receipt_page_url
from oscar.apps.partner import strategy
from oscar.core.loading import get_class, get_model

from .processors import HyperPay, HyperPayMada

Expand Down Expand Up @@ -144,29 +143,29 @@ def _verify_status(self, resource_path):
logger.warning(
'Received a pending status code %s from HyperPay for payment id %s.',
result_code,
response_data['id']
response_data.get('id', 'id-not-found')
)
status = PaymentStatus.PENDING
elif self.PENDING_NOT_CHANGEABLE_SOON_CODES_REGEX.search(result_code):
logger.warning(
'Received a pending status code %s from HyperPay for payment id %s. As this can change '
'after several days, treating it as a failure.',
result_code,
response_data['id']
response_data.get('id', 'id-not-found')
)
status = PaymentStatus.FAILURE
elif self.SUCCESS_CODES_REGEX.search(result_code):
logger.info(
'Received a success status code %s from HyperPay for payment id %s.',
result_code,
response_data['id']
response_data.get('id', 'id-not-found')
)
elif self.SUCCESS_MANUAL_REVIEW_CODES_REGEX.search(result_code):
logger.error(
'Received a success status code %s from HyperPay which requires manual verification for payment id %s.'
'Treating it as a failed transaction.',
result_code,
response_data['id']
response_data.get('id', 'id-not-found')
)

# This is a temporary change till we get clarity on whether this should be treated as a failure.
Expand All @@ -175,7 +174,7 @@ def _verify_status(self, resource_path):
logger.error(
'Received a rejection status code %s from HyperPay for payment id %s',
result_code,
response_data['id']
response_data.get('id', 'id-not-found')
)
status = PaymentStatus.FAILURE

Expand Down Expand Up @@ -229,16 +228,38 @@ def _get_check_status(self, request):
del request.session['hyperpay_dont_check_status']
return check_status

def _generate_user_data(self, user):
"""
This extracts the basic user information.

Args:
user <User model>: User instance.

Returns
dictionary: Basic user data.
"""

if not user:
return {}

return {
"id": user.id,
"email": user.email,
"username": user.username,
}

def get(self, request, encrypted_resource_path=None):
"""
Handle the response from HyperPay and redirect to the appropriate page based on the status.
"""
if encrypted_resource_path is None:
self.payment_processor.record_processor_response(request.GET, transaction_id=request.GET.get('id'))

verification_response = ''
verification_response = {}
basket = None
error = None
transaction_id = 'Unknown'
status = PaymentStatus.PENDING

resource_path = self._get_resource_path(request, encrypted_resource_path)
if resource_path is None:
Expand All @@ -248,21 +269,19 @@ def get(self, request, encrypted_resource_path=None):
check_status = self._get_check_status(request)

try:
status = PaymentStatus.PENDING

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@andrey-canon andrey-canon Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't remove that, I consider this is as part of the initial conditions so I moved that line here

if check_status:
verification_response, status = self._verify_status(resource_path)
if (verification_response and isinstance(verification_response, dict) and
verification_response.get('merchantTransactionId')):
transaction_id = verification_response['merchantTransactionId']

if verification_response and 'merchantMemo' in verification_response:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is better check if isinstance(verification_response, dict) . Maybe this avoid if by x or y reason the

def _verify_status(self, resource_path):

doesn't return a dict for response...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

transaction_id = verification_response['merchantMemo']
basket_id = OrderNumberGenerator().basket_id(transaction_id)
basket = self._get_basket(basket_id)

if status == PaymentStatus.FAILURE:
return redirect(reverse('payment_error'))
if status == PaymentStatus.PENDING:
elif status == PaymentStatus.PENDING:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it better elif than the previous if ?, this is only curiosity of the change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return self._handle_pending_status(request, encrypted_resource_path, resource_path)

basket_id = OrderNumberGenerator().basket_id(verification_response['merchantMemo'])
basket = self._get_basket(basket_id)

transaction_id = verification_response['id']
Copy link

@johanseto johanseto Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actully the correct basket submitted are set with the id that send processor.
2023-09-07_13-41
And the failed to save the merchantxxx in the transaction_id.
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me neither

Copy link

@johanseto johanseto Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if not basket:
Expand All @@ -276,12 +295,22 @@ def get(self, request, encrypted_resource_path=None):
request.user.username,
)
raise Http404
except Exception as exc:
error = exc.__class__.__name__
finally:
verification_response.update({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I test this in queries and its working nice.
image

"request_user": self._generate_user_data(request.user),
"basket_user": self._generate_user_data(getattr(basket, 'owner', None)),
"status": status.name,
"error": error,
})

payment_processor_response = self.payment_processor.record_processor_response(
verification_response,
transaction_id=transaction_id,
basket=basket
)

try:
with transaction.atomic():
try:
Expand Down