Conversation
|
As this PR is large enough as it is i will add integration to the event system in the branch |
theodorklauritzen
requested review from
JohanHjelsethStorstad and
theodorklauritzen
and removed request for
JohanHjelsethStorstad
|
More merge conflicts... 😩 |
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive ledger (money/accounting) system for the application, introducing double-entry bookkeeping, Stripe payment integration, and ledger accounts for users and groups. The implementation builds on top of PR #443 which simplified service method interfaces.
Changes:
- Added complete ledger schema with accounts, transactions, entries, and payment models supporting both Stripe and manual payments
- Implemented double-entry accounting logic with automatic fee calculations and balance tracking
- Integrated Stripe for payment processing, including webhook handling and saved payment method management
- Created UI components for account management, deposits, payouts, and transaction history
- Added test infrastructure with utilities for testing ledger operations
Reviewed changes
Copilot reviewed 101 out of 103 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| src/prisma/schema/ledger.prisma | Core ledger database schema defining accounts, transactions, entries, and payments |
| src/prisma/schema/user.prisma | Added ledger account and Stripe customer relations to users |
| src/prisma/schema/group.prisma | Added ledger account relation to groups |
| src/services/ledger/accounts/operations.ts | Ledger account CRUD operations and balance calculations |
| src/services/ledger/transactions/operations.ts | Transaction creation, state management, and advancement logic |
| src/services/ledger/transactions/calculateFees.ts | Fee calculation formulas for debit and credit entries |
| src/services/ledger/transactions/determineTransactionState.ts | State machine for transaction lifecycle management |
| src/services/ledger/payments/operations.ts | Payment creation and initiation for Stripe and manual payments |
| src/services/ledger/payments/stripeWebhookCallback.ts | Webhook handler for Stripe payment events |
| src/services/ledger/movements/operations.ts | High-level deposit and payout orchestration |
| src/services/stripeCustomers/operations.ts | Stripe customer management and saved payment methods |
| src/lib/currency/convert.ts | Currency conversion utilities replacing old money module |
| src/app/api/stripe-events/route.ts | API route for receiving Stripe webhooks |
| src/app/_components/Ledger/* | UI components for ledger account and transaction management |
| src/app/_components/Stripe/* | Stripe integration components for payments |
| tests/services/ledger/* | Test files for ledger functionality |
| tests/utils.ts | Test utility for handling promise settlement |
| package.json | Added Stripe SDK dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * The lifecycle of the transaction is automatically handled by the system. | ||
| */ | ||
| create: defineOperation({ | ||
| authorizer: () => RequireNothing.staticFields({}).dynamicFields({}), // TODO, |
There was a problem hiding this comment.
Ledger transaction creation also uses RequireNothing authorization. Anyone can create ledger transactions, which is a critical security issue for a financial system.
| }, | ||
| }) | ||
| } else { | ||
| console.error(`Stripe payment is not part of a ledger transaction: ${stripePayment.payment.id}`) |
There was a problem hiding this comment.
The webhook handler uses console.error instead of the logger that's imported and used elsewhere in the file. This inconsistency should be fixed to use the logger for consistent logging behavior.
| const sign = convertedAmount > 0 ? '+' : '-' | ||
|
|
||
| return `${withSign && convertedAmount !== 0 ? sign : ''}${amountString} ${currencySymbol}` |
There was a problem hiding this comment.
The sign logic in displayAmount is incorrect. When convertedAmount is negative, the sign is set to '-', but the amountString already includes the negative sign from toFixed(2). This will result in incorrect output for negative amounts (e.g., "--5.00" instead of "-5.00" when withSign is true). The sign should be based on whether to show a positive sign, and the absolute value should be used for the amountString when withSign is true.
| // If searching by userId/groupId we want to create the account if it doesn't exist. | ||
| // TODO: Is this something we want? | ||
|
|
||
| const account = await prisma.ledgerAccount.findUnique({ | ||
| where: { | ||
| userId: params.userId, | ||
| groupId: params.groupId, | ||
| }, | ||
| }) | ||
|
|
||
| if (account) return account | ||
|
|
||
| return ledgerAccountOperations.create({ session, data: params }) |
There was a problem hiding this comment.
The read operation automatically creates a ledger account if it doesn't exist when searching by userId/groupId (line 103). This behavior is questionable and noted by the TODO comment. Auto-creation during a read operation violates the principle of least surprise and could lead to unintended side effects. Consider separating this into explicit "readOrCreate" and "read" operations, or at least require an explicit flag to enable auto-creation.
| customerId: customer.id, | ||
| }, | ||
| update: { | ||
| // Dont update anything. Let the first created account win. |
There was a problem hiding this comment.
There's a typo in the comment: "Dont" should be "Don't".
| * @returns The created account. | ||
| */ | ||
| create: defineOperation({ | ||
| authorizer: () => RequireNothing.staticFields({}).dynamicFields({}), // TODO: Add proper auther |
There was a problem hiding this comment.
There's a typo in multiple TODO comments: "auther" should be "authorizer". Also, all ledger account operations are using RequireNothing authorization, which means anyone can create, read, update ledger accounts and calculate balances. This is a critical security issue that should be addressed before merging. The PR description mentions tying up loose ends regarding permissions in another PR, but given the financial nature of this feature, authorization should be implemented before merging.
| deleteSavedPaymentMethod: defineOperation({ | ||
| authorizer: () => RequireNothing.staticFields({}).dynamicFields({}), // TODO: This should probably be authed? | ||
| paramsSchema: z.object({ | ||
| paymentMethodId: z.string(), | ||
| }), | ||
| operation: async ({ params: { paymentMethodId } }) => { | ||
| await stripe.paymentMethods.detach(paymentMethodId) | ||
|
|
||
| return { | ||
| success: true | ||
| } | ||
| } | ||
| }), |
There was a problem hiding this comment.
The deleteSavedPaymentMethod operation has no authorization, allowing anyone to delete any payment method by ID. This is a critical security vulnerability. An attacker could potentially delete other users' saved payment methods. This operation must verify that the paymentMethodId belongs to the authenticated user before deleting it.
| test('balance', async () => { | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The ledger accounts test file contains an empty test. This should either be implemented or removed before merging, as empty tests provide no value and can mask missing test coverage.
| try { | ||
| return await stripeWebhookCallback(event) | ||
| } catch { | ||
| return new Response('A server-side error occured.', { status: 500 }) |
There was a problem hiding this comment.
There's a typo in the error message: "occured" should be "occurred".
| try { | ||
| return await stripeWebhookCallback(event) | ||
| } catch { | ||
| return new Response('A server-side error occured.', { status: 500 }) | ||
| } |
There was a problem hiding this comment.
The catch block at line 20 swallows all error information, making debugging difficult. The error should be logged before returning the generic error response. Consider using the logger that's available in the codebase to log the caught error.
1 participant
After more than of year of waiting the ledger (money) system is finally here!* 💵 💸 💰
This includes:
✅ A flexible double entry ledger system for tracking transaction (deposits, payouts, purchases, etc.)
✅ Simple logic to calculate balances (goodbye ginormous sql query)
✅ Uniform ledger account interface between groups and users (no more source and drain accounts)
✅ Payments with Stripe (integrated with webhooks like before)
✅ Card detail saving with Stripe (makes use of new-ish CustomerSessions API for simpler integration)**
This PR is marked as draft until #443 is merged as it builts on top of it. I also have to tie up a few loose ends regarding permissions, although I think it might be best to do that in another PR.I have not tested this fully yet so a few changes might still come.