Fix payment duplicate confirmation to return proper error #1356
+21
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideRaises a dedicated PaymentAlreadyConfirmedException when a payment confirm call hits an already-confirmed payment, and wires this through the model and API layer so duplicate confirmations now return a 409 Conflict instead of silently succeeding. Sequence diagram for payment confirmation with duplicate detectionsequenceDiagram
actor APIClient
participant OrderAPI as OrderAPI_confirm
participant OrderPayment as OrderPayment
participant DB as Database
APIClient->>OrderAPI: POST /orders/{order_id}/payments/{payment_id}/confirm
OrderAPI->>DB: Load OrderPayment by id
DB-->>OrderAPI: OrderPayment instance
alt payment already confirmed (precheck)
OrderAPI->>OrderPayment: Read state
OrderAPI-->>APIClient: HTTP 409 Conflict (Payment already confirmed)
else payment not yet confirmed
OrderAPI->>OrderPayment: confirm(currency, amount, send_mail, force, generate_invoice, invoice_qualified)
OrderPayment->>DB: select_for_update(pk)
DB-->>OrderPayment: locked_instance
alt locked_instance.state is PAYMENT_STATE_CONFIRMED
OrderPayment->>OrderPayment: log concurrent confirm attempt
OrderPayment-->>OrderAPI: raise PaymentAlreadyConfirmedException
OrderAPI-->>APIClient: HTTP 409 Conflict (Payment already confirmed)
else locked_instance.state is not PAYMENT_STATE_CONFIRMED
OrderPayment->>DB: update state, payment_date, save
DB-->>OrderPayment: saved
OrderPayment-->>OrderAPI: return successfully
OrderAPI-->>APIClient: HTTP 200 OK (Payment confirmed)
end
end
Class diagram for payment confirmation exceptions and model updateclassDiagram
class PaymentException {
}
class PaymentAlreadyConfirmedException {
}
class OrderPayment {
PAYMENT_STATE_CONFIRMED
PAYMENT_STATE_PENDING
PAYMENT_STATE_CREATED
confirm(currency, amount, send_mail, force, generate_invoice, invoice_qualified)
}
PaymentException <|-- PaymentAlreadyConfirmedException
class OrderConfirmView {
confirm(request, kwargs)
}
OrderConfirmView ..> OrderPayment : uses
OrderConfirmView ..> PaymentAlreadyConfirmedException : catches
OrderConfirmView ..> PaymentException : catches
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
api/views/order.py,PaymentAlreadyConfirmedExceptionis used in theconfirmhandler but not imported; add it alongside the existing payment-related imports so the newexceptblock works at runtime. - In
OrderPayment.confirm, consider moving thePaymentAlreadyConfirmedExceptionimport to the module level (or adding a brief comment explaining the local import) to avoid repeated imports and make the dependency clearer unless there is a circular-import constraint.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `api/views/order.py`, `PaymentAlreadyConfirmedException` is used in the `confirm` handler but not imported; add it alongside the existing payment-related imports so the new `except` block works at runtime.
- In `OrderPayment.confirm`, consider moving the `PaymentAlreadyConfirmedException` import to the module level (or adding a brief comment explaining the local import) to avoid repeated imports and make the dependency clearer unless there is a circular-import constraint.
## Individual Comments
### Comment 1
<location> `app/eventyay/base/models/orders.py:1673-1674` </location>
<code_context>
- self.full_id,
- )
+ # Payment is already confirmed, raise exception
+ raise PaymentAlreadyConfirmedException(
+ 'Payment {} has already been confirmed.'.format(self.full_id)
)
- return
</code_context>
<issue_to_address>
**suggestion:** Raising instead of logging changes behavior and removes observability of the race condition.
Previously, a concurrent double-confirm was logged and treated as a no-op; now it raises and propagates. If this behavior change is intended, consider adding a log (info/debug) just before raising so you preserve the observability of these race conditions while using the more explicit error handling.
Suggested implementation:
```python
with transaction.atomic():
locked_instance = OrderPayment.objects.select_for_update().get(pk=self.pk)
if locked_instance.state == self.PAYMENT_STATE_CONFIRMED:
# Payment is already confirmed; log and raise exception to preserve observability of race conditions
logger.info(
"Concurrent confirm attempt for already confirmed payment %s",
self.full_id,
)
raise PaymentAlreadyConfirmedException(
"Payment {} has already been confirmed.".format(self.full_id)
)
locked_instance.state = self.PAYMENT_STATE_CONFIRMED
locked_instance.payment_date = payment_date or now()
```
1. Ensure there is a module-level logger defined consistent with the existing conventions in `orders.py`, for example:
```python
import logging
logger = logging.getLogger(__name__)
```
If the file already defines a `logger` (or uses a different logging pattern), reuse that instead of introducing a new name.
2. If your logging policy prefers a different level (`debug` instead of `info`) for expected-but-rare races, adjust `logger.info` accordingly.
</issue_to_address>
### Comment 2
<location> `app/eventyay/base/models/orders.py:1674` </location>
<code_context>
def confirm(
self,
count_waitinglist=True,
send_mail=True,
force=False,
user=None,
auth=None,
mail_text='',
ignore_date=False,
lock=True,
payment_date=None,
):
"""
Marks the payment as complete. If possible, this also marks the order as paid if no further
payment is required
:param count_waitinglist: Whether, when calculating quota, people on the waiting list should be taken into
consideration (default: ``True``).
:type count_waitinglist: boolean
:param force: Whether this payment should be marked as paid even if no remaining
quota is available (default: ``False``).
:param ignore_date: Whether this order should be marked as paid even when the last date of payments is over.
:type force: boolean
:param send_mail: Whether an email should be sent to the user about this event (default: ``True``).
:type send_mail: boolean
:param user: The user who performed the change
:param auth: The API auth token that performed the change
:param mail_text: Additional text to be included in the email
:type mail_text: str
:raises Quota.QuotaExceededException: if the quota is exceeded and ``force`` is ``False``
"""
from eventyay.base.services.invoices import (
generate_invoice,
invoice_qualified,
)
from eventyay.base.payment import PaymentAlreadyConfirmedException
with transaction.atomic():
locked_instance = OrderPayment.objects.select_for_update().get(pk=self.pk)
if locked_instance.state == self.PAYMENT_STATE_CONFIRMED:
# Payment is already confirmed, raise exception
raise PaymentAlreadyConfirmedException(
'Payment {} has already been confirmed.'.format(self.full_id)
)
locked_instance.state = self.PAYMENT_STATE_CONFIRMED
locked_instance.payment_date = payment_date or now()
locked_instance.info = self.info # required for backwards compatibility
locked_instance.save(update_fields=['state', 'payment_date', 'info'])
# Do a cheap manual "refresh from db" on non-complex fields
for field in self._meta.concrete_fields:
if not field.is_relation:
setattr(self, field.attname, getattr(locked_instance, field.attname))
self.refresh_from_db()
self.order.log_action(
'eventyay.event.order.payment.confirmed',
{
'local_id': self.local_id,
'provider': self.provider,
},
user=user,
auth=auth,
)
if self.order.status in (Order.STATUS_PAID, Order.STATUS_CANCELED):
logger.info('Confirmed payment {} but order is in status {}.'.format(self.full_id, self.order.status))
return
payment_sum = self.order.payments.filter(
state__in=(self.PAYMENT_STATE_CONFIRMED, self.PAYMENT_STATE_REFUNDED)
).aggregate(s=Sum('amount'))['s'] or Decimal('0.00')
refund_sum = self.order.refunds.filter(
state__in=(
OrderRefund.REFUND_STATE_DONE,
OrderRefund.REFUND_STATE_TRANSIT,
OrderRefund.REFUND_STATE_CREATED,
)
).aggregate(s=Sum('amount'))['s'] or Decimal('0.00')
if payment_sum - refund_sum < self.order.total:
logger.info(
'Confirmed payment {} but payment sum is {} and refund sum is {}.'.format(
self.full_id, payment_sum, refund_sum
)
)
return
if (self.order.status == Order.STATUS_PENDING and self.order.expires > now() + timedelta(hours=12)) or not lock:
# Performance optimization. In this case, there's really no reason to lock everything and an atomic
# database transaction is more than enough.
lockfn = NoLockManager
else:
lockfn = self.order.event.lock
with lockfn():
self._mark_paid(
force,
count_waitinglist,
user,
auth,
overpaid=payment_sum - refund_sum > self.order.total,
ignore_date=ignore_date,
)
invoice = None
if invoice_qualified(self.order):
invoices = self.order.invoices.filter(is_cancellation=False).count()
cancellations = self.order.invoices.filter(is_cancellation=True).count()
gen_invoice = (
invoices == 0 and self.order.event.settings.get('invoice_generate') in ('True', 'paid')
) or 0 < invoices <= cancellations
if gen_invoice:
invoice = generate_invoice(
self.order,
trigger_pdf=not send_mail or not self.order.event.settings.invoice_email_attachment,
)
if send_mail and self.order.sales_channel in self.order.event.settings.mail_sales_channel_placed_paid:
self._send_paid_mail(invoice, user, mail_text)
if self.order.event.settings.mail_send_order_paid_attendee:
for p in self.order.positions.all():
if p.addon_to_id is None and p.attendee_email and p.attendee_email != self.order.email:
self._send_paid_mail_attendee(p, user)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
- Low code quality found in OrderPayment.confirm - 13% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
```suggestion
f'Payment {self.full_id} has already been confirmed.'
```
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where duplicate payment confirmations returned None silently instead of raising an exception, making it impossible for callers to distinguish between successful confirmations and race condition scenarios. The fix introduces a new PaymentAlreadyConfirmedException that is raised when attempting to confirm an already-confirmed payment, and adds specific handling in the API layer to return HTTP 409 CONFLICT.
Key changes:
- Added
PaymentAlreadyConfirmedExceptionas a subclass ofPaymentExceptionfor semantic clarity - Modified
OrderPayment.confirm()to raise the exception instead of silently returning - Updated API endpoint to catch the exception and return appropriate 409 CONFLICT status
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/eventyay/base/payment.py | Defines new PaymentAlreadyConfirmedException exception class |
| app/eventyay/base/models/orders.py | Changes silent return to exception raise when payment already confirmed |
| app/eventyay/api/views/order.py | Imports new exception and handles it with 409 CONFLICT response |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- API consistency for 409 responses
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
OrderViewSet.confirm,PaymentAlreadyConfirmedExceptionis referenced in theexceptblock but never imported, which will raise aNameErrorat runtime; add the appropriate import or fully qualify the exception name. - The confirmed-payment check is now implemented both in the view (early 409 return) and in
OrderPayment.confirm(raisingPaymentAlreadyConfirmedException); consider consolidating this into the model logic only and letting the view rely on the exception to avoid duplicated state checks. - Since
PaymentAlreadyConfirmedExceptionis imported insideOrderPayment.confirmto work around circular imports, consider extracting shared payment exceptions to a small, dependency-light module so the model andpayment.pycan import it normally without runtime imports.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `OrderViewSet.confirm`, `PaymentAlreadyConfirmedException` is referenced in the `except` block but never imported, which will raise a `NameError` at runtime; add the appropriate import or fully qualify the exception name.
- The confirmed-payment check is now implemented both in the view (early 409 return) and in `OrderPayment.confirm` (raising `PaymentAlreadyConfirmedException`); consider consolidating this into the model logic only and letting the view rely on the exception to avoid duplicated state checks.
- Since `PaymentAlreadyConfirmedException` is imported inside `OrderPayment.confirm` to work around circular imports, consider extracting shared payment exceptions to a small, dependency-light module so the model and `payment.py` can import it normally without runtime imports.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Copilot finished reviewing on behalf of
ArnavBallinCode
November 28, 2025 11:27
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/eventyay/api/views/order.py:1198
- The
create()method callspayment.confirm()but doesn't handle the newPaymentAlreadyConfirmedException. While this scenario may be unlikely (confirming a newly created payment), for consistency and to prevent unexpected errors, this exception should be caught here as well. Consider addingexcept PaymentAlreadyConfirmedException: passor similar handling between lines 1194 and 1195.
if mark_confirmed:
try:
r.confirm(
user=self.request.user if self.request.user.is_authenticated else None,
auth=self.request.auth,
count_waitinglist=False,
force=request.data.get('force', False),
send_mail=send_mail,
)
except Quota.QuotaExceededException:
pass
except SendMailException:
pass
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app/eventyay/api/views/order.py:1195
- 'except' clause does nothing but pass and there is no explanatory comment.
except PaymentAlreadyConfirmedException:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
Please refer #1344 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix: Payment Duplicate Confirmation Returns Proper Error Status
Root Cause
In
OrderPayment.confirm()(lines 1671-1677), when a duplicate confirmation is detected:The method logs a message but returns
Nonesilently. The calling code cannot distinguish this from a successful confirmation.Solution
Created new exception:
PaymentAlreadyConfirmedExceptioninpayment.pyPaymentExceptionfor proper exception hierarchyUpdated model: Changed silent return to raise exception
Updated API: Added specific handler in
api/views/order.pyAddresses the race condition issue where duplicate payment confirmations were indistinguishable from successful confirmations, breaking idempotency contracts expected by payment integrations.
Fixes #1344
Summary by Sourcery
Ensure duplicate payment confirmations return a conflict error instead of silently succeeding, improving observability and idempotency for payment confirmation flows.
Bug Fixes:
Enhancements: