search

signmeup / signmeup

Real-time application to sign up for and manage TA hours.
https://signmeup.cs.brown.edu
MIT License
96 stars 32 forks source link

Allow TAs to reopen closed queues #161

Closed athyuttamre closed 7 years ago

athyuttamre commented 7 years ago

Currently a queue once ended cannot be opened again. This is frustrating if the TA forgot to extend the end time while students are still waiting. They should be able to reopen it.

athyuttamre commented 7 years ago

It might also be nice to show recently ended queue cards on the homepage so students aren't lost as to where the queue went. (#169)

wpovell commented 7 years ago

Hey @athyuttamre ! I'd love to work on this bug. I tried looking through the code base to get a feel for things but I have no experience w/ Meteor so was a bit lost at times. Any suggested resources? I went through the todo list tutorial on the Meteor site which definitely clarified a few things. Also, any tips for plans to attack on this? Thanks!

wpovell commented 7 years ago

After a bit of research looks like making Queues.scheduledEndTime a TTL index would be a good way to address this. Maybe make let the queues live for another 20-30min after expiring? Could be displayed below active queues on main page, TAs could click to reopen. I'm guessing it would ask TAs to enter a new end time for queue, and reset the scheduledEndTime.

athyuttamre commented 7 years ago

Hey @wpovell! I'm loving the interest. And your intuition is pretty much on-point, with a couple of differences.

An ideal place for the TAs to reopen the queue would be from the status dropdown in the action bar. It looks like this:

screenshot 2017-03-06 22 31 14

When the queue is ended, the dropdown goes away, with just the "Ended" status visible:

screenshot 2017-03-06 22 31 22

We could instead make this a dropdown with a "Reopen Queue" option. Note that this won't have anything to do with the TTL. The TTL determines how long a MongoDB document should live before it's permanently deleted from the database. However, we simply change the values of the state of the queue and a couple of timestamps.

What will we have to change in the queue when it's reopened?

It'd also be cool to show recently ended queues on the home page, but we have a separate issue for that (#169).

Before you begin coding, what do you think about commenting below with what files you expect to change and how? That way I can do a quick "design check" before much time is spent on implementation. Let me know if you have any questions!

wpovell commented 7 years ago

Okay, so I just realized that when queues are ended they aren’t deleted from the database (an incorrect assumption I was running on). I was totally misunderstanding what the SyncedCron.remove call was doing, but I think I’m good now. Out of curiosity, though, are the queues ever deleted? Or do you have all the SignMeUp queue data stretching back to the dawn of time?

Anyway, first in implementing queue reopening, I’d imagine you’d want to put a dropdown-menu below “Ended”, looking something like this:

screen shot 2017-03-07 at 1 03 33 am

I’d imagine we’d want to add another field endStatus to Queues that is optional & set by endQueue() to the status that a queue ends in. Then for reopenQueue() we’ll want to:

  1. Set status to endStatus
  2. Set scheduledEndTime to moment().add(1, 'hour').startOf('hour’)
  3. Unset endedAt, endedBy, and endedStatus

In terms of edits:

How does this look to you?

wpovell commented 7 years ago

I got a rough version of what I described above; writing it up was the majority of the work, so I figured I might as well make the changes even if they're not entirely right/have to be redone.

However, I've been having some trouble getting eslint to work. I installed eslint globally so when using the .eslintrc.json it requested I install a bunch of the plugins globally. After installing all these plugins, I get a TON of errors, mostly related to Resolve error: unable to load resolver "meteor". I'm guessing I'm just doing something wrong, I have to imagine there's a way to set up eslint through the npm package. Anyway, thought I'd mention it (I see you made an issue #177 related to updating CONTRIBUTING wrt this)

athyuttamre commented 7 years ago

Great progress! I'm really glad you have a solid understanding of the different components involved.

I'm on my phone, but one quick piece of feedback: rather than have an extra endStatus field, we can refer to the cutoffAt timestamp. It if it is set, we can return to the cutoff state. Otherwise we can return to the open state.

As for the eslint issue, it seems that the latest version is indeed inconsistent with how I've set it up. I haven't updated my local version in a while, which is why I didn't catch it until this evening when i was debugging out CodeClimate setup. I'll take a look at t first thing tomorrow.

Otherwise, feel free to open a PR!

wpovell commented 7 years ago

Awesome, thanks! Also, glad I'm not the only one up at this hour 😴