Skip to content

feat(parser): single-turn reject for invalid recurrence markers - #27

Merged
HeiTang merged 2 commits into
devfrom
feature/invalid-recurrence-single-turn-reject
Jul 10, 2026
Merged

feat(parser): single-turn reject for invalid recurrence markers#27
HeiTang merged 2 commits into
devfrom
feature/invalid-recurrence-single-turn-reject

Conversation

@HeiTang

@HeiTang HeiTang commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Summary

Manual QA 意外挖出的 UX bug:使用者輸入無效的週期性 marker(例如 每個月 45 號 09:00 提醒我),Bot 默默 downgrade 成一次性提醒,時間變成明天 09:00、事項變成「每個月 45 號」— 完全不是使用者的意圖。

本 PR 引入「單輪拒絕」機制:parser 偵測到週期 marker 匹配但語法無效時,直接告知具體錯誤原因,不建 draft、不追問、也不干擾使用者現有的 pending draft/edit session。

使用者體驗對比

Before(bug)

User: 每個月 45 號 09:00 提醒我
Bot:  確認建立提醒?
      時間:2026-07-10 09:00
      事項:每個月 45 號
      [確認] [取消]

After(本 PR)

User: 每個月 45 號 09:00 提醒我
Bot:  『每個月 45 號』不是有效的日期(日期需在 1-31 範圍)。
      請重新輸入完整的提醒。

單輪、無 draft、無 pending state。使用者重打一次即可。

覆蓋範圍

輸入 Bot 回覆
每個月 0 號 09:00 提醒我 X 『每個月 0 號』不是有效的日期(日期需在 1-31 範圍)
每個月 45 號 09:00 提醒我 X 『每個月 45 號』不是有效的日期(日期需在 1-31 範圍)
每年 13/25 08:00 提醒我 X 『每年 13/25』不是有效的日期(月份需在 1-12 範圍)
每年 2/30 08:00 提醒我 X 『每年 2/30』不是有效的日期(2 月最多 29 天)
每年 4/31 08:00 提醒我 X 『每年 4/31』不是有效的日期(4 月最多 30 天)
每個月 15, 45 號 09:00 提醒我發薪 部分合法:保留 15、丟棄 45(正常建立)
每年 2/29 08:00 提醒我生日 建立成功(下次觸發 2028/2/29)

