Skip to content

Commit 3118ad5

Browse files
morremeyermorremeyer
authored
fix: ensure that transfers do not have an envelope set (#755)
This resolves #613.
1 parent fcbf2fe commit 3118ad5

File tree

5 files changed

+212
-0
lines changed

5 files changed

+212
-0
lines changed

‎pkg/controllers/transaction.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ func (co Controller) checkTransaction(c *gin.Context, transaction models.Transac
9797
// Check envelope being set for transfer between on-budget accounts
9898
if transaction.EnvelopeID != nil {
9999
if source.OnBudget && destination.OnBudget {
100+
// TODO: Verify this state in the model hooks
100101
return httperrors.ErrorStatus{Err: errors.New("transfers between two on-budget accounts must not have an envelope set. Such a transaction would be incoming and outgoing for this envelope at the same time, which is not possible"), Status: http.StatusBadRequest}
101102
}
102103
_, err := getResourceByID[models.Envelope](c, co, *transaction.EnvelopeID)

‎pkg/models/account.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package models
22

33
import (
44
"fmt"
5+
"strings"
56
"time"
67

78
"github.com/envelope-zero/backend/v3/internal/types"
@@ -73,6 +74,35 @@ func (a *Account) links(tx *gorm.DB) {
7374
a.Links.Transactions = fmt.Sprintf("%s/v1/transactions?account=%s", url, a.ID)
7475
}
7576

77+
// BeforeUpdate verifies the state of the account before
78+
// committing an update to the database.
79+
func (a *Account) BeforeUpdate(tx *gorm.DB) (err error) {
80+
// Account is being set to be on budget, verify that no transactions
81+
// with this account as destination has an envelope set
82+
if tx.Statement.Changed("OnBudget") && !a.OnBudget {
83+
var transactions []Transaction
84+
err = tx.Model(&Transaction{}).
85+
Joins("JOIN accounts ON transactions.source_account_id = accounts.id").
86+
Where("destination_account_id = ? AND accounts.on_budget AND envelope_id not null", a.ID).
87+
Find(&transactions).Error
88+
if err != nil {
89+
return
90+
}
91+
92+
if len(transactions) > 0 {
93+
strs := make([]string, len(transactions))
94+
for i, t := range transactions {
95+
strs[i] = t.ID.String()
96+
}
97+
98+
ids := strings.Join(strs, ",")
99+
return fmt.Errorf("the account cannot be set to on budget because the following transactions have an envelope set: %s", ids)
100+
}
101+
}
102+
103+
return nil
104+
}
105+
76106
// BeforeSave sets OnBudget to false when External is true.
77107
func (a *Account) BeforeSave(_ *gorm.DB) (err error) {
78108
if a.External {

‎pkg/models/account_test.go‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,3 +304,99 @@ func (suite *TestSuiteStandard) TestAccountDuplicateNames() {
304304

305305
suite.Assert().Contains(err.Error(), "UNIQUE constraint failed: accounts.name, accounts.budget_id", "Error message for account creation fail does not match expected message")
306306
}
307+
308+
func (suite *TestSuiteStandard) TestAccountOnBudgetToOnBudgetTransactionsNoEnvelopes() {
309+
budget := suite.createTestBudget(models.BudgetCreate{
310+
Name: "TestAccountOnBudgetToOnBudgetTransactionsNoEnvelopes",
311+
})
312+
313+
account := suite.createTestAccount(models.AccountCreate{
314+
BudgetID: budget.ID,
315+
OnBudget: true,
316+
External: false,
317+
Name: "TestAccountOnBudgetToOnBudgetTransactionsNoEnvelopes",
318+
})
319+
320+
transferTargetAccount := suite.createTestAccount(models.AccountCreate{
321+
BudgetID: budget.ID,
322+
OnBudget: false,
323+
External: false,
324+
Name: "TestAccountOnBudgetToOnBudgetTransactionsNoEnvelopes:Target",
325+
})
326+
327+
category := suite.createTestCategory(models.CategoryCreate{
328+
BudgetID: budget.ID,
329+
})
330+
331+
envelope := suite.createTestEnvelope(models.EnvelopeCreate{
332+
CategoryID: category.ID,
333+
})
334+
335+
t := suite.createTestTransaction(models.TransactionCreate{
336+
Amount: decimal.NewFromFloat(17.23),
337+
BudgetID: budget.ID,
338+
SourceAccountID: account.ID,
339+
DestinationAccountID: transferTargetAccount.ID,
340+
EnvelopeID: &envelope.ID,
341+
})
342+
343+
// Try saving the account, which must fail
344+
data := models.Account{AccountCreate: models.AccountCreate{OnBudget: true}}
345+
err := suite.db.Model(&transferTargetAccount).Select("OnBudget").Updates(data).Error
346+
347+
if !assert.NotNil(suite.T(), err, "Target account could be updated to be on budget while having transactions with envelopes being set") {
348+
assert.FailNow(suite.T(), "Exiting because assertion was not met")
349+
}
350+
assert.Contains(suite.T(), err.Error(), "the account cannot be set to on budget because the following transactions have an envelope set: ")
351+
352+
// Update the envelope for the transaction
353+
t.EnvelopeID = nil
354+
err = suite.db.Save(&t).Error
355+
assert.Nil(suite.T(), err, "Transaction could not be updated")
356+
357+
// Save again
358+
err = suite.db.Save(&transferTargetAccount).Error
359+
assert.Nil(suite.T(), err, "Target account could not be updated despite transaction having its envelope removed")
360+
}
361+
362+
func (suite *TestSuiteStandard) TestAccountOffBudgetToOnBudgetTransactionsNoEnvelopes() {
363+
budget := suite.createTestBudget(models.BudgetCreate{
364+
Name: "TestAccountOffBudgetToOnBudgetTransactionsNoEnvelopes",
365+
})
366+
367+
account := suite.createTestAccount(models.AccountCreate{
368+
BudgetID: budget.ID,
369+
OnBudget: false,
370+
External: false,
371+
Name: "TestAccountOffBudgetToOnBudgetTransactionsNoEnvelopes",
372+
})
373+
374+
transferTargetAccount := suite.createTestAccount(models.AccountCreate{
375+
BudgetID: budget.ID,
376+
OnBudget: false,
377+
External: false,
378+
Name: "TestAccountOffBudgetToOnBudgetTransactionsNoEnvelopes:Target",
379+
})
380+
381+
category := suite.createTestCategory(models.CategoryCreate{
382+
BudgetID: budget.ID,
383+
})
384+
385+
envelope := suite.createTestEnvelope(models.EnvelopeCreate{
386+
CategoryID: category.ID,
387+
})
388+
389+
_ = suite.createTestTransaction(models.TransactionCreate{
390+
Amount: decimal.NewFromFloat(17.23),
391+
BudgetID: budget.ID,
392+
SourceAccountID: account.ID,
393+
DestinationAccountID: transferTargetAccount.ID,
394+
EnvelopeID: &envelope.ID,
395+
})
396+
397+
// Try saving the account, which must work
398+
data := models.Account{AccountCreate: models.AccountCreate{OnBudget: true}}
399+
err := suite.db.Model(&transferTargetAccount).Select("OnBudget").Updates(data).Error
400+
401+
assert.Nil(suite.T(), err, "Target account could not be updated to be on budget, but it does not have transactions with envelopes being set")
402+
}

‎pkg/models/database.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ func Migrate(db *gorm.DB) (err error) {
2626
return fmt.Errorf("error during DB migration: %w", err)
2727
}
2828

29+
// Migration for https://github.com/envelope-zero/backend/issues/613
30+
// Remove with 4.0.0
31+
err = unsetEnvelopes(db)
32+
if err != nil {
33+
return fmt.Errorf("error during unsetEnvelopes: %w", err)
34+
}
35+
2936
return nil
3037
}
3138

@@ -66,3 +73,35 @@ func migrateDuplicateAccountNames(db *gorm.DB) (err error) {
6673

6774
return nil
6875
}
76+
77+
// unsetEnvelopes removes the envelopes from transfers between
78+
// accounts that are on budget.
79+
func unsetEnvelopes(db *gorm.DB) (err error) {
80+
var accounts []Account
81+
err = db.Where(&Account{AccountCreate: AccountCreate{
82+
OnBudget: true,
83+
}}).Find(&accounts).Error
84+
if err != nil {
85+
return
86+
}
87+
88+
for _, a := range accounts {
89+
var transactions []Transaction
90+
err = db.Model(&Transaction{}).
91+
Joins("JOIN accounts ON transactions.source_account_id = accounts.id").
92+
Where("destination_account_id = ? AND accounts.on_budget AND envelope_id not null", a.ID).
93+
Find(&transactions).Error
94+
if err != nil {
95+
return
96+
}
97+
98+
for _, t := range transactions {
99+
err = db.Model(&t).Select("EnvelopeID").Updates(map[string]interface{}{"EnvelopeID": nil}).Error
100+
if err != nil {
101+
return
102+
}
103+
}
104+
}
105+
106+
return
107+
}

‎pkg/models/database_test.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package models_test
22

33
import (
44
"github.com/envelope-zero/backend/v3/pkg/models"
5+
"github.com/shopspring/decimal"
56
)
67

78
func (suite *TestSuiteStandard) TestMigrate() {
@@ -46,3 +47,48 @@ func (suite *TestSuiteStandard) TestMigrateDuplicateAccountNames() {
4647
err = models.Migrate(suite.db)
4748
suite.Assert().Nil(err)
4849
}
50+
51+
func (suite *TestSuiteStandard) TestUnsetEnvelope() {
52+
// Initialize the database to have all tables
53+
err := suite.db.AutoMigrate()
54+
suite.Assert().Nil(err, err)
55+
56+
budget := suite.createTestBudget(models.BudgetCreate{})
57+
sourceAccount := suite.createTestAccount(models.AccountCreate{
58+
BudgetID: budget.ID,
59+
Name: "TestUnsetEnvelope: Source",
60+
OnBudget: true,
61+
})
62+
63+
destinationAccount := suite.createTestAccount(models.AccountCreate{
64+
BudgetID: budget.ID,
65+
Name: "TestUnsetEnvelope: Destination",
66+
OnBudget: true,
67+
})
68+
69+
envelope := suite.createTestEnvelope(models.EnvelopeCreate{
70+
CategoryID: suite.createTestCategory(models.CategoryCreate{BudgetID: budget.ID}).ID,
71+
Name: "TestUnsetEnvelope: Envelope",
72+
})
73+
74+
transaction := suite.createTestTransaction(models.TransactionCreate{
75+
BudgetID: budget.ID,
76+
SourceAccountID: sourceAccount.ID,
77+
DestinationAccountID: destinationAccount.ID,
78+
Amount: decimal.NewFromFloat(17.36),
79+
Note: "This can only be created for this test - the controllers prevent creating this already",
80+
EnvelopeID: &envelope.ID,
81+
})
82+
83+
// Execute the migration again
84+
err = models.Migrate(suite.db)
85+
suite.Assert().Nil(err)
86+
87+
// Reload the transaction
88+
var checkTransaction models.Transaction
89+
err = suite.db.First(&checkTransaction, transaction.ID).Error
90+
suite.Assert().Nil(err)
91+
92+
// Test thet the envelope has been set to nil by the migration
93+
suite.Assert().Nil(checkTransaction.EnvelopeID)
94+
}

0 commit comments

Comments
 (0)