From f6a873ffc80e1c4242125eccfb234d5fe88f2918 Mon Sep 17 00:00:00 2001 From: juancwu Date: Mon, 4 May 2026 15:06:12 +0000 Subject: [PATCH] fix: transfer exceeding available balance allowed --- internal/app/app.go | 1 + internal/handler/space.go | 5 + internal/service/transaction.go | 35 ++++++- internal/service/transaction_test.go | 148 ++++++++++++++++++++++----- 4 files changed, 160 insertions(+), 29 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 58f32b5..7bff685 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -66,6 +66,7 @@ func New(cfg *config.Config) (*App, error) { allocationService.SetAuditLogger(auditLogService) transactionService := service.NewTransactionService(transactionRepository, categoryRepository, accountService) transactionService.SetAuditLogger(txAuditLogService) + transactionService.SetAllocationService(allocationService) accountActivityService := service.NewAccountActivityService(auditLogService, txAuditLogService) emailService := service.NewEmailService( emailClient, diff --git a/internal/handler/space.go b/internal/handler/space.go index fa19015..b9ab270 100644 --- a/internal/handler/space.go +++ b/internal/handler/space.go @@ -1842,6 +1842,11 @@ func (h *spaceHandler) HandleCreateTransfer(w http.ResponseWriter, r *http.Reque Description: descriptionInput, ActorID: actorID, }); err != nil { + if errors.Is(err, service.ErrTransferExceedsAvailable) { + formProps.AmountErr = "Amount exceeds the available balance for this account." + ui.Render(w, r, forms.CreateTransfer(formProps)) + return + } slog.Error("failed to create transfer", "error", err, "source", accountID, "dest", destInput) formProps.GeneralErr = "Something went wrong. Please try again." ui.Render(w, r, forms.CreateTransfer(formProps)) diff --git a/internal/service/transaction.go b/internal/service/transaction.go index e497efb..06c7fdb 100644 --- a/internal/service/transaction.go +++ b/internal/service/transaction.go @@ -19,11 +19,18 @@ import ( // instead. var ErrTransactionPartOfTransfer = errors.New("transaction is part of a transfer") +// ErrTransferExceedsAvailable is returned when a transfer would move more +// funds out of the source than its available balance (balance minus +// allocations). Bills can still overdraft, but transfers must respect what +// has already been allocated to other purposes. +var ErrTransferExceedsAvailable = errors.New("transfer amount exceeds available balance") + type TransactionService struct { - transactionRepo repository.TransactionRepository - categoryRepo repository.CategoryRepository - accountService *AccountService - auditSvc *TransactionAuditLogService + transactionRepo repository.TransactionRepository + categoryRepo repository.CategoryRepository + accountService *AccountService + allocationService *AllocationService + auditSvc *TransactionAuditLogService } func NewTransactionService( @@ -43,6 +50,13 @@ func (s *TransactionService) SetAuditLogger(audit *TransactionAuditLogService) { s.auditSvc = audit } +// SetAllocationService wires the allocation service so transfers can enforce +// the available-balance constraint. Wired after construction to break the +// circular dependency between allocation and transaction services. +func (s *TransactionService) SetAllocationService(alloc *AllocationService) { + s.allocationService = alloc +} + type PayBillInput struct { AccountID string Title string @@ -239,6 +253,19 @@ func (s *TransactionService) Transfer(input TransferInput) (*TransferResult, err return nil, fmt.Errorf("failed to load destination account: %w", err) } + // Transfers must respect allocations on the source. A transfer is the user + // committing funds elsewhere — if the unallocated cash isn't there, the + // transfer can't happen. (Bills are still allowed to overdraft.) + if s.allocationService != nil { + summary, err := s.allocationService.SummaryForAccount(source.ID) + if err != nil { + return nil, fmt.Errorf("failed to load source allocations: %w", err) + } + if input.Amount.GreaterThan(summary.Available) { + return nil, ErrTransferExceedsAvailable + } + } + // Cross-currency transfers require a conversion rate; same-currency // transfers ignore it (or, for symmetry, accept rate=1). destAmount := input.Amount diff --git a/internal/service/transaction_test.go b/internal/service/transaction_test.go index 75e18e6..3126a51 100644 --- a/internal/service/transaction_test.go +++ b/internal/service/transaction_test.go @@ -29,12 +29,16 @@ func newTxnFixture(t *testing.T, dbi testutil.DBInfo) *txnFixture { txnRepo := repository.NewTransactionRepository(dbi.DB) categoryRepo := repository.NewCategoryRepository(dbi.DB) accountRepo := repository.NewAccountRepository(dbi.DB) + allocationRepo := repository.NewAllocationRepository(dbi.DB) auditRepo := repository.NewTransactionAuditLogRepository(dbi.DB) accountSvc := NewAccountService(accountRepo) + accountSvc.SetAllocationRepository(allocationRepo) + allocationSvc := NewAllocationService(allocationRepo, accountSvc) auditSvc := NewTransactionAuditLogService(auditRepo) svc := NewTransactionService(txnRepo, categoryRepo, accountSvc) svc.SetAuditLogger(auditSvc) + svc.SetAllocationService(allocationSvc) user := testutil.CreateTestUser(t, dbi.DB, t.Name()+"@example.com", nil) space := testutil.CreateTestSpace(t, dbi.DB, user.ID, "S") @@ -251,6 +255,13 @@ func TestTransactionService_Transfer_HappyPath(t *testing.T) { f := newTxnFixture(t, dbi) dest := testutil.CreateTestAccount(t, dbi.DB, f.account.SpaceID, "Savings") + // Seed source so the transfer respects the available-balance constraint. + _, err := f.svc.Deposit(DepositInput{ + AccountID: f.account.ID, Title: "seed", Amount: decimal.NewFromInt(50), + OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + result, err := f.svc.Transfer(TransferInput{ SourceAccountID: f.account.ID, DestAccountID: dest.ID, @@ -267,10 +278,9 @@ func TestTransactionService_Transfer_HappyPath(t *testing.T) { assert.Equal(t, f.account.ID, result.Withdrawal.AccountID) assert.Equal(t, dest.ID, result.Deposit.AccountID) - // Source went from 0 → -50 (overdraft allowed); dest 0 → +50. src, err := f.accounts.ByID(f.account.ID) require.NoError(t, err) - assert.True(t, decimal.NewFromInt(-50).Equal(src.Balance)) + assert.True(t, decimal.Zero.Equal(src.Balance)) dst, err := f.accounts.ByID(dest.ID) require.NoError(t, err) assert.True(t, decimal.NewFromInt(50).Equal(dst.Balance)) @@ -301,12 +311,12 @@ func TestTransactionService_Transfer_HappyPath(t *testing.T) { }) } -func TestTransactionService_Transfer_AllowsOverdraft(t *testing.T) { +func TestTransactionService_Transfer_RejectsOverdraft(t *testing.T) { testutil.ForEachDB(t, func(t *testing.T, dbi testutil.DBInfo) { f := newTxnFixture(t, dbi) dest := testutil.CreateTestAccount(t, dbi.DB, f.account.SpaceID, "B") - // Seed source to 100, then transfer 200 → -100. + // Seed source to 100, then attempt to transfer 200 — must be refused. _, err := f.svc.Deposit(DepositInput{ AccountID: f.account.ID, Title: "seed", Amount: decimal.NewFromInt(100), OccurredAt: time.Now(), ActorID: f.user.ID, @@ -316,22 +326,15 @@ func TestTransactionService_Transfer_AllowsOverdraft(t *testing.T) { SourceAccountID: f.account.ID, DestAccountID: dest.ID, Title: "T1", Amount: decimal.NewFromInt(200), OccurredAt: time.Now(), ActorID: f.user.ID, }) - require.NoError(t, err) + require.ErrorIs(t, err, ErrTransferExceedsAvailable) + // Balances are untouched. src, err := f.accounts.ByID(f.account.ID) require.NoError(t, err) - assert.True(t, decimal.NewFromInt(-100).Equal(src.Balance), "expected -100, got %s", src.Balance.String()) - - // Transfer another 200 from -100 → -300. - _, err = f.svc.Transfer(TransferInput{ - SourceAccountID: f.account.ID, DestAccountID: dest.ID, - Title: "T2", Amount: decimal.NewFromInt(200), OccurredAt: time.Now(), ActorID: f.user.ID, - }) + assert.True(t, decimal.NewFromInt(100).Equal(src.Balance)) + dst, err := f.accounts.ByID(dest.ID) require.NoError(t, err) - - src, err = f.accounts.ByID(f.account.ID) - require.NoError(t, err) - assert.True(t, decimal.NewFromInt(-300).Equal(src.Balance), "expected -300, got %s", src.Balance.String()) + assert.True(t, decimal.Zero.Equal(dst.Balance)) }) } @@ -351,6 +354,13 @@ func TestTransactionService_Transfer_AppearsInActivityFeeds(t *testing.T) { NewTransactionAuditLogService(f.txAudit), ) + // Seed source so the transfer respects the available-balance constraint. + _, err := f.svc.Deposit(DepositInput{ + AccountID: f.account.ID, Title: "seed", Amount: decimal.NewFromInt(75), + OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + result, err := f.svc.Transfer(TransferInput{ SourceAccountID: f.account.ID, DestAccountID: dest.ID, @@ -361,10 +371,11 @@ func TestTransactionService_Transfer_AppearsInActivityFeeds(t *testing.T) { }) require.NoError(t, err) - // Source account activity sees the withdrawal half. + // Source account activity sees the withdrawal half (newest first; the + // seed deposit appears below). srcRows, err := activitySvc.List(f.account.ID, 10, 0) require.NoError(t, err) - require.Len(t, srcRows, 1, "source account feed should include the withdrawal half") + require.Len(t, srcRows, 2, "source account feed should include the seed deposit and the withdrawal half") require.NotNil(t, srcRows[0].TxLog) assert.Equal(t, result.Withdrawal.ID, srcRows[0].TxLog.TransactionID) assertTransferRole(t, srcRows[0].TxLog, "source", dest.ID) @@ -377,10 +388,10 @@ func TestTransactionService_Transfer_AppearsInActivityFeeds(t *testing.T) { assert.Equal(t, result.Deposit.ID, dstRows[0].TxLog.TransactionID) assertTransferRole(t, dstRows[0].TxLog, "destination", f.account.ID) - // Space-level activity feed sees both halves. + // Space-level activity feed sees both transfer halves alongside the seed. spaceRows, err := activitySvc.ListSpace(f.account.SpaceID, 10, 0) require.NoError(t, err) - require.Len(t, spaceRows, 2, "space feed should include both halves of the transfer") + require.Len(t, spaceRows, 3, "space feed should include the seed deposit and both transfer halves") ids := []string{} for _, r := range spaceRows { require.NotNil(t, r.TxLog) @@ -392,15 +403,13 @@ func TestTransactionService_Transfer_AppearsInActivityFeeds(t *testing.T) { // Counts agree with what the feed returns (pagination relies on this). srcCount, err := activitySvc.Count(f.account.ID) require.NoError(t, err) - assert.Equal(t, 1, srcCount) + assert.Equal(t, 2, srcCount) dstCount, err := activitySvc.Count(dest.ID) require.NoError(t, err) assert.Equal(t, 1, dstCount) spaceCount, err := activitySvc.CountSpace(f.account.SpaceID) require.NoError(t, err) - // Source account had no activity before the transfer, dest is brand-new; - // the only activity in the space is the two transfer halves. - assert.Equal(t, 2, spaceCount) + assert.Equal(t, 3, spaceCount) }) } @@ -444,6 +453,12 @@ func TestTransactionService_Update_RejectsTransferTransactions(t *testing.T) { f := newTxnFixture(t, dbi) dest := testutil.CreateTestAccount(t, dbi.DB, f.account.SpaceID, "Savings") + _, err := f.svc.Deposit(DepositInput{ + AccountID: f.account.ID, Title: "seed", Amount: decimal.NewFromInt(20), + OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + result, err := f.svc.Transfer(TransferInput{ SourceAccountID: f.account.ID, DestAccountID: dest.ID, @@ -572,6 +587,12 @@ func TestTransactionService_DeleteTransaction_RejectsTransferHalves(t *testing.T f := newTxnFixture(t, dbi) dest := testutil.CreateTestAccount(t, dbi.DB, f.account.SpaceID, "Savings") + _, err := f.svc.Deposit(DepositInput{ + AccountID: f.account.ID, Title: "seed", Amount: decimal.NewFromInt(20), + OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + result, err := f.svc.Transfer(TransferInput{ SourceAccountID: f.account.ID, DestAccountID: dest.ID, @@ -602,7 +623,7 @@ func TestTransactionService_DeleteTransaction_RejectsTransferHalves(t *testing.T src, err := f.accounts.ByID(f.account.ID) require.NoError(t, err) - assert.True(t, decimal.NewFromInt(-20).Equal(src.Balance)) + assert.True(t, decimal.Zero.Equal(src.Balance)) dst, err := f.accounts.ByID(dest.ID) require.NoError(t, err) assert.True(t, decimal.NewFromInt(20).Equal(dst.Balance)) @@ -662,6 +683,83 @@ func TestTransactionService_DeleteTransaction_RequiresID(t *testing.T) { }) } +func TestTransactionService_Transfer_RejectsExceedingAvailable(t *testing.T) { + testutil.ForEachDB(t, func(t *testing.T, dbi testutil.DBInfo) { + f := newTxnFixture(t, dbi) + dest := testutil.CreateTestAccount(t, dbi.DB, f.account.SpaceID, "Savings") + allocRepo := repository.NewAllocationRepository(dbi.DB) + allocSvc := NewAllocationService(allocRepo, NewAccountService(repository.NewAccountRepository(dbi.DB))) + allocSvc.SetAuditLogger(NewSpaceAuditLogService(repository.NewSpaceAuditLogRepository(dbi.DB))) + + // Source: balance 100, allocate 60 → available 40. + _, err := f.svc.Deposit(DepositInput{ + AccountID: f.account.ID, Title: "seed", Amount: decimal.NewFromInt(100), + OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + _, err = allocSvc.Create(CreateAllocationInput{ + AccountID: f.account.ID, Name: "Rent", Amount: decimal.NewFromInt(60), ActorID: f.user.ID, + }) + require.NoError(t, err) + + // 50 > 40 available, must be refused. + _, err = f.svc.Transfer(TransferInput{ + SourceAccountID: f.account.ID, DestAccountID: dest.ID, + Title: "Too much", Amount: decimal.NewFromInt(50), OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.ErrorIs(t, err, ErrTransferExceedsAvailable) + + // Balances untouched. + src, err := f.accounts.ByID(f.account.ID) + require.NoError(t, err) + assert.True(t, decimal.NewFromInt(100).Equal(src.Balance)) + dst, err := f.accounts.ByID(dest.ID) + require.NoError(t, err) + assert.True(t, decimal.Zero.Equal(dst.Balance)) + + // 40 (== available) is allowed. + _, err = f.svc.Transfer(TransferInput{ + SourceAccountID: f.account.ID, DestAccountID: dest.ID, + Title: "Exact", Amount: decimal.NewFromInt(40), OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + + src, err = f.accounts.ByID(f.account.ID) + require.NoError(t, err) + assert.True(t, decimal.NewFromInt(60).Equal(src.Balance)) + dst, err = f.accounts.ByID(dest.ID) + require.NoError(t, err) + assert.True(t, decimal.NewFromInt(40).Equal(dst.Balance)) + }) +} + +func TestTransactionService_Transfer_AllowsUpToAvailable_NoAllocations(t *testing.T) { + testutil.ForEachDB(t, func(t *testing.T, dbi testutil.DBInfo) { + f := newTxnFixture(t, dbi) + dest := testutil.CreateTestAccount(t, dbi.DB, f.account.SpaceID, "Savings") + + // With no allocations, available == balance, so transferring the full + // balance is allowed but transferring more is not. + _, err := f.svc.Deposit(DepositInput{ + AccountID: f.account.ID, Title: "seed", Amount: decimal.NewFromInt(30), + OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + + _, err = f.svc.Transfer(TransferInput{ + SourceAccountID: f.account.ID, DestAccountID: dest.ID, + Title: "Over", Amount: decimal.NewFromInt(31), OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.ErrorIs(t, err, ErrTransferExceedsAvailable) + + _, err = f.svc.Transfer(TransferInput{ + SourceAccountID: f.account.ID, DestAccountID: dest.ID, + Title: "Full", Amount: decimal.NewFromInt(30), OccurredAt: time.Now(), ActorID: f.user.ID, + }) + require.NoError(t, err) + }) +} + func TestTransactionService_Validations(t *testing.T) { testutil.ForEachDB(t, func(t *testing.T, dbi testutil.DBInfo) { f := newTxnFixture(t, dbi)