office-hours-help-queue icon indicating copy to clipboard operation
office-hours-help-queue copied to clipboard

Add queue randomization (#147)

Open tonyb7 opened this issue 4 years ago • 7 comments

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!

tonyb7 avatar Nov 16 '21 05:11 tonyb7

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 avatar Nov 16 '21 17:11 thomasebsmith

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.

thomasebsmith avatar Nov 16 '21 17:11 thomasebsmith

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?

tonyb7 avatar Nov 16 '21 17:11 tonyb7

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 avatar Nov 16 '21 18:11 thomasebsmith

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:

  • There is a new column in the course_queue_entries table, priority.
  • Requests are now sorted by priority and then created_at.
  • New requests have a priority of 0, so they are added to the bottom of the queue as usual.
  • Clicking the "Randomize Queue" button randomly assigns priorities such that all unresolved requests are now in a random order.
  • Users with unpinned requests are notified if their position in the unpinned section of the queue changes.

thomasebsmith avatar Nov 17 '21 20:11 thomasebsmith

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

mterwill avatar May 19 '22 14:05 mterwill

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

tonyb7 avatar May 20 '22 06:05 tonyb7

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

mterwill avatar Sep 02 '22 02:09 mterwill