feat(billing-platform): add refund types + record_charge_refunds endpoint (REVENG-157)#303
Merged
armcknight merged 11 commits intoJun 12, 2026
Conversation
|
The latest Buf updates on your PR. Results from workflow ci / buf-checks (pull_request).
|
This was referenced Jun 11, 2026
…oint Adds the shared refund types + the charge-service endpoint driven by the `charge.refunded` webhook flow (REVENG-157): - ``common/v1/stripe_charge.proto``: new ``StripeRefund`` message + ``repeated refunds`` field on ``StripeCharge`` so webhook handlers receive per-refund metadata (id, amount, reason) and can record refunds idempotently — the aggregate ``amount_refunded`` on the charge alone isn't enough. - ``services/charge/v1/charge.proto``: new ``PlatformRefund`` canonical projection, paralleling ``PlatformCharge``. The shared response type across all three refund endpoints, so it ships here in the base PR. - ``services/charge/v1/endpoint_record_charge_refunds.proto``: ``ChargeService.record_charge_refunds`` — records per-refund rows from a Stripe webhook payload idempotently (keyed on refund stripe_id) and syncs the aggregate ``amount_refunded`` / ``refunded`` state. The webhook handler in getsentry calls ``ChargeService.record_charge_refunds`` directly. We considered an ``InvoicerService.handle_charge_refunded`` wrapper (mirroring the ``charge.succeeded`` / ``charge.dispute.created`` pattern) but the wrapper would have been a passthrough -- no multi-service orchestration to coordinate, just a single call. ``charge.updated`` set the precedent for skipping the presentation layer when only one data service is involved (getsentry#20559). The other two refund endpoints (``refund_charge``, ``list_refunds_by_invoice``) are split into separate PRs paired with their getsentry consumers. Rust + Python bindings regenerated by CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fc673f0 to
83023b0
Compare
noahsmartin
reviewed
Jun 11, 2026
noahsmartin
reviewed
Jun 11, 2026
noahsmartin
reviewed
Jun 11, 2026
noahsmartin
reviewed
Jun 11, 2026
…e_stripe_id Per review feedback on #303: - ``PlatformRefund.charge_stripe_id`` removed (field 2). Nothing consumes this field today; if a future caller needs to identify the parent charge of a standalone ``PlatformRefund`` (e.g. grouping by charge in a UI), we can add it back as a new field number then. - ``PlatformCharge.refunds`` added (field 10): the recorded refund rows against the charge, ordered by ``date_added_st``. Populated by endpoints that have already loaded the refund rows; left empty by endpoints that only need the aggregate ``amount_refunded`` cache. - ``RecordChargeRefundsResponse.refunds`` removed (field 2). Refunds now live under ``response.charge.refunds`` -- the response only needs the single ``charge`` field. Matches the DB relationship (refunds belong to a specific charge) and avoids duplicating the same data at two levels of the response. Remaining ``PlatformRefund`` field numbers compacted (3..6 -> 2..5) since this message hasn't shipped to any consumer yet. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make the unit explicit on the canonical refund projection. Matches the unit-explicit naming used on the underlying Django model (``PlatformRefund.amount_cents``) and avoids consumers having to guess whether the value is dollars, cents, or something else. Field number is unchanged (3), so wire-compatible with any in-flight serialized messages from earlier #303 iterations. The sibling ``PlatformCharge.amount`` / ``StripeRefund.amount`` / ``StripeCharge.amount`` aren't renamed -- their values come from Stripe or from the legacy ``Charge`` model, where the unit-implicit naming is already a settled convention. Following up on those would be a broader audit (#REVENG-???) and out of scope for this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
noahsmartin
approved these changes
Jun 11, 2026
…arge Per review feedback on getsentry#20611: the cached ``amount_refunded`` / ``refunded`` fields on ``PlatformCharge`` are derivable from the embedded ``refunds`` list -- consumers can sum ``refunds[*].amount_cents`` for the aggregate, and check ``len(refunds) > 0`` (or sum == amount) for "is this refunded." Storing them as separate fields creates a drift risk between the cache and the rows (sentry-protos#303 bugbot). Removing fields 7 (``refunded``) and 8 (``amount_refunded``) from the ``PlatformCharge`` proto. Field numbers reserved so they can't be re-used for an incompatible meaning. The corresponding service-side change (drop the cache write in ``record_charge_refunds``, populate ``refunds`` instead of the aggregate fields) lands in getsentry#20611. The DB columns on ``accounts_platformcharge`` are inherited from ``AbstractCharge`` and remain in place since the legacy ``Charge`` model still uses them; splitting the abstract class to fully drop the columns from ``accounts_platformcharge`` is a separate follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stead of removing The previous attempt (commit 67ce9cf) removed fields 7 (``refunded``) and 8 (``amount_refunded``) and replaced them with ``reserved 7, 8;`` so the field numbers couldn't be re-used. That tripped buf's ``FIELD_NO_DELETE`` rule, which sentry-protos enables deliberately: PR #168 lifted it once to clean up unused prototype protos, and PR #169 immediately restored it as a permanent guardrail. Restoring the fields with ``[deprecated = true]`` so: - buf is happy (no field deletion). - Consumers get a deprecation marker steering them toward ``refunds[*].amount_cents`` as the source of truth. - No drift risk in practice: the getsentry-side producer (``_charge_to_proto``) is already not populating these fields (getsentry#20611 commit ``96fd005``), and a search confirms no getsentry code reads ``PlatformCharge.refunded`` / ``PlatformCharge.amount_refunded`` -- the proto is internal to the billing platform service. This matches the spirit of Noah's review feedback (don't carry a cache that can drift from the rows) while respecting the repo's field- deletion policy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-record-charge-refunds'
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3167124. Configure here.
4 tasks
2 tasks
Closed
3 tasks
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
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.

Summary
Shared refund types + the endpoint driven by the
charge.refundedwebhook flow:common/v1/stripe_charge.proto— newStripeRefundmessage +repeated refundsfield onStripeCharge. Webhook handlers need per-refund metadata (id, amount, reason) for idempotent ingestion; the aggregateamount_refundedon the charge alone isn't enough.services/charge/v1/charge.proto— newPlatformRefundcanonical projection (parallelsPlatformCharge), embedded asrepeated refundsonPlatformCharge. The previously-cached aggregate scalarsrefunded(field 7) andamount_refunded(field 8) stay in the message marked[deprecated = true](persentry-protospolicy that disallows field deletion) and consumers should derive both from therefundslist —len(refunds) > 0for "any refund happened" andsum(refunds[*].amount_cents)for the total.services/charge/v1/endpoint_record_charge_refunds.proto(new) —ChargeService.record_charge_refunds, called directly by thecharge.refundedwebhook layer to record per-refund rows idempotently and return the charge with its refund list embedded.Proto stack
StripeRefund+PlatformRefundshared types,record_charge_refundsendpoint_list_refunds.proto(for customer + admin refund rendering)Test plan
Build Rust bindingsstep + auto-committed🤖 Generated with Claude Code