feat(recurrence): phase 4a — skip next + cancel series on delivery - #26
Conversation
Recurring reminders had no way to say "I'm going on vacation, skip the next one" or "I'm done with this series entirely" from the delivery notification itself. The only escape was /cancel <short_id> which needs the user to type or copy the short id. Phase 4a adds two buttons to the delivery keyboard when reminder.recurrence is set. Renderer: - Add `delivery_recurring_keyboard(short_id, next_day_time_label)` that keeps the three snooze buttons and appends a row with "跳過下次" and "取消整個系列". The existing one-off keyboard is unchanged, so ordinary reminders are not affected. - Add `render_skipped_next` / `render_series_cancelled` used to overwrite the delivery message once the user picks either action. Scheduler: - Pick between the two keyboards based on `reminder.recurrence`. Recurring reminders get the extended keyboard; one-off keeps the three-button variant. Service: - `skip_next_occurrence(chat_id, short_id, actor, now)` runs `next_fire(rule, current_remind_at)` to compute the occurrence *after* the currently scheduled one, then calls `repository.advance_pending` (new) to update remind_at while status is still PENDING. Non-recurring reminders return None so the caller can surface a clear "not applicable" answer. - `cancel_series(chat_id, short_id, actor)` delegates to the existing `repository.cancel` which already enforces the creator guard; the callback layer only needs to reply and repaint. Repository: - `advance_pending` mirrors `snooze` (creator guard, PENDING → new remind_at) but keeps status PENDING and does not touch fired_at. Semantically different from `reschedule` (which is scheduler-owned and transitions FIRING → PENDING). Callback layer: - New `skip_next` and `cancel_series` handlers. Both edit the delivery message in place with cleared keyboard so the same row cannot be triggered twice. Tests: - Service: skip advances one iteration (7/18 → 7/25), skip crosses month boundary (7/25 → 8/1), non-recurring rejected, non-creator rejected, cancel_series marks CANCELLED, non-creator rejected on cancel too. - Scheduler: recurring delivery uses the extended keyboard; one-off delivery does not. Generated with AI Co-Authored-By: AI <ai@example.com>
c8fadf1 to
c5c955e
Compare
There was a problem hiding this comment.
Pull request overview
Phase 4a for recurring reminders: adds delivery-time actions to skip the next occurrence and cancel the whole series, with UI buttons only shown for recurring reminders and no behavior change for one-off reminders.
Changes:
- Add
skip_next_occurrenceandcancel_seriesservice APIs, wiring them into Telegram callback handlers and response editing. - Introduce
advance_pendingrepository/storage operation to move a PENDING reminder’sremind_atforward without changing status. - Update scheduler delivery keyboard selection (recurring vs one-off) and add tests for service + scheduler keyboard behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_service.py | Adds service integration tests for skip-next and cancel-series behaviors/guards. |
| tests/test_scheduler.py | Verifies recurring deliveries show extra buttons; one-off deliveries do not. |
| src/remindly/storage/sqlite.py | Adds advance_pending SQLite implementation to advance remind_at on PENDING reminders. |
| src/remindly/reminders/service.py | Implements skip_next_occurrence (via next_fire) and cancel_series. |
| src/remindly/reminders/scheduler.py | Chooses recurring vs snooze-only delivery keyboard based on recurrence. |
| src/remindly/reminders/repositories.py | Extends repository protocol with advance_pending. |
| src/remindly/reminders/renderer.py | Adds “skipped next” / “series cancelled” renderers and recurring delivery keyboard. |
| src/remindly/bot/response_sender.py | Adds response helpers to edit delivery messages after skip/cancel actions. |
| src/remindly/bot/callback_handlers.py | Registers and handles skip_next / cancel_series callbacks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| connection.execute( | ||
| """ | ||
| update reminders | ||
| set remind_at = ?, updated_at = ? | ||
| where id = ? and status = ? | ||
| """, | ||
| (next_at.isoformat(), now.isoformat(), row["id"], ReminderStatus.PENDING.value), | ||
| ) |
There was a problem hiding this comment.
Fixed in f3dfdcb. advance_pending now checks result.rowcount after the UPDATE and returns None when it is not exactly 1. That covers the race with scheduler.claim_due changing status PENDING → FIRING between the SELECT (via _pending_for_actor) and the guarded UPDATE. Regression test test_skip_next_returns_none_when_reminder_no_longer_pending asserts the guard by pre-marking the reminder as FIRING and confirming skip_next_occurrence returns None with remind_at unchanged.
| if not result: | ||
| self._client.answer_callback_query( | ||
| context.callback.id, "找不到可跳過的週期性提醒" | ||
| ) |
There was a problem hiding this comment.
Fixed in f3dfdcb. Broadened the callback error to '無法跳過：提醒不存在、非週期性，或你不是建立者' so it no longer misleads users when the actual reason is not the recurrence check — skip_next_occurrence also returns None for missing reminder, non-creator, and the race case that was covered by the sibling fix above.
advance_pending() ran SELECT then UPDATE in the same connection but never checked whether the UPDATE actually hit a row. If scheduler claim_due changed status from PENDING to FIRING between the two statements (different connection / worker thread), the update would be 0 rows affected while the method still returned a "success" Reminder object with the new remind_at — DB and return value out of sync, and the calling flow (skip_next callback) would repaint the delivery card as if the skip succeeded. Check result.rowcount and return None when it is not exactly 1. Also expand the callback error message so users are not misled when the reason is not "not recurring" — the skip flow also returns None for missing reminder, non-creator, and the race case. Regression test: mark reminder as FIRING before calling skip_next_occurrence; assert None is returned and the stored remind_at is unchanged. Generated with AI Co-Authored-By: AI <ai@example.com>
| self._client.answer_callback_query( | ||
| context.callback.id, "無法跳過：提醒不存在、非週期性，或你不是建立者" | ||
| ) |
There was a problem hiding this comment.
Fixed in 579af81. _skip_next error is now '無法跳過：提醒不存在、非週期性、處理中，或你不是建立者' — explicitly includes the PENDING → FIRING race case introduced by the round-1 rowcount guard.
| self._client.answer_callback_query( | ||
| context.callback.id, "找不到可取消的提醒，或你不是建立者" | ||
| ) |
There was a problem hiding this comment.
Fixed in 579af81. _cancel_series error is now '無法取消：提醒不存在、處理中，或你不是建立者' — symmetric with the skip fix. cancel_series delegates to repository.cancel which only accepts PENDING, so a click during the fire window is now surfaced accurately instead of blaming permissions.
| return { | ||
| "inline_keyboard": [ | ||
| [ | ||
| { | ||
| "text": "10 分鐘後", |
There was a problem hiding this comment.
Fixed in 579af81. delivery_recurring_keyboard now delegates to delivery_snooze_keyboard(short_id, next_day_time_label) for the three base rows and only appends the phase-4a row on top. Any future edit to snooze button text / order / callback payload propagates automatically.
| def render_skipped_next(self, reminder: Reminder) -> str: | ||
| return "\n".join( | ||
| [ | ||
| f"已跳過下次 {html_escape(reminder.short_id)}", | ||
| f"下次：{format_datetime(reminder.remind_at)}", | ||
| f"事項：{html_escape(reminder.title)}", | ||
| ] | ||
| ) | ||
|
|
||
| def render_series_cancelled(self, reminder: Reminder) -> str: | ||
| return "\n".join( | ||
| [ | ||
| f"已取消整個系列 {html_escape(reminder.short_id)}", | ||
| f"事項：{html_escape(reminder.title)}", | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Fixed in 579af81. Added RenderSkippedNextTest and RenderSeriesCancelledTest in tests/test_renderer.py covering: short_id + title present, next-time line shown for skipped_next (and absent for series_cancelled), HTML special-char escaping (<script> / <b>) for both. Coverage now matches the other render_* helpers.
Follow-ups from the phase-4a review: 1. `_skip_next` error text now covers the "在處理中" case that was introduced by the round-1 rowcount guard. Users clicking right at the fire moment (PENDING → FIRING race) will no longer be told they lack permission or that the reminder does not exist. 2. `_cancel_series` error text has the same problem: `cancel_series` delegates to `repository.cancel` which only accepts PENDING, so a click during the fire window returned None with a misleading "not creator / not found" message. Broaden it symmetrically to include "在處理中". 3. `delivery_recurring_keyboard` was copy-pasting the three snooze button rows from `delivery_snooze_keyboard`, which would drift the next time either side edits button text, ordering, or callback payload. Delegate to `delivery_snooze_keyboard` for the base rows and only append the phase-4a row on top. Behaviour is unchanged, verified by the existing scheduler keyboard tests. 4. Add missing renderer tests for `render_skipped_next` and `render_series_cancelled` — they are the primary messages users see after clicking the new buttons, so both content and HTML escaping matter. Coverage now matches the other render_* helpers. Generated with AI Co-Authored-By: AI <ai@example.com>
| """使用者從到期通知按「取消整個系列」：等同於 /cancel。 | ||
| 對一次性提醒也可用（雖然按鈕只出現在週期性提醒上）。""" | ||
| return self._repository.cancel(chat_id, short_id, actor_user_id) |
There was a problem hiding this comment.
Fixed in 0ec2141. Added the same result.rowcount != 1 → return None guard on repository.cancel as the round-1 fix on advance_pending. This is pre-existing but exposed by the phase-4a cancel-series button; the same code path is shared by /cancel and the detail-page delete, so both benefit. Regression test test_cancel_series_returns_none_when_reminder_no_longer_pending mirrors the skip race test — pre-mark reminder as FIRING, assert None returned and DB status still FIRING (not silently CANCELLED).
Same race window we fixed in advance_pending exists in repository.cancel — SELECT the reminder while it is PENDING, then run an UPDATE guarded on status = PENDING. If scheduler.claim_due transitions the row to FIRING between the two statements (different connection), the UPDATE is 0 rows affected but the method still returns a Reminder object with status=CANCELLED synthesised locally. Users see a "already cancelled" reply while the DB row is untouched and continues to fire on the next cycle. Add the same result.rowcount != 1 → return None guard. This is pre-existing but exposed by Phase 4a's cancel-series button (the same call path is also used by /cancel and the detail-page delete, so both benefit). Regression test mirrors the skip_next race test: pre-mark reminder as FIRING, call cancel_series, expect None returned and DB row status still FIRING (not CANCELLED). Generated with AI Co-Authored-By: AI <ai@example.com>
| def delivery_recurring_keyboard(short_id: str, next_day_time_label: str) -> dict[str, object]: | ||
| """週期性提醒的到期按鈕：延用一次性提醒的三顆延後按鈕，再多一列 | ||
| 「跳過下次 / 取消整個系列」。使用者拿到通知後可以： | ||
| - 延後這次到 10 分/1 小時/明天 HH:MM（Bot 送完後仍會照規則排下次） | ||
| - 跳過下次（直接把 remind_at 推到「下下次」，例如每月 1/18/25 剛推到 18 號可再按跳過推到 25 號） |
There was a problem hiding this comment.
Fixed in 0582d62 with Option A. Added PENDING to repository.snooze's SELECT status filter and the guarded UPDATE, plus the same result.rowcount != 1 guard as advance_pending / repository.cancel. Recurring reminders are transitioned back to PENDING immediately after tick's send_message, so before this fix the delegated snooze row had been silently DOA since Phase 1.
Recurrence pattern self-heals: snoozing overwrites the already-scheduled next fire with now + delay; when that snooze fire runs, tick calls next_fire(rule, now) which returns the next rule-derived occurrence. So the user sees an extra 'remind me later' for the current instance while the rule pattern continues.
Button labels don't need to change — user intuition for '10 分鐘後 / 1 小時後 / 明天 HH:MM' remains 'now + delay' consistent across one-off and recurring reminders.
Regression test test_snooze_works_on_pending_recurring_reminder constructs a PENDING recurring reminder (mirroring the state after tick's reschedule) and confirms 10m snooze returns click_at + 10 minutes.
The snooze SQL only accepted status IN (FIRING, FIRED) but the scheduler.tick recurrence branch calls repository.reschedule immediately after send_message, transitioning the row from FIRING back to PENDING before the user has time to click any delivery button. So the "10 分鐘後 / 1 小時後 / 明天 HH:MM" row that phase 4a delegated onto the recurring keyboard has been silently DOA since Phase 1: every click returned None and the reply text mumbled about "not creator or not found" without matching reality. Add PENDING to both the SELECT status filter and the guarded UPDATE so recurring reminders can be snoozed while pending. Behavior for one-off reminders is unchanged (they still snooze from FIRING or FIRED). Add the same rowcount != 1 guard as advance_pending / repository.cancel for the SELECT → UPDATE race window with scheduler.claim_due. Interaction with the recurrence schedule: snoozing overwrites the already-scheduled next fire with `now + delay`. When that snooze fire runs, tick calls next_fire(rule, now) which returns the next occurrence per the original rule, so the recurrence pattern self-heals — the user only sees an extra "remind me later" notification for the current instance. Regression test constructs a PENDING recurring reminder (mirroring the state after tick's reschedule) and confirms 10m snooze returns click_at + 10 minutes. Generated with AI Co-Authored-By: AI <ai@example.com>
| if result.rowcount != 1: | ||
| return None | ||
|
|
||
| return replace(row_to_reminder(row), status=ReminderStatus.CANCELLED) |
There was a problem hiding this comment.
Fixed in 56d98d5. cancel now keeps the mutation timestamp as a datetime alongside the ISO string handed to SQLite and passes it through replace(...) on the returned Reminder. Consistent with every other update-shaped repository method (snooze, advance_pending, update_title, update_remind_at) — cancel was the outlier. Regression test test_cancel_series_returned_reminder_has_fresh_updated_at asserts the returned reminder's updated_at differs from the pre-cancel row and matches what get_by_short_id reads back.
cancel() only replaced `status` on the returned Reminder while the UPDATE wrote a fresh `updated_at` to the database. The returned object therefore reported the pre-cancel timestamp, out of sync with the persisted row. Every other update-shaped repository method (snooze, advance_pending, update_title, update_remind_at) already returns updated_at from the mutation moment; cancel was the outlier. Keep the mutation timestamp as a datetime alongside the ISO string we hand to SQLite, then pass it through `replace(...)` on the returned Reminder so callers see the same value that landed in the DB. Regression test asserts the returned reminder's updated_at differs from the pre-cancel row and matches what get_by_short_id reads back. Generated with AI Co-Authored-By: AI <ai@example.com>
The button label is "取消整個系列" and the edit message after success is "已取消整個系列", but the callback toast omitted "整個" and said "已取消系列". Match the button/edit wording so users see consistent wording across the entire click flow. Generated with AI Co-Authored-By: AI <ai@example.com>
Summary
Phase 4a — 到期通知加「跳過下次」+「取消整個系列」按鈕。只對週期性提醒顯示，一次性提醒行為完全不變。
使用者情境
/cancel再重建。/cancel R-XXXX。Keyboard layout（週期性提醒到期時）
一次性提醒維持三顆延後按鈕。
資料流
跳過下次
service.skip_next_occurrence：從reminder.remind_at（已排程的下次）呼叫next_fire(rule, current_next_at)拿到「再下一次」repository.advance_pending：新 method，PENDING 狀態下更新remind_at（跟snooze類似但保留 PENDING、不動 fired_at；跟reschedule不同因為那是 scheduler-owned FIRING → PENDING）取消整個系列
service.cancel_seriesdelegate 到既有repository.cancel（已有 creator guard）Test plan
make test— 154 tests 全過（146 → 154，新增 8）make lint— ruff 全綠新增測試（8 個）
Service（6 case）：
test_skip_next_advances_one_iteration（7/18 → 7/25）test_skip_next_crosses_month_boundary（7/25 → 8/1）test_skip_next_rejects_non_recurring_remindertest_skip_next_rejects_non_creatortest_cancel_series_marks_cancelledtest_cancel_series_rejects_non_creatorScheduler（2 case）：
test_recurring_delivery_uses_recurring_keyboard（有跳過/取消系列按鈕）test_one_off_delivery_does_not_show_skip_or_cancel_buttonsOut of scope（Phase 4b，延後）
相關