Payments flow adaptation for lazy payments

[Tiago-Salles]

Context

When the user clicks Continuar on PayGate, PayGate redirects back to PayGateCallbackSuccessResponseView. Until now, this view called handle_payment_and_create_order synchronously, which in turn invoked PayGate.handle_processor_response. That method queries the PayGate BackOfficeSearchTransactions API expecting to find a completed transaction for the basket.

For asynchronous payment methods (MB references, MBWAY) the upstream payment is not yet confirmed at this exact moment — the user still has to pay at an ATM / home-banking / phone. PayGate correctly returns an empty list, handle_processor_response raises:

oscar.apps.payment.exceptions.GatewayError: PayGate couldn't double check if basket has been payed

…and the user is shown the misleading "You have not been charged." page even when, minutes later, PayGate will confirm the payment via the server-to-server callback.

What this PR changes

The success callback no longer attempts to fulfil the order synchronously when a Thank-You URL is configured. Instead, it records the callback response and redirects the user to a Thank-You page in the ecommerce micro-frontend, where the actual payment status is lazily resolved per basket by the new nau_extensions.BasketPaymentStatusView.

• processors.py — PayGate now exposes a new thank_you_url property that reads the optional thank_you_url payment-processor configuration entry. Returns None when not configured.
• views.py — PayGateCallbackSuccessResponseView.get:
  • records the PayGate callback as before;
  • if thank_you_url is configured → redirects to <thank_you_url>?order_number=<basket.order_number> without calling handle_payment_and_create_order and without calling PayGate BackOfficeSearchTransactions;
  • if thank_you_url is not configured → falls back to the previous behaviour (synchronous fulfillment + redirect to the receipt page). This preserves backwards compatibility for any deployment that has not yet rolled out the Thank-You page.

Configuration

Add thank_you_url to the paygate entry of PAYMENT_PROCESSOR_CONFIG to enable the new behaviour. Example:

paygate:
  access_token: PwdX_XXXX_YYYY
  merchant_code: NAU
  api_checkout_url: https://lab.optimistic.blue/paygateWS/api/CheckOut
  api_back_search_transactions: https://lab.optimistic.blue/paygateWS/api/BackOfficeSearchTransactions
  api_basic_auth_user: username
  api_basic_auth_pass: password
  payment_types: ["VISA", "MASTERCARD", "MBWAY", "REFMB", "DUC"]
  # NEW: when set, the success callback redirects here instead of running
  # handle_processor_response synchronously. The basket order_number is
  # appended as a query string parameter.
  thank_you_url: https://orders.nau.edu.pt/thank-you

Related PRs

• ecommerce-nau-extensions: Payments flow adaptation for lazy payments ecommerce-nau-extensions#37
• frontend-app-ecommerce: Payments flow adaptation for lazy payments frontend-app-ecommerce#3

Related to: https://github.com/fccn/nau-technical/issues/923

[Tiago-Salles]

Hey, @ManuelStarDo and @rguerra-fccn. You can start reviewing this PR, while I deal with the pipeline errors. Thanks.

[ManuelStarDo]

The following are concrete code-level items worth addressing before merge:

• paygate/pending_orders.py::confirm_pending_order — no guard against concurrent double-confirmation. If the lazy-resolution endpoint (in the nau_extensions PR) and the server-to-server callback both call confirm_pending_order for the same order at nearly the same time, both could pass the "still Pending" check before either commits, and both would call save_payment_details(order) / set_status(ORDER.OPEN), potentially double-recording a PaymentSource/PaymentEvent. There's no select_for_update() or a re-check of order.status after acquiring a lock.

  • Fix: add a lock/guard, e.g. order = Order.objects.select_for_update().get(pk=order.pk); if order.status != ORDER.PENDING: return order at the top of confirm_pending_order, run inside transaction.atomic().

