feat(notifications_service): adds endpoint to list registered devices of a user #339
Conversation
gateway/services/notifications.go
Outdated
| arbor.Route{ | ||
| "GetAllSubscriptions", | ||
| "GET", | ||
| "/notifications/subscriptions", |
gateway/services/notifications.go
Outdated
| arbor.Route{ | ||
| "GetAllRegisteredDevices", | ||
| "GET", | ||
| "/notifications/devices", |
gateway/services/notifications.go
Outdated
| alice.New(middleware.IdentificationMiddleware, middleware.AuthMiddleware([]models.Role{models.UserRole})).ThenFunc(GetAllSubscriptions).ServeHTTP, | ||
| }, | ||
| arbor.Route{ | ||
| "GetAllRegisteredDevices", |
There was a problem hiding this comment.
I think something more like "GetUserRegisteredDevices" might be better, since you're getting the registered devices of a specific user, not all users.
gateway/services/notifications.go
Outdated
| ) | ||
|
|
||
| const NotificationsFormat string = "JSON" | ||
|
|
||
| var NotificationsRoutes = arbor.RouteCollection{ | ||
| arbor.Route{ | ||
| "GetAllSubscriptions", |
There was a problem hiding this comment.
same thing as my comment below about the naming -- "GetUserSubscriptions"
| topics, err := service.GetSubscriptions(id) | ||
|
|
||
| if err != nil { | ||
| errors.WriteError(w, r, errors.DatabaseError(err.Error(), "Could not retrieve notifications.")) |
There was a problem hiding this comment.
Error message should be changed from "notifications" to "subscriptions"
| devices, err := service.GetUserDevices(id) | ||
|
|
||
| if err != nil { | ||
| errors.WriteError(w, r, errors.DatabaseError(err.Error(), "Could not retrieve notifications.")) |
There was a problem hiding this comment.
Hi Heesoo,
After revisiting, I think you're right. My interpretation of the PR was a bit different. Based on the work done so far, I'd like to merge these into master and hopefully open a new PR to address #247 more properly soon. Additionally, I think this can count as my "coding challenge" for the API team right? Please get back to me when you get a chance : )
|
Hey @RohanSreerama5! Other than some small things, the new endpoints look good! But your PR doesn't close #247 -- I think you might have misinterpreted the issue. We have a We want to add endpoints to the Take a look at |
Adds 2 new endpoints:
Note: This is part of my application to be on the Backend HackIllinois team. Hope you like it! : )