[Aichat] Fetch student chat history and display in workspace by fisher-alice · Pull Request #60322 · code-dot-org/code-dot-org

fisher-alice · Aug 9, 2024

This PR adds the frontend api call to fetch student chat history and follows up #60256 which added the backend controller action. It also displays the teacher view of student chat history in the aichat workspace.

Note that UI improvements/updates (auto-scroll to last message, ability to show/hide profane user messages, ...) will be taken care of in a follow-up PR.

Reminder that the teacher view of the student chat is hidden behind an experiment flag:
'?enableExperiments=view_chat_history'.

After update

Screencast video of teacher view of section with 3 students at '/s/allthethings/lessons/47/levels/1':

Screen.Recording.2024-08-12.at.9.07.56.AM.mov

Screencast video of teacher view of section with 3 students on a sublevel at '/s/allthethings/lessons/52/levels/8/sublevel/1':

Screen.Recording.2024-08-12.at.9.09.15.AM.mov

Note that for chatEvents that have property hideForParticipants set to true (e.g., 'The user clears the chat workspace'), these events are NOT visible in the workspace. However, when a teacher is viewing the student chat history, the property isTeacherView is set to true so that these chatEvents are visible in the workspace.

Teacher view of student chat history:

Screencast video of user as participant - chat events are not shown if hideForParticipants is set to true (e.g., 'The user clears the chat workspace.', 'The user loads the aichat level.').

Screen.Recording.2024-08-12.at.9.13.44.AM.mov

Links

jira

Testing story

Added a dashboard unit test for checking that expected chat events returned by the student_chat_history endpoint.
Tested locally on aichat levels in /allthethings including the aichat sublevel at '/s/allthethings/lessons/52/levels/8/sublevel/1'

Deployment strategy

Follow-up work

UI updates to teacher view of student chat history jira.

Privacy

Security

Caching

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

fisher-alice · Aug 9, 2024

-  ChatMessage,
  ChatEvent,
+  ChatMessage,
+  LogChatEventApiResponse,


Alphabetized.

…endpoint

fisher-alice · Aug 12, 2024

    assert_response :forbidden
  end
+
+  test 'student_chat_history successfully returns list of student aichat_events' do


Resolves comment from #60256 (comment)

thomasoniii

couple of tiny structural suggestions, but overall lgtm 🚀

sanchitmalhotra126

So cool to see the history coming back from the server!

A few minor comments and refactor suggestion but the approach looks right to me.

sanchitmalhotra126 · Aug 12, 2024

+  events: ChatEvent[];
+}
+
+const StudentChatHistoryView: React.FunctionComponent<


Alright, another refactoring suggestion 😅 this is coming out of a couple things I noticed:

StudentChatHistoryView and ChatWithModel are almost identical (other than adding isTeacherView to the events in the former)

Relatedly, it seems like we need isTeacherView to override the check for hideForParticipants in ChatEventView

Looking at this again, I'm also wondering if the student's chat history even needs to be in Redux if it's only going to be used and updated in one place (whichever component renders the history).

So, here's a potential suggestion to hopefully make things easier to follow:

Remove the check for hideForParticipants in ChatEventView. ChatEventView simply renders the given ChatEvent.

Create a component like <ChatWithModel> that simply renders a list of ChatEvents in a container. This can just replace ChatWithModel or even replace ChatEventView (i.e. turn ChatEventView into some ChatEventsList that renders a list of ChatEvents in a div container). We can also move autoscroll logic to this container so the student chat history gets it for free.

We can then have a separate component for Student Chat History that 1) fetches and stores (in state) the list of chat event history from the server on mount or whenever dependencies change (level ID, channel ID, user ID, etc) and 2) uses the above component to render the list of returned ChatEvent. No need to add a separate isTeacherView flag since ChatItemView will just render whatever is handed to it

To display the current user's chat with the model, get the list of user events from redux and filter out anything with hideForParticipants: true. Use the above <ChatEventsList> component to render this filtered list of events.

My thinking is that this would result in less conditional logic, more reused components, and a bit of separation between the actual items in the student chat history and the items in the current chat with the model.

Thanks Sanchit!
I made the following updates based on your suggestions above:

Removed the internal StudentChatHistoryView and ChatWithModel components.

Added ChatEventsList component that is used to display the teacher view of student chat history and the visible chat events.

Removed isTeacherView for now - see [Aichat] Fetch student chat history and display in workspace #60322 (comment)

Changed name of selector selectAllMessages to selectAllVisibleMessages and then removed condition to check for hideForParticipants

Kept studentChatHistory stored in redux because of some of the complex logic (checking if sublevel) and the async call, and already checking student list for selected student name in Teacher panel. (discussed this offline).

fisher-alice · Aug 13, 2024

  delete: deleteRequest,
  fetchJson,
+  post,
+  put,


Alphabetize

sanchitmalhotra126

LGTM!

One question on the screen recordings - it looks like the notification events in the history view have a close button. Does this mean they're dismissible?

sanchitmalhotra126 · Aug 13, 2024

