[AI Tutor] CT-253: Teachers can view flagged chats #57669

ebeastlake · 2024-03-29T23:13:55Z

This PR:

Adds three dropdowns to the AI Tutor teacher dashboard to filter chats by status, student name, and time.
Refactors a couple of the shared constants related to AI Tutor and updates the names everywhere they're used.
Exports the CheckboxOption type from CheckboxDropdown so it can be used in TypeScript code that creates the options for the dropdown.

Follow-ups:

Iterate on UI
Add pagination

Video of filters in action:

initial_dropdown_work.mov

Links

Jira ticket: https://codedotorg.atlassian.net/browse/CT-253

Testing story

I tested manually. My manual testing was not the most comprehensive, but there's a lot of follow-up work to flag and persist the PII/profane status in the database, so I expect to continue testing this in the coming days.

TODO:

Confirm it's safe to change shared constants and there aren't additional steps to take

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

ebeastlake · 2024-04-01T23:15:51Z

apps/script/generateSharedConstants.rb

@@ -96,8 +96,9 @@ def main
    CHILD_ACCOUNT_COMPLIANCE_STATES
    CENSUS_CONSTANTS
    DANCE_SONG_MANIFEST_FILENAME
-    AI_TUTOR_INTERACTION_SAVE_STATUS
+    AI_TUTOR_INTERACTION_STATUS


See the rationale below for refactoring here. Is it dangerous to refactor shared constants?

Seems ok to me, especially since it's something pretty clearly owned/consumed by your team?

Thinking about Ben's comment at https://github.com/code-dot-org/code-dot-org/pull/57669/files#r1548141051 - are their plans to use STATUS on the backend for aichat @sanchitmalhotra126 ?

I think we could but it's a bit unclear since our API design is still being finalized. I'm okay with keeping this in the AI Tutor space for now and revisiting/refactoring when the API is more solidified.

When I said "dangerous," what I was thinking of was, "Is there some script or something I need to reseed this constant file?" If tests are passing, I'll assume not. I'm happy to refactor this more down the line if it's useful for generative AI.

ebeastlake · 2024-04-01T23:17:15Z

apps/src/aiTutor/types.ts

+export type AITutorTypesValue =
+  (typeof AITutorTypes)[keyof typeof AITutorTypes];
+export type AITutorInteractionStatusValue =
+  (typeof AITutorInteractionStatus)[keyof typeof AITutorInteractionStatus];


I was operating at the edge of my TypeScript knowledge, so I'd appreciate a second set of eyes. I'm also tripped up by naming conventions here and would appreciate any guidance.

I haven't seen this syntax before, but let's try to figure it out! AITutorInteractionStatus is

AI_TUTOR_INTERACTION_STATUS = { ERROR: 'error', PII_VIOLATION: 'pii_violation', PROFANITY_VIOLATION: 'profanity_violation', OK: 'ok', UNKNOWN: 'unknown', }

keyof typeof AITutorInteractionStatus gives you all the keys in an object (helpful article). In this case it is 'ERROR' | 'PII_VIOLATION' | 'PROFANITY_VIOLATION' | 'OK' | 'UNKNOWN'

typeof AITutorInteractionStatus is (referencing that same article) {ERROR: string, PII_VIOLATION: string, ...}.