• paygate/pending_orders.py::place_pending_order — basket.submit() is called without catching a possible double-submit if two requests race before either commits the new Order row (i.e., before the IntegrityError that the caller in views.py already handles). If basket.submit() itself raises (Oscar generally rejects submitting an already-submitted basket) rather than the Order.number uniqueness constraint, that exception is not IntegrityError and falls into the view's generic except Exception branch, which unconditionally redirects to the error page — even though the "losing" request's sibling actually succeeded.

  • Fix: in the view's generic except Exception handler (not just the IntegrityError branch), also attempt get_order(basket) before deciding to show the error page, mirroring the recovery logic already used for IntegrityError.

• paygate/utils.py — minor duplication between get_order() and order_exist()
  Risk: get_order(basket) (new in this PR) and order_exist(basket) (pre-existing) both answer the same underlying question — "is there an Order for this basket?" — via two separate Order.objects.filter(...) queries. order_exist() returns a bool and is still used in paygate/processors.py::retry_baskets_payed_in_paygate(); get_order() returns the object and is used in paygate/views.py, which needs the actual Order (e.g. to check existing_order.status). Neither is dead code, so nothing needs to be removed, but having two independent lookups for the same thing is a small maintainability smell — if the lookup logic ever needs to change (e.g. adding a filter), it's easy to update one and forget the other.
  Fix: Rewrite order_exist() to delegate to get_order() so there's a single source of truth:

def order_exist(basket: Basket) -> bool:
    """
    Utility method that check if there is an Order for the Basket
    """
    return get_order(basket) is not None

Possible Improvements / Suggestions

• paygate/views.py::run_post_order: the docstring explaining why it's not named handle_post_order (to avoid shadowing the mixin's method) is good self-documentation; consider moving that explanation into the class-level docstring so it's visible alongside the other helper methods (create_new_order, handle_payment_and_create_order) for a reader scanning the class body.
• The module docstring in pending_orders.py explicitly flags the "nothing expires a Pending order" limitation as accepted for now — worth linking to or creating a follow-up issue if one doesn't already exist, so it doesn't get lost.

[Tiago-Salles]

@ManuelStarDo, I have applied the changes you requested. Ready to review.

[rguerra-fccn]

Tested locally: 52/52 tests passing. The race handling (select_for_update), idempotency, and IntegrityError recovery in pending_orders.py/views.py are solid and well covered.

However, the success-callback fallback path (no thank_you_url configured) doesn't match the PR description's claim of 'falls back to the previous behaviour (synchronous fulfillment + redirect to the receipt page)'. In the current code, handle_payment_and_create_order is never called from PayGateCallbackSuccessResponseView.get() anymore, for any payment method, regardless of thank_you_url. It unconditionally places a Pending order (if none exists) and redirects — thank_you_url only changes the redirect destination, not whether synchronous fulfillment happens.

This is confirmed by your own test, test_success_callback_without_thank_you_url_uses_the_receipt_page:

self.assertEqual(Order.objects.get(number=basket.order_number).status, ORDER.PENDING)
mock__make_api_json_request.assert_not_called()  # BackOfficeSearchTransactions never called

This isn't just a transient window either: there's no nau-tutor-configs PR yet that sets thank_you_url in PAYMENT_PROCESSOR_CONFIG, so this will be the default production behaviour immediately after merge, for however long until a follow-up config PR lands. A paying card user could be redirected straight to the receipt page for an order that is still Pending at that exact moment, relying entirely on the server-to-server callback (which does correctly pick up and confirm the pending order) to land afterwards.

Could you either:

1. Gate the pending-order-only path behind thank_you_url being configured, so the old synchronous path is genuinely preserved until the flag is turned on, or
2. Confirm the receipt page tolerates a momentarily-Pending order gracefully, and update the PR description to state plainly that this is an intentional, unconditional change in reliance on the server-to-server callback.

Requesting changes pending that clarification — happy to re-review quickly once addressed.

[ManuelStarDo]

@Tiago-Salles
How do we handle payments that never get paid?
There should be a method to clean stale payments that never got paid.

Two points worth addressing:

[MEDIUM] paygate/views.py:364 — thank_you_url query-string append is naive string concatenation
Risk: If a misconfigured thank_you_url already contains a trailing ? or &, the redirect URL becomes malformed, potentially breaking the Thank-You page load.
Fix: Use urllib.parse.urlparse/urlencode (or at minimum .rstrip('?&') on thank_you_url before appending) for a more robust join.

