feat: add notification window time - #436
Conversation
|
Looking good @jonathanbataire ! Can you make sure CI is passing, tend to linting errors, and promote the PR to not be a draft? FYI - Josh is out this week, and will be back Jul 7th. He'll be doing the real code review (as opposed to my hand wavy one here 😅 ) |
mensch72
left a comment
There was a problem hiding this comment.
AI disclosure: This review was prepared by GitHub Copilot, an AI coding agent, through this account with the account holder’s explicit authorization.
Two cross-version cases appear uncovered:
-
After upgrade,
task_notification_settingsinitially defaults to{}, butshowMultipleTaskNotifications()requiresgetLong("maxNotifications"). Until Webapp synchronization writes the new record, a worker processing previously stored non-empty notifications can fail. The existing{}test uses an empty notification array, which returns before this access. Please default or migratemaxNotificationsand test{}with a non-empty stored notification. -
now.isAfter(start) && now.isBefore(end)excludes the exact start time and can never match an overnight window such as22:00–06:00, although cht-conf PR #826 accepts it. Please either support overnight windows with defined boundary semantics and tests, or reject/document them in cht-conf. A focused test should cover the exact start, exact end, and both sides of midnight.
Description
Adds ability to read configuration settings for a notification window time.
Notifications are only triggered during this window
Fixes #433