If you put those together, we are getting the types for each key in typeof AITutorInteractionStatus, which ends up being string | string | string ..., which gets combined into just string (I tried this out locally and that's what I ended up with, assuming my initial assumption on the object was correct). If you hover over AITutorInteractionStatusValue do you see string or something else? I'm guessing we actually wanted the values of the object.

I can't find an easy way to turn the values into a type if that's what we want.

The as const assertion can be handy here - if you add that to the original object, TS treat the types as the literal values (instead of string)

For example -

const Status = { key1: 'value1', key2: 'value2', }; // type is string type StatusValues = (typeof Status)[keyof typeof Status]; const StatusConst = { key1: 'value1', key2: 'value2', } as const; // type is 'value1' | 'value2' type StatusConstValue = (typeof StatusConst)[keyof typeof StatusConst];

Thank you so much for the deep dive here, Molly and Sanchit!

Wrinkle: The imported constant comes from here:

code-dot-org/apps/script/generateSharedConstants.rb

Lines 40 to 41 in 15c5893

raw = source_module.const_get(shared_const_name)

begin

.

I tried

import { AiTutorTypes } from '@cdo/apps/util/sharedConstants'; const AITutorTypes = AiTutorTypes as const;

And that doesn't work. One idea is to modify generateSharedConstants to re-export with const assertion, but that doesn't seem worth the trouble assuming it's possible.

yeah I think just copying the constants over seems fine here

ebeastlake · 2024-04-01T23:17:50Z

apps/src/aiTutor/types.ts

+  studentName: string;
+  type: AITutorTypesValue;
+  updatedAt?: string;
+  userId: number;


These fields seemed missing but were on the chatMessage on the front end.

ebeastlake · 2024-04-01T23:18:28Z

apps/src/aiTutor/types.ts


-export type Level = {
+export interface Level {


Per recommendation from @molly-moen, prefer interface to type where possible.

ebeastlake · 2024-04-01T23:20:07Z

apps/src/aichat/types.ts

+import {
+  AiTutorInteractionStatus as AITutorInteractionStatus,
+  PiiTypes as PII,
+} from '@cdo/apps/util/sharedConstants';


I don't think this file is used much anymore, but it shares types with AI tutor code. Tagging @sanchitmalhotra126 @bencodeorg @fisher-alice for confirmation this is okay.

The new "generative AI" lab is using being built on the aichat lab, so it's back in use!

ebeastlake · 2024-04-01T23:22:13Z

apps/src/componentLibrary/checkboxDropdown/CheckboxDropdown.tsx

@@ -26,6 +26,12 @@ import {
 } from '@cdo/apps/componentLibrary/common/contexts/DropdownContext';
 import {dropdownColors} from '@cdo/apps/componentLibrary/common/constants';

+export interface CheckboxOption {


This wasn't exported for use, so I added it because I have a lot of functions creating filters in TypeScript that benefitted from it. @levadadenys Is it okay that I added it?

ebeastlake · 2024-04-01T23:27:12Z

lib/cdo/shared_constants.rb

-    PROFANITY: 'profanity',
-    INAPPROPRIATE: 'inappropriate',
+    PII_VIOLATION: 'pii_violation',
+    PROFANITY_VIOLATION: 'profanity_violation',


Changes in these two constants + rationale:

Rename AI_TUTOR_INTERACTION_SAVE_STATUS to AI_TUTOR_INTERACTION_STATUS because, in this case, "saved" and "ok" are the same, so the original name included the success case described within it.

Rename "personal" to "PII_VIOLATION" and "profanity" to "PROFANITY_VIOLATION" to match the AI TA code:

code-dot-org/lib/cdo/shared_constants.rb

Line 62 in d779649

}

. I think this also hints that these two types of messages map directly to checks we already have in our system (WebPurify checking for PII and profanity), instead of generically being personal, for example.

Move "email", "address", and "phone" into their own constant. These are not interaction statuses, they're types of PII that might be flagged, so I think they should live somewhere else.

Doing this refactor also allowed me to build my initial status filter to map directly to the states of this "AI_TUTOR_INTERACTION_STATUS" constant without unused fields.

This seems reasonable to me! did you find any usages of the email/phone/address values in Ruby? It looks like they might have been moved there in an earlier refactor, might make sense for them just to live in a JS file:

https://github.com/code-dot-org/code-dot-org/pull/56361/files

I expected to, but I didn't, Ben! Some of my tasks include sending AI Tutor chats through the PII/profanity filters and ensuring they're flagged appropriately. Do you mind leaving this here until I decide whether to use them on the backend? I filed a follow-up task to remove if I don't end up using them: https://codedotorg.atlassian.net/browse/CT-470

ebeastlake · 2024-04-01T23:28:21Z

apps/src/aiTutor/redux/aiTutorRedux.ts

-    const generalChat = tutorType === TutorType.GENERAL_CHAT;
-    const compilation = tutorType === TutorType.COMPILATION;
-    const validation = tutorType === TutorType.VALIDATION;
+    const generalChat = tutorType === TutorTypes.GENERAL_CHAT;


I got a little refactor happy and renamed TutorType to TutorTypes to indicate that this references an object that stores all of the types/multiple fields 😅

bencodeorg

I just looked at the stuff in the aichat directory / shared code, but that all looks reasonable to me!

bencodeorg · 2024-04-02T15:48:10Z

apps/src/aichat/types.ts

@@ -1,11 +1,19 @@
 import {LevelProperties} from '@cdo/apps/lab2/types';
-import {AiTutorInteractionSaveStatus} from '@cdo/apps/util/sharedConstants';
+import {
+  AiTutorInteractionStatus as AITutorInteractionStatus,


Slightly out of scope, but should this be called AiInteractionStatus if it's used across aichat and aitutor?

@bencodeorg I think my ultimate goal is in the spirit of this comment, which is to condense/standardize things. I don't know how long the AI Chat use case will continue to be relevant.

Per @sanchitmalhotra126's comment above, the best way to achieve clarity/reusability might be to 1) wait until we have a better understanding of the API for the gen AI unit, 2) use it to make these values more reusable across gen AI and AI tutor (and then rename them then if applicable), and 3) ultimately look to deprecate any aichat code that doesn't move to AI Tutor or Gen AI code? As far as I know, the AI Chat allthethings level is no longer used...

Are you all creating a new directory or using the aichat directory?

We're actively doing development of the gen AI unit using the AI Chat allthethings level! AI Chat is being repurposed as gen AI. The API will probably be replaced though, to Sanchit's point. Fine to leave as-is for now, just a thought :)

sorry just saw comment re: directory, using the aichat directory!

bencodeorg · 2024-04-02T15:49:47Z

apps/src/aichat/types.ts

+import {
+  AiTutorInteractionStatus as AITutorInteractionStatus,
+  PiiTypes as PII,
+} from '@cdo/apps/util/sharedConstants';


The new "generative AI" lab is using being built on the aichat lab, so it's back in use!

bencodeorg · 2024-04-02T16:00:48Z

apps/script/generateSharedConstants.rb

@@ -96,8 +96,9 @@ def main
    CHILD_ACCOUNT_COMPLIANCE_STATES
    CENSUS_CONSTANTS
    DANCE_SONG_MANIFEST_FILENAME
-    AI_TUTOR_INTERACTION_SAVE_STATUS
+    AI_TUTOR_INTERACTION_STATUS


Seems ok to me, especially since it's something pretty clearly owned/consumed by your team?

bencodeorg · 2024-04-02T16:19:23Z

lib/cdo/shared_constants.rb

-    PROFANITY: 'profanity',
-    INAPPROPRIATE: 'inappropriate',
+    PII_VIOLATION: 'pii_violation',
+    PROFANITY_VIOLATION: 'profanity_violation',


This seems reasonable to me! did you find any usages of the email/phone/address values in Ruby? It looks like they might have been moved there in an earlier refactor, might make sense for them just to live in a JS file:

https://github.com/code-dot-org/code-dot-org/pull/56361/files

sanchitmalhotra126

The aichat changes look good to me as well!

sanchitmalhotra126 · 2024-04-02T16:28:33Z

apps/src/aichat/chatApi.ts

FWIW I expect that for Gen AI we may just create a new API file since we're no longer using this chat API. If it makes more sense to just move this to the aiTutor directory, feel free!

I think you're right that refactoring this code makes sense. It looks like the only way this file is used is to import {getChatCompletionMessage} from '../chatApi'; in two files: aichatRedux.ts and aiTutorRedux.ts. There will be a fairly significant diff with that refactor, so I'd like to address it as a follow-up task!

molly-moen · 2024-04-02T19:17:37Z

apps/src/code-studio/components/aiTutor/aiTutorChatMessagesTable.tsx

+    const messageDate = moment(message.createdAt);
+    let timeMatch = false;
+    switch (selectedTimeFilter) {
+      case 'lastHour':


is there a way for us to have these values as constants instead of hard-coding them in multiple places? Could we have an enum that we use to generate the formatted dropdown options?

molly-moen · 2024-04-02T19:18:48Z

apps/src/code-studio/components/aiTutor/aiTutorChatMessagesTable.tsx

+          allOptions={statusOptions}
+          checkedOptions={selectedStatuses}
+          color="black"
+          labelText="Filter by Status"


Is it worth turning these into translated strings now or are things still too in flux for that? Should a follow-up task be followed for translation?

Good call, @molly-moen. Given the quick timeline for the closed beta and uncertainty about whether we will be offering AI Tutor in languages other than English, I'd like permission to handle this as a follow-up task: https://codedotorg.atlassian.net/browse/CT-469

I've added it to the closed beta release because I think the easiest time to add the translated strings is when you write the code, but noted it as non-blocking.

yep a follow up seems reasonable!

molly-moen

🎉

ebeastlake · 2024-04-02T23:48:00Z

apps/src/aiTutor/types.ts

-  (typeof AITutorInteractionStatus)[keyof typeof AITutorInteractionStatus];
+// TODO: Update this once https://codedotorg.atlassian.net/browse/CT-471 is resolved
+export type AITutorTypesValue = string;
+export type AITutorInteractionStatusValue = string;


Functionally, this was only checking for a string before. Narrowing the type-checking here was difficult, and throwing many errors because these are generated constants. I filed https://codedotorg.atlassian.net/browse/CT-471 to address it.

Erin007 and others added 8 commits March 5, 2024 16:04

controller method to fetch single student's AiTutorInteractions

aa332c1

add endpoint to interactionsApi

9528eae

refactor api routes for ai tutor interactions

8a51b60

refactor to more RESTful single route

dcf308a

remove puts

45cd8f9

fix querystring logic and autocorrect ruby linting

2b3f1e0

refactor ai tutor constants

f1286fb

resolve type issues

424ecf5

ebeastlake changed the base branch from staging to eb-single-student-api-endpoints-for-ai-tutor March 29, 2024 23:14

ebeastlake added 2 commits March 29, 2024 17:32

populate dropdown

cd3ca98

add filters for status, student, and time

bde8c88

ebeastlake commented Apr 1, 2024

View reviewed changes

ebeastlake requested review from bencodeorg, sanchitmalhotra126, fisher-alice and a team April 1, 2024 23:34

Base automatically changed from eb-single-student-api-endpoints-for-ai-tutor to staging April 2, 2024 04:34

resolve merge conflicts

15c5893

bencodeorg reviewed Apr 2, 2024

View reviewed changes

sanchitmalhotra126 approved these changes Apr 2, 2024

View reviewed changes

molly-moen reviewed Apr 2, 2024

View reviewed changes

ebeastlake added 2 commits April 2, 2024 14:31

add enum for time filters

9b3f650

missed usage of enum

3f1a094

molly-moen approved these changes Apr 2, 2024

View reviewed changes

ebeastlake added 2 commits April 2, 2024 16:44

loosen type checking

dd20f0f

use AITutorInteractionStatusValue over string

67fd49d

ebeastlake commented Apr 2, 2024

View reviewed changes

ebeastlake added 2 commits April 2, 2024 16:49

Merge branch 'staging' into emily/ct-253/view-flagged-chats

2708594

remove errant semicolon

56979a7

bencodeorg mentioned this pull request Apr 8, 2024

Gen AI lab: update styling / content of model update messages #57814

Merged

ebeastlake merged commit 9a4ae51 into staging Apr 9, 2024
2 checks passed

ebeastlake deleted the emily/ct-253/view-flagged-chats branch April 9, 2024 01:21

fisher-alice mentioned this pull request Apr 12, 2024

Add backend controller and client for Gen AI #57944

Merged

8 tasks

[AI Tutor] CT-253: Teachers can view flagged chats #57669

[AI Tutor] CT-253: Teachers can view flagged chats #57669

Conversation

ebeastlake commented Mar 29, 2024 • edited

Links

Testing story

PR Checklist:

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

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

ebeastlake Apr 1, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencodeorg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebeastlake Apr 2, 2024 • edited

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

sanchitmalhotra126 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

molly-moen left a comment

Choose a reason for hiding this comment

ebeastlake Apr 2, 2024 • edited

Choose a reason for hiding this comment

ebeastlake commented Mar 29, 2024 •

edited

ebeastlake Apr 1, 2024 •

edited

ebeastlake Apr 2, 2024 •

edited

ebeastlake Apr 2, 2024 •

edited