mterwill / office-hours-help-queue

A queue to help manage office hours for large courses
GNU General Public License v3.0
86 stars 21 forks source link

Add queue randomization (#147) #207

Closed tonyb7 closed 2 years ago

tonyb7 commented 2 years ago

Add queue randomization as suggested in #147.

I am having a bit of trouble setting up the server locally to test my changes, as I am getting

ActiveRecord::PendingMigrationError Migrations are pending. To resolve this issue, run: bin/rails db:migrate RAILS_ENV=development

but I figured I would open this PR to get some feedback, and to see if I could get help with testing and a couple questions I had. I am currently unsure about two changes:

1) I am not familiar at all with ruby, so I am guessing the way you shuffle the outstanding requests is by doing @course_queue.outstanding_requests.shuffle (code here), but again I haven't been able to test this, and I am not sure if there are other metadata which also need to be shuffled around.

2) I am not positive if calling course_queue's broadcastInstructorMessage from instructor_panel will work (code here). My intention is to send a notification to students whenever the queue is randomized.

Thanks for any help anyone can provide!

thomasebsmith commented 2 years ago

To fix the migrations error, I was able to start the container and then run

$ docker-compose run web bash
# bin/rails db:migrate RAILS_ENV=development
thomasebsmith commented 2 years ago

I think the shuffle also needs to be a little more complicated. It looks like the outstanding requests are stored as rows in the course_queue_entries table. They are then always displayed ordered by the created_at column (when the request was created).

To randomize these requests, I think we'd need to either rewrite all the created_at times (which isn't great) or add another column to the table to store a custom order.

tonyb7 commented 2 years ago

Thanks Thomas! Got the queue running locally now. You're right -- the shuffling is not working, so there's more that needs to be changed there.

Also, what is the best way to test notifications to students? Currently I'm just opening the queue on multiple browsers and signing up as a student. However, my student browser is not receiving notifications even when the instructor broadcasts a message. Is there a better way to test multiple users (from the instructor's perspective as well as the student's perspective) at the same time other than with multiple browsers?

thomasebsmith commented 2 years ago

I've only been able to test with multiple browsers (or the same browser with one user in private browsing mode). For me, broadcasting from one browser/window does bring up an alert in other browsers/windows.

thomasebsmith commented 2 years ago

I've implemented a version of randomization that seems to work. More manual testing is required, and I haven't written any automated tests yet. I'd also like feedback on whether the way I implemented it seems like the best way to do it.

How it works:

mterwill commented 2 years ago

👋 thanks for the contribution! Sorry for the super long delay in reviewing this. 💭 on #216 as potential alternative?

tonyb7 commented 2 years ago

I think the solution in #216 looks good too! It's a more systematic solution than the one in this PR.

mterwill commented 2 years ago

Closing in favor of https://github.com/mterwill/office-hours-help-queue/pull/216. Thanks for your contribution!