+
+interface ChatEventsListProps {
+  events: ChatEvent[];
+  showWaitingAnimation: () => React.ReactNode;


Not your change, but passing a function instead of a boolean here reads a little weird to me (maybe because previously it was an internal function?). Not necessary for this PR, but maybe we could change this to a showWaitingAnimation: boolean and have this component render the appropriate waiting image if true?

Oh yeah - I agree a boolean would be expected here. I'll update in follow-up!

fisher-alice · Aug 14, 2024

LGTM!

One question on the screen recordings - it looks like the notification events in the history view have a close button. Does this mean they're dismissible?

Great question! Interestingly, they're not dismissible. Looking into why they're not able to be dismissed, when the user clicks on the 'x' button, removeUpdateMessage is called and then modelUpdateMessageInfo is assigned undefined by getUpdateMessageLocation which takes an id and state. Don't fully understand this, but state is assigned a Proxy(Object) so that messageList is a Proxy(Array) so that no messages are found.

Do you have any ideas on why Proxy(Object) is found here?

FYI - in the follow-up, I'll remove the 'x' close buttons.

fisher-alice · Aug 14, 2024

    }
    return null;
-  }, [viewAsUserId, students]);
+  }, [viewAsUserId, students, dispatch, currentLevelId]);


FYI - added currentLevelId so that student chat history is updated when level changes (after pr approval) cc: @sanchitmalhotra126

fisher-alice · Aug 14, 2024

    end

-    aichat_events = AichatEvent.where(user_id: student_user_id, level_id: level_id, script_id: script_id).order(created_at: :desc).pluck(:aichat_event)
+    aichat_events = AichatEvent.where(user_id: student_user_id, level_id: level_id, script_id: script_id).order(:created_at).pluck(:aichat_event).map do |event|


Want the oldest event to be first, not last since student chat history lists events from oldest to most recent.

fisher-alice added 2 commits August 9, 2024 22:15

add getStudentChatHistory api

37122bf

add teacher view of chat workspace

c8df88f

fisher-alice commented Aug 9, 2024

View reviewed changes

fisher-alice added 6 commits August 9, 2024 22:53

remove todo comment

1d5eb21

add test for checking chat events returned from student_chat_history …

20539c9

…endpoint

update names

219ef32

change name to isTeacherView

99a7346

merge staging

94ee6f2

use stored row in table

dc55aef

fisher-alice mentioned this pull request Aug 12, 2024

Add Aichat controller action to fetch student chat history #60256

Merged

8 tasks

fisher-alice marked this pull request as ready for review August 12, 2024 15:00

fisher-alice requested review from a team and sanchitmalhotra126 August 12, 2024 15:00

fisher-alice commented Aug 12, 2024

View reviewed changes

thomasoniii reviewed Aug 12, 2024

View reviewed changes

Comment thread apps/src/aichat/aichatApi.ts

thomasoniii reviewed Aug 12, 2024

View reviewed changes

Comment thread apps/src/aichat/redux/aichatRedux.ts Outdated

thomasoniii reviewed Aug 12, 2024

View reviewed changes

Comment thread apps/src/aichat/views/ChatWorkspace.tsx Outdated

thomasoniii approved these changes Aug 12, 2024

View reviewed changes

fisher-alice added 2 commits August 12, 2024 16:10

clean up syntax

975ba28

clean up syntax apps

5fbec46

sanchitmalhotra126 reviewed Aug 12, 2024

View reviewed changes

fisher-alice added 3 commits August 13, 2024 02:13

return parsed aichat_events

2bf300d

just use fetchJson

3d46c90

merge staging

853da02

fisher-alice commented Aug 13, 2024

View reviewed changes

Comment thread apps/src/util/HttpClient.ts

delete: deleteRequest,

fetchJson,

post,

put,

fisher-alice Aug 13, 2024

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

Alphabetize

fisher-alice added 4 commits August 13, 2024 17:19

fix test

353d73d

refactor ChatEventsList

206c02f

fix name

34b2446

tweak comments

ca07615

fisher-alice requested a review from sanchitmalhotra126 August 13, 2024 18:05

sanchitmalhotra126 approved these changes Aug 13, 2024

View reviewed changes

ascending events, change on levelid, remove redundancy

8927adc

fisher-alice commented Aug 14, 2024

View reviewed changes

update unit test

e29e151

fisher-alice merged commit 623d19e into staging Aug 14, 2024

fisher-alice deleted the alice/frontend-student-chat-history branch August 14, 2024 20:18

This was referenced Aug 16, 2024

[Aichat] Teacher view of student chat history - UI updates #60483

Merged

[Aichat] Remove teacher view of student chat history experiment flag #60560

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

fisher-alice commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

After update

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

fisher-alice Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

fisher-alice Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasoniii left a comment

Choose a reason for hiding this comment

Uh oh!

sanchitmalhotra126 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanchitmalhotra126 Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisher-alice Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisher-alice Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

sanchitmalhotra126 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanchitmalhotra126 Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

fisher-alice Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

fisher-alice commented Aug 14, 2024

Uh oh!

fisher-alice Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

fisher-alice Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fisher-alice commented Aug 9, 2024 •

edited

Loading

sanchitmalhotra126 Aug 12, 2024 •

edited

Loading

fisher-alice Aug 13, 2024 •

edited

Loading