Notification events #22
Conversation
WalkthroughIntroduces an event notification system with database schema, service layer, and integration points. Adds fire-and-forget notifications for event creation and scheduled one-hour event reminders, complete with multi-language support and deduplication tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant HTTP as HTTP Handler
participant Handler as EventHandler
participant Notifier as EventNotificationService
participant DB as Database
participant I18N as i18n
participant Push as Push Service
HTTP->>Handler: CreateEvent()
Handler->>DB: Insert event record
Handler->>Notifier: SendEventCreated() [async, non-blocking]
Handler-->>HTTP: Response (200 OK)
par Async Notification Flow
Notifier->>DB: getEventCore()
DB-->>Notifier: Event name, clubID, startDate
Notifier->>DB: getRecipients()
DB-->>Notifier: Club members + attendees (deduplicated)
Notifier->>DB: GetUsersWithLanguageByEmails()
DB-->>Notifier: Emails → tokens & language codes
loop Per Language Group
Notifier->>DB: AlreadySent? (dedup check)
alt Not already sent
Notifier->>I18N: Localize title & message
I18N-->>Notifier: Localized strings
Notifier->>Push: Send push notification
Push-->>Notifier: Success/Failure
Notifier->>DB: LogNotification()
end
end
end
sequenceDiagram
participant Cron as Cron Job (5 min)
participant Notifier as EventNotificationService
participant DB as Database
participant I18N as i18n
participant Push as Push Service
Cron->>Notifier: SendDueOneHourReminders()
Notifier->>DB: Query events starting within 1 hour window
DB-->>Notifier: Matching event IDs
loop Per Event
Notifier->>Notifier: sendOneHourReminderForEvent()
Notifier->>DB: getEventCore()
Notifier->>DB: getRecipients()
Notifier->>DB: GetUsersWithLanguageByEmails()
loop Per Language Group
Notifier->>DB: AlreadySent? (dedup check)
alt Not already sent
Notifier->>I18N: Localize reminder message
Notifier->>Push: Send push reminder
Notifier->>DB: LogNotification()
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The diff introduces a multi-layered feature with heterogeneous changes: new database schema with constraints and cascading deletes, a substantial new service with complex recipient resolution and language-based grouping logic, integration points across multiple files, i18n support in four languages, and background job scheduling. The EventNotificationService contains non-trivial control flow involving deduplication, language routing, and error-skipping patterns that warrant careful review despite consistent patterns within. Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a notification system for event-related push notifications. The system sends localized push notifications to users when events are created and provides automated one-hour reminders before events start.
- Adds
EventNotificationServiceto manage event-related push notifications with deduplication logic - Implements automatic one-hour reminders via cron job running every 5 minutes
- Adds localized notification messages in English, French, Spanish, and German
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/notification.go | Added GetUsersWithLanguageByEmails method to resolve email addresses to notification tokens and language codes |
| services/event_notification.go | New service implementing event creation notifications and one-hour reminder logic with deduplication |
| main.go | Integrated EventNotificationService and configured cron job for periodic reminder checks |
| handlers/event/event_handler.go | Integrated notification service to trigger event creation notifications asynchronously |
| db/migrations/00019_event_notifications_log.sql | Created database table for tracking sent notifications to prevent duplicates |
| i18n/active.*.toml | Added localized notification text for event creation and reminder notifications in all supported languages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| query := ` | ||
| SELECT n.email, COALESCE(n.notification_token, '') AS notification_token, l.code AS language_code | ||
| FROM newf n |
There was a problem hiding this comment.
The table name 'newf' appears to be a typo or placeholder. Consider renaming to a more descriptive name like 'users' or 'new_users'.
There was a problem hiding this comment.
nan il a juste pas la da
| } | ||
|
|
||
| // Users who marked interest in the event (attendees table) | ||
| evtQuery := `SELECT email FROM events_attendents WHERE id_events = $1` |
There was a problem hiding this comment.
Corrected spelling of 'attendents' to 'attendees' in the table name.
| evtQuery := `SELECT email FROM events_attendents WHERE id_events = $1` | |
| evtQuery := `SELECT email FROM events_attendees WHERE id_events = $1` |
| title = "📅 Nouvel évènement créé" | ||
| message = "L'évènement '{{.Name}}' est créé pour le {{.StartTime}}." | ||
|
|
||
| [event.reminder] | ||
| title = "⏰ L'évènement commence dans 1 heure" |
There was a problem hiding this comment.
In modern French, 'événement' is the standard spelling (with 'é'), not 'évènement' (with 'è').
| title = "📅 Nouvel évènement créé" | |
| message = "L'évènement '{{.Name}}' est créé pour le {{.StartTime}}." | |
| [event.reminder] | |
| title = "⏰ L'évènement commence dans 1 heure" | |
| title = "📅 Nouvel événement créé" | |
| message = "L'événement '{{.Name}}' est créé pour le {{.StartTime}}." | |
| [event.reminder] | |
| title = "⏰ L'événement commence dans 1 heure" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
handlers/event/event_handler.go (1)
784-789: Consider logging errors from the notification goroutine.While fire-and-forget is appropriate for non-critical notifications, silently ignoring errors makes debugging difficult. Consider adding error logging.
Apply this diff to add error logging:
// Fire-and-forget creation notification (non-blocking) if h.notifier != nil { go func(id int) { - _ = h.notifier.SendEventCreated(id) + if err := h.notifier.SendEventCreated(id); err != nil { + utils.LogLineKeyValue(utils.LevelWarn, "Failed to send event creation notification", err.Error()) + } }(eventID) }main.go (1)
113-116: Consider logging errors from the reminder cron job.Background jobs that silently fail can be difficult to debug. Adding error logging would help identify issues with reminder delivery.
Apply this diff to add error logging:
// Add periodic job to send 1-hour event reminders every 5 minutes _, _ = c.AddFunc("CRON_TZ=Europe/Paris */5 * * * *", func() { - _ = eventNotifier.SendDueOneHourReminders() + if err := eventNotifier.SendDueOneHourReminders(); err != nil { + log.Printf("Failed to send event reminders: %v", err) + } })services/event_notification.go (8)
62-106: Harden recipient resolution: filter null/empty emails and normalize caseAvoid scanning/adding invalid emails and dedupe case-insensitively. Also reduces scan errors.
Apply this diff:
- clubQuery := `SELECT email FROM clubs_members WHERE id_clubs = $1` + clubQuery := `SELECT email FROM clubs_members WHERE id_clubs = $1 AND email IS NOT NULL AND email <> ''` @@ - for rowsClub.Next() { + for rowsClub.Next() { var email string - if err := rowsClub.Scan(&email); err == nil { - emailSet[email] = struct{}{} + if err := rowsClub.Scan(&email); err == nil { + e := strings.ToLower(strings.TrimSpace(email)) + if e != "" { + emailSet[e] = struct{}{} + } } } @@ - evtQuery := `SELECT email FROM events_attendents WHERE id_events = $1` + evtQuery := `SELECT email FROM events_attendents WHERE id_events = $1 AND email IS NOT NULL AND email <> ''` @@ - for rowsEvt.Next() { + for rowsEvt.Next() { var email string - if err := rowsEvt.Scan(&email); err == nil { - emailSet[email] = struct{}{} + if err := rowsEvt.Scan(&email); err == nil { + e := strings.ToLower(strings.TrimSpace(email)) + if e != "" { + emailSet[e] = struct{}{} + } } }Add import:
import "strings"
131-144: Avoid N+1 AlreadySent queries; batch prefetch sent emailsCurrent loop issues one query per user. Prefetch into a set: SELECT email FROM event_notifications_log WHERE id_events=$1 AND notification_type=$2 AND email = ANY($3). Filter in-memory before grouping.
31-39: Dedup race (TOCTOU): may double-send under concurrent workersPattern is check (AlreadySent) → send → log. Two workers can both send before either logs. Consider insert-first idempotency (attempt to insert a “send lock” row and only send if inserted), or transact per user group with advisory locks.
Also applies to: 41-51
182-191: Log send failures and log-insert errors for visibilitySilent continue hides operational issues and hampers debugging.
Apply this diff:
- if err := s.notificationService.SendPushNotification(payload); err != nil { - // continue to next language group - continue - } + if err := s.notificationService.SendPushNotification(payload); err != nil { + log.Printf("SendEventCreated: push failed lang=%s eventID=%d err=%v", lang, eventID, err) + continue + } @@ - for _, email := range languageToEmails[lang] { - _ = s.LogNotification(eventID, email, EventNotificationCreated) - } + for _, email := range languageToEmails[lang] { + if err := s.LogNotification(eventID, email, EventNotificationCreated); err != nil { + log.Printf("SendEventCreated: log insert failed eventID=%d email=%s err=%v", eventID, email, err) + } + }Add import:
import "log"
233-246: Handle creation_date scan error explicitly (or drop this guard)Currently ignores errors; unexpected DB errors get silently skipped.
Apply this diff:
- var creationDate time.Time - _ = s.db.QueryRow(`SELECT creation_date FROM events WHERE id_events = $1`, eventID).Scan(&creationDate) + var creationDate time.Time + if err := s.db.QueryRow(`SELECT creation_date FROM events WHERE id_events = $1`, eventID).Scan(&creationDate); err != nil && err != sql.ErrNoRows { + return fmt.Errorf("query creation_date: %w", err) + }
277-312: Mirror send/log observability for remindersSame silent continue/logging pattern as created notifications. Apply the same logging changes here.
27-29: Constructor nil guardsIf db or notificationService is nil, methods will panic later. Guard early.
Example:
func NewEventNotificationService(db *sql.DB, notificationService *NotificationService) *EventNotificationService { if db == nil || notificationService == nil { panic("EventNotificationService: db and notificationService must be non-nil") } return &EventNotificationService{db: db, notificationService: notificationService} }
204-211: Use context with timeouts for DB/external callsBackground/cron paths should use context with deadlines for DB queries and push sends to avoid hanging tasks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
db/migrations/00019_event_notifications_log.sql(1 hunks)handlers/event/event_handler.go(2 hunks)i18n/active.de.toml(1 hunks)i18n/active.en.toml(1 hunks)i18n/active.es.toml(1 hunks)i18n/active.fr.toml(1 hunks)main.go(3 hunks)services/event_notification.go(1 hunks)services/notification.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
services/notification.go (1)
models/notification.go (1)
NotificationTargetWithLanguage(17-21)
main.go (2)
services/event_notification.go (1)
NewEventNotificationService(27-29)handlers/event/event_handler.go (1)
NewEventHandler(21-26)
services/event_notification.go (4)
services/notification.go (1)
NotificationService(22-25)i18n/i18n.go (1)
GetLocalizer(61-74)utils/time.go (2)
FormatParis(71-73)Now(23-25)models/notification.go (1)
NotificationPayload(24-37)
handlers/event/event_handler.go (1)
services/event_notification.go (1)
EventNotificationService(22-25)
🔇 Additional comments (9)
i18n/active.fr.toml (1)
5-7: LGTM!The event reminder translations are consistent with the event creation section and follow the same template variable pattern.
i18n/active.en.toml (1)
54-61: LGTM!English translations follow the same structure as other locales. Template variable consistency will be verified through the script in the French locale review.
i18n/active.de.toml (1)
1-7: LGTM!German translations are consistent with other locales.
db/migrations/00019_event_notifications_log.sql (1)
11-13: LGTM!The foreign key constraints with ON DELETE CASCADE and the unique constraint for deduplication are well-designed. This ensures referential integrity and prevents duplicate notifications for the same event-user-type combination.
handlers/event/event_handler.go (1)
17-18: LGTM!The notifier field and constructor changes properly integrate the event notification service into the handler.
Also applies to: 21-25
i18n/active.es.toml (1)
1-7: LGTM!Spanish translations are consistent with other locales.
main.go (1)
64-64: LGTM!The event notifier is properly instantiated and wired into the event handler.
Also applies to: 104-104
services/notification.go (1)
201-233: LGTM!The implementation is well-structured with:
- Defensive handling of empty input
- SQL filtering of invalid tokens
- Proper error wrapping for debugging
- Resource cleanup with deferred row closing
- Graceful handling of scan errors
services/event_notification.go (1)
83-89: No issue found: the table name "events_attendents" is correctThe table is defined in
db/migrations/00001_initial_schema.sql:269with exactly that spelling, so the query at line 86 is accurate.
| // LogNotification creates a deduped log entry for a sent notification | ||
| func (s *EventNotificationService) LogNotification(eventID int, email string, notifType EventNotificationType) error { | ||
| _, err := s.db.Exec(` | ||
| INSERT INTO event_notifications_log (id_events, email, notification_type) | ||
| VALUES ($1, $2, $3) | ||
| ON CONFLICT (id_events, email, notification_type) DO NOTHING | ||
| `, eventID, email, string(notifType)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
PII in logs: storing plain emails in event_notifications_log
Emails are personal data; long-term storage raises compliance risk (GDPR/CCPA). Prefer user_id FK or a salted hash of email for dedup. If email required, define retention and access controls.
| // fetch tokens+language | ||
| targets, err := s.notificationService.GetUsersWithLanguageByEmails(recipients) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(targets) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // group by language and filter out already-sent | ||
| languageToTokens := map[string][]string{} | ||
| languageToEmails := map[string][]string{} | ||
| for _, t := range targets { | ||
| sent, err := s.AlreadySent(eventID, t.Email, EventNotificationCreated) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if sent || t.NotificationToken == "" { | ||
| continue | ||
| } | ||
| languageToTokens[t.LanguageCode] = append(languageToTokens[t.LanguageCode], t.NotificationToken) | ||
| languageToEmails[t.LanguageCode] = append(languageToEmails[t.LanguageCode], t.Email) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Token readiness and chunking
Ensure GetUsersWithLanguageByEmails returns unique, active tokens and that SendPushNotification chunks requests per provider limits (e.g., Expo). Otherwise large sends may fail.
🏁 Script executed:
#!/bin/bash
# Inspect implementations
rg -n -C3 'GetUsersWithLanguageByEmails|SendPushNotification' services || trueLength of output: 3978
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full implementations of both functions
rg -n -A 40 'func \(ns \*NotificationService\) GetUsersWithLanguageByEmails' services/notification.go
echo "---"
rg -n -A 60 'func \(ns \*NotificationService\) SendPushNotification' services/notification.go
Length of output: 3718
Deduplicate tokens and batch notifications
GetUsersWithLanguageByEmails should enforce unique tokens (e.g., SQL DISTINCT or in-code set) and SendPushNotification should group tokens into ≤100-per-request batches (Expo limit) to avoid rate limits and failures.
🤖 Prompt for AI Agents
In services/event_notification.go around lines 122 to 144, the code currently
collects tokens per language but does not deduplicate tokens and does not batch
requests; update the logic to (1) deduplicate notification tokens per language
(use an in-memory set when iterating targets or ensure
GetUsersWithLanguageByEmails returns unique tokens) while keeping a
representative list of emails per unique token, and (2) when calling
SendPushNotification (or wherever notifications are sent) split each language's
token list into chunks of at most 100 tokens and send one request per chunk to
respect Expo limits and avoid rate limits/failures.
| // SendDueOneHourReminders scans for events starting within the next hour and sends reminders if not already sent | ||
| func (s *EventNotificationService) SendDueOneHourReminders() error { | ||
| now := utils.Now() | ||
| lower := now.Add(1 * time.Hour) | ||
| upper := lower.Add(5 * time.Minute) | ||
|
|
||
| rows, err := s.db.Query(` | ||
| SELECT id_events FROM events | ||
| WHERE start_date >= $1 AND start_date < $2 | ||
| `, lower, upper) | ||
| if err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
Indexes and windowing: ensure query performance and correctness
- Ensure index on events.start_date to keep the scan fast.
- Confirm unique index supporting ON CONFLICT(id_events,email,notification_type).
- If jobs can drift >5m, consider widening the window or running every minute.
Run to verify:
Also applies to: 224-231
🏁 Script executed:
#!/bin/bash
# Check migration creating event_notifications_log and its unique index
rg -nC2 -i 'event_notifications_log|notification_type|unique' db/migrations || true
# Confirm index on events.start_date
rg -nC2 -i 'create\s+index.*start_date|index.*start_date|events.*start_date' db/migrations || trueLength of output: 8647
I'll search for the events table definition and any indexes on start_date to verify the review comment's concerns.
🏁 Script executed:
#!/bin/bash
# Search for events table definition and start_date index more comprehensively
rg -n 'CREATE TABLE.*events|CREATE INDEX.*start_date' db/migrations --type=sql || true
# Check if there's an index file or separate index migration
fd -e sql db/migrations -exec grep -l 'events' {} \; | head -10Length of output: 786
🏁 Script executed:
#!/bin/bash
# Look for start_date index specifically on events table
rg -n 'idx.*start_date|start_date.*idx' db/migrations --type=sql || true
# Check the events table definition
rg -n -A 20 'CREATE TABLE.*events\b' db/migrations --type=sql | head -40Length of output: 1784
🏁 Script executed:
#!/bin/bash
# Find events table definition and any indexes on start_date
rg -n -A 25 'CREATE TABLE.*events\b' db/migrations --type=sql
# Search for any index on start_date
rg -n 'CREATE INDEX.*start_date' db/migrations --type=sqlLength of output: 2067
Add a B-tree index on events.start_date
- Create an index (e.g.
CREATE INDEX idx_events_start_date ON events(start_date);) to optimize the 1-hour reminder query. - The unique constraint on
(id_events, email, notification_type)is already defined in00019_event_notifications_log.sql. - If job drift may exceed 5 min, widen the window or run the reminder job more frequently.
🤖 Prompt for AI Agents
In services/event_notification.go around lines 198 to 208, the reminder query
that scans events.start_date for the 1-hour window will benefit from a B-tree
index; add a migration that creates a B-tree index on events.start_date (e.g.
CREATE INDEX idx_events_start_date ON events(start_date);) to speed the range
scan, and apply the migration alongside deployment; additionally, if job drift
could exceed 5 minutes either widen the upper window or increase job frequency
to ensure no events are missed.
Summary by CodeRabbit
New Features
Internationalization