From 993b1ac184e1969af6c962eff511ef87168d0625 Mon Sep 17 00:00:00 2001
From: Morre <morre@mor.re>
Date: Sun, 24 Dec 2023 14:05:44 +0100
Subject: [PATCH] fix: income at correct position in recommended envelopes

This fixes an issue where income was not at the correct position in the
list of recommended envelopes as per the frequency it ocurrs with in the
recent transactions.

You will now see income being at the correct position in the recommended envelopes list.
---
 pkg/models/account.go      |  5 +++--
 pkg/models/account_test.go | 36 +++++++++++++++++++++++-------------
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/pkg/models/account.go b/pkg/models/account.go
index 3d34505b..20b2f912 100644
--- a/pkg/models/account.go
+++ b/pkg/models/account.go
@@ -183,7 +183,7 @@ func (a Account) RecentEnvelopes(db *gorm.DB) ([]*uuid.UUID, error) {
 	latest := db.
 		Model(&Transaction{}).
 		Joins("LEFT JOIN envelopes ON envelopes.id = transactions.envelope_id AND envelopes.deleted_at IS NULL").
-		Select("envelopes.id as id, datetime(envelopes.created_at) as created").
+		Select("envelopes.id as e_id, datetime(envelopes.created_at) as created").
 		Where(&Transaction{
 			TransactionCreate: TransactionCreate{
 				DestinationAccountID: a.ID,
@@ -195,7 +195,8 @@ func (a Account) RecentEnvelopes(db *gorm.DB) ([]*uuid.UUID, error) {
 	// Group by frequency
 	err := db.
 		Table("(?)", latest).
-		Select("id").
+		// Set the nil UUID as ID if the envelope ID is NULL, since count() only counts non-null values
+		Select("IIF(e_id IS NOT NULL, e_id, '00000000-0000-0000-0000-000000000000') as id").
 		Group("id").
 		Order("count(id) DESC").
 		Order("created ASC").
diff --git a/pkg/models/account_test.go b/pkg/models/account_test.go
index 51f8f3da..aa13b98b 100644
--- a/pkg/models/account_test.go
+++ b/pkg/models/account_test.go
@@ -308,7 +308,7 @@ func (suite *TestSuiteStandard) TestAccountRecentEnvelopes() {
 		// Sleep for 10 milliseconds because we only save timestamps with 1 millisecond accuracy
 		// This is needed because the test runs so fast that all envelopes are sometimes created
 		// within the same millisecond, making the result non-deterministic
-		time.Sleep(1 * time.Second)
+		time.Sleep(1 * time.Millisecond)
 	}
 
 	// Create 15 transactions:
@@ -329,14 +329,24 @@ func (suite *TestSuiteStandard) TestAccountRecentEnvelopes() {
 		})
 	}
 
-	// Create one income transaction
-	_ = suite.createTestTransaction(models.TransactionCreate{
-		BudgetID:             budget.ID,
-		EnvelopeID:           nil,
-		SourceAccountID:      externalAccount.ID,
-		DestinationAccountID: account.ID,
-		Amount:               decimal.NewFromFloat(1337.42),
-	})
+	// Create three income transactions
+	//
+	// This is a regression test for income always showing at the last
+	// position in the recent envelopes (before the LIMIT) since count(id) for
+	// income was always 0. This is due to the envelope ID for income being NULL
+	// and count() not counting NULL values.
+	//
+	// Creating three income transactions puts "income" as the second most common
+	// envelope, verifying the fix
+	for i := 0; i < 3; i++ {
+		_ = suite.createTestTransaction(models.TransactionCreate{
+			BudgetID:             budget.ID,
+			EnvelopeID:           nil,
+			SourceAccountID:      externalAccount.ID,
+			DestinationAccountID: account.ID,
+			Amount:               decimal.NewFromFloat(1337.42),
+		})
+	}
 
 	ids, err := account.RecentEnvelopes(suite.db)
 	if err != nil {
@@ -351,9 +361,9 @@ func (suite *TestSuiteStandard) TestAccountRecentEnvelopes() {
 	// has been the most common one
 	suite.Assert().Equal(envelopeIDs[2], ids[0])
 
-	// Order for envelopes with the same frequency is undefined
-
-	// Income is the last one since it only appears once
+	// Income is the second one since it appears three times
 	var nilUUIDPointer *uuid.UUID
-	suite.Assert().Equal(nilUUIDPointer, ids[3])
+	suite.Assert().Equal(nilUUIDPointer, ids[1])
+
+	// Order for envelopes with the same frequency is undefined
 }