Skip to content
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

Cancel and refund an order with payment provider #930

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jonathanreveille
Copy link
Member

@jonathanreveille jonathanreveille commented Sep 24, 2024

Purpose

This PR will solve this issue

We want to able to cancel and then refund an order if all conditions are met.
The refund state can only be accessible if the order is canceled, not free and the student has paid at least 1 installment in the payment schedule.

Proposal

  • Add new state 'refund' in the order flows.
  • Add endpoint refund for payment backends.
  • Add admin API endpoint to cancel and refund an order.
  • Fix debug views for payment and mail reminders for paid installments

@jonathanreveille jonathanreveille self-assigned this Sep 24, 2024
@jonathanreveille jonathanreveille changed the base branch from main to dev September 24, 2024 17:20
@jonathanreveille jonathanreveille force-pushed the dev_/cancel_and_refund_an_order branch 14 times, most recently from 4b58c2a to c490589 Compare October 1, 2024 17:06
@jonathanreveille jonathanreveille force-pushed the dev_/cancel_and_refund_an_order branch 2 times, most recently from 0bd8a1e to 34b8fe8 Compare October 3, 2024 16:48
@jonathanreveille jonathanreveille force-pushed the dev_/cancel_and_refund_an_order branch 9 times, most recently from e626aea to 3e1c709 Compare October 10, 2024 16:44
@jonathanreveille jonathanreveille force-pushed the dev_/cancel_and_refund_an_order branch 3 times, most recently from e1b2424 to 5f7da1a Compare October 10, 2024 17:09
@jonathanreveille jonathanreveille changed the base branch from dev to main October 10, 2024 17:11
@jonathanreveille jonathanreveille force-pushed the dev_/cancel_and_refund_an_order branch 2 times, most recently from f0dbbc0 to b8f8313 Compare October 10, 2024 17:18
We want to be able to refund the installments paid on an order
for a student. This state is only accessible when the order is
cancelled. Some adjustments were made for the OrderGeneratorFactory
to use this new state 'refund' in tests. There will be very few
cases where an order can be refund, for example : the session
is canceled, the professor is on sick leave, or the admin
decides to refund the installments for the student in specific
cases.
For now, we have added the refund and cancel action with
payment backend Lyra. It allows to refund or cancel
a transaction created with the payment provider. The payment
provider decides whether the transaction can be cancelled
or refunded, depending on the capture date of the amount.
For our case, we let the payment provider decide the
operation to do, and we create a transaction and a
credit note when we receive the notification through
our payment webhook. Adjustments were done to
set an installment as refunded once we receive the
notification from the payment provider.
For admin users, we have created two new endpoints
to change the state of an order. The first one is
to cancel an order, and once the order is cancelled,
the admin user may use the refund endpoint to trigger
the refund of all paid installments on an order's
payment schedule.
Since we have updated the `OrderGeneratorFactory` that
now creates the Invoice and the Transactions linked to
paid installments from the order's payment schedule. This
commit fix it by taking away the creation of the invoice
and the transaction within the debug view class and let the
factory create them.
Reproductible seed was causing an issue while using the
debug payment view. Duplicated address issues. Since
the OrderGeneratorFactory now handles the invoices, and
the recipient main address, we can let it generate
random data for debug purposes.
Copy link
Member

@lunika lunika left a comment

Choose a reason for hiding this comment

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

I don't fully understand what is made in this PR, We have to discuss it to be sure everything is covered.

@@ -1140,6 +1139,21 @@ def set_installment_refused(self, installment_id):
ActivityLog.create_payment_failed_activity_log(self)
self._set_installment_state(installment_id, enums.PAYMENT_STATE_REFUSED)

def set_installment_refunded(self, installment_id):
Copy link
Member

Choose a reason for hiding this comment

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

The order state should not be check before trying to change an installment state ? When this method will be called ?

if (
invoice.state == INVOICE_STATE_REFUNDED
and invoice.order.state != ORDER_STATE_REFUND
):
invoice.order.flow.cancel()
Copy link
Member

Choose a reason for hiding this comment

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

Should not be refund instead of cancel ?

notification_request.body.decode("utf-8")
)

return True
Copy link
Member

Choose a reason for hiding this comment

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

Why returning a boolean ?

"exist at the payment provider."
)

return True
Copy link
Member

Choose a reason for hiding this comment

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

Same here, why returning a boolean ?

def _do_on_refund(amount, invoice, refund_reference):
def _do_on_refund(
amount, invoice, refund_reference, installment_id, is_transaction_canceled=False
):
Copy link
Member

Choose a reason for hiding this comment

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

IMO there is an issue with this method. I don't understand how you can work on refunding installment, etc while still having an order not in refund state and at the end the order is still not in refund state. Probably I'm missing something.

@@ -244,3 +253,8 @@ def abort_payment(self, payment_id):
payplug.Payment.abort(payment_id)
except (Forbidden, NotFound) as error:
raise exceptions.AbortPaymentFailed(str(error)) from error

def cancel_or_refund(self, amount, transaction_reference):
Copy link
Member

Choose a reason for hiding this comment

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

If it should be not implemented, then you should not override and keep the default behavior

)

self.assertEqual(transaction.invoice.type, "credit_note")

# - Order has been canceled
order.refresh_from_db()
self.assertEqual(order.state, "canceled")
Copy link
Member

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 at the end we are in canceled state and not in refund. We have to discuss about that

Comment on lines +663 to +673
if order.state != enums.ORDER_STATE_CANCELED:
return Response(
"Cannot refund an order not canceled.",
status=HTTPStatus.UNPROCESSABLE_ENTITY,
)

if not has_installment_paid(order):
return Response(
"Cannot refund an order without paid installments in payment schedule.",
status=HTTPStatus.BAD_REQUEST,
)
Copy link
Member

Choose a reason for hiding this comment

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

All this check should be called when applytin flow.refund(). IMO it is useless to do it twice. You should do a try/except on the order.flow.refund() and return the good response based on what is happening

backend.handle_notification(request)

order.refresh_from_db()
self.assertEqual(
Copy link
Member

Choose a reason for hiding this comment

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

Also all installments should have a refunded state ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants