-
Notifications
You must be signed in to change notification settings - Fork 9
feat(docs/api): Add proposed endpoints for swags #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(docs/api): Add proposed endpoints for swags #30
Conversation
* Fix endpoints of "proposals" app to support "lightning talks". * Add another endpoint in "proposals" app to notify speakers if their proposal is accepted. * Add proposed endpoints for "swags" app.
CuriousLearner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please find my comments inline.
docs/api/proposed_endpoints.md
Outdated
| title | text | Title of proposal | ||
| speaker | text | Speaker for the talk | ||
| kind | text | Type of proposal like talk, dev sprint, workshop | ||
| speaker | UUID | Foreign Key to `User` Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client doesn't care if you're using SQL and have a foreign key. It is just user_id for the user :)
docs/api/proposed_endpoints.md
Outdated
|
|
||
| __Note__: | ||
| - For `lightning_talk`, duration will be fixed like `00:05:00`, etc. which would be configuration through settings. | ||
| - For `lightning_talk`, `level` of proposal is like speaker's experience which can be `beginner`, `intermediate` or `advanced`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A person can have a different experience in different domains. You cannot assume it as constant. Also, anyone from an audience can be a speaker for lightning talks.
docs/api/proposed_endpoints.md
Outdated
| ## Send notification to speaker | ||
|
|
||
| ``` | ||
| POST /api/proposal/:id/notify (requires authentication) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might not want to send individual notifications.
docs/api/proposed_endpoints.md
Outdated
| **Request** | ||
| ```json | ||
| { | ||
| "name": "Tshirt", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be all lower case. There should be a mapping for FE on what to send here.
Internally this would be a choice field. The mapping can be provided in a meta-data API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing internal choice for such a broad key like name might not be possible because name can be anything like tshirt, cap, thumb drive, mug, notebook, chargers, etc. What we can do is to create a key category with possible general choices like wearables, devices, kit, bag and other (while leaving the key name as a common text_field). This way it would be more flexible. What do you think?
docs/api/proposed_endpoints.md
Outdated
| ## Update swag details | ||
|
|
||
| ``` | ||
| PATCH /api/swag/:swag_id (requires authentication) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| PATCH /api/swag/:swag_id (requires authentication) | |
| PATCH /api/swag/:id (requires authentication) |
docs/api/proposed_endpoints.md
Outdated
| } | ||
| ``` | ||
|
|
||
| # User-Swag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is User-Swag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserSwag is a through-table to relate Users with Swags as a many-to-many relation. All the following endpoints are dealing on this model. It will exist in the Swags app itself and its usage lies in marking if a Swag is given to a User and the time when it is given.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we having an entry in the API for swag when the swag is not given yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! We don't need to create an entry if the swag is not given yet. Following this, we won't be needing is_given, because the creation of the entry will itself mean that the swag is given and created_at will be the same as given_at. So we can remove both is_given and given_at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
docs/api/proposed_endpoints.md
Outdated
| ## Upload image of a swag | ||
|
|
||
| ``` | ||
| POST /api/swag/:swag_id/upload_image (requires authentication) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these APIs should have a plural form like */api/swags/*
* Add key "item" in model "Swag" as a foreign key to hold "SwagItems". These items will behave like choices for creating the swag.
docs/api/proposed_endpoints.md
Outdated
| status | text | Status of proposal like `retracted`, `accepted`, `unaccepted`, `submitted`, etc. | ||
|
|
||
| __Note__: | ||
| - For `lightning_talk`, duration will be fixed like `00:05:00`, etc. which would be configuration through settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - For `lightning_talk`, duration will be fixed like `00:05:00`, etc. which would be configuration through settings. | |
| - For `lightning_talk`, the duration will be fixed like `00:05:00`, etc. which would be configurable through settings. |
docs/api/proposed_endpoints.md
Outdated
| { | ||
| "title": "Corrected title of talk", | ||
| "level": "advanced" | ||
| "id": "0f342ac1-ac32-4bd1-3612-efa32bc3d9a0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's a PATCH request and we are providing id in the URL itself, no need to keep it in the request then.
docs/api/proposed_endpoints.md
Outdated
| **Request** | ||
| ```json | ||
| { | ||
| "id": "0f342ac1-ac32-4bd1-3612-efa32bc3d9a0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above for id here. :)
docs/api/proposed_endpoints.md
Outdated
| ``` | ||
|
|
||
| **Response** | ||
| Status: 201 Created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't a PATCH return a 200 Ok status as it doesn't create any resource rather modify an existing resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be 200 OK.
docs/api/proposed_endpoints.md
Outdated
| **Request** | ||
| ```json | ||
| { | ||
| "id": "0f342ac1-ac32-4bd1-3612-efa32bc3d9a0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above for id :)
CuriousLearner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few suggestions and questions.
Please find my review inline.
| ## Send notification to individual speaker | ||
|
|
||
| ``` | ||
| POST /api/proposals/:id/notify (requires authentication) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be either acceptance/rejection.
| ```json | ||
| { | ||
| "id": "0f342ac1-ac32-4bd1-3612-efa32bc3d9a0", | ||
| "item": "12abcff0-bcbc-01fc-abcd-01012bc4e0b1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why an id here? Please show the resource here.
| "id": "0f342ac1-ac32-4bd1-3612-efa32bc3d9a0", | ||
| "item": "12abcff0-bcbc-01fc-abcd-01012bc4e0b1", | ||
| "description": "sponsered by someone", | ||
| "image": null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it as URL so it is pretty evident for someone integrating the API on what to expect.
| ```json | ||
| { | ||
| "item": "12abcff0-bcbc-01fc-abcd-01012bc4e0b1", | ||
| "description": "sponsered by someone else", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just provide a few fields here.
Also, what are an item and a swag exactly?
| ## Delete the image of swag | ||
|
|
||
| ``` | ||
| DELETE /api/swags/:id/delete_image (requires authentication) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be POST.
| **Response** | ||
| Status: 200 OK | ||
| ```json | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is just one item, why not give a name to swag altogether rather than trying to link them through a foreign key?
proposal is accepted.