Add goal notifications - #1620
Conversation
Greptile SummaryAdds configurable Slack and email warnings for goals that remain incomplete near the end of their period.
Confidence Score: 1/5This PR should not merge until email consent preservation, reference-time handling and duplicate-delivery protection are corrected. Unrelated goal edits can reverse an unsubscribe, delayed jobs can evaluate the wrong calendar period and overlapping jobs can deliver duplicate warnings because notification state is recorded only after the external side effect. Files Needing Attention: app/controllers/settings/goals_controller.rb, app/jobs/goal_user_notification_job.rb, app/jobs/goal_notification_fanout_job.rb
|
| Filename | Overview |
|---|---|
| app/controllers/settings/goals_controller.rb | Persists channel preferences but can reverse an explicit email unsubscribe during an unrelated goal update. |
| app/jobs/goal_notification_fanout_job.rb | Fans out eligible users on a schedule but does not prevent overlapping per-user jobs. |
| app/jobs/goal_user_notification_job.rb | Implements delivery and deduplication but ignores the supplied reference time, permits duplicate concurrent sends and broadly catches unexpected failures. |
| app/models/goal.rb | Adds notification state, period windows and warning-threshold logic with focused model tests. |
| app/javascript/pages/Users/Settings/Goals.svelte | Adds channel controls and status text while preserving notification values in every edit payload. |
| db/migrate/20260819010000_add_notification_settings_to_goals.rb | Adds non-null channel defaults and a nullable per-period delivery marker. |
Sequence Diagram
sequenceDiagram
participant Cron as GoodJob Cron
participant Fanout as GoalNotificationFanoutJob
participant UserJob as GoalUserNotificationJob
participant Progress as ProgrammingGoalsProgressService
participant Slack as Slack API
participant Mailer as GoalMailer
Cron->>Fanout: Run every 10 minutes
Fanout->>UserJob: Enqueue user id and reference time
UserJob->>Progress: Calculate enabled goal progress
Progress-->>UserJob: Tracked seconds per goal
alt Goal is incomplete after warning threshold
opt Slack enabled
UserJob->>Slack: Send direct message
end
opt Email enabled and subscribed
UserJob->>Mailer: Deliver warning email
end
UserJob->>UserJob: Record notified period start
end
Prompt To Fix All With AI
### Issue 1
app/controllers/settings/goals_controller.rb:70
**Unrelated edits reverse unsubscribe**
When a user unsubscribes through the email footer and later edits another field on an email-enabled goal, this callback subscribes them again without a new email opt-in, causing goal emails to resume. **How this was verified:** The edit form always resubmits the existing email setting, and this callback subscribes any currently unsubscribed user without checking for a false-to-true transition.
```suggestion
@user.subscribe("goal_notifications") if @goal.saved_change_to_notify_email? && @goal.notify_email? && !@user.subscribed?("goal_notifications")
```
### Issue 2
app/jobs/goal_user_notification_job.rb:14-18
**Reference time is discarded**
When this low-priority job executes after crossing the user's day, week or month boundary, it uses execution time instead of the fanout's reference time, causing progress to be evaluated for the new period and the ending period's warning to be missed.
### Issue 3
app/jobs/goal_user_notification_job.rb:45-55
**Delivery marker permits duplicates**
If two notification jobs overlap for the same user and goal, both can read the unset period marker and deliver Slack or email notifications before either updates it, causing duplicate warnings for one goal period.
### Issue 4
app/jobs/goal_user_notification_job.rb:75-78
**Broad rescue hides job defects**
Rescuing every `StandardError` treats programming and serialization defects as ordinary channel failures, obscuring the failing operation and preventing GoodJob from recording the user job itself as failed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Add goal notifications" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # notifications on any goal, so an earlier unsubscribe via a mail footer does | ||
| # not silently swallow newly enabled goals. | ||
| def ensure_goal_email_subscription | ||
| @user.subscribe("goal_notifications") if @goal.notify_email? && !@user.subscribed?("goal_notifications") |
There was a problem hiding this comment.
When a user unsubscribes through the email footer and later edits another field on an email-enabled goal, this callback subscribes them again without a new email opt-in, causing goal emails to resume. How this was verified: The edit form always resubmits the existing email setting, and this callback subscribes any currently unsubscribed user without checking for a false-to-true transition.
| @user.subscribe("goal_notifications") if @goal.notify_email? && !@user.subscribed?("goal_notifications") | |
| @user.subscribe("goal_notifications") if @goal.saved_change_to_notify_email? && @goal.notify_email? && !@user.subscribed?("goal_notifications") |
Knowledge Base Used: Roll Back Mailkick weekly summary subscriptions
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/settings/goals_controller.rb
Line: 70
Comment:
**Unrelated edits reverse unsubscribe**
When a user unsubscribes through the email footer and later edits another field on an email-enabled goal, this callback subscribes them again without a new email opt-in, causing goal emails to resume. **How this was verified:** The edit form always resubmits the existing email setting, and this callback subscribes any currently unsubscribed user without checking for a false-to-true transition.
```suggestion
@user.subscribe("goal_notifications") if @goal.saved_change_to_notify_email? && @goal.notify_email? && !@user.subscribed?("goal_notifications")
```
**Knowledge Base Used:** [Roll Back Mailkick weekly summary subscriptions](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/reverts/revert_1019-20260301-mailkick-prod-errors-0c7fb13.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| now_utc = Time.zone.parse(now_utc_iso8601) | ||
| timezone = ActiveSupport::TimeZone[user.timezone] || ActiveSupport::TimeZone["UTC"] | ||
|
|
||
| Time.use_zone(timezone) do | ||
| now = Time.zone.now |
There was a problem hiding this comment.
When this low-priority job executes after crossing the user's day, week or month boundary, it uses execution time instead of the fanout's reference time, causing progress to be evaluated for the new period and the ending period's warning to be missed.
Context Used: AGENTS.md (source)
Knowledge Base Used: Background Jobs Infrastructure (GoodJob)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/jobs/goal_user_notification_job.rb
Line: 14-18
Comment:
**Reference time is discarded**
When this low-priority job executes after crossing the user's day, week or month boundary, it uses execution time instead of the fanout's reference time, causing progress to be evaluated for the new period and the ending period's warning to be missed.
**Context Used:** AGENTS.md ([source](https://github.com/hackclub/hackatime/blob/main/AGENTS.md))
**Knowledge Base Used:** [Background Jobs Infrastructure (GoodJob)](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/background-jobs-infra.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return if goal.last_missed_notification_period_start == window.begin | ||
| return unless goal.about_to_miss?(now: now, tracked_seconds: progress[:tracked_seconds]) | ||
|
|
||
| notification = Notification.new( | ||
| tracked_seconds: progress[:tracked_seconds], | ||
| remaining_seconds: (window.end - now).to_i | ||
| ) | ||
|
|
||
| return unless deliver_notifications(user, goal, notification) | ||
|
|
||
| goal.update!(last_missed_notification_period_start: window.begin) |
There was a problem hiding this comment.
If two notification jobs overlap for the same user and goal, both can read the unset period marker and deliver Slack or email notifications before either updates it, causing duplicate warnings for one goal period.
Context Used: AGENTS.md (source)
Knowledge Base Used: Background Jobs Infrastructure (GoodJob)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/jobs/goal_user_notification_job.rb
Line: 45-55
Comment:
**Delivery marker permits duplicates**
If two notification jobs overlap for the same user and goal, both can read the unset period marker and deliver Slack or email notifications before either updates it, causing duplicate warnings for one goal period.
**Context Used:** AGENTS.md ([source](https://github.com/hackclub/hackatime/blob/main/AGENTS.md))
**Knowledge Base Used:** [Background Jobs Infrastructure (GoodJob)](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/background-jobs-infra.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| # A failed delivery must never abort the run and skip other goals or | ||
| # channels; report it and let the next scheduled run retry this goal. | ||
| def safely(channel) | ||
| yield |
There was a problem hiding this comment.
Rescuing every StandardError treats programming and serialization defects as ordinary channel failures, obscuring the failing operation and preventing GoodJob from recording the user job itself as failed.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/jobs/goal_user_notification_job.rb
Line: 75-78
Comment:
**Broad rescue hides job defects**
Rescuing every `StandardError` treats programming and serialization defects as ordinary channel failures, obscuring the failing operation and preventing GoodJob from recording the user job itself as failed.
**Context Used:** AGENTS.md ([source](https://github.com/hackclub/hackatime/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary of the problem
Goals weren't that motivating!
Describe your changes
Adds notification system
Screenshots / Media