refactor(billing): extract transaction reporting into its own service - #3502
Closed
ygrishajev wants to merge 1 commit into
Closed
refactor(billing): extract transaction reporting into its own service#3502ygrishajev wants to merge 1 commit into
ygrishajev wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughTransaction reporting moves from ChangesTransaction reporting extraction
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
Move getCustomerTransactions and the CSV export stream out of StripeService into a dedicated TransactionReportingService, and delegate to it from the controller. Behavior-preserving: the reporting tests move alongside the new service and StripeService sheds its CSV/reporting dependencies.
ygrishajev
force-pushed
the
refactor/billing-transaction-reporting-service-1
branch
from
July 22, 2026 12:45
13a9ee3 to
d2c9ba0
Compare
baktun14
approved these changes
Jul 22, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/billing/services/transaction-reporting/transaction-reporting.service.spec.ts`:
- Around line 473-505: Add a test alongside “handles error during streaming
gracefully” that makes the initial getCustomerTransactions call reject, then
consume exportTransactionsCsvStream and assert the CSV contains the expected
headers and generic “Error: unable to fetch transactions” message without
exposing the raw Stripe error. Keep the existing second-page error coverage
unchanged.
In
`@apps/api/src/billing/services/transaction-reporting/transaction-reporting.service.ts`:
- Around line 129-179: Update the transaction-fetch catch path in the reporting
generator so an emitted fetch-error row marks the export as having yielded a
result, preventing the trailing “No transactions found” row from being emitted
after a first-page failure. Also guard the pagination update after
getCustomerTransactions so a hasMore response without a nextPage terminates
iteration instead of refetching the same page indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6895c69-5b6c-42b1-8339-ada8ffbf695a
📒 Files selected for processing (6)
apps/api/src/billing/controllers/stripe/stripe.controller.spec.tsapps/api/src/billing/controllers/stripe/stripe.controller.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/services/stripe/stripe.service.tsapps/api/src/billing/services/transaction-reporting/transaction-reporting.service.spec.tsapps/api/src/billing/services/transaction-reporting/transaction-reporting.service.ts
Comment on lines
+473
to
+505
| it("handles error during streaming gracefully", async () => { | ||
| const { service } = setup(); | ||
| const mockTransaction = createTestTransaction(); | ||
|
|
||
| vi.spyOn(service, "getCustomerTransactions") | ||
| .mockResolvedValueOnce({ | ||
| transactions: [mockTransaction], | ||
| hasMore: true, | ||
| nextPage: "ch_123", | ||
| prevPage: null | ||
| }) | ||
| .mockRejectedValueOnce(new Error("Stripe API error")); | ||
|
|
||
| const csvStream = service.exportTransactionsCsvStream(TEST_CONSTANTS.CUSTOMER_ID, { | ||
| startDate: "2022-01-01T00:00:00Z", | ||
| endDate: "2022-01-31T23:59:59Z", | ||
| timezone: "America/New_York" | ||
| }); | ||
|
|
||
| const chunks: string[] = []; | ||
| for await (const chunk of csvStream) { | ||
| chunks.push(chunk); | ||
| } | ||
|
|
||
| const fullCsv = chunks.join(""); | ||
|
|
||
| expect(fullCsv).toContain( | ||
| "Transaction ID,Date (America/New_York),Amount,Bonus,Currency,Status,Payment Method,Card Brand,Card Last 4,Description,Receipt URL" | ||
| ); | ||
| expect(fullCsv).toContain("ch_123"); | ||
| expect(fullCsv).toContain("Error: unable to fetch transactions"); | ||
| expect(fullCsv).not.toContain("Stripe API error"); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Solid coverage, and good that the error test asserts the raw Stripe message isn't leaked into the CSV. Note this suite only exercises an error on the second page — the first-page-error edge (flagged in transaction-reporting.service.ts) is untested. Worth adding once that fix lands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/api/src/billing/services/transaction-reporting/transaction-reporting.service.spec.ts`
around lines 473 - 505, Add a test alongside “handles error during streaming
gracefully” that makes the initial getCustomerTransactions call reject, then
consume exportTransactionsCsvStream and assert the CSV contains the expected
headers and generic “Error: unable to fetch transactions” message without
exposing the raw Stripe error. Keep the existing second-page error coverage
unchanged.
Comment on lines
+129
to
+179
| while (hasMore) { | ||
| try { | ||
| const batch = await this.getCustomerTransactions(customerId, { | ||
| limit: batchSize, | ||
| startingAfter, | ||
| startDate: options.startDate, | ||
| endDate: options.endDate | ||
| }); | ||
|
|
||
| for (const transaction of batch.transactions) { | ||
| hasYieldedAny = true; | ||
|
|
||
| yield this.transformTransactionForCsv(transaction, options.timezone); | ||
| } | ||
|
|
||
| hasMore = batch.hasMore; | ||
| startingAfter = batch.nextPage || undefined; | ||
| } catch (error) { | ||
| this.loggerService.error({ event: "TRANSACTION_FETCH_ERROR", error, customerId, startingAfter }); | ||
| yield { | ||
| id: this.sanitizeForCsv("Error: unable to fetch transactions"), | ||
| date: "", | ||
| amount: "", | ||
| bonusAmount: "", | ||
| currency: "", | ||
| status: "", | ||
| paymentMethodType: "", | ||
| cardBrand: "", | ||
| cardLast4: "", | ||
| description: "", | ||
| receiptUrl: "" | ||
| }; | ||
| hasMore = false; | ||
| } | ||
| } | ||
|
|
||
| if (!hasYieldedAny) { | ||
| yield { | ||
| id: "No transactions found for the specified date range", | ||
| date: "", | ||
| amount: "", | ||
| bonusAmount: "", | ||
| currency: "", | ||
| status: "", | ||
| paymentMethodType: "", | ||
| cardBrand: "", | ||
| cardLast4: "", | ||
| description: "", | ||
| receiptUrl: "" | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
First-page fetch error emits a contradictory "no transactions" row. The catch block never sets hasYieldedAny = true. If the very first getCustomerTransactions call throws, you yield the "Error: unable to fetch transactions" row, exit the loop, and then the trailing if (!hasYieldedAny) also yields "No transactions found for the specified date range". The exported CSV then reports both an error and "no transactions" simultaneously. The existing test only exercises an error on the second page (page 1 succeeds), so it misses this.
🐛 Proposed fix
} catch (error) {
this.loggerService.error({ event: "TRANSACTION_FETCH_ERROR", error, customerId, startingAfter });
+ hasYieldedAny = true;
yield {
id: this.sanitizeForCsv("Error: unable to fetch transactions"),Separately, consider a no-progress guard: if batch.hasMore is ever true while batch.nextPage is null, startingAfter stays undefined and the same page is refetched indefinitely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (hasMore) { | |
| try { | |
| const batch = await this.getCustomerTransactions(customerId, { | |
| limit: batchSize, | |
| startingAfter, | |
| startDate: options.startDate, | |
| endDate: options.endDate | |
| }); | |
| for (const transaction of batch.transactions) { | |
| hasYieldedAny = true; | |
| yield this.transformTransactionForCsv(transaction, options.timezone); | |
| } | |
| hasMore = batch.hasMore; | |
| startingAfter = batch.nextPage || undefined; | |
| } catch (error) { | |
| this.loggerService.error({ event: "TRANSACTION_FETCH_ERROR", error, customerId, startingAfter }); | |
| yield { | |
| id: this.sanitizeForCsv("Error: unable to fetch transactions"), | |
| date: "", | |
| amount: "", | |
| bonusAmount: "", | |
| currency: "", | |
| status: "", | |
| paymentMethodType: "", | |
| cardBrand: "", | |
| cardLast4: "", | |
| description: "", | |
| receiptUrl: "" | |
| }; | |
| hasMore = false; | |
| } | |
| } | |
| if (!hasYieldedAny) { | |
| yield { | |
| id: "No transactions found for the specified date range", | |
| date: "", | |
| amount: "", | |
| bonusAmount: "", | |
| currency: "", | |
| status: "", | |
| paymentMethodType: "", | |
| cardBrand: "", | |
| cardLast4: "", | |
| description: "", | |
| receiptUrl: "" | |
| }; | |
| } | |
| while (hasMore) { | |
| try { | |
| const batch = await this.getCustomerTransactions(customerId, { | |
| limit: batchSize, | |
| startingAfter, | |
| startDate: options.startDate, | |
| endDate: options.endDate | |
| }); | |
| for (const transaction of batch.transactions) { | |
| hasYieldedAny = true; | |
| yield this.transformTransactionForCsv(transaction, options.timezone); | |
| } | |
| hasMore = batch.hasMore; | |
| startingAfter = batch.nextPage || undefined; | |
| } catch (error) { | |
| this.loggerService.error({ event: "TRANSACTION_FETCH_ERROR", error, customerId, startingAfter }); | |
| hasYieldedAny = true; | |
| yield { | |
| id: this.sanitizeForCsv("Error: unable to fetch transactions"), | |
| date: "", | |
| amount: "", | |
| bonusAmount: "", | |
| currency: "", | |
| status: "", | |
| paymentMethodType: "", | |
| cardBrand: "", | |
| cardLast4: "", | |
| description: "", | |
| receiptUrl: "" | |
| }; | |
| hasMore = false; | |
| } | |
| } | |
| if (!hasYieldedAny) { | |
| yield { | |
| id: "No transactions found for the specified date range", | |
| date: "", | |
| amount: "", | |
| bonusAmount: "", | |
| currency: "", | |
| status: "", | |
| paymentMethodType: "", | |
| cardBrand: "", | |
| cardLast4: "", | |
| description: "", | |
| receiptUrl: "" | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/api/src/billing/services/transaction-reporting/transaction-reporting.service.ts`
around lines 129 - 179, Update the transaction-fetch catch path in the reporting
generator so an emitted fetch-error row marks the export as having yielded a
result, preventing the trailing “No transactions found” row from being emitted
after a first-page failure. Also guard the pagination update after
getCustomerTransactions so a hasMore response without a nextPage terminates
iteration instead of refetching the same page indefinitely.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3502 +/- ##
==========================================
- Coverage 73.45% 72.37% -1.09%
==========================================
Files 1134 1044 -90
Lines 29606 27190 -2416
Branches 7416 6933 -483
==========================================
- Hits 21748 19678 -2070
+ Misses 6932 6616 -316
+ Partials 926 896 -30
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
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
2 participants
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.
Why
StripeServiceis still a god object — charging cards, coupons, customers, payment methods, and transaction reporting all live in one class. Reporting (listing a customer's charges for the UI, streaming the CSV export) is a self-contained concern with its own dependencies —csv-stringify, a paging generator, timezone/CSV formatting — that has nothing to do with taking a payment.This PR carves reporting out into a dedicated
TransactionReportingService.StripeServicesheds ~200 lines and its CSV/stream imports; reporting gets a single-responsibility, independently testable home. It's a pure move — no behavior changes — and the next step in the layeredStripeServicesplit.Closes CON-721 · Part of CON-718. Stacked on #3498 (the
StripeServiceinjected-client refactor) — review and merge that first; this PR's diff should be read on top of it.What
apps/api— behavior-preserving, no new runtime behavior, no migrations.TransactionReportingService(injectedSTRIPE_CLIENT+StripeTransactionRepository+ logger). It ownsgetCustomerTransactionsandexportTransactionsCsvStream, plus their private helpers (paging generator, CSV-row transform, timezone normalization, CSV-injection sanitization) — all moved verbatim fromStripeService.StripeControllernow delegates the two reporting endpoints toTransactionReportingService; every other endpoint still goes throughStripeService.StripeServicedrops the six reporting methods and the now-deadcsv-stringify/stream/Transaction/TransactionCsvRowimports.stripe.servicespec into a newtransaction-reporting.servicespec, unchanged.Verification
tsc: no new errors in the changed files (pre-existing apps/api baseline unchanged).Summary by CodeRabbit
New Features
Bug Fixes