feat: implement standing order feature with creation, listing, and deletion commands#56
feat: implement standing order feature with creation, listing, and deletion commands#56mrcentimetre wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces standing order functionality, enabling users to schedule automatic recurring transfers to savings pots on specific calendar days. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as StandingOrderScheduler
participant DB as Database
participant User as User Service
participant Bot as TipBot
participant Telegram as Telegram API
loop Periodic Check
Scheduler->>DB: Fetch active standing orders
DB-->>Scheduler: Orders list
alt For Each Due Order
Note over Scheduler: Check day-of-month<br/>(with month-end clamp)
Note over Scheduler: Verify not executed today
Scheduler->>DB: Load associated user
DB-->>Scheduler: User record
alt User Valid & Has Wallet
Scheduler->>Bot: TransferToPot (amount, pot_name)
Bot->>DB: Execute transfer
DB-->>Bot: Success
Bot-->>Scheduler: Success
Scheduler->>DB: Update LastExecutedAt
DB-->>Scheduler: Updated
Scheduler->>Telegram: Send success notification
Telegram-->>Scheduler: Sent
else User Banned/No Wallet
Scheduler->>Telegram: Send failure notification
Telegram-->>Scheduler: Sent
end
else Order Not Due
Note over Scheduler: Skip to next order
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/telegram/standing_order_scheduler.go (1)
26-35: Make the scheduler loop cancellable.This goroutine runs forever, so
GracefulShutdown()cannot stop or wait for it. A stop signal plustime.Tickerwould make the new background worker behave predictably during shutdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/telegram/standing_order_scheduler.go` around lines 26 - 35, The scheduler goroutine currently loops forever (Start -> run -> processDueOrders) so GracefulShutdown cannot stop it; change Start and run to use a cancellable mechanism (context.Context or a stop channel) and replace time.Sleep with time.Ticker so the loop selects on ticker.C and the cancel/stop signal. Add a Stop or Shutdown method on StandingOrderScheduler that cancels the context or closes the stop channel, and ensure run returns when the cancel/stop is received; keep processDueOrders unchanged but call it from the select when ticker fires, using the existing CheckInterval for the ticker duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/telegram/standing_order_scheduler.go`:
- Around line 97-106: executeOrder currently calls s.bot.TransferToPot then
updates order.LastExecutedAt with s.bot.DB.Users.Save(order), which can fail and
cause duplicate transfers or unintended INSERTs; wrap the transfer plus
timestamp update in a single atomic DB transaction (or perform an UPDATE-only
statement) so both succeed or both roll back: start a transaction on
s.bot.DB.Users (or higher-level DB handle), set LastExecutedAt, call
s.bot.TransferToPot inside that transaction, and then perform an UPDATE that
only updates the existing record (not Save) to persist LastExecutedAt (or
rollback and return error if any step fails); key symbols to modify:
StandingOrderScheduler.executeOrder, s.bot.TransferToPot, s.bot.DB.Users.Save
(replace with tx and UPDATE-only), and ensure
processDueOrders/shouldExecuteToday semantics remain based on the DB-updated
LastExecutedAt.
In `@internal/telegram/standing_orders.go`:
- Around line 29-32: The current code maps any error from bot.GetPot(user,
potName) to a “create it first” message; instead, preserve non-not-found errors
by checking whether the returned error is the not-found sentinel and only
translate that case. Change the error handling around bot.GetPot(user, potName)
to: if err != nil { if errors.Is(err, pots.ErrNotFound) { return nil,
fmt.Errorf("pot '%s' not found — create it first with /createpot", potName) }
return nil, err } (ensure you import errors and reference the same ErrNotFound
used by GetPot).
- Around line 56-59: The ListStandingOrders function sorts only by day_of_month
which can make ordering non-deterministic for multiple orders on the same day;
change the query in ListStandingOrders (the
bot.DB.Users.Where(...).Order(...).Find(&orders) call) to include deterministic
secondary sorts such as "created_at ASC" and "id ASC" (e.g., Order("day_of_month
ASC, created_at ASC, id ASC")) so the index from the displayed list matches the
index used by `/so delete <number>`.
- Around line 34-37: The Count() call's error is not being checked, so if
bot.DB.Users.Model(&lnbits.StandingOrder{}).Where(...).Count(&orderCount) fails
the code will incorrectly treat orderCount as zero; update the block to capture
and check the query error (from the Count call's .Error) before enforcing
MaxStandingOrdersPerUser and return that error (or wrap it) instead of
proceeding to the limit check; adjust the logic around the orderCount variable
and the MaxStandingOrdersPerUser guard accordingly.
---
Nitpick comments:
In `@internal/telegram/standing_order_scheduler.go`:
- Around line 26-35: The scheduler goroutine currently loops forever (Start ->
run -> processDueOrders) so GracefulShutdown cannot stop it; change Start and
run to use a cancellable mechanism (context.Context or a stop channel) and
replace time.Sleep with time.Ticker so the loop selects on ticker.C and the
cancel/stop signal. Add a Stop or Shutdown method on StandingOrderScheduler that
cancels the context or closes the stop channel, and ensure run returns when the
cancel/stop is received; keep processDueOrders unchanged but call it from the
select when ticker fires, using the existing CheckInterval for the ticker
duration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a678fc0-aec9-4f1b-adb7-eab23c8c3440
📒 Files selected for processing (7)
.gitignoreinternal/lnbits/types.gointernal/telegram/bot.gointernal/telegram/database.gointernal/telegram/handler.gointernal/telegram/standing_order_scheduler.gointernal/telegram/standing_orders.go
| func (s *StandingOrderScheduler) executeOrder(order *lnbits.StandingOrder, user *lnbits.User) error { | ||
| if err := s.bot.TransferToPot(user, order.PotName, order.Amount); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| now := time.Now() | ||
| order.LastExecutedAt = &now | ||
| if err := s.bot.DB.Users.Save(order).Error; err != nil { | ||
| log.Errorf("[StandingOrderScheduler] Failed to update LastExecutedAt for order %s: %v", order.ID, err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the TransferToPot function in pots.go
fd "pots.go" internal/telegramRepository: CeyLabs/BitcoinDeepaBot
Length of output: 92
🏁 Script executed:
# Get the executeOrder method to see the full context
sed -n '97,106p' internal/telegram/standing_order_scheduler.goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 479
🏁 Script executed:
# Check the TransferToPot function implementation
sed -n '95,131p' internal/telegram/pots.goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 1374
🏁 Script executed:
# Check the standing order deletion logic mentioned
sed -n '71,80p' internal/telegram/standing_orders.goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 401
🏁 Script executed:
# Search for any database transaction patterns in the codebase
rg -A 5 "\.WithTxn\|\.Transaction\|BeginTx" internal/telegram --type goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 49
🏁 Script executed:
# Check the Save method implementation to understand its behavior
rg -B 5 -A 10 "func.*Save.*order" internal/telegram --type goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 49
🏁 Script executed:
# Find where executeOrder is called
rg -B 3 -A 3 "executeOrder" internal/telegram --type goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 1072
🏁 Script executed:
# Check the scheduler logic to understand execution frequency and retry behavior
rg -B 10 -A 10 "hourly\|cron\|ticker" internal/telegram/standing_order_scheduler.go --type goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 49
🏁 Script executed:
# Check if there's any wrapping transaction in the scheduler
sed -n '1,150p' internal/telegram/standing_order_scheduler.go | head -80Repository: CeyLabs/BitcoinDeepaBot
Length of output: 2439
🏁 Script executed:
# Look at the full standing_order_scheduler.go to understand the context
wc -l internal/telegram/standing_order_scheduler.goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 116
🏁 Script executed:
# Get the full standing_order_scheduler.go to see the complete flow
cat -n internal/telegram/standing_order_scheduler.go | sed -n '50,127p'Repository: CeyLabs/BitcoinDeepaBot
Length of output: 3243
🏁 Script executed:
# Check what the Save method does - search for the database model and Save implementation
rg -B 2 -A 15 "func.*\(.*gorm\.DB.*\).*Save" internal --type go | head -50Repository: CeyLabs/BitcoinDeepaBot
Length of output: 49
🏁 Script executed:
# Check if there are any other places where standing orders are deleted or updated
rg "StandingOrder" internal/telegram --type go -lRepository: CeyLabs/BitcoinDeepaBot
Length of output: 204
🏁 Script executed:
# Check the actual risk: does Save overwrite with stale data?
rg -B 5 -A 10 "\.Save\(" internal/telegram --type go | grep -A 10 "order"Repository: CeyLabs/BitcoinDeepaBot
Length of output: 1924
🏁 Script executed:
# Check GORM Save behavior - look for how it handles deleted records
rg -B 5 -A 15 "type StandingOrder struct" internal/lnbits --type goRepository: CeyLabs/BitcoinDeepaBot
Length of output: 1192
🏁 Script executed:
# Check if there's a primary key or how Save identifies records
rg "ID.*primary\|gorm.*tag" internal/lnbits/models.go --type go | head -20Repository: CeyLabs/BitcoinDeepaBot
Length of output: 132
Wrap transfer and execution state in a single transaction.
The transfer completes successfully, but if Save(order) fails, the LastExecutedAt timestamp is never persisted—only logged. Since processDueOrders() reads fresh orders from the database each hour and shouldExecuteToday() checks the database LastExecutedAt, a failed save causes the same order to re-execute on the next hourly check, triggering a duplicate transfer. Additionally, GORM's Save() will INSERT a new record if the primary key is missing; if the order is concurrently deleted, Save() can recreate it. Both the transfer and timestamp update must be atomic, either within a transaction wrapping both operations or via an UPDATE-only approach that fails atomically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_order_scheduler.go` around lines 97 - 106,
executeOrder currently calls s.bot.TransferToPot then updates
order.LastExecutedAt with s.bot.DB.Users.Save(order), which can fail and cause
duplicate transfers or unintended INSERTs; wrap the transfer plus timestamp
update in a single atomic DB transaction (or perform an UPDATE-only statement)
so both succeed or both roll back: start a transaction on s.bot.DB.Users (or
higher-level DB handle), set LastExecutedAt, call s.bot.TransferToPot inside
that transaction, and then perform an UPDATE that only updates the existing
record (not Save) to persist LastExecutedAt (or rollback and return error if any
step fails); key symbols to modify: StandingOrderScheduler.executeOrder,
s.bot.TransferToPot, s.bot.DB.Users.Save (replace with tx and UPDATE-only), and
ensure processDueOrders/shouldExecuteToday semantics remain based on the
DB-updated LastExecutedAt.
| potName = strings.TrimSpace(potName) | ||
| if _, err := bot.GetPot(user, potName); err != nil { | ||
| return nil, fmt.Errorf("pot '%s' not found — create it first with /createpot", potName) | ||
| } |
There was a problem hiding this comment.
Preserve non-not-found errors from pot lookup.
GetPot in internal/telegram/pots.go:82-93 already distinguishes “not found” from real storage failures. Mapping every error here to “create it first” will mislead users whenever the lookup failed for an operational reason.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_orders.go` around lines 29 - 32, The current code
maps any error from bot.GetPot(user, potName) to a “create it first” message;
instead, preserve non-not-found errors by checking whether the returned error is
the not-found sentinel and only translate that case. Change the error handling
around bot.GetPot(user, potName) to: if err != nil { if errors.Is(err,
pots.ErrNotFound) { return nil, fmt.Errorf("pot '%s' not found — create it first
with /createpot", potName) } return nil, err } (ensure you import errors and
reference the same ErrNotFound used by GetPot).
| var orderCount int64 | ||
| bot.DB.Users.Model(&lnbits.StandingOrder{}).Where("user_id = ? AND active = true", user.ID).Count(&orderCount) | ||
| if orderCount >= MaxStandingOrdersPerUser { | ||
| return nil, fmt.Errorf("maximum number of standing orders reached (%d)", MaxStandingOrdersPerUser) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/telegram/standing_orders.go | sed -n '30,45p'Repository: CeyLabs/BitcoinDeepaBot
Length of output: 774
🏁 Script executed:
rg "\.Count\(" --type go -A 2 -B 2 | head -50Repository: CeyLabs/BitcoinDeepaBot
Length of output: 1510
🏁 Script executed:
rg "Where.*Count" --type go -A 3 -B 1 | head -60Repository: CeyLabs/BitcoinDeepaBot
Length of output: 1358
Handle the count query error before enforcing the limit.
If the Count() call on line 35 fails, orderCount remains zero and the max-order guard is skipped. The codebase already demonstrates the correct error-handling pattern elsewhere (e.g., internal/api/send.go), where .Count(&count).Error is checked. Apply the same pattern here:
Suggested change
var orderCount int64
-bot.DB.Users.Model(&lnbits.StandingOrder{}).Where("user_id = ? AND active = true", user.ID).Count(&orderCount)
+if err := bot.DB.Users.Model(&lnbits.StandingOrder{}).
+ Where("user_id = ? AND active = true", user.ID).
+ Count(&orderCount).Error; err != nil {
+ return nil, fmt.Errorf("failed to count standing orders: %w", err)
+}
if orderCount >= MaxStandingOrdersPerUser {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var orderCount int64 | |
| bot.DB.Users.Model(&lnbits.StandingOrder{}).Where("user_id = ? AND active = true", user.ID).Count(&orderCount) | |
| if orderCount >= MaxStandingOrdersPerUser { | |
| return nil, fmt.Errorf("maximum number of standing orders reached (%d)", MaxStandingOrdersPerUser) | |
| var orderCount int64 | |
| if err := bot.DB.Users.Model(&lnbits.StandingOrder{}). | |
| Where("user_id = ? AND active = true", user.ID). | |
| Count(&orderCount).Error; err != nil { | |
| return nil, fmt.Errorf("failed to count standing orders: %w", err) | |
| } | |
| if orderCount >= MaxStandingOrdersPerUser { | |
| return nil, fmt.Errorf("maximum number of standing orders reached (%d)", MaxStandingOrdersPerUser) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_orders.go` around lines 34 - 37, The Count()
call's error is not being checked, so if
bot.DB.Users.Model(&lnbits.StandingOrder{}).Where(...).Count(&orderCount) fails
the code will incorrectly treat orderCount as zero; update the block to capture
and check the query error (from the Count call's .Error) before enforcing
MaxStandingOrdersPerUser and return that error (or wrap it) instead of
proceeding to the limit check; adjust the logic around the orderCount variable
and the MaxStandingOrdersPerUser guard accordingly.
| func (bot *TipBot) ListStandingOrders(user *lnbits.User) ([]lnbits.StandingOrder, error) { | ||
| var orders []lnbits.StandingOrder | ||
| err := bot.DB.Users.Where("user_id = ? AND active = true", user.ID).Order("day_of_month ASC").Find(&orders).Error | ||
| return orders, err |
There was a problem hiding this comment.
Make the listing order stable before deleting by number.
/so delete <number> re-runs ListStandingOrders() and deletes by index. Sorting only by day_of_month means two same-day orders can swap positions between the list and delete calls, so the wrong order can be removed. Add a deterministic secondary sort such as created_at, id.
Suggested change
-err := bot.DB.Users.Where("user_id = ? AND active = true", user.ID).Order("day_of_month ASC").Find(&orders).Error
+err := bot.DB.Users.
+ Where("user_id = ? AND active = true", user.ID).
+ Order("day_of_month ASC, created_at ASC, id ASC").
+ Find(&orders).Error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (bot *TipBot) ListStandingOrders(user *lnbits.User) ([]lnbits.StandingOrder, error) { | |
| var orders []lnbits.StandingOrder | |
| err := bot.DB.Users.Where("user_id = ? AND active = true", user.ID).Order("day_of_month ASC").Find(&orders).Error | |
| return orders, err | |
| func (bot *TipBot) ListStandingOrders(user *lnbits.User) ([]lnbits.StandingOrder, error) { | |
| var orders []lnbits.StandingOrder | |
| err := bot.DB.Users. | |
| Where("user_id = ? AND active = true", user.ID). | |
| Order("day_of_month ASC, created_at ASC, id ASC"). | |
| Find(&orders).Error | |
| return orders, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_orders.go` around lines 56 - 59, The
ListStandingOrders function sorts only by day_of_month which can make ordering
non-deterministic for multiple orders on the same day; change the query in
ListStandingOrders (the bot.DB.Users.Where(...).Order(...).Find(&orders) call)
to include deterministic secondary sorts such as "created_at ASC" and "id ASC"
(e.g., Order("day_of_month ASC, created_at ASC, id ASC")) so the index from the
displayed list matches the index used by `/so delete <number>`.
1 participant
As per title...
Summary by CodeRabbit
New Features
/socommand to create, list, and delete standing orders with validation and user-facing error messages.Chores
.gitignoreto exclude macOS metadata files.