Together-100Devs / Together

Together is a group calendar application using the MERN stack intended to bring discord communities closer!
https://together.cyclic.app/
MIT License
167 stars 112 forks source link

Add an API Endpoint that will return all events #406

Closed Caleb-Cohen closed 12 months ago

Caleb-Cohen commented 1 year ago

Related Issues Project: Admin Dashboard Depends on:

Description Add getEventsForAdmin method in the events.js controller that will return all events.

Acceptance Criteria

cblanken commented 12 months ago

I think I'd like to try my hand at this issue. But before I do, could I get some clarification on how this API endpoint should differ from the getAll method that already seems able to return all events?

Caleb-Cohen commented 12 months ago

I think I'd like to try my hand at this issue. But before I do, could I get some clarification on how this API endpoint should differ from the getAll method that already seems able to return all events?

that's a great question. It's been awhile since I checked this, but if memory serves me right, getAll only gets all the events for the current month, but I'll need to check that when I get home. if I'm wrong and getAll is truly a getAll then I'll need to adjust the ticket.

Caleb-Cohen commented 12 months ago

@cblanken

It looks like I was correct that getAll requires a date parameter.


  const response = await DataService.getAll(date.monthStart, date.monthEnd);

  getAll(from, to) {
    return URL.get("/events", { params: { from, to } });
  }

  getAll: async (req, res) => {
    const { from, to } = req.query;

    let where = {};
    if (from || to) where = { startAt: { $gte: from, $lt: to } };

    const query = !req.user
      ? Event.find(where).select("-user")
      : Event.find(where).populate("user", "displayName");

    res.json(await query.lean());
  }

Now, while I guess we could pass in dates from the beginning of time to the end of time, I would still prefer a second endpoint. This is because we may want to change the endpoint only to pass back pending events at some point; additionally having the same endpoint for two entirely different functions does not seem natural.

I'm open to ideas if you think there's a better way to do this! I'll admit I thought a bit about this as work, and there are good reasons both ways.

cblanken commented 12 months ago

@Caleb-Cohen From my tests in Postman, the from and to parameters don't seem to be required. If you call the base endpoint without them, you'll get all the events.

I don't know the easiest way to add more users while the app is running in development so I haven't tried it with multiple users yet, but here's the Postman response from the getAll endpoint without parameters.

I added two events in addition to the default event added when running in development and they all get returned.

Image

cblanken commented 12 months ago

I just realized the code you posted wasn't from server/controllers/events.js. What file is that code snippet from?

Caleb-Cohen commented 12 months ago

I just realized the code you posted wasn't from server/controllers/events.js. What file is that code snippet from?

I pulled the last bit from the controller in events.js, the top bits are from the client side that calls the routes.

cblanken commented 12 months ago

I pulled the last bit from the controller in events.js, the top bits are from the client side that calls the routes.

Oh I see. I didn't realize the quote wasn't all one file.

So do we still want to add a separate method (getEventsForAdmin) or would it make more sense to just call the current getAll method with no parameters from the frontend of the Admin Dashboard?

If I'm not mistaken it should return all events if the from and to parameters are left empty (undefined).

Caleb-Cohen commented 12 months ago

I pulled the last bit from the controller in events.js, the top bits are from the client side that calls the routes.

Oh I see. I didn't realize the quote wasn't all one file.

So do we still want to add a separate method (getEventsForAdmin) or would it make more sense to just call the current getAll method with no parameters from the frontend of the Admin Dashboard?

If I'm not mistaken it should return all events if the from and to parameters are left empty (undefined).

Let's use the getAll, we can re-open this ticket if we need a more specialized route in the future!

Thanks for looking into this and correcting my mistake!