fix(api): harden API — strip PII/Stripe IDs, unify pagination, normalise cents naming#51
Conversation
55a6b1d to
5273130
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens the public API wire format by removing internal/operational identifiers from serialized responses, standardizing pagination and money field naming, and aligning frontend BFF/types with the updated backend contracts. It also adds a missing Helm ingress template so existing ingress values are actually applied at deploy time.
Changes:
- Remove Stripe/internal IDs from donation/subscription JSON output and introduce a public
DonationSummaryprojection for initiative donation listings. - Standardize pagination (
limit/offset) and enforce integer parsing for pagination query params; unify transaction pagination away frompage/size. - Normalize JSON field naming from
*_in_centsto*_centsacross backend models and frontend request/response types; add Helm ingress template.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/shared/types/transaction.types.ts | Align shared transaction list shape to limit/offset. |
| frontend/shared/types/payment.types.ts | Remove Stripe payment intent ID from donation result type. |
| frontend/server/types/transactions.types.ts | Update backend transaction list typing to limit/offset. |
| frontend/server/types/payment.types.ts | Remove stripe_payment_intent_id from backend wire type. |
| frontend/server/api/static-pages/featured-initiatives.get.ts | Fetch featured initiatives using backend-supported sorting by total raised. |
| frontend/server/api/initiatives/[id]/transactions.get.ts | Proxy transactions using limit/offset query params and map updated envelope fields. |
| frontend/server/api/initiatives/[id]/donations.post.ts | Send amount_cents and stop mapping Stripe payment intent ID from the response. |
| frontend/app/composables/initiatives/useInitiativeTransactions.ts | Update composable to request/cache by limit/offset instead of size. |
| backend/internal/service/initiative_service.go | Update service signature to pass limit/offset into the ledger client. |
| backend/internal/service/donation_service.go | Return []DonationSummary for public initiative donation listing and enrich donor display data. |
| backend/internal/infrastructure/clients/ledger.go | Change ledger transaction filter to limit/offset and convert to ledger paging model. |
| backend/internal/handler/subscription_handler.go | Use shared pagination parsing helper and return 400 on invalid pagination params. |
| backend/internal/handler/respond.go | Add parsePaginationParams helper to validate limit/offset. |
| backend/internal/handler/initiative_handler.go | Use new pagination parsing and update transactions handler to accept limit/offset. |
| backend/internal/handler/donation_handler.go | Use new pagination parsing and return public donation summaries for initiative donation list. |
| backend/internal/domain/models/transaction.go | Rename transaction list envelope fields to limit/offset. |
| backend/internal/domain/models/subscription.go | Suppress internal fields from JSON and rename cents fields/tags to *_cents. |
| backend/internal/domain/models/initiative_relations.go | Normalize goal/OSTIF budget JSON field names to *_cents. |
| backend/internal/domain/models/donation.go | Suppress internal fields from JSON, normalize cents tag, and add DonationSummary. |
| backend/charts/lfx-v2-initiatives-service/templates/ingress.yaml | Add ingress template so chart ingress values are no longer a no-op. |
Comments suppressed due to low confidence (1)
backend/internal/domain/models/subscription.go:45
- Now that the JSON field is
amount_cents, user-facing validation/errors should refer toamount_centsas well. There are still code paths emitting messages likeamount_in_cents must be positiveduring subscription creation, which will confuse API callers after this wire-format rename.
// SubscriptionCreateInput is the request body for creating a subscription.
type SubscriptionCreateInput struct {
AmountCents int64 `json:"amount_cents"`
Frequency string `json:"frequency"`
Category string `json:"category,omitempty"`
OrganizationID string `json:"organization_id,omitempty"`
StripePaymentMethodID string `json:"stripe_payment_method_id"`
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ise cents naming
Four related API quality issues addressed in one pass so the public wire
format is consistent before the first external consumer is onboarded.
**1 — Strip internal IDs and PII from serialised responses**
`Donation` and `Subscription` were serialising `user_id`, `organization_id`,
and all Stripe internal IDs (`stripe_payment_intent_id`, `stripe_charge_id`,
`stripe_subscription_id`, etc.) directly to API consumers. These are
operational fields used only by the webhook reconciliation flow and must
never appear on public or user-facing endpoints.
Changed all affected fields to `json:"-"`. Added `DonationSummary` — a
purpose-built projection for the public donation list that carries only
display-safe fields (donor name/type/avatar, amount, status, date). The
full `Donation` struct is still used for the authenticated `/v1/me/donations`
endpoint, where the caller owns the record.
Matching frontend BFF types cleaned up: `stripe_payment_intent_id` removed
from `DonationResultWire` and `DonationResult`; the mapping in
`donations.post.ts` simplified accordingly.
**2 — Return 400 for non-integer pagination params**
All list handlers were calling `strconv.Atoi(q.Get("limit"), _)` and
silently ignoring parse errors, causing `?limit=abc` to be treated as
`?limit=0` and fall through to the service default. Callers got no
feedback that their request was malformed.
Extracted a `parsePaginationParams` helper in `respond.go` that returns
a proper `400 Bad Request` with an explanatory message when `limit` or
`offset` cannot be parsed. All six list handlers (`List`/`ListForUser` on
initiatives, donations, and subscriptions) now use it.
**3 — Normalise `_in_cents` suffix to `_cents` across wire format**
Several fields used the redundant `_in_cents` suffix in their JSON tags
(`amount_in_cents`, `current_amount_in_cents`, `total_budget_in_cents`)
while others (transactions, `DonationSummary`) already used the shorter
`_cents`. Standardised everything to `_cents`:
- `Donation.CurrentAmountCents` → `"amount_cents"`
- `DonationCreateInput.AmountCents` → `"amount_cents"`
- `Subscription.CurrentAmountCents` → `"amount_cents"`
- `SubscriptionCreateInput.AmountCents` → `"amount_cents"`
- `GoalInput.AmountInCents` → `"amount_cents"`
- `OSTIFDetailInput/OSTIFDetail.TotalBudgetInCents` → `"total_budget_cents"`
BFF `donations.post.ts` updated to send `amount_cents` in the request body.
**4 — Unify transaction pagination to limit/offset**
`TransactionList` was using `page`/`size` (1-based) while every other
paginated resource used `limit`/`offset`. Unified to `limit`/`offset`
end-to-end: Go model, ledger client (converts internally to Ledger's
1-based page API), service method signature, handler, BFF server route,
frontend server/shared types, and the `useInitiativeTransactions` composable.
**5 — Add missing Helm ingress template**
The backend Helm chart had `ingress` values wired up in ArgoCD but no
`ingress.yaml` template, so `ingress.enabled: true` was silently a no-op.
Added the template following the standard pattern used by other LFX services.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
5273130 to
f0ac32a
Compare
…g tests Three issues identified in PR self-review: - donation_service: use existing donorTypeOrganization/donorTypeIndividual constants from statistics_service instead of inline string literals - donation_service: record span errors in projectDonationSummaries when user or org batch lookups fail (errors are still silenced for graceful degradation, but now visible in traces) - add TestProjectDonationSummaries_* covering: empty input, individual donor, org donor, user ID deduplication, user lookup error degradation, org lookup error degradation, unknown user ID with no name - add TestParsePaginationParams_* covering: valid limit+offset, absent params defaulting to zero, non-integer limit returns 400, non-integer offset returns 400, float rejected, negative integers accepted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
backend/internal/domain/models/donation.go:46
DonationCreateInputnow uses the JSON fieldamount_cents, but validation errors in the service layer still refer toamount_in_cents(e.g. “amount_in_cents must be positive”), which will confuse API consumers. Update error messages to match the new wire field name.
// DonationCreateInput is the request body for creating a donation.
type DonationCreateInput struct {
AmountCents int64 `json:"amount_cents"`
Category string `json:"category,omitempty"`
PONumber string `json:"po_number,omitempty"`
PaymentMethod string `json:"payment_method,omitempty"`
OrganizationID string `json:"organization_id,omitempty"`
// StripePaymentMethodID is used to create a Stripe charge
StripePaymentMethodID string `json:"stripe_payment_method_id,omitempty"`
backend/internal/domain/models/subscription.go:46
SubscriptionCreateInputnow uses the JSON fieldamount_cents, but validation errors in the service layer still refer toamount_in_cents(e.g. “amount_in_cents must be positive”), which will confuse API consumers. Update error messages to match the new wire field name.
// SubscriptionCreateInput is the request body for creating a subscription.
type SubscriptionCreateInput struct {
AmountCents int64 `json:"amount_cents"`
Frequency string `json:"frequency"`
Category string `json:"category,omitempty"`
OrganizationID string `json:"organization_id,omitempty"`
StripePaymentMethodID string `json:"stripe_payment_method_id"`
// IdempotencyKey is set by the handler from the Idempotency-Key HTTP header.
Address review feedback on PR #51: - models/donation.go: rename DonorAvatar → DonorAvatarURL (consistent with statistics.go), fix doc comment to describe actual omission policy - service/donation_service.go: propagate span ctx through projectDonationSummaries so downstream DB calls are correctly parented; add slog.WarnContext on degraded paths; fix error message amount_in_cents → amount_cents - service/subscription_service.go: fix error message amount_in_cents → amount_cents - service/donation_service_test.go: update DonorAvatar → DonorAvatarURL references - clients/ledger.go: document page-alignment constraint on offset/limit conversion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Review Feedback AddressedChanges Made
Declined
Threads Resolved10 of 10 unresolved threads addressed in this iteration. |
computed was used in the enabled option but missing from the vue import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
emlimlf
left a comment
There was a problem hiding this comment.
Frontend changes looks good to me
Summary
API hardening pass before the first external consumer is onboarded: strips internal/Stripe IDs from public responses, introduces a
DonationSummaryprojection for the public donation list, standardises pagination error handling, normalises money field naming to_cents, and unifies transaction pagination tolimit/offset.All four changes are applied together because they touch the same wire format — shipping them as one PR avoids a window where the API is partially cleaned up.
Changes
1 — Strip internal IDs from public responses
DonationandSubscriptionwere serialisinguser_id,organization_id, and all Stripe IDs (stripe_payment_intent_id,stripe_charge_id,stripe_subscription_id, etc.) to any API caller. These are internal operational fields used only by the webhook reconciliation flow.json:"-"DonationSummary— a purpose-built read model for the public list endpoint (GET /v1/initiatives/{id}/donations) that includes only display-safe fields: donor name, type, avatar, amount, status, and dateDonationstruct is still returned byGET /v1/me/donations(authenticated — caller owns the record)DonationService.ListByInitiativenow returns[]DonationSummary; a helper batch-resolves donor display info from the DBstripe_payment_intent_idfrom wire types; updateddonations.post.ts2 — Return 400 for non-integer pagination params
All list handlers were silently swallowing
strconv.Atoiparse errors —?limit=abcwas treated as?limit=0with no feedback to the caller.parsePaginationParamsinrespond.go: returns400 Bad Requestwith an error message whenlimitoroffsetis not a valid integerList/ListForUseracross initiatives, donations, subscriptions) now use itrespond_test.go3 — Normalise money fields to
_centsSome fields used
_in_centsin their JSON tags, others already used_cents. Standardised everything to_cents:Donation.CurrentAmountCentscurrent_amount_in_centsamount_centsDonationCreateInput.AmountCentsamount_in_centsamount_centsSubscription.CurrentAmountCentscurrent_amount_in_centsamount_centsSubscriptionCreateInput.AmountCentsamount_in_centsamount_centsGoalInput.AmountInCentsamount_in_centsamount_centsOSTIFDetailInput/OSTIFDetail.TotalBudgetInCentstotal_budget_in_centstotal_budget_cents4 — Unify transaction pagination to
limit/offsetTransactionListwas usingpage/size(1-based) while every other paginated resource useslimit/offset. Unified end-to-end:TransactionListmodel:page/size→limit/offsetTransactionFilter):Page/Size→Limit/Offset; converts to Ledger's 1-basedpage/perPageinternally — upstream contract unchangedInitiativeService.GetTransactionsandInitiativeHandler.GetTransactionsupdatedtransactions.types.ts(server + shared),useInitiativeTransactionscomposableTest plan
go build ./...andgo vet ./...passpnpm tsc-checkpasses infrontend/GET /v1/initiatives/{id}/donationsno longer includesuser_id,organization_id, or anystripe_*fieldGET /v1/me/donationsstill returns full donation records for the authenticated userGET /v1/initiatives/{id}/donations?limit=abcreturns400with an error messageamount_centsGET /v1/initiatives/{id}/transactions?limit=5&offset=0returns correctlimit/offsetin the response envelope🤖 Generated with Claude Code