Fallout from PR #298, which removed the last two @frappe.whitelist() decorators from buzz/payments.py. The module is now purely a service layer with no HTTP surface, and a read through it turned up more than that PR's scope covered. Filing rather than folding into the buzz/api/ refactor stack — it's a money path and deserves its own review.
On pydantic: mostly no, one yes
No wire here anymore, so APIRequest/APIResponse models would be a category error — those bases exist for frappe's serializer, which never sees these values.
One exception. payments.py:81-93 hand-builds a 12-key payment_details dict, .update()s it at :96, then splats it into a controller whose signature is def get_payment_url(self, **kwargs) — every gateway accepts anything. And that dict does not disappear: the controller writes it into an Integration Request, and mark_payment_as_received reads it back at :139 to find data.payment and settle the Event Payment row. It's a schema that round-trips through a black box and nothing says so. A typo'd key is silent in both directions.
Model the outbound dict with a plain pydantic.BaseModel. Leave the read-back a frappe._dict — the gateway owns that shape, not us.
The bigger wins need no pydantic
1. mark_payment_as_received (:125-170). The worst function in the money path.
:126 if frappe.in_test: return — structurally untestable. Nothing verifies a real payment ever gets recorded.:129 import json inside the function, then json.loads on stored data. Should be top-level frappe.parse_json.:146-158 four-branch if/elif mapping gateway to payment-id field. elif "Stripe" in payment_gateway is a substring test, not equality — a dict lookup can't replace it naively.- Does nothing, silently, when no Integration Request exists. Payment authorized but never recorded, no log.
Split into find_gateway_response(reference_doctype, reference_docname) -> dict | None and apply_payment_response(response). The mapping becomes testable without a gateway and the frappe.in_test guard disappears — it exists only because the function is one indivisible lump.
2. get_payment_link_for_booking (:19) and get_payment_link_for_sponsorship (:40) are near-duplicates. Both resolve a gateway, fetch the event title, delegate — and both carry the same frappe.throw(_("No payment gateway configured for this event")). Extract resolve_gateway(event, payment_gateway) -> str so the throw lives once.
3. Dead surface. get_controller (:15) is a one-line alias with no caller outside the file. save_address (:173, # TODO: use it later!) has no caller anywhere. sponsorship_tier.py:7 imports mark_payment_as_received and never calls it.
4. # TODO: rethink later (:55). db.set_value writes tier onto the enquiry from inside a link-generation function — a getter with a side effect. Belongs in SponsorshipService.payment_link, where "the user picked this tier" is the actual event.
5. Worth confirming, not assuming. :94 pre-creates the order for Razorpay/Paymob, but razorpay's own get_payment_url creates one when order_id doesn't start with order_. May be deliberate double-creation avoidance.
Structure
Keep it one module. 192 lines, under the 300-line ceiling, and it's a service module imported by doctype controllers — not an API domain, so no folder. Reorder into gateway resolution, link creation, settlement. Only if the mark_payment_as_received split pushes it past ~250 lines does buzz/payments/ with gateways.py/links.py/settlement.py earn itself.
No class. Stateless functions over doc ids, no instance state to hang onto. SponsorshipService earned its class because five methods shared one enquiry; a PaymentLinkBuilder would be a namespace with extra syntax.
Sequencing
Tests first, and expect the first one to fail to even reach the function until the frappe.in_test return is gone. That's the chicken-and-egg to plan for — there is no coverage of this path today.
Overlaps the buzz/api/ refactor at exactly one line: PR 7 of that stack already has to touch the json.loads at :154. Either fold that into this issue or leave it to PR 7 explicitly — don't do it twice.
Fallout from PR #298, which removed the last two
@frappe.whitelist()decorators frombuzz/payments.py. The module is now purely a service layer with no HTTP surface, and a read through it turned up more than that PR's scope covered. Filing rather than folding into thebuzz/api/refactor stack — it's a money path and deserves its own review.On pydantic: mostly no, one yes
No wire here anymore, so
APIRequest/APIResponsemodels would be a category error — those bases exist for frappe's serializer, which never sees these values.One exception.
payments.py:81-93hand-builds a 12-keypayment_detailsdict,.update()s it at:96, then splats it into a controller whose signature isdef get_payment_url(self, **kwargs)— every gateway accepts anything. And that dict does not disappear: the controller writes it into an Integration Request, andmark_payment_as_receivedreads it back at:139to finddata.paymentand settle the Event Payment row. It's a schema that round-trips through a black box and nothing says so. A typo'd key is silent in both directions.Model the outbound dict with a plain
pydantic.BaseModel. Leave the read-back afrappe._dict— the gateway owns that shape, not us.The bigger wins need no pydantic
1.
mark_payment_as_received(:125-170). The worst function in the money path.:126if frappe.in_test: return— structurally untestable. Nothing verifies a real payment ever gets recorded.:129import jsoninside the function, thenjson.loadson stored data. Should be top-levelfrappe.parse_json.:146-158four-branch if/elif mapping gateway to payment-id field.elif "Stripe" in payment_gatewayis a substring test, not equality — a dict lookup can't replace it naively.Split into
find_gateway_response(reference_doctype, reference_docname) -> dict | Noneandapply_payment_response(response). The mapping becomes testable without a gateway and thefrappe.in_testguard disappears — it exists only because the function is one indivisible lump.2.
get_payment_link_for_booking(:19) andget_payment_link_for_sponsorship(:40) are near-duplicates. Both resolve a gateway, fetch the event title, delegate — and both carry the samefrappe.throw(_("No payment gateway configured for this event")). Extractresolve_gateway(event, payment_gateway) -> strso the throw lives once.3. Dead surface.
get_controller(:15) is a one-line alias with no caller outside the file.save_address(:173,# TODO: use it later!) has no caller anywhere.sponsorship_tier.py:7importsmark_payment_as_receivedand never calls it.4.
# TODO: rethink later(:55).db.set_valuewritestieronto the enquiry from inside a link-generation function — a getter with a side effect. Belongs inSponsorshipService.payment_link, where "the user picked this tier" is the actual event.5. Worth confirming, not assuming.
:94pre-creates the order for Razorpay/Paymob, but razorpay's ownget_payment_urlcreates one whenorder_iddoesn't start withorder_. May be deliberate double-creation avoidance.Structure
Keep it one module. 192 lines, under the 300-line ceiling, and it's a service module imported by doctype controllers — not an API domain, so no folder. Reorder into gateway resolution, link creation, settlement. Only if the
mark_payment_as_receivedsplit pushes it past ~250 lines doesbuzz/payments/withgateways.py/links.py/settlement.pyearn itself.No class. Stateless functions over doc ids, no instance state to hang onto.
SponsorshipServiceearned its class because five methods shared one enquiry; aPaymentLinkBuilderwould be a namespace with extra syntax.Sequencing
Tests first, and expect the first one to fail to even reach the function until the
frappe.in_testreturn is gone. That's the chicken-and-egg to plan for — there is no coverage of this path today.Overlaps the
buzz/api/refactor at exactly one line: PR 7 of that stack already has to touch thejson.loadsat:154. Either fold that into this issue or leave it to PR 7 explicitly — don't do it twice.