Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Priority Queue #405

Merged
merged 34 commits into from Nov 14, 2020
Merged

Priority Queue #405

merged 34 commits into from Nov 14, 2020

Conversation

NEUDitao
Copy link
Collaborator

@NEUDitao NEUDitao commented Nov 1, 2020

ezgif-1-a1ae0453ca98

We need help with the "Requeue Student" button's design in terms of color and stuff

dajinchu and others added 20 commits September 13, 2020 11:36
screwing around
4b75c98
fsm screwing around
c6a0e40
fixing new limbo status references
0dffd42
delete xstate
72b072c
Merge branch 'master' into dc-limbo-fsm
21fbe61
added question integration and cleanedup
22ef6ea
actually close queues :|
d924855
imports
5fb89bd
fsm status check in PATCH endpoitn
c129276
list questions response
f9054b5
@ofloody
Refactor getQuestions to get new questions response
2de08d4
Merge branch 'master' into dc-limbo-fsm
1e5ee08
@ofloody
Got App to almost work
b31ccb1
@NEUDitao
got the app back to a workable state with questions refactor
7b6760f
@NEUDitao
Got core functionality working for frontend :)
34cacc3
@NEUDitao
cypress tests for priority queue on student side
1054066
@NEUDitao
cypress tests for priority queue on TA side
3a04824
@NEUDitao
allowed for student questsiont o be deleted more in FSM
7dd6254
@NEUDitao
made test to prove student can leave priority queue
025ab69
@NEUDitao
Merge remote-tracking branch 'origin/master' into dc-limbo-fsm
25dd547
@NEUDitao NEUDitao added this to In Progress in Office Hours v2 via automation Nov 1, 2020
@vercel
Copy link

vercel bot commented Nov 1, 2020 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sandboxneu/office-hours/m31gt99g5
✅ Preview: https://office-hours-git-dc-limbo-fsm.sandboxneu.vercel.app

This was linked to issues Nov 1, 2020
Implement Priority Queue #337
Closed
Priority Queue #391
Closed
willstenzel
willstenzel reviewed Nov 2, 2020
View reviewed changes
Copy link
Member

@willstenzel willstenzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great overall! I left some suggestions for minor improvements and had a few general questions.

packages/app/components/Queue/Student/StudentQueueList.tsx Show resolved Hide resolved
packages/server/src/queue/queue.service.ts Show resolved Hide resolved
packages/app/components/Queue/Student/StudentBanner.tsx Outdated Show resolved Hide resolved
packages/app/components/Queue/Student/StudentQueueList.tsx Outdated Show resolved Hide resolved
packages/app/components/Queue/Student/StudentQueueList.tsx Show resolved Hide resolved
packages/app/components/Queue/Student/StudentQueueList.tsx Outdated Show resolved Hide resolved
packages/server/src/question/question.controller.ts Outdated Show resolved Hide resolved
packages/server/src/question/question-fsm.ts Show resolved Hide resolved
packages/server/src/queue/queue-sse.service.ts Show resolved Hide resolved
@willstenzel
Copy link
Member

For re-queued questions, I saw that "last helped by" was added to the student banner which I liked.

Should the "last helped by" be displayed for TA's as well? That way the TA's have more visibility on who was helping whom.

@NEUDitao
Copy link
Collaborator Author

NEUDitao commented Nov 3, 2020

For re-queued questions, I saw that "last helped by" was added to the student banner which I liked.

Should the "last helped by" be displayed for TA's as well? That way the TA's have more visibility on who was helping whom.

We were thinking about this, but couldn't figure out a way to render it

@liustanley liustanley removed this from In Review in Office Hours v2 Nov 4, 2020
@liustanley
Copy link
Member

liustanley commented Nov 4, 2020 •
edited

When running locally, here are some things I found:

  • After a student rejoins the queue (going into priority queue), the "Priority Queue" and "Queue" headers don't show up on the TA side
  • Unable to leave the queue as a student who has been re-queued, gives me a 401 error
  • Same problem as a student who couldn't be found, gives a 401 error when I click leave queue
  • Clicking Rejoin Queue as a student doesn't update/mutate the frontend, have to click out and click in the window to update banner state
  • 404 error when removing a student from the queue (although I believe this was an existing issue)

dajinchu
dajinchu requested changes Nov 14, 2020
View reviewed changes
packages/server/src/queue/queue.service.ts Outdated
relations: ['creator', 'taHelped'],
where: {
queueId,
status: In(StatusSentToCreator),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be In( ) of all the statuses in queue, help, and priorityQueue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatusSentToCreator includes all of those

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes but we dont' need them in th is function fuck

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah but we don't need them in this funciton fuck

packages/server/src/queue/queue.service.ts
@@ -1,9 +1,16 @@
import {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm alright not doing it now, but a big TODO should be queue service tests. The new ListQuestions stuff has like no tests afaik

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't the whole thing we didn't know how to do this currently?

@NEUDitao
Revert "changed back"
cacecc0 
This reverts commit 6401447.
dajinchu
dajinchu approved these changes Nov 14, 2020
View reviewed changes
Copy link
Member

@dajinchu dajinchu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛵ ship!

@NEUDitao NEUDitao merged commit aa72ed8 into master Nov 14, 2020
@willstenzel willstenzel deleted the dc-limbo-fsm branch November 14, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dajinchu dajinchu dajinchu approved these changes

@willstenzel willstenzel willstenzel approved these changes

@liustanley liustanley Awaiting requested review from liustanley

@dfarooq610 dfarooq610 Awaiting requested review from dfarooq610

Assignees

@dajinchu dajinchu

@ofloody ofloody

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Priority Queue Implement Priority Queue
5 participants
@NEUDitao @willstenzel @liustanley @dajinchu @ofloody