Event CRUD API

[Zeko369]

Closes#102

[robertt]

#302 just got merged, meaning you can replace all your if(e.code === '23503')s with if(e.code === PostgresErrorCodes.FOREIGN_KEY_VIOLATION)!

[timmyichen]

I think the last two commits (updating and rerunning prettier) might be in a separate PR as it dilutes the changes made that are actually relevant to the PR (ideally we can point to this PR as an example of how to do things)

[Zeko369]

@timmyichen I agree, but I can't run prettier on the ts3.7.3 and needed to update prettier, the new prettier automatically added these changes, maybe just ignoring the lint steps?

[AryanJ-NYC]

@Zeko369 Just remove the changes to files that are irrelevant to the scope of the Events CRUD API from the branch:

server/models/Chapter.ts
server/models/Event.ts
server/models/EventSponsor.ts
server/models/Location.ts
server/models/Rsvp.ts
server/models/SocialProviderUser.ts
...
etc.

[vaibhavsingh97]

@timmyichen@AryanJ-NYC I agree that the last 2 commits are not related to this PR, but it's just linting, so can we agree and go ahead with this PR?

[AryanJ-NYC]

I mean, sure. It's not even necessarily about this one PR. We just want to be sure to not set a precedent of allowing out-of-scope and unrelated changes made.

[vaibhavsingh97]

we should return 400 bad request as response and with the following content?

{
"errors": [
{
"message": "please add your chapter name",
"field": "name"
},
{
"message": "please add your chapter description",
"field": "description"
}
]
}

[Zeko369]

Yeah, we should do that instead of using 404, but also keep the 500 error on line 71

[vaibhavsingh97]

according to rest API conventions, we should give 500 when there is a server-side error, but here is the fault is from the client-side so I think we should not give 500 as a response code.
Ref: https://restfulapi.net/http-status-codes/

[AryanJ-NYC]

@vaibhavsingh97 What makes you think it is the fault of the client?

[vaibhavsingh97]

Client fault as in, the client didn't send all the details required by API. Here server didn't break, it's the data which is missing

[Zeko369]

I changed this to 400 for now, we can open a new issue to discuss what we're going to use. But I think this is fine for now

[vaibhavsingh97]

Yes, we can fix this in another PR after discussion. Approving

[AryanJ-NYC]

@vkWeb I don't follow your logic. If I think thing A, I have to give an example of thing B? Why the pop quiz?

[vkWeb]

@AryanJ-NYC I asked that question to make you think of situations when a client-side error may creep.

I checked the models. We aren't allowing NULL values for event name and capacity. So shouldn't we catch NOT_NULL_VIOLATION for them?

[AryanJ-NYC]

Instead of asking questions to try and make me think, wouldn't it be easier to just explicitly say: "Hey, we're missing a catch for the NOT_NULL_VIOLATION?

I'm done arguing this, FWIW.

[vaibhavsingh97]

same as above

[Zeko369]

@Zeko369 Just remove the changes to files that are irrelevant to the scope of the Events CRUD API from the branch:

server/models/Chapter.ts
server/models/Event.ts
server/models/EventSponsor.ts
server/models/Location.ts
server/models/Rsvp.ts
server/models/SocialProviderUser.ts
...
etc.

I tried doing that, but the problem is that if I skip that the CI fails, and if I revert back to the old TS and old prettier I'd have to change ?? to something else

[ankorGH]

@Zeko369 can't you create a new pull request for the new prettier/ts config and rebase this branch when its merged ? Or I am missing something

[Zeko369]

@ankorGH OK, I can do this, that seems like a great idea

[vaibhavsingh97]

Merging since we have two approval