Fixed RFC email notifications, adjusting to the feedback received #79
Conversation
|
This RFC already has a discussion opened by my partner @efrenruizrubio here. |
|
While we were fixing the RFC, I thought that sending email notifications could be a REST API active 27/7 listening for requests to send emails, since doing it through the notification system module requires the developers of the other modules to execute a method to send the email. In this sense, we have to make a request to a REST API or execute a method of the notification module (in both cases passing the required data), each time we want to send email notifications to users. The problem with the REST API is that (from what I understood) it would not be compatible with what we call an SDK. |
|
|
||
| ```json | ||
| { | ||
| "user": "oneMoreUser", |
There was a problem hiding this comment.
Is this information required? The endpoint should return info about a specific user notifications, usually the returned data have a format like:
{
status: 200,
message: 'The request was successful,
data: []
pagination: {}
}
|
|
||
| **Requests need the user's token to return its corresponding notifications. For the example, imagine that the token belongs to the user 'oneMoreUser'.** | ||
|
|
||
| - `[GET] /notifications/` - Get all notifications |
There was a problem hiding this comment.
As we dont need to get all notifications info this looks good, but im thinking if using user/:id/notifications could be a more semantic approach?
How are you planning to send used info?
There was a problem hiding this comment.
I thought getting the user by its token or something related to the authentication, I noticed that MercadoLibre and Facebook, for example, do not send user information in the endpoint, so I guess they get it from authentication.
There was a problem hiding this comment.
I have use that method in a project and It's very useful and comfortable 👌🏼
|
|
||
| ## Get only one notification by its ID | ||
|
|
||
| - **Endpoint:** `[GET] /notfications/:notification` |
There was a problem hiding this comment.
Where this endpoint will be used in the UI?
There was a problem hiding this comment.
This and the "get all notifications" endpoints would be more used to make queries from an admin user, or for other teams that need to consult the information about notifications. This way they don't access our database directly, but by our endpoints.
|
|
||
| ## Mark a notification as read | ||
|
|
||
| - **Endpoint:** `[POST] /notifications/:notification/mark_read` |
There was a problem hiding this comment.
POST is usually used to indicate a resource creation not a modification, to edit something there a two ways:
PUT: This is used when the payload that you send contains all the fields of the resource
PATCH: This is used when you want to modify a particular property of a resource
For this case I recommend to use the second one since you are only going to send something like:
{
read: true
}
This endpoint could help us to mark it as read or viceversa.
There was a problem hiding this comment.
- What if we send the notification id as part of the payload? This will eliminate the id from the url
- I encourage you to use hyphens instead of underscore,
mark_read->mark-read
There was a problem hiding this comment.
Thanks for the clarification, I'll make the corrections
|
|
||
| ## Set all notifications as read | ||
|
|
||
| - **Endpoint:** `[POST] /notifications/all_read` |
There was a problem hiding this comment.
- Same comment with the format of
all_read->all-read
| { | ||
| "token": "theuserauthtoken" | ||
| } |
There was a problem hiding this comment.
Usually the token will be place in the authorization header, check this https://www.loginradius.com/blog/engineering/everything-you-want-to-know-about-authorization-headers/
| "read": true, | ||
| "action": {} | ||
| }, | ||
| "token:": "theusertauthtoken" |
There was a problem hiding this comment.
same comment related with the authorization header
|
|
||
| ```json | ||
| { | ||
| "token:": "theusertauthtoken" |
There was a problem hiding this comment.
same comment related with the authorization token
| "read": true, | ||
| ... | ||
| "token:": "theusertauthtoken", | ||
| "id": "123abcd" |
There was a problem hiding this comment.
To be honest, I didn't consider this approach in the past, where you pass the id in the body, looks very clean!
| ... | ||
| "read": true, | ||
| ... | ||
| "token:": "theusertauthtoken", |
There was a problem hiding this comment.
same comment related with the authorization token
| "data": [ | ||
| { | ||
| "id": "123abcd", | ||
| ... | ||
| "read": true, | ||
| ... | ||
| }, | ||
| { | ||
| "id": "124abce", | ||
| ... | ||
| "read": true, | ||
| ... | ||
| } | ||
| { | ||
| "id": "125abcf", | ||
| ... | ||
| "read": true, | ||
| ... | ||
| } | ||
| ... | ||
| ] |
There was a problem hiding this comment.
The update operation usually doesn't return anything, the status code its usually 204 (check this).
I suggest a response body like:
{
status: 204,
message: 'Operation successful',
body: {}
}
This is a fixed version of the RFC for email notifications, following recommendations received.