Fixing Alerts flow #836

VKong6019 · 2022-02-13T20:47:22Z

Description

Fixing the alerts/rephrase question flow that never got merged in before. Should allow TAs/Profs to ask students to rephrase their question multiple times. Note: the rephrase tooltip is not displayed for grouped questions cause they're grouped xd.

Closes #803
Closes #661

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe how you tested this PR (both manually and with tests)
Provide instructions so we can reproduce.

With the power of manual testing

Checklist:

I have performed a self-review of my own code
I have commented my code where needed
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

- Need 2 alert GET calls for some reason in order to fetch back the new alerts list - Pass correct AlertModel type to service - Fetch 'stafflist` relation on queue so that `checkIsOpen` works

vercel · 2022-02-13T20:47:24Z

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/FhGThhkgzpqiBsJmAbzCsKACMq2z
✅ Preview: https://office-hours-git-vk-fix-fix-alerts-sandboxneu.vercel.app

IrisLiu-00

Thanks for getting this 🙏 🙏
I manual tested it some, looks good as far as i can tell!

packages/app/components/Nav/AlertsContainer.tsx

packages/app/components/Queue/TA/TAQueueDetailButtons.tsx

packages/server/src/alerts/alerts.entity.ts

packages/server/src/alerts/alerts.service.spec.ts

VKong6019 · 2022-02-24T04:53:35Z

packages/app/components/Queue/TA/TAQueueDetailButtons.tsx

@@ -136,6 +142,21 @@ export default function TAQueueDetailButtons({
        return [true, "Help Student"];
      }
    })();
+    const [canRephrase, rephraseTooltip] = ((): [boolean, string] => {


merged the two ternaries into this pair. kinda just followed the same pattern as [canHelp, helpTooltip], but do lmk if there's a better way to organize them

VKong6019 · 2022-02-24T04:54:51Z

packages/server/src/queue/queue.module.ts

  ],
  exports: [QueueCleanService, QueueSSEService],
-  imports: [SSEModule],
+  imports: [SSEModule, AlertsModule],


im boomer, AlertsModule needs to be added/kept here right? for dependencies and stuff

~~oh i see the problem~~

I think so? I forget bc it's been a while since I've used worked with these

yea i think we do in order to inject the AlertsService I added

dfarooq610 · 2022-02-24T15:03:23Z

I don't know if we can do much about this, but the alert is resolved when the question modal pops up -- not when the person closes out of the question modal. So it can result in some weird scenarios like this (asking a student to resolve their question again as they are trying to address their previous question).

It doesnt error out though so it shouldnt be too bad

IrisLiu-00 · 2022-02-24T15:16:41Z

I don't know if we can do much about this, but the alert is resolved when the question modal pops up -- not when the person closes out of the question modal. So it can result in some weird scenarios like this (asking a student to resolve their question again as they are trying to address their previous question).

Hmm I think we can maybe consider that as just people using the app incorrectly - it doesn't seem all that likely that a TA will be impatient enough to ask a student to requestion while the student is still editing

Unless Vera is really gung ho to do some refactoring of our alerts architecture lol

IrisLiu-00

Great stuff!

dfarooq610

Overall, everything seems to work nicely!

Some really minor nits, but otherwise it's good! Thanks for picking up this ticket!

dfarooq610 · 2022-02-24T15:15:07Z

packages/server/src/alerts/alerts.controller.ts

+        user,
+        resolved: null,
+      },
+    });
    return { alerts: await this.alertsService.removeStaleAlerts(alerts) };


Not related to your PR exactly: but do we clean up alerts at night? I wonder what happens when when I ask a student to rephrase their question today, they never address it, and they open up the app tomorrow?

Maybe nothing because the questions are cleaned from the queue (and hence there will be no alerts in the ListQuestionsResponse). Do we want to mark alerts as Stale when closing these questions?

i think it should be fine because theoretically this removeStaleAlerts gets called whenever alerts are fetched. So when the student checks in the next time in the course, i think the old alerts should be culled

const alerts = await AlertModel.find({ where: { courseId, user, resolved: null, }, });

dfarooq610 · 2022-02-24T15:16:46Z

packages/server/src/alerts/alerts.service.ts

