Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a notification preferences feature comprising database migration, models, repository layer, service layer, controller, and routing. It enables users to manage six boolean notification settings with full CRUD operations and automatic default creation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/controllers/notification_preferences.go`:
- Around line 37-40: Replace the unsafe direct type assertion
c.Locals("userID").(string) with the safe helper validators.ExtractUserID and
handle its error return; specifically update all controller methods
CreatePreferences, UpdatePreferences, DeletePreferences, GetOrCreateDefault (and
the other method using the assertion) to call validators.ExtractUserID(c) (or
the package's equivalent), check for a non-nil error and return
errs.Unauthorized() (or the existing error flow) when extraction fails, then use
the returned userID for subsequent calls instead of casting Locals directly.
In `@backend/internal/migrations/20260226233030_notification_preferences.sql`:
- Around line 3-13: The boolean columns in the notification_preferences table
(push_enabled, upcoming_trip, voting_reminders, finalized_decisions,
trip_activity, deadline_reminders) currently have DEFAULT true but are nullable;
add NOT NULL to each of these columns so they cannot be set to NULL (update the
CREATE TABLE statement for notification_preferences to mark each boolean column
as NOT NULL) and ensure the migration enforces non-null booleans to match
application expectations.
In `@backend/internal/models/notifications.go`:
- Around line 30-37: Remove the redundant validate:"omitempty" struct tags from
UpdateNotificationPreferencesRequest pointer fields (PushEnabled, UpcomingTrip,
VotingReminders, FinalizedDecisions, TripActivity, DeadlineReminders) because
nil pointer fields are already skipped by the validator; update the struct
definition to use either no validate tag or only include concrete validation
rules where needed for each field.
- Around line 9-19: The NotificationPreferences struct lacks an explicit Bun
table tag which can cause table name inference mismatches; add a struct-level
tag bun:"table:notification_preferences" to the NotificationPreferences type
(update the type declaration for NotificationPreferences) so Bun uses the
explicit table name, keeping it consistent with other models like Pitches and
preventing ORM inference issues.
In `@backend/internal/repository/notification_preferences.go`:
- Around line 113-120: The Delete method on notificationPreferencesRepository
currently succeeds even when no rows are affected, while Update returns
ErrNotFound; make the behavior consistent by either (A) returning ErrNotFound
when the NewDelete().Exec(ctx) reports zero rows affected (inspect Exec result
to get RowsAffected and return repository.ErrNotFound), or (B) explicitly
document in the Delete method comment that delete is intentionally idempotent
and will not return ErrNotFound; update the tests accordingly and reference the
methods notificationPreferencesRepository.Delete and the Update behavior and
ErrNotFound constant when making the change.
- Around line 55-111: The Update method currently runs an UPDATE then a separate
SELECT, causing a race; modify the update flow in Update (symbols: updateQuery,
r.db.NewUpdate(), Exec(ctx)) to append a RETURNING * (or RETURNING column list)
to the updateQuery and scan the returned row directly into a
models.NotificationPreferences instance (e.g., updatedPrefs) instead of calling
Exec + RowsAffected + NewSelect; remove the subsequent r.db.NewSelect() block
and the rowsAffected logic, and handle the case where the RETURNING scan yields
no rows by returning errs.ErrNotFound or the appropriate error.
In `@backend/internal/repository/repository.go`:
- Line 48: NotificationPreferences is being initialized directly with a struct
literal while other repos use constructors; add a constructor function named
NewNotificationPreferencesRepository that returns
NotificationPreferencesRepository and constructs
¬ificationPreferencesRepository{db: db}, then replace the direct struct
literal initialization of NotificationPreferences with a call to
NewNotificationPreferencesRepository(db); reference the types and functions
notificationPreferencesRepository, NotificationPreferencesRepository, and
NewNotificationPreferencesRepository when making the change.
In `@backend/internal/services/notification_preferences.go`:
- Around line 34-65: Create a single source of truth for the default
NotificationPreferences to avoid duplication: implement a helper like
newDefaultPreferences(userID uuid.UUID) *models.NotificationPreferences that
constructs the default struct (PushEnabled, UpcomingTrip, VotingReminders,
FinalizedDecisions, TripActivity, DeadlineReminders all true) and replace the
inline defaults in both CreatePreferences and GetOrCreateDefaultPreferences to
call this helper, then apply the request overrides in CreatePreferences (keep
the existing nil checks and assignments) before calling
s.NotificationPreferences.Create.
- Around line 75-95: GetOrCreateDefaultPreferences has a race: concurrent Find
-> Create causes unique constraint errors; replace the Find + Create flow in
GetOrCreateDefaultPreferences with an atomic call to
s.NotificationPreferences.Upsert(ctx, prefs) (using the same prefs struct
currently built) and return the upserted preferences and error; if you need to
preserve existing non-default fields instead of overwriting, implement INSERT
... ON CONFLICT DO NOTHING then re-Find, otherwise use Upsert to ensure a single
atomic operation and avoid propagating raw DB unique-constraint errors.
In `@backend/internal/tests/notification_preferences_test.go`:
- Around line 281-337: The current test suite lacks a concurrency test for the
GetOrCreateDefaultPreferences path, leaving the Find-then-Create race
unverified; add a new subtest inside TestGetOrCreateDefaultPreferences that
spawns multiple goroutines which simultaneously POST to
"/api/v1/users/me/notification-preferences/default" for the same userID (use the
same test app and createUser helper), collect responses/errors, and then assert
that all responses are OK and the resulting preference record is valid and
identical (e.g., same user_id and default flags) and that no duplicate records
were created; this will exercise the GetOrCreateDefault service code path under
contention and reveal race conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 584a2a33-5b65-45b6-8558-bbc0a23ff0b0
📒 Files selected for processing (10)
backend/internal/controllers/notification_preferences.gobackend/internal/migrations/20260226233030_notification_preferences.sqlbackend/internal/models/notifications.gobackend/internal/repository/notification_preferences.gobackend/internal/repository/repository.gobackend/internal/server/routers/notification_preferences.gobackend/internal/server/routers/routers.gobackend/internal/services/notification_preferences.gobackend/internal/tests/mocks/mock_S3PresignClient.gobackend/internal/tests/notification_preferences_test.go
Comment on lines
+3
to
+13
| CREATE TABLE notification_preferences ( | ||
| user_id UUID PRIMARY KEY REFERENCES users(id) ON DELETE CASCADE, | ||
| push_enabled BOOLEAN DEFAULT true, | ||
| upcoming_trip BOOLEAN DEFAULT true, | ||
| voting_reminders BOOLEAN DEFAULT true, | ||
| finalized_decisions BOOLEAN DEFAULT true, | ||
| trip_activity BOOLEAN DEFAULT true, | ||
| deadline_reminders BOOLEAN DEFAULT true, | ||
| created_at TIMESTAMP DEFAULT NOW(), | ||
| updated_at TIMESTAMP DEFAULT NOW() | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding NOT NULL constraints on boolean columns.
The boolean columns default to true but lack NOT NULL constraints. Direct SQL manipulation could set them to NULL, potentially causing unexpected behavior in application code that expects boolean values.
Suggested improvement
CREATE TABLE notification_preferences (
user_id UUID PRIMARY KEY REFERENCES users(id) ON DELETE CASCADE,
- push_enabled BOOLEAN DEFAULT true,
- upcoming_trip BOOLEAN DEFAULT true,
- voting_reminders BOOLEAN DEFAULT true,
- finalized_decisions BOOLEAN DEFAULT true,
- trip_activity BOOLEAN DEFAULT true,
- deadline_reminders BOOLEAN DEFAULT true,
- created_at TIMESTAMP DEFAULT NOW(),
- updated_at TIMESTAMP DEFAULT NOW()
+ push_enabled BOOLEAN NOT NULL DEFAULT true,
+ upcoming_trip BOOLEAN NOT NULL DEFAULT true,
+ voting_reminders BOOLEAN NOT NULL DEFAULT true,
+ finalized_decisions BOOLEAN NOT NULL DEFAULT true,
+ trip_activity BOOLEAN NOT NULL DEFAULT true,
+ deadline_reminders BOOLEAN NOT NULL DEFAULT true,
+ created_at TIMESTAMP NOT NULL DEFAULT NOW(),
+ updated_at TIMESTAMP NOT NULL DEFAULT NOW()
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE notification_preferences ( | |
| user_id UUID PRIMARY KEY REFERENCES users(id) ON DELETE CASCADE, | |
| push_enabled BOOLEAN DEFAULT true, | |
| upcoming_trip BOOLEAN DEFAULT true, | |
| voting_reminders BOOLEAN DEFAULT true, | |
| finalized_decisions BOOLEAN DEFAULT true, | |
| trip_activity BOOLEAN DEFAULT true, | |
| deadline_reminders BOOLEAN DEFAULT true, | |
| created_at TIMESTAMP DEFAULT NOW(), | |
| updated_at TIMESTAMP DEFAULT NOW() | |
| ); | |
| CREATE TABLE notification_preferences ( | |
| user_id UUID PRIMARY KEY REFERENCES users(id) ON DELETE CASCADE, | |
| push_enabled BOOLEAN NOT NULL DEFAULT true, | |
| upcoming_trip BOOLEAN NOT NULL DEFAULT true, | |
| voting_reminders BOOLEAN NOT NULL DEFAULT true, | |
| finalized_decisions BOOLEAN NOT NULL DEFAULT true, | |
| trip_activity BOOLEAN NOT NULL DEFAULT true, | |
| deadline_reminders BOOLEAN NOT NULL DEFAULT true, | |
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | |
| updated_at TIMESTAMP NOT NULL DEFAULT NOW() | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/migrations/20260226233030_notification_preferences.sql`
around lines 3 - 13, The boolean columns in the notification_preferences table
(push_enabled, upcoming_trip, voting_reminders, finalized_decisions,
trip_activity, deadline_reminders) currently have DEFAULT true but are nullable;
add NOT NULL to each of these columns so they cannot be set to NULL (update the
CREATE TABLE statement for notification_preferences to mark each boolean column
as NOT NULL) and ensure the migration enforces non-null booleans to match
application expectations.
Comment on lines
+9
to
+19
| type NotificationPreferences struct { | ||
| UserID uuid.UUID `bun:"user_id,pk,type:uuid" json:"user_id"` | ||
| PushEnabled bool `bun:"push_enabled" json:"push_enabled"` | ||
| UpcomingTrip bool `bun:"upcoming_trip" json:"upcoming_trip"` | ||
| VotingReminders bool `bun:"voting_reminders" json:"voting_reminders"` | ||
| FinalizedDecisions bool `bun:"finalized_decisions" json:"finalized_decisions"` | ||
| TripActivity bool `bun:"trip_activity" json:"trip_activity"` | ||
| DeadlineReminders bool `bun:"deadline_reminders" json:"deadline_reminders"` | ||
| CreatedAt time.Time `bun:"created_at,nullzero" json:"created_at"` | ||
| UpdatedAt time.Time `bun:"updated_at,nullzero" json:"updated_at"` | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other models in the codebase use explicit bun table tags
rg -n 'bun:"table:' backend/internal/models/Repository: GenerateNU/toggo
Length of output: 335
Add explicit table name to the struct for Bun ORM.
Other models in this codebase use explicit bun:"table:..." tags on their structs. The pitches.go model includes a comment explaining that Bun's default table name inference can cause issues. Add an explicit table tag to NotificationPreferences for consistency and to prevent potential mismatches.
Suggested fix
type NotificationPreferences struct {
+ bun.BaseModel `bun:"table:notification_preferences"`
UserID uuid.UUID `bun:"user_id,pk,type:uuid" json:"user_id"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type NotificationPreferences struct { | |
| UserID uuid.UUID `bun:"user_id,pk,type:uuid" json:"user_id"` | |
| PushEnabled bool `bun:"push_enabled" json:"push_enabled"` | |
| UpcomingTrip bool `bun:"upcoming_trip" json:"upcoming_trip"` | |
| VotingReminders bool `bun:"voting_reminders" json:"voting_reminders"` | |
| FinalizedDecisions bool `bun:"finalized_decisions" json:"finalized_decisions"` | |
| TripActivity bool `bun:"trip_activity" json:"trip_activity"` | |
| DeadlineReminders bool `bun:"deadline_reminders" json:"deadline_reminders"` | |
| CreatedAt time.Time `bun:"created_at,nullzero" json:"created_at"` | |
| UpdatedAt time.Time `bun:"updated_at,nullzero" json:"updated_at"` | |
| } | |
| type NotificationPreferences struct { | |
| bun.BaseModel `bun:"table:notification_preferences"` | |
| UserID uuid.UUID `bun:"user_id,pk,type:uuid" json:"user_id"` | |
| PushEnabled bool `bun:"push_enabled" json:"push_enabled"` | |
| UpcomingTrip bool `bun:"upcoming_trip" json:"upcoming_trip"` | |
| VotingReminders bool `bun:"voting_reminders" json:"voting_reminders"` | |
| FinalizedDecisions bool `bun:"finalized_decisions" json:"finalized_decisions"` | |
| TripActivity bool `bun:"trip_activity" json:"trip_activity"` | |
| DeadlineReminders bool `bun:"deadline_reminders" json:"deadline_reminders"` | |
| CreatedAt time.Time `bun:"created_at,nullzero" json:"created_at"` | |
| UpdatedAt time.Time `bun:"updated_at,nullzero" json:"updated_at"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/models/notifications.go` around lines 9 - 19, The
NotificationPreferences struct lacks an explicit Bun table tag which can cause
table name inference mismatches; add a struct-level tag
bun:"table:notification_preferences" to the NotificationPreferences type (update
the type declaration for NotificationPreferences) so Bun uses the explicit table
name, keeping it consistent with other models like Pitches and preventing ORM
inference issues.
Comment on lines
+30
to
+37
| type UpdateNotificationPreferencesRequest struct { | ||
| PushEnabled *bool `validate:"omitempty" json:"push_enabled"` | ||
| UpcomingTrip *bool `validate:"omitempty" json:"upcoming_trip"` | ||
| VotingReminders *bool `validate:"omitempty" json:"voting_reminders"` | ||
| FinalizedDecisions *bool `validate:"omitempty" json:"finalized_decisions"` | ||
| TripActivity *bool `validate:"omitempty" json:"trip_activity"` | ||
| DeadlineReminders *bool `validate:"omitempty" json:"deadline_reminders"` | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The validate:"omitempty" tags on pointer fields are redundant.
For pointer types, omitempty only means "skip validation if nil," which is already the default behavior for nil pointers. These tags can be removed without changing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/models/notifications.go` around lines 30 - 37, Remove the
redundant validate:"omitempty" struct tags from
UpdateNotificationPreferencesRequest pointer fields (PushEnabled, UpcomingTrip,
VotingReminders, FinalizedDecisions, TripActivity, DeadlineReminders) because
nil pointer fields are already skipped by the validator; update the struct
definition to use either no validate tag or only include concrete validation
rules where needed for each field.
Comment on lines
+55
to
+111
| func (r *notificationPreferencesRepository) Update(ctx context.Context, userID uuid.UUID, req *models.UpdateNotificationPreferencesRequest) (*models.NotificationPreferences, error) { | ||
| updateQuery := r.db.NewUpdate(). | ||
| Model(&models.NotificationPreferences{}). | ||
| Where("user_id = ?", userID) | ||
|
|
||
| if req.PushEnabled != nil { | ||
| updateQuery = updateQuery.Set("push_enabled = ?", *req.PushEnabled) | ||
| } | ||
|
|
||
| if req.UpcomingTrip != nil { | ||
| updateQuery = updateQuery.Set("upcoming_trip = ?", *req.UpcomingTrip) | ||
| } | ||
|
|
||
| if req.VotingReminders != nil { | ||
| updateQuery = updateQuery.Set("voting_reminders = ?", *req.VotingReminders) | ||
| } | ||
|
|
||
| if req.FinalizedDecisions != nil { | ||
| updateQuery = updateQuery.Set("finalized_decisions = ?", *req.FinalizedDecisions) | ||
| } | ||
|
|
||
| if req.TripActivity != nil { | ||
| updateQuery = updateQuery.Set("trip_activity = ?", *req.TripActivity) | ||
| } | ||
|
|
||
| if req.DeadlineReminders != nil { | ||
| updateQuery = updateQuery.Set("deadline_reminders = ?", *req.DeadlineReminders) | ||
| } | ||
|
|
||
| updateQuery = updateQuery.Set("updated_at = NOW()") | ||
|
|
||
| result, err := updateQuery.Exec(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to update notification preferences: %w", err) | ||
| } | ||
|
|
||
| rowsAffected, err := result.RowsAffected() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check update result: %w", err) | ||
| } | ||
|
|
||
| if rowsAffected == 0 { | ||
| return nil, errs.ErrNotFound | ||
| } | ||
|
|
||
| updatedPrefs := &models.NotificationPreferences{} | ||
| err = r.db.NewSelect(). | ||
| Model(updatedPrefs). | ||
| Where("user_id = ?", userID). | ||
| Scan(ctx) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch updated notification preferences: %w", err) | ||
| } | ||
|
|
||
| return updatedPrefs, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Non-atomic update and fetch creates a potential race condition.
The Update method executes an UPDATE query and then a separate SELECT to fetch the updated record. Between these two operations, another request could modify the data. Use RETURNING * directly on the update query to make this atomic.
Suggested fix
func (r *notificationPreferencesRepository) Update(ctx context.Context, userID uuid.UUID, req *models.UpdateNotificationPreferencesRequest) (*models.NotificationPreferences, error) {
+ updatedPrefs := &models.NotificationPreferences{}
updateQuery := r.db.NewUpdate().
- Model(&models.NotificationPreferences{}).
+ Model(updatedPrefs).
Where("user_id = ?", userID)
if req.PushEnabled != nil {
updateQuery = updateQuery.Set("push_enabled = ?", *req.PushEnabled)
}
// ... other field updates ...
updateQuery = updateQuery.Set("updated_at = NOW()")
+ updateQuery = updateQuery.Returning("*")
- result, err := updateQuery.Exec(ctx)
+ result, err := updateQuery.Exec(ctx)
if err != nil {
return nil, fmt.Errorf("failed to update notification preferences: %w", err)
}
rowsAffected, err := result.RowsAffected()
if err != nil {
return nil, fmt.Errorf("failed to check update result: %w", err)
}
if rowsAffected == 0 {
return nil, errs.ErrNotFound
}
- updatedPrefs := &models.NotificationPreferences{}
- err = r.db.NewSelect().
- Model(updatedPrefs).
- Where("user_id = ?", userID).
- Scan(ctx)
-
- if err != nil {
- return nil, fmt.Errorf("failed to fetch updated notification preferences: %w", err)
- }
-
return updatedPrefs, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *notificationPreferencesRepository) Update(ctx context.Context, userID uuid.UUID, req *models.UpdateNotificationPreferencesRequest) (*models.NotificationPreferences, error) { | |
| updateQuery := r.db.NewUpdate(). | |
| Model(&models.NotificationPreferences{}). | |
| Where("user_id = ?", userID) | |
| if req.PushEnabled != nil { | |
| updateQuery = updateQuery.Set("push_enabled = ?", *req.PushEnabled) | |
| } | |
| if req.UpcomingTrip != nil { | |
| updateQuery = updateQuery.Set("upcoming_trip = ?", *req.UpcomingTrip) | |
| } | |
| if req.VotingReminders != nil { | |
| updateQuery = updateQuery.Set("voting_reminders = ?", *req.VotingReminders) | |
| } | |
| if req.FinalizedDecisions != nil { | |
| updateQuery = updateQuery.Set("finalized_decisions = ?", *req.FinalizedDecisions) | |
| } | |
| if req.TripActivity != nil { | |
| updateQuery = updateQuery.Set("trip_activity = ?", *req.TripActivity) | |
| } | |
| if req.DeadlineReminders != nil { | |
| updateQuery = updateQuery.Set("deadline_reminders = ?", *req.DeadlineReminders) | |
| } | |
| updateQuery = updateQuery.Set("updated_at = NOW()") | |
| result, err := updateQuery.Exec(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to update notification preferences: %w", err) | |
| } | |
| rowsAffected, err := result.RowsAffected() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to check update result: %w", err) | |
| } | |
| if rowsAffected == 0 { | |
| return nil, errs.ErrNotFound | |
| } | |
| updatedPrefs := &models.NotificationPreferences{} | |
| err = r.db.NewSelect(). | |
| Model(updatedPrefs). | |
| Where("user_id = ?", userID). | |
| Scan(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch updated notification preferences: %w", err) | |
| } | |
| return updatedPrefs, nil | |
| } | |
| func (r *notificationPreferencesRepository) Update(ctx context.Context, userID uuid.UUID, req *models.UpdateNotificationPreferencesRequest) (*models.NotificationPreferences, error) { | |
| updatedPrefs := &models.NotificationPreferences{} | |
| updateQuery := r.db.NewUpdate(). | |
| Model(updatedPrefs). | |
| Where("user_id = ?", userID) | |
| if req.PushEnabled != nil { | |
| updateQuery = updateQuery.Set("push_enabled = ?", *req.PushEnabled) | |
| } | |
| if req.UpcomingTrip != nil { | |
| updateQuery = updateQuery.Set("upcoming_trip = ?", *req.UpcomingTrip) | |
| } | |
| if req.VotingReminders != nil { | |
| updateQuery = updateQuery.Set("voting_reminders = ?", *req.VotingReminders) | |
| } | |
| if req.FinalizedDecisions != nil { | |
| updateQuery = updateQuery.Set("finalized_decisions = ?", *req.FinalizedDecisions) | |
| } | |
| if req.TripActivity != nil { | |
| updateQuery = updateQuery.Set("trip_activity = ?", *req.TripActivity) | |
| } | |
| if req.DeadlineReminders != nil { | |
| updateQuery = updateQuery.Set("deadline_reminders = ?", *req.DeadlineReminders) | |
| } | |
| updateQuery = updateQuery.Set("updated_at = NOW()") | |
| updateQuery = updateQuery.Returning("*") | |
| result, err := updateQuery.Exec(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to update notification preferences: %w", err) | |
| } | |
| rowsAffected, err := result.RowsAffected() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to check update result: %w", err) | |
| } | |
| if rowsAffected == 0 { | |
| return nil, errs.ErrNotFound | |
| } | |
| return updatedPrefs, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/repository/notification_preferences.go` around lines 55 -
111, The Update method currently runs an UPDATE then a separate SELECT, causing
a race; modify the update flow in Update (symbols: updateQuery,
r.db.NewUpdate(), Exec(ctx)) to append a RETURNING * (or RETURNING column list)
to the updateQuery and scan the returned row directly into a
models.NotificationPreferences instance (e.g., updatedPrefs) instead of calling
Exec + RowsAffected + NewSelect; remove the subsequent r.db.NewSelect() block
and the rowsAffected logic, and handle the case where the RETURNING scan yields
no rows by returning errs.ErrNotFound or the appropriate error.
| ActivityRSVP: NewActivityRSVPRepository(db), | ||
| TripInvite: NewTripInviteRepository(db), | ||
| Search: NewSearchRepository(db), | ||
| NotificationPreferences: ¬ificationPreferencesRepository{db: db}, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent repository initialization pattern.
All other repositories use constructor functions (e.g., NewUserRepository(db), NewTripRepository(db)), but NotificationPreferences is initialized directly with a struct literal. Add a constructor function for consistency.
Suggested fix
In notification_preferences.go, add:
func NewNotificationPreferencesRepository(db *bun.DB) NotificationPreferencesRepository {
return ¬ificationPreferencesRepository{db: db}
}Then in repository.go:
- NotificationPreferences: ¬ificationPreferencesRepository{db: db},
+ NotificationPreferences: NewNotificationPreferencesRepository(db),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/repository/repository.go` at line 48,
NotificationPreferences is being initialized directly with a struct literal
while other repos use constructors; add a constructor function named
NewNotificationPreferencesRepository that returns
NotificationPreferencesRepository and constructs
¬ificationPreferencesRepository{db: db}, then replace the direct struct
literal initialization of NotificationPreferences with a call to
NewNotificationPreferencesRepository(db); reference the types and functions
notificationPreferencesRepository, NotificationPreferencesRepository, and
NewNotificationPreferencesRepository when making the change.
Comment on lines
+34
to
+65
| func (s *NotificationPreferencesService) CreatePreferences(ctx context.Context, userID uuid.UUID, req models.CreateNotificationPreferencesRequest) (*models.NotificationPreferences, error) { | ||
| prefs := &models.NotificationPreferences{ | ||
| UserID: userID, | ||
| PushEnabled: true, | ||
| UpcomingTrip: true, | ||
| VotingReminders: true, | ||
| FinalizedDecisions: true, | ||
| TripActivity: true, | ||
| DeadlineReminders: true, | ||
| } | ||
|
|
||
| if req.PushEnabled != nil { | ||
| prefs.PushEnabled = *req.PushEnabled | ||
| } | ||
| if req.UpcomingTrip != nil { | ||
| prefs.UpcomingTrip = *req.UpcomingTrip | ||
| } | ||
| if req.VotingReminders != nil { | ||
| prefs.VotingReminders = *req.VotingReminders | ||
| } | ||
| if req.FinalizedDecisions != nil { | ||
| prefs.FinalizedDecisions = *req.FinalizedDecisions | ||
| } | ||
| if req.TripActivity != nil { | ||
| prefs.TripActivity = *req.TripActivity | ||
| } | ||
| if req.DeadlineReminders != nil { | ||
| prefs.DeadlineReminders = *req.DeadlineReminders | ||
| } | ||
|
|
||
| return s.NotificationPreferences.Create(ctx, prefs) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate default values definition.
The default preference values (true for all flags) are defined in both CreatePreferences (lines 37-42) and GetOrCreateDefaultPreferences (lines 86-91). Extract these to a shared helper or constant to avoid drift.
Suggested refactor
func newDefaultPreferences(userID uuid.UUID) *models.NotificationPreferences {
return &models.NotificationPreferences{
UserID: userID,
PushEnabled: true,
UpcomingTrip: true,
VotingReminders: true,
FinalizedDecisions: true,
TripActivity: true,
DeadlineReminders: true,
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/services/notification_preferences.go` around lines 34 - 65,
Create a single source of truth for the default NotificationPreferences to avoid
duplication: implement a helper like newDefaultPreferences(userID uuid.UUID)
*models.NotificationPreferences that constructs the default struct (PushEnabled,
UpcomingTrip, VotingReminders, FinalizedDecisions, TripActivity,
DeadlineReminders all true) and replace the inline defaults in both
CreatePreferences and GetOrCreateDefaultPreferences to call this helper, then
apply the request overrides in CreatePreferences (keep the existing nil checks
and assignments) before calling s.NotificationPreferences.Create.
Comment on lines
+75
to
+95
| func (s *NotificationPreferencesService) GetOrCreateDefaultPreferences(ctx context.Context, userID uuid.UUID) (*models.NotificationPreferences, error) { | ||
| existingPrefs, err := s.NotificationPreferences.Find(ctx, userID) | ||
| if err == nil { | ||
| return existingPrefs, nil | ||
| } | ||
| if !errs.IsNotFound(err) { | ||
| return nil, err | ||
| } | ||
|
|
||
| prefs := &models.NotificationPreferences{ | ||
| UserID: userID, | ||
| PushEnabled: true, | ||
| UpcomingTrip: true, | ||
| VotingReminders: true, | ||
| FinalizedDecisions: true, | ||
| TripActivity: true, | ||
| DeadlineReminders: true, | ||
| } | ||
|
|
||
| return s.NotificationPreferences.Create(ctx, prefs) | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition in GetOrCreateDefaultPreferences.
Two concurrent calls for the same user could both pass the Find check (both get ErrNotFound) and then both attempt Create. The second insert will fail with a unique constraint violation. This raw database error propagates without domain mapping, potentially returning a 500 instead of the expected behavior.
Use the existing Upsert method to make this atomic:
Suggested fix
func (s *NotificationPreferencesService) GetOrCreateDefaultPreferences(ctx context.Context, userID uuid.UUID) (*models.NotificationPreferences, error) {
- existingPrefs, err := s.NotificationPreferences.Find(ctx, userID)
- if err == nil {
- return existingPrefs, nil
- }
- if !errs.IsNotFound(err) {
- return nil, err
- }
-
prefs := &models.NotificationPreferences{
UserID: userID,
PushEnabled: true,
UpcomingTrip: true,
VotingReminders: true,
FinalizedDecisions: true,
TripActivity: true,
DeadlineReminders: true,
}
- return s.NotificationPreferences.Create(ctx, prefs)
+ return s.NotificationPreferences.Upsert(ctx, prefs)
}Note: Using Upsert changes semantics slightly - it will overwrite existing preferences with defaults. If preserving existing preferences is required, use INSERT ... ON CONFLICT DO NOTHING and then Find.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *NotificationPreferencesService) GetOrCreateDefaultPreferences(ctx context.Context, userID uuid.UUID) (*models.NotificationPreferences, error) { | |
| existingPrefs, err := s.NotificationPreferences.Find(ctx, userID) | |
| if err == nil { | |
| return existingPrefs, nil | |
| } | |
| if !errs.IsNotFound(err) { | |
| return nil, err | |
| } | |
| prefs := &models.NotificationPreferences{ | |
| UserID: userID, | |
| PushEnabled: true, | |
| UpcomingTrip: true, | |
| VotingReminders: true, | |
| FinalizedDecisions: true, | |
| TripActivity: true, | |
| DeadlineReminders: true, | |
| } | |
| return s.NotificationPreferences.Create(ctx, prefs) | |
| } | |
| func (s *NotificationPreferencesService) GetOrCreateDefaultPreferences(ctx context.Context, userID uuid.UUID) (*models.NotificationPreferences, error) { | |
| prefs := &models.NotificationPreferences{ | |
| UserID: userID, | |
| PushEnabled: true, | |
| UpcomingTrip: true, | |
| VotingReminders: true, | |
| FinalizedDecisions: true, | |
| TripActivity: true, | |
| DeadlineReminders: true, | |
| } | |
| return s.NotificationPreferences.Upsert(ctx, prefs) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/services/notification_preferences.go` around lines 75 - 95,
GetOrCreateDefaultPreferences has a race: concurrent Find -> Create causes
unique constraint errors; replace the Find + Create flow in
GetOrCreateDefaultPreferences with an atomic call to
s.NotificationPreferences.Upsert(ctx, prefs) (using the same prefs struct
currently built) and return the upserted preferences and error; if you need to
preserve existing non-default fields instead of overwriting, implement INSERT
... ON CONFLICT DO NOTHING then re-Find, otherwise use Upsert to ensure a single
atomic operation and avoid propagating raw DB unique-constraint errors.
Comment on lines
+281
to
+337
| func TestGetOrCreateDefaultPreferences(t *testing.T) { | ||
| app := fakes.GetSharedTestApp() | ||
|
|
||
| t.Run("get-or-create returns existing preferences", func(t *testing.T) { | ||
| userID := createUser(t, app) | ||
|
|
||
| pushEnabled := false | ||
| tripActivity := false | ||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/users/me/notification-preferences", | ||
| Method: testkit.POST, | ||
| UserID: &userID, | ||
| Body: models.CreateNotificationPreferencesRequest{ | ||
| PushEnabled: &pushEnabled, | ||
| TripActivity: &tripActivity, | ||
| }, | ||
| }). | ||
| AssertStatus(http.StatusCreated) | ||
|
|
||
| resp := testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/users/me/notification-preferences/default", | ||
| Method: testkit.POST, | ||
| UserID: &userID, | ||
| }). | ||
| AssertStatus(http.StatusOK). | ||
| GetBody() | ||
|
|
||
| require.Equal(t, false, resp["push_enabled"]) | ||
| require.Equal(t, false, resp["trip_activity"]) | ||
| }) | ||
|
|
||
| t.Run("get-or-create creates defaults when none exist", func(t *testing.T) { | ||
| userID := createUser(t, app) | ||
|
|
||
| resp := testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/users/me/notification-preferences/default", | ||
| Method: testkit.POST, | ||
| UserID: &userID, | ||
| }). | ||
| AssertStatus(http.StatusOK). | ||
| GetBody() | ||
|
|
||
| require.Equal(t, userID, resp["user_id"]) | ||
| require.Equal(t, true, resp["push_enabled"]) | ||
| require.Equal(t, true, resp["upcoming_trip"]) | ||
| require.Equal(t, true, resp["voting_reminders"]) | ||
| require.Equal(t, true, resp["finalized_decisions"]) | ||
| require.Equal(t, true, resp["trip_activity"]) | ||
| require.Equal(t, true, resp["deadline_reminders"]) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a concurrency test for GetOrCreateDefault.
The GetOrCreateDefaultPreferences service method has a potential race condition (Find-then-Create). Adding a test that makes concurrent calls to /default for the same user would help verify the behavior under contention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/tests/notification_preferences_test.go` around lines 281 -
337, The current test suite lacks a concurrency test for the
GetOrCreateDefaultPreferences path, leaving the Find-then-Create race
unverified; add a new subtest inside TestGetOrCreateDefaultPreferences that
spawns multiple goroutines which simultaneously POST to
"/api/v1/users/me/notification-preferences/default" for the same userID (use the
same test app and createUser helper), collect responses/errors, and then assert
that all responses are OK and the resulting preference record is valid and
identical (e.g., same user_id and default flags) and that no duplicate records
were created; this will exercise the GetOrCreateDefault service code path under
contention and reveal race conditions.
in-mai-space
requested changes
Mar 13, 2026
Contributor
in-mai-space
left a comment
in-mai-space
left a comment
There was a problem hiding this comment.
just some small nits but otherwise looks good to me @jleung40 🔥🔥🔥
| // @Router /api/v1/users/me/notification-preferences [get] | ||
| // @ID getNotificationPreferences | ||
| func (n *NotificationPreferencesController) GetPreferences(c *fiber.Ctx) error { | ||
| userID, err := validators.ValidateID(c.Locals("userID").(string)) |
Contributor
There was a problem hiding this comment.
i believe there's a util called ExtractUserID from validators package
| // @Router /api/v1/users/me/notification-preferences [post] | ||
| // @ID createNotificationPreferences | ||
| func (n *NotificationPreferencesController) CreatePreferences(c *fiber.Ctx) error { | ||
| userID, err := validators.ValidateID(c.Locals("userID").(string)) |
Contributor
There was a problem hiding this comment.
ditto the util here and rest of the file
| ActivityRSVP: NewActivityRSVPRepository(db), | ||
| TripInvite: NewTripInviteRepository(db), | ||
| Search: NewSearchRepository(db), | ||
| NotificationPreferences: ¬ificationPreferencesRepository{db: db}, |
Contributor
There was a problem hiding this comment.
small nit but can we make it consistent constructor with other repository
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
set up crud for notification preferences. temporary settings, to be changed as we get updates from design team on what settings should exist.
How has this been tested?
notification preferences tests
Checklist
Summary
User-visible changes
/api/v1/users/me/notification-preferenceswith GET, POST, PATCH, DELETE, and GET /default operationsChanges by category
Features
Infra
notification_preferencestable with user_id foreign key (ON DELETE CASCADE), six boolean feature flags, and timestampsTesting
Author contribution