[LOW / SUGGESTION] pending_orders.py — unresolved "Pending orders never expire" limitation
Risk: Over time, unpaid Multibanco baskets accumulate as permanently Pending orders with no automated cleanup, relying entirely on manual support intervention.
Fix: Confirm a tracking issue exists (referenced PR mentions nau-technical#923 — verify this specific limitation is captured there or file a dedicated follow-up) for a future management command to expire stale Pending orders using the already-scaffolded but commented-out REFMB_START_DATE/REFMB_END_DATE fields.

[Tiago-Salles]

@ManuelStarDo, applied the suggestion.

About the pending order, it's not a concern for this ticket, please don't mix subjects. This ticket (with the three PRs that implement it) has the intention of building the structure where payments are able to be handled in an asynchronous way, meaning the platform will not expect the confirmation right after it just created an order.

I am about to create an issue to handle pending payments management subejct. Any specific MBREF business logic, or payments management related topics have nothing to do with the ability of a person paying a course asynchronously.

[ManuelStarDo]

@Tiago-Salles
You are right that stale pending orders is out of scope here, i missed that from the description in fccn/nau-technical#923

Going back on topic, this PR is still not ready to merge. Please address @rguerra-fccn 's concern above.

[Tiago-Salles]

Hey @ManuelStarDo, please approave if it meets your suggestions. @rguerra-fccn, I have applied the changes you suggested, we are good to merge.

[ManuelStarDo]

Hello @Tiago-Salles ,

The main problem Rui raised was not addressed, you can see more details below on the CRITICAL finding. Besides that, I have identified some other issues that should be fixed before merging.

Findings:

• [CRITICAL] paygate/views.py — get_or_place_pending_order runs unconditionally whenever thank_you_url is configured, for every payment method, not just asynchronous ones. Once thank_you_url is set (which the PR description implies will happen immediately post-merge for all NAU deployments, since it's the whole point of the change), a card/MB WAY payer landing on the success callback will get a Pending order placed and be redirected to the Thank-You page without handle_processor_response / handle_payment ever being invoked from this view. Fulfilment for card payments then depends entirely on the server-to-server callback landing and calling confirm_pending_order afterward. I confirmed this by tracing get(): when thank_you_url is truthy, the only path is get_or_place_pending_order → redirect; fulfil_synchronously (which is the only caller of handle_payment_and_create_order from this view) is only reached in the not thank_you_url branch. This directly contradicts the PR description's claim ("Card and MB WAY payments are unaffected... immediate enrolment") and issue #923's explicit acceptance criterion. This exact gap was already raised by rguerra-fccn's review (2026-08-31) and does not appear to have been resolved in the diff I fetched — no subsequent commit changes this branching logic.

  • Risk: If the server-to-server callback is delayed, dropped, or fails (network blip, PayGate outage, IP allowlist misconfiguration), a card payer who has genuinely and instantly paid will sit in Pending indefinitely with no enrolment and no receipt, and no lazy-resolution safety net analogous to what asynchronous payments get from the Order History page (since the whole premise of "lazy" resolution is that the user visits their orders — for a card payer landing on Thank-You, nothing prompts them to check status until the companion nau_extensions view is hit, which is outside this repo's control).
  • Fix: Gate get_or_place_pending_order's pending-order path specifically to asynchronous payment types, or alternatively still call handle_processor_response synchronously for card/MB WAY on the success callback (distinguish based on the callback payload's payment_type_code, which is already available in the recorded response) before falling back to placing a Pending order only for REFMB. As proposed by rguerra-fccn: confirm this is deliberate and document it plainly, or fix the gating.

• [HIGH] paygate/views.py — the class docstring for PayGateCallbackSuccessResponseView still says the previous "synchronous fulfilment + redirect to receipt page" is preserved "until thank_you_url is configured", but as shown above this is only true for the thank_you_url unset branch; the moment it's set, no deployment gets synchronous fulfilment for any payment type. The docstring should be corrected to avoid misleading future maintainers, independent of whichever direction the CRITICAL fix above takes.

• [LOW] paygate/views.py::get_or_place_pending_order — the except Exception branch (catching non-IntegrityError placement failures, e.g. basket.submit() rejecting an already-submitted basket) re-fetches the order and treats any found order as success. This is good, but the logging there is logger.exception and would be more actionable if it also logged the payment type/basket total, since a silent swallow-and-recover pattern here can otherwise be hard to triage in production without reproducing the race.

Possible Improvements

• pending_orders.py::confirm_pending_order catches a bare except Exception around handle_post_order (mirroring the existing pattern in views.py::run_post_order) — consider centralizing this "log and swallow post-order errors" logic into a shared helper since it's now duplicated across pending_orders.py and views.py, to avoid future drift between the two call sites.

[Tiago-Salles]

Thanks @ManuelStarDo — the [HIGH] and [LOW] are done. On the [CRITICAL] I disagree with the proposed fix, but since it makes sense I have applied a fix. Let me separate the two.

On routing by payment type — deliberate, and I'd like to keep it.

Uniform routing is the intent of the change, not an oversight. On every platform I've paid on, the redirect after the provider is a "thank you for your order" page for every method; what differs is how fast the confirmation then arrives, not where you land. An immediate payment isn't routed differently, it's just confirmed quickly.

Branching the success callback on payment_type_code would put a payment-method conditional in the view for no user-visible benefit, and it would make the view own a list of which PayGate types count as "immediate". That list isn't as obvious as cards vs. REFMB: MBWAY is asynchronous on the provider side (the payer approves on their phone within a PayGate countdown) even though the learner only reaches our success callback after that approval. Keeping the callback method-agnostic avoids encoding PayGate's per-type behaviour here at all.

On the underlying risk — I've fixed it where it actually lives.

The scenario you describe (S2S callback delayed, dropped, or blocked by an IP allowlist mistake) is real, but it isn't specific to cards, and the safety net for it was never the Order History page. It's retry_baskets_payed_in_paygate, the management command that asks PayGate for completed transactions in a time window and re-fires the server callback for any we haven't recorded.

What I'd missed, and what your review made me go and check, is that this PR silently broke that net:

if order_exist(basket):
    logger.info("Order already exists for payment_ref=%s", payment_ref)
    continue

Under the new flow every basket that comes back through the success callback has an order — a Pending one — so the sweep was skipping precisely the baskets it exists to rescue. That, rather than uniform routing, is the mechanism by which a paid card user could have sat Pending indefinitely.

Fixed in paygate/processors.py: the sweep now skips only a real, non-Pending order.

order = get_order(basket)
if order and order.status != ORDER.PENDING:
    continue

Two tests cover it: the callback is re-fired for a basket whose only order is Pending, and not fired for one already fulfilled. order_exist had no callers left after this, so it's gone — which also closes out the duplicate-lookup point from your first review.

Net effect: a dropped S2S callback is now recovered automatically by the next cron run, with no user action, for all payment types — a stronger guarantee than the per-type branch, which would still have left REFMB dependent on the learner opening Order History. A Pending order now has three independent resolution routes and depends on none of them individually: the S2S callback, the cron sweep, and the Order History page.

[HIGH] — done. The PayGateCallbackSuccessResponseView docstring now says plainly that thank_you_url is an on/off switch for the whole asynchronous flow rather than a redirect choice, and that once it's set no deployment gets synchronous fulfilment here for any payment type. It also records why uniform treatment is deliberate, and lists the three resolution routes above.

[LOW] — done. That logger.exception now includes the payment type and the basket total. The payment type is read defensively from the recorded callback payload through a small helper, so logging can't raise on a malformed or missing response.

On the shared post-order helper: fair point, the pattern is genuinely duplicated between pending_orders.py::confirm_pending_order and views.py::run_post_order. I'd rather not move code you and Rui have already reviewed at this stage of the PR — happy to do it here if you prefer, or to open a follow-up issue. Your call.

Two corrections for the record, since they affect how the findings read:

• The PR description doesn't contain the sentence quoted in the [CRITICAL] ("Card and MB WAY payments are unaffected... immediate enrolment") — I've checked the body and there's no such claim. If that acceptance criterion is in #923 I'm happy to discuss it there, but it isn't something this PR's description promised and then contradicted.
• @rguerra-fccn's point was addressed, in c792c16: the pending-order path is gated behind thank_you_url, so with the flag unset the previous synchronous path is preserved intact. The test he quoted (test_success_callback_without_thank_you_url_uses_the_receipt_page) no longer exists; it was replaced by ..._keeps_the_synchronous_flow, which asserts handle_processor_response is called and that the order is not Pending. Your [CRITICAL] is about the flag-set branch, which is a different question from his.

[Tiago-Salles]

@ManuelStarDo, remmember there is a bug with MBREF in PROD, and these three PR's, despite of adding a new feature, are highly focused on fixing this bug.

These three are not inteded to be the final solution, and the goal is mainly testing on DEV to identify the potential bugs this enviroment can raise on running it.

I strongly suggest you to focus on what really matters on this implementation, and gather any other not related concern on other tickets. For instance, you have mentioned a bug on the path of payments with card. We don't have payments with card, neither a clean deadline to serve this method, but we still have a real bug on PROD with MBREF.

I thank you for focusing your review on points that break the flow of what we currently serve, and gather any other not related concern on their dedicated tickets. This is a new path being created on the project, and for sure will bring new challenges, but for them to be truly identified we need to test in a real enviroments. Thanks.

[ManuelStarDo]

@Tiago-Salles
MBREF isn't enabled in PROD yet, so I don't see the same urgency to rush this to PROD.

The raised concern was not only for card payments, although they were also mentioned, they apply to MBway as well.

Here is a sample worst case scenario that might occur:

Learner pays via MBWAY (approves on phone) ✅ payment succeeds at PayGate
      │
      ▼
Browser redirects → PayGateCallbackSuccessResponseView.get()
      │
      ▼
thank_you_url is set (production intent)
      │
      ▼
get_or_place_pending_order(basket)
      │
      ├─ handle_payment_and_create_order() → NEVER CALLED
      ├─ handle_processor_response()        → NEVER CALLED
      │
      ▼
Order created with status = PENDING
      │
      ▼
redirect(thank_you_url?order_number=...)
      │
      ▼
Learner sees "Thank You" page — but order is NOT fulfilled, NOT enrolled

And the current 3 Path ways this can be solved:

Path A (expected, fast):
PayGate → server-to-server callback → PayGateCallbackServerResponseView
      → handle_payment_and_create_order()
      → confirm_pending_order()
      → Order: PENDING → OPEN → fulfilled ✅

Path B (fallback, if server callback is dropped/delayed/blocked):
retry_baskets_payed_in_paygate  (cron: 23:30 & 06:30 daily)
      → get_order(basket).status == PENDING
      → send_callback_to_itself_to_retry()
      → same as Path A
      = up to ~12h delay

Path C (manual):
Learner opens Order History → OrderPaymentStatusView
      → confirm_pending_order()
      = depends entirely on learner action

And Path A should solve the issue, but there are edge cases where it fails:

IF server callback (Path A) is delayed / dropped / blocked (network blip, IP allowlist, PayGate outage)
      │
      ▼
Order stays PENDING
      │
      ▼
Learner already paid, already approved on phone — but is NOT enrolled
      │
      ▼
Learner has no reason to revisit Order History (thinks it's done)
      │
      ▼
Enrollment only happens when cron sweep runs (up to 12h later)

Previously there was a fallback to this:

Browser returns → PayGateCallbackSuccessResponseView.get()
      → handle_payment_and_create_order()   ← attempts fulfillment RIGHT HERE
      → handle_processor_response() → BackOfficeSearchTransactions (queries PayGate directly)
      → if PayGate confirms paid → Order created & fulfilled immediately
      → if not confirmed yet → GatewayError → "you have not been charged" page

And with the PR changes it goes like this:

Browser returns → places PENDING order only, no fulfillment attempt at all
      → fulfillment now depends SOLELY on:
            S2S callback (Path A)  OR
            cron sweep (~12h)      OR
            learner opening Order History

The browser-return path's own ability to fulfill immediately has been removed for every payment type, not just the async ones that needed it removed. Before, the browser-return leg gave synchronous methods a second, independent, immediate shot. Now that redundancy is gone across the board, and everything collapses onto the same single dependency (S2S, then cron as backup).

Conclusion

My real concern is that a user will make an MBWay payment, see a Thank You page for their payment and then NOT be enrolled in the course. The user then has a valid reason to create a ticket for this, not knowing that the cron will pick up their order later.

My questions for you:
Since MBway can be as fast as card payment, do you see value in attempting handle_processor_response() once on the browser-return path before falling back to Pending, so a fast, already-confirmed MBWAY payment doesn't lose that opportunity?
In your eyes, does the current design prevent this from happening? If so, how? I want to make sure I'm not missing a path. If not, what's your proposal, fix it here, or document it as an accepted trade-off in the PR description?
If we just document it, can we agree on stating the actual worst-case bound (currently ~12h per the cron schedule) rather than leaving it implicit?

[Tiago-Salles]

@ManuelStarDo thanks, I disagree on what the fix should be and on where the discussion belongs, so let me answer your three questions.

1. Should the browser-return path try handle_processor_response() once before falling back to Pending?

No, and this is the core design decision of the PR rather than a side effect of it. The path that creates an order should never be the path that validates it. The browser return is a user redirect: it proves the learner pressed "Continuar", nothing more. Using it to query PayGate and fulfil in the same request is exactly the coupling that produced the "you have not been charged" bug, because it assumes the world is consistent at the instant the learner lands back. Adding it back "just once, for fast methods" keeps that assumption alive for every method where it happens to hold today, and moves the boundary of the bug instead of removing it.

The way this is meant to work, and what the PR sets up, is a payments flow in separate steps: a basket, then a Pending order that is a record of intent even when the money has already left the payer's account, and then a validation flow that is fully decoupled from the request that created the order. Order status, receipt issuing and enrolment are triggered from that validation flow, never from the redirect. Systems go down and callbacks get dropped, so recovery has to live in a layer that can be retried, scheduled and monitored, not in a browser round trip that runs once and cannot be replayed. That is the structure a platform with more than two million enrollments needs, and it is what this PR starts.

MBWAY illustrates this rather than contradicting it. PayGate holds the payer on a countdown and only returns to our success URL after the approval, so by the time the browser leg runs the server callback has normally already fired and the order is already Open or Complete. The browser leg then reads the existing order and redirects. The scenario you describe only occurs when the server callback is lost, and that is precisely the case the browser leg is the wrong tool for.

2. Does the current design prevent a learner from seeing "Thank you" and not being enrolled?

The confirmation flow is not the subject of this PR. This PR changes one thing: the success callback stops fulfilling synchronously and places a Pending order instead, so that a Multibanco reference no longer ends in an error page. How a Pending order is confirmed, how fast, and with which fallbacks, is the validation layer, and it exists today as the server callback, the Order History resolution and the retry_baskets_payed_in_paygate sweep. Hardening that layer, starting with the sweep schedule, is real work and I will open a dedicated ticket for it. It is not a reason to put fulfilment back on the redirect here.

3. Stating the worst-case bound in the PR description.

The bound is a property of the confirmation flow, not of this change, and it will be defined in its dedicated ticket, where the schedule is actually decided. I can't see why describing this on the PR really matters for the purpose it has of validating on DEV...

On urgency

MBREF is disabled in production precisely because the current implementation breaks it. We are offering a payment method we cannot serve, and we cannot even run it as a controlled bug, because it fails the whole execution for the learner. That seems urgent to me. This PR is what makes it possible to enable MBREF at all, and the goal of these three PRs is to get the flow running in DEV and find the problems that only show up against a real PayGate.