[BE]: Push Notification Service#91
Merged
afnantuffaha merged 20 commits intomainfrom Jan 29, 2026
Merged
Conversation
7 tasks
in-mai-space
requested changes
Jan 24, 2026
Contributor
📝 WalkthroughWalkthroughAdds a push-notification subsystem: DB migration for device tokens, models and repository support, an Expo HTTP client, notification service with batching and mocks, controller and routes, and unit/integration tests covering validation, auth, and success/failure scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as Controller
participant Service as Service
participant Repository as Repository
participant ExpoAPI as ExpoAPI
Client->>Controller: POST /send (SendNotificationRequest)
Controller->>Service: SendNotification(req)
Service->>Repository: GetUsersWithDeviceTokens([userID])
Repository-->>Service: User with device token
Service->>ExpoAPI: SendNotifications([token], title, body, data)
ExpoAPI-->>Service: ExpoBulkResponse
Service-->>Controller: success or error
Controller-->>Client: 200 OK or error status
sequenceDiagram
participant Client as Client
participant Controller as Controller
participant Service as Service
participant Repository as Repository
participant ExpoAPI as ExpoAPI
Client->>Controller: POST /send-bulk (SendBulkNotificationRequest)
Controller->>Service: SendNotificationBatch(req)
Service->>Repository: GetUsersWithDeviceTokens([userIDs])
Repository-->>Service: Users with device tokens
Service->>Service: Batch tokens (max 100 per call)
loop For each batch
Service->>ExpoAPI: SendNotifications(batchTokens, title, body, data)
ExpoAPI-->>Service: ExpoBulkResponse
Service->>Service: Aggregate results & errors
end
Service-->>Controller: NotificationResponse
Controller-->>Client: 200 OK with response body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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
Comment |
afnantuffaha
force-pushed
the
amogh/BE-49
branch
from
1d8a63d to
08f2fd6
Compare
![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: 12
🤖 Fix all issues with AI agents
In `@backend/internal/controllers/notifications.go`:
- Around line 51-52: Do not return err.Error() to the client; instead log the
original error and return a generic 500 message. Replace returns like `return
fiber.NewError(http.StatusInternalServerError, err.Error())` (occurring after
`n.notificationService.SendNotification(c.Context(), req)` and the similar block
at lines ~80-83) with code that logs the error (e.g., `n.logger.Errorf(...)` or
`log.Printf(...)` including the error and any request/context info) and returns
`fiber.NewError(http.StatusInternalServerError, "internal server error")` so
internals are not leaked while preserving the original error in logs.
In `@backend/internal/models/users.go`:
- Line 28: The DeviceToken field currently accepts any string; add validation to
prevent storing invalid Expo push tokens by updating the DeviceToken tag on the
User model (DeviceToken *string) to include either a length constraint (e.g.,
validate:"omitempty,max=200") or a custom validator key (e.g.,
validate:"omitempty,expo_token"); if you choose a custom validator, register a
validator function for the "expo_token" tag in your validator setup that checks
the "ExponentPushToken[" prefix and acceptable characters/length and use that
validator when persisting or binding the User struct.
In `@backend/internal/repository/repository.go`:
- Line 32: GetUsersWithDeviceTokens currently builds a query using
bun.In(userIDs) which produces invalid SQL for empty or nil slices; add a guard
at the start of the GetUsersWithDeviceTokens function to check if len(userIDs)
== 0 and immediately return an empty []*models.User and nil error (or
alternatively append Where("FALSE") to the query) so the generated SQL is valid
and Postgres won't error on "IN ()".
In `@backend/internal/repository/users.go`:
- Around line 112-127: In GetUsersWithDeviceTokens, guard against an empty
userIDs slice before building the query: if len(userIDs) == 0 return an empty
[]*models.User{} and nil error to avoid bun.In(userIDs) producing an invalid IN
() SQL clause; keep the rest of the function using
r.db.NewSelect().Model(&users).Where("id IN (?)", bun.In(userIDs))... unchanged.
- Around line 69-74: The assignment updates["device_token"] = trimmedToken is
dead code because the surrounding logic builds SQL via updateQuery.Set (using
trimmedToken) and never reads the updates map; either remove that unused map
entry or, if the intention was to batch column updates into the updates map,
switch the implementation to apply the updates map (instead of updateQuery.Set)
later in the same function (e.g., consolidate into the existing updateQuery
logic), ensuring consistency between updates, trimmedToken, and updateQuery.Set;
specifically edit the block that references req.DeviceToken, remove the
updates["device_token"] line if you keep updateQuery.Set("device_token = ?",
trimmedToken), or replace the updateQuery.Set calls with logic that consumes the
updates map.
In `@backend/internal/services/expo_client.go`:
- Around line 25-29: NewExpoClient currently constructs an http.Client with no
timeout which can hang; update NewExpoClient (and/or the expoClient struct) to
set a sensible timeout on the http.Client (e.g. Timeout: time.Second * 10) or
accept an injected *http.Client parameter so callers can control timeouts;
modify NewExpoClient to initialize httpClient with a timeout or add a
constructor overload that takes an *http.Client and ensure expoClient.httpClient
uses that instance.
In `@backend/internal/services/notification.go`:
- Around line 62-76: The current token collection loop only builds tokens and
tokenToUserID for users with DeviceToken, which silently omits requested users
without device tokens; update the logic (around the tokens, tokenToUserID and
users variables before sending batches) to iterate the original requested user
list and, for any user with nil/empty DeviceToken, increment
response.FailureCount and append a models.NotificationError entry mapping that
user’s ID to an appropriate error (e.g., "no device token") so the final
models.NotificationResponse reflects failures for tokenless users before
proceeding to batch send.
In `@backend/internal/tests/notification_integration_test.go`:
- Around line 289-317: Update the "send bulk returns response structure" test to
assert actual values, not just keys: after receiving resp from the Request in
the t.Run block, convert/assert resp["success_count"] equals 2 and
resp["failure_count"] equals 0 (since both user1ID and user2ID have valid tokens
in this test) and assert resp["errors"] is an empty slice/array; reference the
existing SendBulkNotificationRequest, authUserID1, user1ID and user2ID and
perform type-safe checks (e.g. cast to float64/int and length checks) on the
resp map entries before using them in expectations.
- Around line 159-191: The controller currently converts all service errors to
HTTP 500; change the error mapping in the notifications controller
(notifications.go handler around the SendNotification handling) to detect the
specific service error returned when a user has no device token (use the
service's exported error sentinel or errors.Is against the error from
backend/internal/services/notification.go) and return
http.StatusUnprocessableEntity (422) instead of 500; also update the integration
test in notification_integration_test.go to assert 422 for the "send
notification to user without token" case. Ensure you use errors.Is (or compare
against the exported ErrMissingDeviceToken constant) so the handler correctly
maps that case while leaving other service errors mapped to 500.
In `@backend/internal/tests/notification_test.go`:
- Around line 138-140: The mock method
mockNotificationUserRepo.GetUsersWithDeviceTokens currently returns m.users
unfiltered; change it to filter m.users by the provided userIDs slice and return
only users whose ID is present in userIDs (e.g., build a lookup set from the
userIDs and collect matching entries from m.users), preserving the method
signature and returning the filtered slice and nil error.
- Around line 117-124: The mockNotificationUserRepo.Find implementation
currently returns nil, nil for missing users; change it to return a not-found
error instead of nil to mimic real repo behavior (e.g., return
errs.ErrNotFound). Update the method mockNotificationUserRepo.Find(ctx
context.Context, id uuid.UUID) to return (*models.User, error) where it returns
(u, nil) when found and (nil, errs.ErrNotFound) when not found so tests relying
on error handling behave like production.
In `@backend/internal/tests/user_test.go`:
- Around line 180-235: Extend TestDeviceTokenUpdate to add three subtests: one
that clears the device token by sending UpdateUserRequest with DeviceToken set
to nil or empty string and assert the API returns 200 and "device_token" is
null/empty; one that after a successful update asserts the response includes a
non-empty "device_token_updated_at" field (use AssertField or GetBody to parse
and validate it is present and parsable as a timestamp) referencing the PATCH
route "/api/v1/users/%s" and the UpdateUserRequest struct; and one that performs
a second device token update with a different value and asserts that
"device_token_updated_at" has changed compared to the first update (store the
first timestamp, perform second PATCH, then compare). Ensure you reuse
TestDeviceTokenUpdate's created userID and authUserID and use
testkit.Request/Testkit.New helpers for consistency.
Comment on lines
+51
to
+52
| if err := n.notificationService.SendNotification(c.Context(), req); err != nil { | ||
| return fiber.NewError(http.StatusInternalServerError, err.Error()) |
Contributor
There was a problem hiding this comment.
Return generic 500 errors to avoid leaking internals.
Line 51-52 and Line 80-83 return err.Error() to clients. This can expose internal details (database, network, or third‑party errors). Return a generic message and log the original error.
Proposed change
- if err := n.notificationService.SendNotification(c.Context(), req); err != nil {
- return fiber.NewError(http.StatusInternalServerError, err.Error())
- }
+ if err := n.notificationService.SendNotification(c.Context(), req); err != nil {
+ return fiber.NewError(http.StatusInternalServerError, "internal server error")
+ }
...
- response, err := n.notificationService.SendNotificationBatch(c.Context(), req)
- if err != nil {
- return fiber.NewError(http.StatusInternalServerError, err.Error())
- }
+ response, err := n.notificationService.SendNotificationBatch(c.Context(), req)
+ if err != nil {
+ return fiber.NewError(http.StatusInternalServerError, "internal server error")
+ }Also applies to: 80-83
🤖 Prompt for AI Agents
In `@backend/internal/controllers/notifications.go` around lines 51 - 52, Do not
return err.Error() to the client; instead log the original error and return a
generic 500 message. Replace returns like `return
fiber.NewError(http.StatusInternalServerError, err.Error())` (occurring after
`n.notificationService.SendNotification(c.Context(), req)` and the similar block
at lines ~80-83) with code that logs the error (e.g., `n.logger.Errorf(...)` or
`log.Printf(...)` including the error and any request/context info) and returns
`fiber.NewError(http.StatusInternalServerError, "internal server error")` so
internals are not leaked while preserving the original error in logs.
| Find(ctx context.Context, id uuid.UUID) (*models.User, error) | ||
| Update(ctx context.Context, id uuid.UUID, user *models.UpdateUserRequest) (*models.User, error) | ||
| Delete(ctx context.Context, id uuid.UUID) error | ||
| GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the implementation of GetUsersWithDeviceTokens
find backend/internal/repository -type f -name "*.go" | head -20Repository: GenerateNU/toggo
Length of output: 176
🏁 Script executed:
# Search for the implementation of GetUsersWithDeviceTokens
rg -n "GetUsersWithDeviceTokens" -B 2 -A 15 --type go backend/internal/repositoryRepository: GenerateNU/toggo
Length of output: 1652
🏁 Script executed:
# Look for any SQL query construction related to this method
rg -n "device.*token" -i -B 3 -A 3 --type go backend/internal/repositoryRepository: GenerateNU/toggo
Length of output: 2586
🏁 Script executed:
# Check if there are other uses of bun.In in the codebase to see patterns
rg -n "bun\.In\(" --type go -B 2 -A 2 backend/internal/repositoryRepository: GenerateNU/toggo
Length of output: 414
🏁 Script executed:
# Check if there are any input validation patterns elsewhere in the repository
rg -n "len\(.*\) == 0" --type go -B 1 -A 1 backend/internal/repositoryRepository: GenerateNU/toggo
Length of output: 42
🌐 Web query:
bun.In() empty slice behavior sql query postgres
💡 Result:
In Bun (the Go ORM), bun.In(slice) is meant to be used like:
q.Where("id IN (?)", bun.In(ids))and it expands the slice into the IN (...) list. (bun.uptrace.dev)
PostgreSQL does not allow an empty IN () list (it’s a syntax error), so ORMs can’t safely generate IN () when ids is empty. (postgresql.org)
What to do when the slice can be empty (recommended)
Handle the empty case in application code:
- If “no ids” should match nothing:
if len(ids) == 0 {
q = q.Where("FALSE")
} else {
q = q.Where("id IN (?)", bun.In(ids))
}- If “no ids” should mean “don’t filter”, just skip adding the
INclause whenlen(ids) == 0.
Alternative SQL pattern that does support empty arrays in Postgres
Use = ANY($1) with an array parameter; ANY works with zero-length arrays (it just matches no rows). (postgresql.org)
Citations:
- 1: https://bun.uptrace.dev/guide/placeholders.html?utm_source=openai
- 2: https://www.postgresql.org/message-id/454A654B.2000306%40krogh.cc?utm_source=openai
- 3: https://www.postgresql.org/message-id/26899.1162535774%40sss.pgh.pa.us?utm_source=openai
🌐 Web query:
golang bun ORM empty IN clause handling
💡 Result:
In Bun (uptrace/bun), the supported way to build IN (...) is to keep the SQL as IN (?) and pass bun.In(slice) as the single arg, e.g.:
q := db.NewSelect().Model(&rows).
Where("id IN (?)", bun.In(ids))This is the documented helper for IN. [1]
Empty-slice handling: SQL IN () is invalid, so you should handle len(ids)==0 yourself before adding the predicate, depending on intended semantics:
- Return no rows (common for
INfilters):if len(ids) == 0 { q = q.Where("FALSE") // or: Where("1=0") } else { q = q.Where("id IN (?)", bun.In(ids)) }
- Ignore the filter (treat empty as “no constraint”):
if len(ids) > 0 { q = q.Where("id IN (?)", bun.In(ids)) }
NOT INspecial-case: iflen(ids)==0, you typically want no predicate (i.e., everything matches), rather than emittingNOT IN (...).
The migration guide also notes the go-pg pattern WhereIn("... IN (?)", slice) becomes Where("... IN (?)", bun.In(slice)). [2]
Sources:
[1] Bun docs – SQL placeholders: bun.In helper for IN (...) https://bun.uptrace.dev/guide/placeholders.html
[2] Bun docs – Migrating from go-pg: replace WhereIn(..., slice) with Where(..., bun.In(slice)) https://bun.uptrace.dev/guide/pg-migration.html
Add validation for empty userIDs slice.
The implementation does not handle empty or nil userIDs slices. When an empty slice is passed, bun.In(userIDs) generates invalid SQL (WHERE id IN ()), which PostgreSQL rejects. Add a length check before the query:
if len(userIDs) == 0 {
return []*models.User{}, nil
}Or add Where("FALSE") to the query chain if you prefer to execute the query regardless.
🤖 Prompt for AI Agents
In `@backend/internal/repository/repository.go` at line 32,
GetUsersWithDeviceTokens currently builds a query using bun.In(userIDs) which
produces invalid SQL for empty or nil slices; add a guard at the start of the
GetUsersWithDeviceTokens function to check if len(userIDs) == 0 and immediately
return an empty []*models.User and nil error (or alternatively append
Where("FALSE") to the query) so the generated SQL is valid and Postgres won't
error on "IN ()".
Comment on lines
+112
to
+127
| func (r *userRepository) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) { | ||
| var users []*models.User | ||
|
|
||
| err := r.db.NewSelect(). | ||
| Model(&users). | ||
| Where("id IN (?)", bun.In(userIDs)). | ||
| Where("device_token IS NOT NULL"). | ||
| Where("device_token != ''"). | ||
| Scan(ctx) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get users with device tokens: %w", err) | ||
| } | ||
|
|
||
| return users, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Handle empty userIDs slice to avoid invalid SQL.
When userIDs is empty, bun.In(userIDs) generates IN () which is invalid SQL in PostgreSQL and most databases. This will cause a runtime error. Return early with an empty slice when no IDs are provided.
Proposed fix
func (r *userRepository) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) {
+ if len(userIDs) == 0 {
+ return []*models.User{}, nil
+ }
+
var users []*models.User
err := r.db.NewSelect().📝 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 *userRepository) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) { | |
| var users []*models.User | |
| err := r.db.NewSelect(). | |
| Model(&users). | |
| Where("id IN (?)", bun.In(userIDs)). | |
| Where("device_token IS NOT NULL"). | |
| Where("device_token != ''"). | |
| Scan(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get users with device tokens: %w", err) | |
| } | |
| return users, nil | |
| } | |
| func (r *userRepository) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) { | |
| if len(userIDs) == 0 { | |
| return []*models.User{}, nil | |
| } | |
| var users []*models.User | |
| err := r.db.NewSelect(). | |
| Model(&users). | |
| Where("id IN (?)", bun.In(userIDs)). | |
| Where("device_token IS NOT NULL"). | |
| Where("device_token != ''"). | |
| Scan(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get users with device tokens: %w", err) | |
| } | |
| return users, nil | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/repository/users.go` around lines 112 - 127, In
GetUsersWithDeviceTokens, guard against an empty userIDs slice before building
the query: if len(userIDs) == 0 return an empty []*models.User{} and nil error
to avoid bun.In(userIDs) producing an invalid IN () SQL clause; keep the rest of
the function using r.db.NewSelect().Model(&users).Where("id IN (?)",
bun.In(userIDs))... unchanged.
Comment on lines
+159
to
+191
| t.Run("send notification to user without token returns 500", func(t *testing.T) { | ||
| authUserID3 := fakes.GenerateUUID() | ||
| username3 := fakes.GenerateRandomUsername() | ||
| resp3 := testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/users", | ||
| Method: testkit.POST, | ||
| UserID: &authUserID3, | ||
| Body: models.CreateUserRequest{ | ||
| Name: "User Without Token", | ||
| Username: username3, | ||
| PhoneNumber: "+16175559003", | ||
| }, | ||
| }). | ||
| AssertStatus(http.StatusCreated). | ||
| GetBody() | ||
| user3ID := resp3["id"].(string) | ||
|
|
||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/notifications/send", | ||
| Method: testkit.POST, | ||
| UserID: &authUserID3, | ||
| Body: models.SendNotificationRequest{ | ||
| UserID: uuid.MustParse(user3ID), | ||
| Title: "Test", | ||
| Body: "Test", | ||
| }, | ||
| }). | ||
| AssertStatus(http.StatusInternalServerError) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for SendNotification handler in controllers
rg -n "SendNotification" backend/internal/controllers/ -A 20 --type goRepository: GenerateNU/toggo
Length of output: 3938
🏁 Script executed:
# Search for SendNotification in service implementations
rg -n "SendNotification" backend/internal/services/ -A 30 --type go | head -100Repository: GenerateNU/toggo
Length of output: 8154
Controller returns 500 for missing device token; should use 4xx status.
The service correctly rejects notifications for users without a registered device token (lines 37-39 and 42-44 in backend/internal/services/notification.go). However, the controller at backend/internal/controllers/notifications.go:51-52 indiscriminately converts all service errors to HTTP 500. Missing device token is a client-side issue (user has not registered a token), not a server error. The controller should catch this specific error and return 422 (Unprocessable Entity) or 400 (Bad Request) instead. This change will require updating both the controller error handling and the test assertion.
🤖 Prompt for AI Agents
In `@backend/internal/tests/notification_integration_test.go` around lines 159 -
191, The controller currently converts all service errors to HTTP 500; change
the error mapping in the notifications controller (notifications.go handler
around the SendNotification handling) to detect the specific service error
returned when a user has no device token (use the service's exported error
sentinel or errors.Is against the error from
backend/internal/services/notification.go) and return
http.StatusUnprocessableEntity (422) instead of 500; also update the integration
test in notification_integration_test.go to assert 422 for the "send
notification to user without token" case. Ensure you use errors.Is (or compare
against the exported ErrMissingDeviceToken constant) so the handler correctly
maps that case while leaving other service errors mapped to 500.
Comment on lines
+289
to
+317
| t.Run("send bulk returns response structure", func(t *testing.T) { | ||
| resp := testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/notifications/send-bulk", | ||
| Method: testkit.POST, | ||
| UserID: &authUserID1, | ||
| Body: models.SendBulkNotificationRequest{ | ||
| UserIDs: []uuid.UUID{ | ||
| uuid.MustParse(user1ID), | ||
| uuid.MustParse(user2ID), | ||
| }, | ||
| Title: "Bulk Test", | ||
| Body: "Testing bulk notifications", | ||
| }, | ||
| }). | ||
| AssertStatus(http.StatusOK). | ||
| GetBody() | ||
|
|
||
| if _, ok := resp["success_count"]; !ok { | ||
| t.Error("expected success_count in response") | ||
| } | ||
| if _, ok := resp["failure_count"]; !ok { | ||
| t.Error("expected failure_count in response") | ||
| } | ||
| if _, ok := resp["errors"]; !ok { | ||
| t.Error("expected errors array in response") | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen assertions on bulk response.
The test only verifies that success_count, failure_count, and errors keys exist in the response. It does not verify their values. For a more robust test, assert expected counts based on the two users with valid tokens.
Proposed improvement
if _, ok := resp["success_count"]; !ok {
t.Error("expected success_count in response")
}
+ if count, ok := resp["success_count"].(float64); !ok || count < 0 {
+ t.Errorf("expected success_count to be a non-negative number, got %v", resp["success_count"])
+ }
if _, ok := resp["failure_count"]; !ok {
t.Error("expected failure_count in response")
}
if _, ok := resp["errors"]; !ok {
t.Error("expected errors array in response")
}📝 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
| t.Run("send bulk returns response structure", func(t *testing.T) { | |
| resp := testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/notifications/send-bulk", | |
| Method: testkit.POST, | |
| UserID: &authUserID1, | |
| Body: models.SendBulkNotificationRequest{ | |
| UserIDs: []uuid.UUID{ | |
| uuid.MustParse(user1ID), | |
| uuid.MustParse(user2ID), | |
| }, | |
| Title: "Bulk Test", | |
| Body: "Testing bulk notifications", | |
| }, | |
| }). | |
| AssertStatus(http.StatusOK). | |
| GetBody() | |
| if _, ok := resp["success_count"]; !ok { | |
| t.Error("expected success_count in response") | |
| } | |
| if _, ok := resp["failure_count"]; !ok { | |
| t.Error("expected failure_count in response") | |
| } | |
| if _, ok := resp["errors"]; !ok { | |
| t.Error("expected errors array in response") | |
| } | |
| }) | |
| t.Run("send bulk returns response structure", func(t *testing.T) { | |
| resp := testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: "/api/v1/notifications/send-bulk", | |
| Method: testkit.POST, | |
| UserID: &authUserID1, | |
| Body: models.SendBulkNotificationRequest{ | |
| UserIDs: []uuid.UUID{ | |
| uuid.MustParse(user1ID), | |
| uuid.MustParse(user2ID), | |
| }, | |
| Title: "Bulk Test", | |
| Body: "Testing bulk notifications", | |
| }, | |
| }). | |
| AssertStatus(http.StatusOK). | |
| GetBody() | |
| if _, ok := resp["success_count"]; !ok { | |
| t.Error("expected success_count in response") | |
| } | |
| if count, ok := resp["success_count"].(float64); !ok || count < 0 { | |
| t.Errorf("expected success_count to be a non-negative number, got %v", resp["success_count"]) | |
| } | |
| if _, ok := resp["failure_count"]; !ok { | |
| t.Error("expected failure_count in response") | |
| } | |
| if _, ok := resp["errors"]; !ok { | |
| t.Error("expected errors array in response") | |
| } | |
| }) |
🤖 Prompt for AI Agents
In `@backend/internal/tests/notification_integration_test.go` around lines 289 -
317, Update the "send bulk returns response structure" test to assert actual
values, not just keys: after receiving resp from the Request in the t.Run block,
convert/assert resp["success_count"] equals 2 and resp["failure_count"] equals 0
(since both user1ID and user2ID have valid tokens in this test) and assert
resp["errors"] is an empty slice/array; reference the existing
SendBulkNotificationRequest, authUserID1, user1ID and user2ID and perform
type-safe checks (e.g. cast to float64/int and length checks) on the resp map
entries before using them in expectations.
Comment on lines
+117
to
+124
| func (m *mockNotificationUserRepo) Find(ctx context.Context, id uuid.UUID) (*models.User, error) { | ||
| for _, u := range m.users { | ||
| if u.ID == id { | ||
| return u, nil | ||
| } | ||
| } | ||
| return nil, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Mock Find should return an error for missing users.
Returning nil, nil when a user is not found deviates from typical repository behavior (which would return an error like errs.ErrNotFound). This may hide bugs in the service layer that depend on error handling for missing users.
Proposed fix
+import "toggo/internal/errs"
+
func (m *mockNotificationUserRepo) Find(ctx context.Context, id uuid.UUID) (*models.User, error) {
for _, u := range m.users {
if u.ID == id {
return u, nil
}
}
- return nil, nil
+ return nil, errs.ErrNotFound
}📝 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 (m *mockNotificationUserRepo) Find(ctx context.Context, id uuid.UUID) (*models.User, error) { | |
| for _, u := range m.users { | |
| if u.ID == id { | |
| return u, nil | |
| } | |
| } | |
| return nil, nil | |
| } | |
| import "toggo/internal/errs" | |
| func (m *mockNotificationUserRepo) Find(ctx context.Context, id uuid.UUID) (*models.User, error) { | |
| for _, u := range m.users { | |
| if u.ID == id { | |
| return u, nil | |
| } | |
| } | |
| return nil, errs.ErrNotFound | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/tests/notification_test.go` around lines 117 - 124, The
mockNotificationUserRepo.Find implementation currently returns nil, nil for
missing users; change it to return a not-found error instead of nil to mimic
real repo behavior (e.g., return errs.ErrNotFound). Update the method
mockNotificationUserRepo.Find(ctx context.Context, id uuid.UUID) to return
(*models.User, error) where it returns (u, nil) when found and (nil,
errs.ErrNotFound) when not found so tests relying on error handling behave like
production.
Comment on lines
+138
to
+140
| func (m *mockNotificationUserRepo) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) { | ||
| return m.users, nil | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mock GetUsersWithDeviceTokens does not filter by userIDs.
The mock returns all users regardless of the userIDs parameter. This weakens test coverage because it does not verify that the service correctly passes the expected user IDs to the repository. Consider filtering by the provided IDs.
Proposed fix
func (m *mockNotificationUserRepo) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) {
- return m.users, nil
+ var result []*models.User
+ idSet := make(map[uuid.UUID]struct{}, len(userIDs))
+ for _, id := range userIDs {
+ idSet[id] = struct{}{}
+ }
+ for _, u := range m.users {
+ if _, ok := idSet[u.ID]; ok && u.DeviceToken != nil && *u.DeviceToken != "" {
+ result = append(result, u)
+ }
+ }
+ return result, 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 (m *mockNotificationUserRepo) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) { | |
| return m.users, nil | |
| } | |
| func (m *mockNotificationUserRepo) GetUsersWithDeviceTokens(ctx context.Context, userIDs []uuid.UUID) ([]*models.User, error) { | |
| var result []*models.User | |
| idSet := make(map[uuid.UUID]struct{}, len(userIDs)) | |
| for _, id := range userIDs { | |
| idSet[id] = struct{}{} | |
| } | |
| for _, u := range m.users { | |
| if _, ok := idSet[u.ID]; ok && u.DeviceToken != nil && *u.DeviceToken != "" { | |
| result = append(result, u) | |
| } | |
| } | |
| return result, nil | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/tests/notification_test.go` around lines 138 - 140, The mock
method mockNotificationUserRepo.GetUsersWithDeviceTokens currently returns
m.users unfiltered; change it to filter m.users by the provided userIDs slice
and return only users whose ID is present in userIDs (e.g., build a lookup set
from the userIDs and collect matching entries from m.users), preserving the
method signature and returning the filtered slice and nil error.
Comment on lines
+180
to
+235
| func TestDeviceTokenUpdate(t *testing.T) { | ||
| app := fakes.GetSharedTestApp() | ||
| authUserID := fakes.GenerateUUID() | ||
| username := fakes.GenerateRandomUsername() | ||
|
|
||
| var userID string | ||
|
|
||
| resp := testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: "/api/v1/users", | ||
| Method: testkit.POST, | ||
| UserID: &authUserID, | ||
| Body: models.CreateUserRequest{ | ||
| Name: "Device Token User", | ||
| Username: username, | ||
| PhoneNumber: "+16175551234", | ||
| }, | ||
| }). | ||
| AssertStatus(http.StatusCreated). | ||
| GetBody() | ||
|
|
||
| userID = resp["id"].(string) | ||
|
|
||
| t.Run("update device token successfully", func(t *testing.T) { | ||
| deviceToken := "ExponentPushToken[xxxxxxxxxxxxxxxxxxxxxx]" | ||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: fmt.Sprintf("/api/v1/users/%s", userID), | ||
| Method: testkit.PATCH, | ||
| UserID: &authUserID, | ||
| Body: models.UpdateUserRequest{ | ||
| DeviceToken: &deviceToken, | ||
| }, | ||
| }). | ||
| AssertStatus(http.StatusOK). | ||
| AssertField("device_token", deviceToken) | ||
| }) | ||
|
|
||
| t.Run("update device token for non-existent user returns 404", func(t *testing.T) { | ||
| deviceToken := "ExponentPushToken[xxxxxxxxxxxxxxxxxxxxxx]" | ||
| fakeUserID := fakes.GenerateUUID() | ||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: fmt.Sprintf("/api/v1/users/%s", fakeUserID), | ||
| Method: testkit.PATCH, | ||
| UserID: &authUserID, | ||
| Body: models.UpdateUserRequest{ | ||
| DeviceToken: &deviceToken, | ||
| }, | ||
| }). | ||
| AssertStatus(http.StatusNotFound) | ||
| }) | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests cover basic happy path and 404 case.
The test structure is clear and follows existing patterns. Consider adding coverage for:
- Clearing a device token (setting it to empty string or null)
- Verifying that
device_token_updated_atis populated after update - Updating the token a second time to confirm the timestamp changes
These would strengthen confidence in the device token update flow.
🤖 Prompt for AI Agents
In `@backend/internal/tests/user_test.go` around lines 180 - 235, Extend
TestDeviceTokenUpdate to add three subtests: one that clears the device token by
sending UpdateUserRequest with DeviceToken set to nil or empty string and assert
the API returns 200 and "device_token" is null/empty; one that after a
successful update asserts the response includes a non-empty
"device_token_updated_at" field (use AssertField or GetBody to parse and
validate it is present and parsable as a timestamp) referencing the PATCH route
"/api/v1/users/%s" and the UpdateUserRequest struct; and one that performs a
second device token update with a different value and asserts that
"device_token_updated_at" has changed compared to the first update (store the
first timestamp, perform second PATCH, then compare). Ensure you reuse
TestDeviceTokenUpdate's created userID and authUserID and use
testkit.Request/Testkit.New helpers for consistency.
aahiltn
reviewed
Jan 29, 2026
| PhoneNumber *string `validate:"omitempty,phone" json:"phone_number"` | ||
| } | ||
| DeviceToken *string `validate:"omitempty" json:"device_token"` | ||
| } No newline at end of file |
| @@ -0,0 +1,86 @@ | |||
| package controllers | |||
Contributor
There was a problem hiding this comment.
can we run just api-doc so we can see it in the api documentation? and also run it on the frontend with bun kubb so it generates api call?
Contributor
There was a problem hiding this comment.
generated api docs in the frontend PR by accident, should i move it here?
Contributor
There was a problem hiding this comment.
the api doc for scalar should be in /docs in backend yeh, you can see your endpoints in localhost:8000/docs once you generate and restart server
| } | ||
|
|
||
| if err := n.notificationService.SendNotification(c.Context(), req); err != nil { | ||
| return fiber.NewError(http.StatusInternalServerError, err.Error()) |
Contributor
There was a problem hiding this comment.
we have errors defined in the /errs directory, please use predefined errors so errors are consistent throughout the app. if there are errors you want to throw but not being handled, feel free to add them as well!
| UserIDs []uuid.UUID `validate:"required,min=1,max=1000,dive,uuid" json:"user_ids"` | ||
| Title string `validate:"required,min=1,max=100" json:"title"` | ||
| Body string `validate:"required,min=1,max=500" json:"body"` | ||
| Data map[string]interface{} `json:"data,omitempty"` |
Contributor
There was a problem hiding this comment.
what is the data here used for, are there fields that are always there or the structure changes? just a question!
Contributor
There was a problem hiding this comment.
this could theoretically be used for deep linking, is what i've seen
Contributor
There was a problem hiding this comment.
also lines up with docs @in-mai-space https://docs.expo.dev/push-notifications/sending-notifications/
afnantuffaha
dismissed stale reviews from aahiltn and in-mai-space
via
0c3059a
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/internal/models/users.go`:
- Around line 9-16: The User struct currently exposes sensitive push data via
JSON tags on DeviceToken and DeviceTokenUpdatedAt; update the User model by
removing those JSON tags or changing them to json:"-" (referencing fields
DeviceToken and DeviceTokenUpdatedAt on type User) so they are not serialized,
or alternatively introduce a dedicated response DTO that copies only
non-sensitive fields from User for API responses and use that DTO instead.
Comment on lines
9
to
+16
| type User struct { | ||
| ID uuid.UUID `bun:"id,pk,type:uuid" json:"id"` | ||
| Name string `bun:"name" json:"name"` | ||
| Username string `bun:"username" json:"username"` | ||
| ProfilePicture *uuid.UUID `bun:"profile_picture,unique,type:uuid" json:"profile_picture,omitempty"` | ||
| PhoneNumber string `bun:"phone_number" json:"phone_number"` | ||
| ID uuid.UUID `bun:"id,pk,type:uuid" json:"id"` | ||
| Name string `bun:"name" json:"name"` | ||
| Username string `bun:"username" json:"username"` | ||
| PhoneNumber string `bun:"phone_number" json:"phone_number"` | ||
| ProfilePicture *uuid.UUID `bun:"profile_picture,unique,type:uuid" json:"profile_picture,omitempty"` | ||
| DeviceToken *string `bun:"device_token" json:"device_token"` | ||
| DeviceTokenUpdatedAt *time.Time `bun:"device_token_updated_at" json:"device_token_updated_at"` |
Contributor
There was a problem hiding this comment.
Do not expose device tokens in JSON responses.
On Line 15–16, DeviceToken and DeviceTokenUpdatedAt are tagged for JSON. If models.User is returned by any API, this can leak push tokens to other clients, enabling unsolicited notifications. Prefer json:"-" or a dedicated response DTO that omits these fields.
Proposed change
- DeviceToken *string `bun:"device_token" json:"device_token"`
- DeviceTokenUpdatedAt *time.Time `bun:"device_token_updated_at" json:"device_token_updated_at"`
+ DeviceToken *string `bun:"device_token" json:"-"`
+ DeviceTokenUpdatedAt *time.Time `bun:"device_token_updated_at" json:"-"`🤖 Prompt for AI Agents
In `@backend/internal/models/users.go` around lines 9 - 16, The User struct
currently exposes sensitive push data via JSON tags on DeviceToken and
DeviceTokenUpdatedAt; update the User model by removing those JSON tags or
changing them to json:"-" (referencing fields DeviceToken and
DeviceTokenUpdatedAt on type User) so they are not serialized, or alternatively
introduce a dedicated response DTO that copies only non-sensitive fields from
User for API responses and use that DTO instead.
in-mai-space
approved these changes
Jan 29, 2026
This was referenced Jan 30, 2026
Merged
3 tasks
3 tasks
3 tasks
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
4 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
Implemented push notification system using Expo Push Service, enabling the backend to send notifications to users' mobile devices with support for both individual and bulk sending.
API Endpoints
POST api/v1/notifications/send- Send notification to single userPOST api/v1/notifications/send-bulk- Send notification to multiple usersKey changes
device_tokenanddevice_token_updated_atcolumns to user table with index on device_token for performancePATCH /users/{id}GetUsersWithDeviceTokens()repository method to fetch all tokens in a single query instead of N individual queriesHow has this been tested?
Checklist