-
Notifications
You must be signed in to change notification settings - Fork 22
Team matching subscription api #252
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
Conversation
uy-seng
commented
Mar 8, 2024
- Subscribe to posting
- Unsubscribe to posting
- Get all subscriptions
Team matching api functionality: Subscribe to posting
Team matching api functionality: Unsubscibe to posting
Team matching api functionality: Get all user's subscription
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
💯
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.
Overall I think the PR is good. I have suggested one possible fix. I also have a few questions below.
-
Is subscription here the same concept as interest of a user for a posting?
-
About the data type, should we wait for everyone to finish so we can resolve all the type conflicts before merging?
- In my PR, I have declared some types there. Can someone check it out and review my PR too if you're free?
| }); | ||
| } | ||
| snapshot.forEach((doc) => { | ||
| doc.ref.delete(); |
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 think delete returns a Promsie. Should we add await here? But since we are sure that this is a one-to-one relationship, we can take the first found doc and await for delete on that doc.
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.
yeah, i think that make sense, i have updated the file and added await and async for deleting the snapshot
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.
Can you change it one more time?
More information in this: https://masteringjs.io/tutorials/fundamentals/async-foreach
Generally, using async inside forEach is not the right way to handle promises
About my other questions, do you have any answers, or should we discuss them with Mike too?
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 updated it using Promise.all instead, and sorry i just saw the other question, i think we can you the types that you created instead but need Mike input on that.
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.
LGTM now!
| } | ||
|
|
||
| // delete one subscription for user that unsubscribe them to a post | ||
| async function deleteSubscription(req: NextApiRequest, res: NextApiResponse) { |
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 think we'll need a way to verify whether request initiator matches user id provided in the request body.
| const db = firestore(); | ||
|
|
||
| // get all subscriptions for user | ||
| async function getSubscriptions(req: NextApiRequest, res: NextApiResponse) { |
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.
^