Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 351
Event CRUD API#299
Uh oh!
There was an error while loading. Please reload this page.
Event CRUD API #299
Changes from all commits
158785f670fa9bdaffbc91ab8e015a43f223695458d544c9f7d06f3a6cdd6233d5b531File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| import { Request, Response } from 'express'; | ||
| import { Event } from 'server/models/Event'; | ||
| import { PostgresErrorCodes } from 'server/util/PostgresErrorConstants'; | ||
| // The whole model is a json response, fix that if there's some sensitive data here | ||
| export default { | ||
| async index(req: Request, res: Response) { | ||
| const { chapterId } = req.params; | ||
| const events = await Event.find({ where: { chapter: chapterId } }); | ||
| res.json(events); | ||
| }, | ||
| async show(req: Request, res: Response) { | ||
| const { id, chapterId } = req.params; | ||
| const event = await Event.findOne({ | ||
| where: { | ||
| id: parseInt(id), | ||
| chapter: chapterId, | ||
| }, | ||
| }); | ||
| if (event) { | ||
| res.json(event); | ||
| } else { | ||
| res.status(404).json({ error: "Can't find event" }); | ||
| } | ||
| }, | ||
| async create(req: Request, res: Response) { | ||
| const { | ||
| name, | ||
| chapter, | ||
| description, | ||
| capacity, | ||
| venue, | ||
| canceled, | ||
| start_at, | ||
| ends_at, | ||
| } = req.body; | ||
| const event = new Event({ | ||
| name, | ||
| chapter, | ||
| description, | ||
| capacity, | ||
| venue, | ||
| canceled, | ||
| start_at, | ||
| ends_at, | ||
| }); | ||
| try { | ||
| await event.save(); | ||
| res.status(201).json(event); | ||
| } catch (e) { | ||
| if (e.code === PostgresErrorCodes.FOREIGN_KEY_VIOLATION) { | ||
| if (e.detail.includes('venue')) { | ||
| return res.status(400).json({ message: 'venue not found' }); | ||
| } | ||
| if (e.detail.includes('chapter')) { | ||
| return res.status(400).json({ message: 'chapter not found' }); | ||
| } | ||
| } | ||
| res.status(500).json({ error: e }); | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should return 400 bad request as response and with the following content? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we should do that instead of using Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vaibhavsingh97 What makes you think it is the fault of the client? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can fix this in another PR after discussion. Approving Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I'm done arguing this, FWIW. | ||
| } | ||
| }, | ||
| async update(req: Request, res: Response) { | ||
| const { id, chapterId } = req.params; | ||
| const { | ||
| name, | ||
| description, | ||
| capacity, | ||
| venue, | ||
| canceled, | ||
| start_at, | ||
| ends_at, | ||
| } = req.body; | ||
| const event = await Event.findOne({ | ||
| where: { id: parseInt(id), chapter: chapterId }, | ||
| }); | ||
| if (event) { | ||
| event.name = name ?? event.name; | ||
| event.description = description ?? event.description; | ||
| event.capacity = capacity ?? event.capacity; | ||
| event.venue = venue ?? event.venue; | ||
| event.canceled = canceled ?? event.canceled; | ||
| event.start_at = start_at ?? event.start_at; | ||
| event.ends_at = ends_at ?? event.ends_at; | ||
| try { | ||
| await event.save(); | ||
| res.json(event); | ||
| } catch (e) { | ||
| if (e.code === PostgresErrorCodes.FOREIGN_KEY_VIOLATION) { | ||
| if (e.detail.includes('venue')) { | ||
| return res.status(400).json({ message: 'venue not found' }); | ||
| } | ||
| if (e.detail.includes('chapter')) { | ||
| return res.status(400).json({ message: 'chapter not found' }); | ||
| } | ||
| } | ||
| res.status(500).json({ error: e }); | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above | ||
| } | ||
| } else { | ||
| res.status(404).json({ error: "Can't find event" }); | ||
| } | ||
| }, | ||
| async remove(req: Request, res: Response) { | ||
| const { id, chapterId } = req.params; | ||
| const event = await Event.findOne({ | ||
| where: { id: parseInt(id), chapter: chapterId }, | ||
| }); | ||
| if (event) { | ||
| try { | ||
| await event.remove(); | ||
| res.json({ id }); | ||
| } catch (e) { | ||
| res.status(500).json({ error: e }); | ||
| } | ||
| } else { | ||
| res.status(404).json({ error: "Can't find event" }); | ||
| } | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import Faker from 'faker'; | ||
| import { define } from 'typeorm-seeding'; | ||
| import { Event } from '../models/Event'; | ||
| import { Chapter } from 'server/models/Chapter'; | ||
| import { Venue } from 'server/models/Venue'; | ||
| define(Event, ( | ||
| faker: typeof Faker, | ||
| params: { chapter: Chapter; venue: Venue }, | ||
| ) => { | ||
| const name = faker.company.companyName(); | ||
| const description = faker.lorem.words(); | ||
| const { chapter, venue } = params; | ||
| const event = new Event({ | ||
| name, | ||
| chapter, | ||
| description, | ||
| capacity: faker.random.number(1000), | ||
| venue, | ||
| canceled: Math.random() > 0.5, | ||
| start_at: new Date(), | ||
| ends_at: new Date(Date.now() + 1000 * 60 * 60 * 5 * Math.random()), | ||
| }); | ||
| return event; | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import Faker from 'faker'; | ||
| import { define } from 'typeorm-seeding'; | ||
| import { Location } from '../models/Location'; | ||
| import { Venue } from '../models/Venue'; | ||
| define(Venue, (faker: typeof Faker, params: { location: Location }) => { | ||
| const name = faker.company.companyName(); | ||
| const { location } = params; | ||
| const venue = new Venue({ | ||
| name, | ||
| location, | ||
| }); | ||
| return venue; | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import express from 'express'; | ||
| import asyncHandler from 'express-async-handler'; | ||
| import eventsController from 'server/controllers/eventsController'; | ||
| const router = express.Router(); | ||
| router.get( | ||
| '/api/chapters/:chapterId/events', | ||
| asyncHandler(eventsController.index), | ||
| ); | ||
| router.post( | ||
| '/api/chapters/:chapterId/events', | ||
| asyncHandler(eventsController.create), | ||
| ); | ||
| router.get( | ||
| '/api/chapters/:chapterId/events/:id', | ||
| asyncHandler(eventsController.show), | ||
| ); | ||
| router.patch( | ||
| '/api/chapters/:chapterId/events/:id', | ||
| asyncHandler(eventsController.update), | ||
| ); | ||
| router.delete( | ||
| '/api/chapters/:chapterId/events/:id', | ||
| asyncHandler(eventsController.remove), | ||
| ); | ||
| export default router; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { Factory, Seeder } from 'typeorm-seeding'; | ||
| import { Location } from '../models/Location'; | ||
| import { Venue } from '../models/Venue'; | ||
| export default class CreateVenues implements Seeder { | ||
| public async run(factory: Factory): Promise<any> { | ||
| const location = await Location.findOne(); | ||
| await factory(Venue)({ location }).seedMany(2); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { Factory, Seeder } from 'typeorm-seeding'; | ||
| import { Venue } from '../models/Venue'; | ||
| import { Event } from '../models/Event'; | ||
| import { Chapter } from '../models/Chapter'; | ||
| export default class CreateEvents implements Seeder { | ||
| public async run(factory: Factory): Promise<any> { | ||
| const venue = await Venue.findOne(); | ||
| const chapter = await Chapter.findOne(); | ||
| await factory(Event)({ chapter, venue }).seedMany(2); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.