+    return await AlertModel.createQueryBuilder('alert')
+      .where('alert.resolved IS NULL')
+      .andWhere('alert.alertType = :alertType', { alertType })
+      .andWhere("(alert.payload ->> 'queueId')::INTEGER = :queueId ", {


packages/server/src/alerts/alerts.service.ts

dfarooq610 · 2022-02-24T15:24:22Z

packages/server/src/alerts/alerts.service.ts

-          const queue = await QueueModel.findOne(payload.queueId);
-          if (question.closedAt || !(await queue.checkIsOpen())) {
-            console.log(`Rephrase Question alert with id ${alert.id} expired`);
+          const queue = await QueueModel.findOne(payload.queueId, {


minor: Why do we need to load the stafflist here? Is it to check if the queue is open?

cc: @IrisLiu-00
iirc, i think it had smth to do with fetching correctly. related to this pr:#834

dfarooq610 · 2022-02-24T15:27:06Z

packages/server/src/queue/queue.module.ts

  ],
  exports: [QueueCleanService, QueueSSEService],
-  imports: [SSEModule],
+  imports: [SSEModule, AlertsModule],


I think so? I forget bc it's been a while since I've used worked with these

VKong6019 · 2022-02-24T17:37:00Z

I don't know if we can do much about this, but the alert is resolved when the question modal pops up -- not when the person closes out of the question modal. So it can result in some weird scenarios like this (asking a student to resolve their question again as they are trying to address their previous question).

Hmm I think we can maybe consider that as just people using the app incorrectly - it doesn't seem all that likely that a TA will be impatient enough to ask a student to requestion while the student is still editing

Unless Vera is really gung ho to do some refactoring of our alerts architecture lol

i have smol amount of faith the TA doesn't spam the button

dfarooq610

🥳

NEUDitao and others added 9 commits February 1, 2022 02:41

alerts save...

e1ba49a

alerts service tests

146ba4d

started integration tests

d2f5860

more test

73da3a2

include unresolved alert payload and disable tooltip effect

83ca314

tooltip messages

523b555

rm debugger oops

eb81c92

So many stupid fixes

f9ee243

- Need 2 alert GET calls for some reason in order to fetch back the new alerts list - Pass correct AlertModel type to service - Fetch 'stafflist` relation on queue so that `checkIsOpen` works

not adding 5 api calls

8f612d7

take my bullshit-fix missing prop

9ff7340

vercel bot deployed to Preview February 13, 2022 21:29 View deployment

can't forget about question grouping

7e0eab9

vercel bot deployed to Preview February 13, 2022 21:48 View deployment

VKong6019 added 2 commits February 17, 2022 00:35

fix un unit test

fbd1c05

gdi lint

d538537

vercel bot deployed to Preview February 17, 2022 16:54 View deployment

VKong6019 marked this pull request as ready for review February 17, 2022 17:09

fix my shit test

24866c7

vercel bot deployed to Preview February 17, 2022 17:27 View deployment

fuxk this shit im out yeehaw

094a201

vercel bot deployed to Preview February 17, 2022 19:25 View deployment

VKong6019 requested a review from a team February 18, 2022 23:41

IrisLiu-00 reviewed Feb 24, 2022

View reviewed changes

VKong6019 added 2 commits February 23, 2022 22:23

damn curly braces

847643a

move fetching unresolved alerts to service with tests

2e90586

vercel bot deployed to Preview February 24, 2022 04:50 View deployment

delete commentted code

5d187eb

vercel bot deployed to Preview February 24, 2022 04:52 View deployment

VKong6019 commented Feb 24, 2022

View reviewed changes

i forgot what a dependency is

e7974bb

vercel bot deployed to Preview February 24, 2022 05:03 View deployment

VKong6019 requested review from IrisLiu-00 and dfarooq610 February 24, 2022 05:06

IrisLiu-00 approved these changes Feb 24, 2022

View reviewed changes

dfarooq610 reviewed Feb 24, 2022

View reviewed changes

rm console log

d326adc

vercel bot deployed to Preview February 24, 2022 17:43 View deployment

dfarooq610 approved these changes Feb 24, 2022

View reviewed changes

VKong6019 merged commit cca52eb into master Feb 24, 2022

VKong6019 deleted the vk-fix-fix-alerts branch February 24, 2022 18:00

dfarooq610 mentioned this pull request Jan 2, 2023

As course staff, I would like to ask a student in the queue to add more detail without removing them from the queue #386

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Alerts flow #836

Fixing Alerts flow #836

VKong6019 commented Feb 13, 2022 •

edited by IrisLiu-00

vercel bot commented Feb 13, 2022 •

edited

IrisLiu-00 left a comment

VKong6019 Feb 24, 2022

VKong6019 Feb 24, 2022

VKong6019 Feb 24, 2022 •

edited

dfarooq610 Feb 24, 2022

VKong6019 Feb 24, 2022

dfarooq610 commented Feb 24, 2022

IrisLiu-00 commented Feb 24, 2022

IrisLiu-00 left a comment

dfarooq610 left a comment

dfarooq610 Feb 24, 2022

VKong6019 Feb 24, 2022

dfarooq610 Feb 24, 2022

dfarooq610 Feb 24, 2022

VKong6019 Feb 24, 2022

dfarooq610 Feb 24, 2022

VKong6019 commented Feb 24, 2022

dfarooq610 left a comment

Fixing Alerts flow #836

Fixing Alerts flow #836

Conversation

VKong6019 commented Feb 13, 2022 • edited by IrisLiu-00

Description

Type of change

How Has This Been Tested?

Checklist:

vercel bot commented Feb 13, 2022 • edited

IrisLiu-00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VKong6019 Feb 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfarooq610 commented Feb 24, 2022

IrisLiu-00 commented Feb 24, 2022

IrisLiu-00 left a comment

Choose a reason for hiding this comment

dfarooq610 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VKong6019 commented Feb 24, 2022

dfarooq610 left a comment

Choose a reason for hiding this comment

VKong6019 commented Feb 13, 2022 •

edited by IrisLiu-00

vercel bot commented Feb 13, 2022 •

edited

VKong6019 Feb 24, 2022 •

edited