Skip to content
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

Update server.js #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

nirbhaysinghnarang
Copy link
Collaborator

No description provided.

Copy link
Owner

@Archit404Error Archit404Error left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR: this PR looks rushed to me and shows a lack of attention/effort. It is also just plain wrong. I want to see clean, well-thought out, and thoroughly tested code for the next one. Keep reading to see how to do that.

This code is wrong. All this does is send a socket event to every single user, and all this socket event does is re-render a few pages like the event stats page when a user creates an event -- this event should only fire when a user creates an event.

The function that needs to be used is sendNotifs() defined within helperMethods.js. Read how it's used elsewhere in the code and TEST YOUR PR prior to submitting so that we don't end up w/ faulty backend code like this.

TO TEST: try creating an event locally via the route and start off by only sending yourself the notifications. See if this works and then delete the test event from DB. Additionally, try printing out the expo notification tokens of every user in the DB to make sure those are loaded properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants