refactor(api): give sponsorships a service class and clean up payments - #298
Open
harshtandiya wants to merge 3 commits into
Open
refactor(api): give sponsorships a service class and clean up payments#298harshtandiya wants to merge 3 commits into
harshtandiya wants to merge 3 commits into
Conversation
get_sponsorship_details, get_user_sponsorship_inquiries, create_sponsorship_payment_link and withdraw_sponsorship_enquiry each hand-built a dict and raised a bare frappe.throw, so every refusal came back as a generic 417 with the copy inline at the call site. The three per-enquiry endpoints now go through SponsorshipService, which reads the enquiry and its event as plain properties over get_cached_doc and owns the permission checks. The list endpoint stays a module-level function — a list query is not one enquiry, and a class with a single method is ceremony. Responses are pydantic models declared to match the wire byte for byte: same keys, same order, same date rendering. Errors are five named classes carrying their own status and copy — three 403s (view, pay, withdraw) and two 409s (already paid, already withdrawn). frappe.DoesNotExistError is left alone on an unknown enquiry; it already says the right thing with the right status. The two per-row title lookups in the list endpoint are batched into maps. Buzz Event autonames to integers on some sites while Link fields arrive as strings, so those map keys need an explicit str() cast — the get_value calls this replaces were coercing in SQL. No endpoint renamed and no payload reshaped, so the dashboard is untouched. Both read endpoints were diffed against a copy of the pre-change module and came out identical, then re-checked over HTTP for key order, date format and the 409 reaching err.messages[0]. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_payment_link_for_booking and get_payment_link_for_sponsorship were whitelisted, but their only callers live inside buzz/api — they are service functions, not endpoints, and being remotely callable let a client name its own amount source. The decorators go; the functions stay where they are, since buzz/payments.py already is the payments service layer and doctype controllers import from it. Two more things in the module were dead rather than merely unused. get_payment_gateway_for_event had no caller anywhere. And the "fallback to legacy field" branch in get_payment_gateways_for_event read Buzz Event.payment_gateway, a field the doctype no longer has, so get_cached_value returned None and the branch always produced []. Deleting both leaves behaviour unchanged. test_payments covers the one endpoint the domain has. It is thin enough that a service layer would be an empty hop, so payments/ gets no services.py and no schemas.py — get_event_payment_gateways returns a list of strings, and modelling that would be fiction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
setUpClass picked the first existing Payment Gateway record, which works on a bench that has one configured and raises IndexError on CI, where none exist. The test now creates its own gateway per test, inside the transaction that tearDown rolls back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
PR 3 of the
buzz/api/refactor, stacked on #297. Review the incremental diff.Sponsorships. Four endpoints that hand-built dicts and threw bare, so every refusal was a generic 417. The three per-enquiry ones now delegate to
SponsorshipService— enquiry and event as plain properties overget_cached_doc, permission checks in one place. The list endpoint stays a module-level function; a list query isn't one enquiry, and a one-method class is ceremony. Five response models matching the wire byte for byte, five named errors (3× 403, 2× 409).DoesNotExistErrorleft alone on an unknown enquiry — right status, well-understoodexc_type.Payments.
get_payment_link_for_bookingandget_payment_link_for_sponsorshiplose their@frappe.whitelist(); only callers were insidebuzz/api. Functions stay put — doctype controllers import them. Two things there were dead, not unused:get_payment_gateway_for_eventhad no caller at all, and the "legacy field" fallback readBuzz Event.payment_gateway, a field the doctype no longer has, so it always produced[]. Noservices.py, noschemas.py— one endpoint returning a list of strings.Gotcha.
Buzz Eventautonames to integers, Link fields arrive as strings. Batching the list endpoint's per-row title lookups into maps silently missed every row until the keys werestr()-cast; theget_valuecalls it replaced were coercing in SQL. Caught by a test, not by reading the diff.Not changed. No endpoint renamed, no payload reshaped — dashboard untouched. The two
No payment gateway configuredthrows stay 417s: retyping needs the class importable frombuzz/payments.py, andbuzz/api/payments/exceptions.pycan't be — importing it runs__init__.py, which importsbuzz.paymentsback mid-initialisation.Verified. 17 new tests;
test_sponsorship_enquiry,test_event_sponsor,test_event_booking(39) still green. Both read endpoints byte-identical against a pre-change copy, then re-checked over HTTP: key order,creationas"2026-07-29 01:36:38.702894",start_dateas"2026-07-29",has_sponsora bool, second withdraw 409 with the message in_server_messages. ruff clean.🤖 Generated with Claude Code