From 0d4de82a9d6e689044f9cd694b8c68b002a9e524 Mon Sep 17 00:00:00 2001 From: Luca Bernstein Date: Mon, 12 Sep 2022 18:13:29 +0200 Subject: [PATCH] Remove suggestion limit logic (#187) --- README.md | 2 +- bot/config.go | 93 ------------------------------- bot/controller.go | 5 -- db/crud/bot_cache.go | 113 -------------------------------------- db/crud/bot_cache_test.go | 90 ------------------------------ db/migrations/v11.go | 10 ---- helpers/constants.go | 1 - 7 files changed, 1 insertion(+), 313 deletions(-) diff --git a/README.md b/README.md index 5b575c4..67b3022 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ Supports you in keeping track of your beancount transactions also if you need to ## Features and advantages * [x] Quickly record beancount transactions while on-the-go. Start as simple as entering the amount - no boilerplate -* [x] Suggestions for accounts and descriptions used in the past or configured manually (can be limited or turned off) +* [x] Suggestions for accounts and descriptions used in the past or configured manually * [x] Templates with variables and advanced amount splitting for recurring or more complex transactions * [x] Reminder notifications of recorded transactions with flexible schedule * [x] Many optional commands, shorthands and parameters, leaving the full flexibility up to you diff --git a/bot/config.go b/bot/config.go index 311b36f..2aaccdc 100644 --- a/bot/config.go +++ b/bot/config.go @@ -17,7 +17,6 @@ func (bc *BotController) configHandler(m *tb.Message) { Add("currency", bc.configHandleCurrency). Add("tag", bc.configHandleTag). Add("notify", bc.configHandleNotification). - Add("limit", bc.configHandleLimit). Add("about", bc.configHandleAbout). Add("tz_offset", bc.configHandleTimezoneOffset). Add("delete_account", bc.configHandleAccountDelete) @@ -51,11 +50,6 @@ Create a schedule to be notified of open transactions (i.e. not archived or dele /{{.CONFIG_COMMAND}} notify off - Disable reminder notifications /{{.CONFIG_COMMAND}} notify - Notify of open transaction after days at of the day. Honors configured timezone offset (see below) -Set suggestion cache limits (i.e. only cache new values until limit is reached, then old ones get dismissed if new ones are added): - -/{{.CONFIG_COMMAND}} limit - Get currently set cache limits -/{{.CONFIG_COMMAND}} limit |off - Set or disable suggestion limit for a type - Timezone offset from {{.TZ}} to honor for notifications and current date (if set automatically) in new transactions: /{{.CONFIG_COMMAND}} tz_offset - Get current timezone offset from {{.TZ}} (default 0) @@ -225,92 +219,6 @@ func (bc *BotController) configHandleNotification(m *tb.Message, params ...strin bc.configHelp(m, fmt.Errorf("invalid amount of parameters specified")) } -func (bc *BotController) configHandleLimit(m *tb.Message, params ...string) { - if len(params) == 0 { - // GET limits for all types - cacheLimits, err := bc.Repo.CacheUserSettingGetLimits(m) - if err != nil { - bc.Logf(WARN, m, "CacheUserSettingGetLimits failed: %s", err.Error()) - bc.configHelp(m, fmt.Errorf("could not get your cache limits")) - return - } - message := "You have the following cache limits configured:\n" - for limit, value := range cacheLimits { - message += fmt.Sprintf("\n%s: %d", limit, value) - } - message += "\n\n" - message += "If new cache entries are created for the given types, old ones are automatically deleted.\nPlease note: If suggestions were added using /suggestions add, they will not be deleted automatically by this mechanism. '-1' means no limit." - _, err = bc.Bot.Send(Recipient(m), message) - if err != nil { - bc.Logf(ERROR, m, "Sending bot message failed: %s", err.Error()) - } - return - } else if len(params) == 1 { - // GET limit for type - cacheType := params[0] - cacheLimits, err := bc.Repo.CacheUserSettingGetLimits(m) - if err != nil { - bc.Logf(WARN, m, "CacheUserSettingGetLimits failed: %s", err.Error()) - bc.configHelp(m, fmt.Errorf("an application error occurred while retrieving cache limits from database")) - return - } - message := "You have the following cache limit configured for '" + cacheType + "': " + strconv.Itoa(cacheLimits[cacheType]) - message += "\n\n" - message += "If new cache entries are created for the given types, old ones are automatically deleted.\nPlease note: If suggestions were added using /suggestions add, they will not be deleted automatically by this mechanism. '-1' means no limit." - _, err = bc.Bot.Send(Recipient(m), message) - if err != nil { - bc.Logf(ERROR, m, "Sending bot message failed: %s", err.Error()) - } - return - } else if len(params) == 2 { - // SET limit for type - cacheType := params[0] - limitValue := params[1] - - if !helpers.ArrayContains(helpers.AllowedSuggestionTypes(), cacheType) { - bc.configHelp(m, fmt.Errorf("unknown suggestion type: %s. Must be one from: %v", cacheType, helpers.AllowedSuggestionTypes())) - return - } - - limitValueParsed, errParsing := strconv.Atoi(limitValue) - if errParsing != nil { - bc.configHelp(m, fmt.Errorf("an application error occurred while interpreting your amount as a number to set the limit to")) - return - } - - if limitValue == "off" || (errParsing == nil && limitValueParsed < 0) { - err := bc.Repo.CacheUserSettingSetLimit(m, cacheType, -1) - if err != nil { - bc.Logf(ERROR, m, "Error disabling suggestions cache limit: %s", err.Error()) - bc.configHelp(m, fmt.Errorf("an application error occurred while disabling suggestions cache: %s", err.Error())) - return - } - _, err = bc.Bot.Send(Recipient(m), fmt.Sprintf("Successfully disabled suggestions cache limits for type %s", cacheType)) - if err != nil { - bc.Logf(ERROR, m, "Sending bot message failed: %s", err.Error()) - } - } else { - err := bc.Repo.CacheUserSettingSetLimit(m, cacheType, limitValueParsed) - if err != nil { - bc.Logf(ERROR, m, "Error setting suggestions cache limit: %s", err.Error()) - bc.configHelp(m, fmt.Errorf("an application error occurred while setting a new suggestions cache limit: %s", err.Error())) - return - } - _, err = bc.Bot.Send(Recipient(m), fmt.Sprintf("Successfully set suggestions cache limits for type %s to %d", cacheType, limitValueParsed)) - if err != nil { - bc.Logf(ERROR, m, "Sending bot message failed: %s", err.Error()) - } - } - err := bc.Repo.PruneUserCachedSuggestions(m) - if err != nil { - bc.Logf(WARN, m, "Could not prune suggestions: %s", err.Error()) - } - return - } - - bc.configHelp(m, fmt.Errorf("invalid amount of parameters specified")) -} - func (bc *BotController) configHandleAbout(m *tb.Message, params ...string) { if len(params) > 0 { bc.configHelp(m, fmt.Errorf("no parameters expected")) @@ -420,7 +328,6 @@ func (bc *BotController) deleteUserData(m *tb.Message) { errors.handle1(bc.Repo.SetUserSetting(helpers.USERSET_ADM, "", m.Chat.ID)) errors.handle1(bc.Repo.SetUserSetting(helpers.USERSET_CUR, "", m.Chat.ID)) - errors.handle1(bc.Repo.SetUserSetting(helpers.USERSET_LIM_PREFIX, "", m.Chat.ID)) errors.handle1(bc.Repo.SetUserSetting(helpers.USERSET_TAG, "", m.Chat.ID)) errors.handle1(bc.Repo.SetUserSetting(helpers.USERSET_TZOFF, "", m.Chat.ID)) diff --git a/bot/controller.go b/bot/controller.go index ed6ffc9..edfd1c2 100644 --- a/bot/controller.go +++ b/bot/controller.go @@ -687,11 +687,6 @@ func (bc *BotController) finishTransaction(m *tb.Message, tx Tx) { bc.Logf(ERROR, m, "Something went wrong while caching transaction. Error: %s", err.Error()) // Don't return, instead continue flow (if recording was successful) } - err = bc.Repo.PruneUserCachedSuggestions(m) - if err != nil { - bc.Logf(ERROR, m, "Something went wrong while pruning suggestions cache. Error: %s", err.Error()) - // Don't return, instead continue flow (if recording was successful) - } _, err = bc.Bot.Send(Recipient(m), fmt.Sprintf("Successfully recorded your transaction.\n"+ "You can get a list of all your transactions using /%s. "+ diff --git a/db/crud/bot_cache.go b/db/crud/bot_cache.go index efc3b84..d50fecb 100644 --- a/db/crud/bot_cache.go +++ b/db/crud/bot_cache.go @@ -2,9 +2,6 @@ package crud import ( "database/sql" - "fmt" - "strconv" - "strings" "github.com/LucaBernstein/beancount-bot-tg/helpers" tb "gopkg.in/tucnak/telebot.v2" @@ -44,41 +41,6 @@ func (r *Repo) PutCacheHints(m *tb.Message, values map[string]string) error { return r.FillCache(m) } -func (r *Repo) PruneUserCachedSuggestions(m *tb.Message) error { - limits, err := r.CacheUserSettingGetLimits(m) - if err != nil { - return err - } - for key, limit := range limits { - if limit < 0 { - continue - } - // Cleanup automatically added values above cache limit - q := `DELETE FROM "bot::cache" WHERE "tgChatId" = $1 AND "type" = $2` - params := []interface{}{m.Chat.ID, key} - if limit != 0 { - q += ` AND ID NOT IN (SELECT "id" FROM "bot::cache" WHERE "tgChatId" = $1 AND "type" = $2 ORDER BY "lastUsed" DESC LIMIT $3)` - params = append(params, limit) - } - res, err := r.db.Exec(q, params...) - if err != nil { - return err - } - count, err := res.RowsAffected() - if err != nil { - LogDbf(r, helpers.WARN, m, "could not get affected rows count from pruning operation") - } - if count > 0 { - LogDbf(r, helpers.INFO, m, "Pruned %d '%s' suggestions", count, key) - } - } - err = r.FillCache(m) - if err != nil { - LogDbf(r, helpers.WARN, m, "could not get refill cache after pruning") - } - return nil -} - func (r *Repo) GetCacheHints(m *tb.Message, key string) ([]string, error) { if _, exists := CACHE_LOCAL[m.Chat.ID]; !exists { LogDbf(r, helpers.TRACE, m, "No cached data found for chat. Will fill cache first.") @@ -154,78 +116,3 @@ func (r *Repo) DeleteAllCacheEntries(m *tb.Message) error { } return r.FillCache(m) } - -func (r *Repo) CacheUserSettingGetLimits(m *tb.Message) (limits map[string]int, err error) { - userSettingPrefix := helpers.USERSET_LIM_PREFIX - - rows, err := r.db.Query(`SELECT "setting", "value" FROM "bot::userSetting" WHERE "tgChatId" = $1 AND "setting" LIKE $2`, - m.Chat.ID, userSettingPrefix+"%") - if err != nil { - return nil, fmt.Errorf("error selecting userSettingCacheLimits: %s", err.Error()) - } - - allCacheLimits := map[string]int{} - - var setting string - var value string - for rows.Next() { - err = rows.Scan(&setting, &value) - if err != nil { - return nil, err - } - valueI, err := strconv.Atoi(value) - if err != nil { - return nil, fmt.Errorf("could not convert stored limit number to integer: %s", err.Error()) - } - allCacheLimits[strings.TrimPrefix(setting, userSettingPrefix)] = valueI - } - - // Fill with possible but not modified cache limits - for _, key := range helpers.AllowedSuggestionTypes() { - _, exists := allCacheLimits[key] - if !exists { - allCacheLimits[key] = -1 - } - } - - return allCacheLimits, nil -} - -// CacheUserSettingSetLimit deletes probably existing and (re)creates caching limit in userSettings -func (r *Repo) CacheUserSettingSetLimit(m *tb.Message, key string, limit int) (err error) { - if !helpers.ArrayContains(helpers.AllowedSuggestionTypes(), key) { - return fmt.Errorf("the key you provided is invalid. Should be one of the following: %s", helpers.AllowedSuggestionTypes()) - } - - tx, err := r.db.Begin() - defer tx.Rollback() - if err != nil { - return fmt.Errorf("caching userSettingLimit did not work as db tx could not be begun: %s", err.Error()) - } - - compositeKey := helpers.USERSET_LIM_PREFIX + key - - q := `DELETE FROM "bot::userSetting" WHERE "tgChatId" = $1 AND "setting" = $2;` - params := []interface{}{m.Chat.ID, compositeKey} - - _, err = tx.Exec(q, params...) - if err != nil { - return fmt.Errorf("could not delete existing userSetting on set: %s", err.Error()) - } - - if limit >= 0 { - q := `INSERT INTO "bot::userSetting" ("tgChatId", "setting", "value") VALUES ($1, $2, $3);` - params := []interface{}{m.Chat.ID, compositeKey, strconv.Itoa(limit)} - _, err = tx.Exec(q, params...) - if err != nil { - return fmt.Errorf("could not delete existing userSetting on set: %s", err.Error()) - } - } - - err = tx.Commit() - if err != nil { - return fmt.Errorf("could not commit db tx for setting userSetting: %s", err.Error()) - } - - return nil -} diff --git a/db/crud/bot_cache_test.go b/db/crud/bot_cache_test.go index 7a56e92..ff031dd 100644 --- a/db/crud/bot_cache_test.go +++ b/db/crud/bot_cache_test.go @@ -2,13 +2,11 @@ package crud_test import ( "log" - "strings" "testing" "github.com/DATA-DOG/go-sqlmock" "github.com/LucaBernstein/beancount-bot-tg/bot" "github.com/LucaBernstein/beancount-bot-tg/db/crud" - "github.com/LucaBernstein/beancount-bot-tg/helpers" tb "gopkg.in/tucnak/telebot.v2" ) @@ -58,91 +56,3 @@ func TestCacheOnlySuggestible(t *testing.T) { t.Errorf("there were unfulfilled expectations: %s", err) } } - -func TestSetCacheLimit(t *testing.T) { - // create test dependencies - crud.TEST_MODE = true - chat := &tb.Chat{ID: 12345} - db, mock, err := sqlmock.New() - if err != nil { - log.Fatal(err) - } - defer db.Close() - - bc := crud.NewRepo(db) - message := &tb.Message{Chat: chat} - - mock.ExpectBegin() - mock. - ExpectExec(`DELETE FROM "bot::userSetting"`). - WithArgs(chat.ID, helpers.USERSET_LIM_PREFIX+"description"). - WillReturnResult(sqlmock.NewResult(1, 1)) - mock. - ExpectExec(`INSERT INTO "bot::userSetting"`). - WithArgs(chat.ID, helpers.USERSET_LIM_PREFIX+"description", "23"). - WillReturnResult(sqlmock.NewResult(1, 1)) - mock.ExpectCommit() - - err = bc.CacheUserSettingSetLimit(message, "description", 23) - if err != nil { - t.Errorf("CacheUserSettingSetLimit unexpectedly threw an error: %s", err.Error()) - } - - mock.ExpectBegin() - mock. - ExpectExec(`DELETE FROM "bot::userSetting"`). - WithArgs(chat.ID, helpers.USERSET_LIM_PREFIX+"description"). - WillReturnResult(sqlmock.NewResult(1, 1)) - mock.ExpectCommit() - - err = bc.CacheUserSettingSetLimit(message, "description", -1) - if err != nil { - t.Errorf("CacheUserSettingSetLimit (with delete only) unexpectedly threw an error: %s", err.Error()) - } - - err = bc.CacheUserSettingSetLimit(message, "thisCacheKeyDefinitelyIsInvalidAndShouldFail", 5) - if err == nil { - t.Errorf("CacheUserSettingSetLimit should fail for invalid cache key") - } - if !strings.Contains(err.Error(), "key you provided is invalid") { - t.Errorf("CacheUserSettingSetLimit error message should provide further information for failure due to invalid key: %s", err.Error()) - } - - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("there were unfulfilled expectations: %s", err) - } -} - -func TestGetCacheLimit(t *testing.T) { - // create test dependencies - crud.TEST_MODE = true - chat := &tb.Chat{ID: 12345} - db, mock, err := sqlmock.New() - if err != nil { - log.Fatal(err) - } - defer db.Close() - - bc := crud.NewRepo(db) - message := &tb.Message{Chat: chat} - - mock. - ExpectQuery(`SELECT "setting", "value" FROM "bot::userSetting"`). - WithArgs(chat.ID, helpers.USERSET_LIM_PREFIX+"%"). - WillReturnRows(sqlmock.NewRows([]string{"setting", "value"}).AddRow(helpers.USERSET_LIM_PREFIX+"description", "79")) - - limits, err := bc.CacheUserSettingGetLimits(message) - if err != nil { - t.Errorf("TestSetCacheLimitGet unexpectedly threw an error: %s", err.Error()) - } - if len(limits) != 2 { - t.Errorf("TestSetCacheLimitGet limit result was unexpected") - } - if limits["description"] != 79 { - t.Errorf("TestSetCacheLimitGet should return values correctly: %d", limits["txDesc"]) - } - - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("there were unfulfilled expectations: %s", err) - } -} diff --git a/db/migrations/v11.go b/db/migrations/v11.go index c50ce07..6b01c5f 100644 --- a/db/migrations/v11.go +++ b/db/migrations/v11.go @@ -83,14 +83,4 @@ func v11RemoveUserSettingTypesLimits(db *sql.Tx) { if err != nil { log.Fatal(err) } - - sqlStatement = ` - INSERT INTO "bot::userSettingTypes" - ("setting", "description") - VALUES ('user.limitCache', 'limit cached value count for transactions. Array by type.');; - ` - _, err = db.Exec(sqlStatement) - if err != nil { - log.Fatal(err) - } } diff --git a/helpers/constants.go b/helpers/constants.go index 23655a7..d407d52 100644 --- a/helpers/constants.go +++ b/helpers/constants.go @@ -19,7 +19,6 @@ const ( USERSET_CUR = "user.currency" USERSET_ADM = "user.isAdmin" USERSET_TAG = "user.vacationTag" - USERSET_LIM_PREFIX = "user.limitCache." USERSET_TZOFF = "user.tzOffset" DEFAULT_CURRENCY = "EUR"