資料流

  1. models.py:新增 RecurrenceError(marker_text, reason) + ParseResult.recurrence_error 欄位
  2. parser.py
    • _detect_recurrence_marker 現在區分「有效 marker」/「無效 marker (RecurrenceError)」/「無 marker (None)」三種輸出
    • _try_recurrence_parse propagate RecurrenceError
    • parse() 在錯誤時回傳只帶 recurrence_error 的 ParseResult(其他欄位 None)
  3. service.py
    • preview_parse(text, message, now) — 無副作用地拿 ParseResult
    • begin_createparse_result kwarg 讓 router 重用 preview 結果(避免二次解析)
  4. renderer.pyrender_recurrence_error(error)
  5. response_sender.pysend_recurrence_error(chat_id, error)
  6. router.py
    • _handle_messagestarts_new_reminder 分支先呼 preview_parse
    • parse_result.recurrence_error → 送錯誤訊息、return(不清 pending draft/edit
    • 否則走既有的 clear pending + dismiss + begin_create(重用 preview 結果)

為什麼「不清 pending draft」

若使用者正在建立另一個提醒(有 pending draft)時打錯,Bot 不應把那個 draft 也清掉。單輪拒絕的語意是「這則訊息無效」,不是「使用者要放棄前面的建立流程」。

Test plan

  • make test — 202 tests 全過(199 → 202,新增 3 個測試組合)
  • make lint — ruff 全綠

新增測試

  • Parser(test_parser.py
    • test_monthly_out_of_range_days_produce_recurrence_error(0 號 + 45 號 → error + marker_text + reason)
    • test_monthly_partial_valid_days_are_kept(15, 45 → 保留 15)
    • test_yearly_rejects_invalid_month(13/25 → 月份錯誤)
    • test_yearly_rejects_impossible_date(2/30 → 天數錯誤 with 2 月最多 29)
    • test_yearly_rejects_april_31(4/31 → 4 月最多 30)
  • Router(test_bot_router.py
    • test_invalid_recurrence_gets_single_turn_reject(送錯誤訊息、無 keyboard、無 draft 建立)
    • test_invalid_recurrence_does_not_disturb_pending_draft(既有 pending draft 完全不受影響)

手動 QA checklist 更新

原本 C3 寫「fall through 追問缺欄位」是基於舊 bug 的妥協行為。本 PR 後 C3 應改為:

輸入 新預期
每個月 0 號 09:00 提醒我 X 『每個月 0 號』不是有效的日期(1-31)。請重新輸入完整的提醒。
每個月 45 號 09:00 提醒我 X 『每個月 45 號』不是有效的日期(1-31)。請重新輸入完整的提醒。
每年 13/25 08:00 提醒我 X 『每年 13/25』不是有效的日期(月份 1-12)。請重新輸入完整的提醒。
每年 2/30 08:00 提醒我 X 『每年 2/30』不是有效的日期(2 月最多 29 天)。請重新輸入完整的提醒。
每年 2/29 08:00 提醒我生日 不變:建立成功

Before this change, "每個月 45 號 09:00 提醒我" would silently
downgrade to a one-off reminder for "tomorrow 09:00" with title
"每個月 45 號" — the marker text bleeding into the title while the
user's intent (a monthly recurrence) was ignored. Same for
"每年 13/25", "每年 2/30", "每年 4/31", etc. The recurrence path
already rejected these at validation but simply returned None and
let the split parser scavenge whatever it could from the leftovers.

Introduce a proper "single-turn reject" signal:

- Add RecurrenceError(marker_text, reason) dataclass to models.
- Extend ParseResult with recurrence_error field.
- _detect_recurrence_marker now distinguishes three outcomes:
  valid tuple, RecurrenceError, or None. Monthly with any 1..31
  values still succeeds (partial-valid tolerated); only all-invalid
  raises. Yearly reports a specific reason (month out of range vs
  month max days) using the same leap-year probe as is_valid_month_day.
- _try_recurrence_parse propagates RecurrenceError; parse() sets it
  on ParseResult so callers can react without inspecting internals.
- service.preview_parse lets router peek at parse output without any
  save side-effect; begin_create accepts a pre-computed parse_result
  to avoid double-parsing after a successful preview.
- renderer.render_recurrence_error + response_sender.send_recurrence_error
  emit the user-visible message.
- Router runs preview_parse first: on recurrence_error it sends the
  error message and returns without touching pending draft/edit
  sessions, so the user's in-progress work is not clobbered by a
  typo.

Tests cover parser signals for all invalid inputs (monthly 0/45,
yearly 13/25, 2/30, 4/31), the partial-valid tolerance path, the
happy-path 2/29 leap-day case, router's single-turn reject flow,
and the "invalid recurrence does not disturb a pending draft" case.

Generated with AI

Co-Authored-By: AI <ai@example.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a “single-turn reject” flow for invalid recurrence markers (e.g., 每個月 45 號 ...) so the bot explains the specific date error instead of silently falling back to a one-off reminder and creating/prompting through a draft flow.

Changes:

  • Add RecurrenceError + ParseResult.recurrence_error and propagate it from the parser when recurrence markers match but are invalid.
  • Add a router pre-parse (“preview”) step that sends a dedicated recurrence-error response and returns early (without clearing pending draft/edit sessions).
  • Add rendering/sending helpers for recurrence error messages, plus tests for parser and router behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_parser.py Updates/expands parser tests to assert recurrence_error behavior and “no draft fields” semantics on invalid recurrence markers.
tests/test_bot_router.py Adds router tests ensuring invalid recurrence gets a single-turn reject and does not disturb an existing pending flow.
src/remindly/reminders/service.py Adds preview_parse() and allows begin_create() to reuse a previously computed ParseResult.
src/remindly/reminders/renderer.py Adds render_recurrence_error() to produce the single-turn reject message text (HTML-escaped).
src/remindly/reminders/parser.py Introduces invalid-marker detection that returns RecurrenceError instead of falling through; parse() returns a ParseResult carrying that error.
src/remindly/reminders/models.py Adds RecurrenceError dataclass and ParseResult.recurrence_error field.
src/remindly/bot/router.py Adds the preview-parse + early-return behavior on recurrence_error, preserving pending draft/edit sessions.
src/remindly/bot/response_sender.py Adds send_recurrence_error() to send the new error message without keyboards/prompts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/remindly/reminders/parser.py Outdated
Comment on lines +183 to 187
if parse_result is None:
parse_result = self._parser.parse(
text, message, now=now.astimezone(ZoneInfo(timezone))
)
draft = ReminderDraft(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8dda4da. Added RecurrenceRejected result type; begin_create() now short-circuits when parse_result.recurrence_error is not None (both the passed-in and self-parsed case), and send_draft_result dispatches it to send_recurrence_error. This puts the guard at the layer closest to the side effect, so /remind and any future entry points that skip the router preview also single-turn reject. Router still calls preview_parse first — needed to avoid clearing pending sessions before knowing the input is valid — but begin_create no longer trusts callers. Test: test_begin_create_returns_recurrence_rejected_for_invalid_marker in tests/test_service.py.

Copilot review round 1 flagged that begin_create() would still build and
persist a draft when parse_result carried a recurrence_error. Any entry
point that skipped the router's preview_parse guard (e.g. /remind) would
regress to the pre-#27 UX: silent draft + "什麼時候提醒?" follow-up.

Add RecurrenceRejected result type and short-circuit inside begin_create
so the guard sits at the layer closest to the side effect. send_draft_result
dispatches it to send_recurrence_error. Also fix parser docstring that
said "raise" instead of "return" for RecurrenceError.

Generated with AI

Co-Authored-By: AI <ai@example.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@HeiTang
HeiTang merged commit f280063 into dev Jul 10, 2026
2 checks passed
@HeiTang
HeiTang deleted the feature/invalid-recurrence-single-turn-reject branch July 10, 2026 06:55
@HeiTang HeiTang mentioned this pull request Jul 10, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants