feat: Fetches chat history when loading Xpert #64
Conversation
rijuma
force-pushed
the
rijuma/fetch-chat-history
branch
from
885a305 to
c5149f8
Compare
src/data/thunks.js
Outdated
| @@ -81,6 +81,34 @@ export function clearMessages() { | |||
| }; | |||
| } | |||
|
|
|||
| export function fetchChatHistory(courseId) { | |||
There was a problem hiding this comment.
Nit: similar to feedback above, on naming. Do we want to have it be something like getLearningAssistantMessageHistory? Not sure if that is too verbose but to match the above? Also I've noticed in the past that fetch is usually used in api and for thunks you can use something like get. And having it this way could allow you to still have the LearningAssistantMessageHistory part without it having the same name (i.e. get vs fetch).
rijuma
changed the title
rijuma
force-pushed
the
rijuma/fetch-chat-history
branch
from
c5149f8 to
5646e47
Compare
| @@ -30,5 +30,15 @@ async function fetchLearningAssistantEnabled(courseId) { | |||
| return data; | |||
| } | |||
|
|
|||
| export default fetchChatResponse; | |||
There was a problem hiding this comment.
There's no need for mixing default and named exports on these type of modules.
rijuma
force-pushed
the
rijuma/fetch-chat-history
branch
from
da4904a to
cf367ac
Compare
| "version": "1.0.30001680", | ||
| "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001680.tgz", | ||
| "integrity": "sha512-rPQy70G6AGUMnbwS1z6Xg+RkHYPAi18ihs47GH0jcxIG7wArmPgY3XbS2sRdBbxJljp3thdT8BIqv9ccCypiPA==", |
There was a problem hiding this comment.
This fixes the warning:
Browserslist: caniuse-lite is outdated. Please run:
npx update-browserslist-db@latest
Why you should do it regularly: https://github.com/browserslist/update-db#readme
| @@ -20,7 +20,8 @@ | |||
| "snapshot": "fedx-scripts jest --updateSnapshot", | |||
| "start": "fedx-scripts webpack-dev-server --progress", | |||
| "test": "fedx-scripts jest --coverage --passWithNoTests", | |||
| "test:ci": "fedx-scripts jest --silent --coverage --passWithNoTests" | |||
| "test:ci": "fedx-scripts jest --silent --coverage --passWithNoTests", | |||
| "test:watch": "fedx-scripts jest --passWithNoTests --watch" | |||
There was a problem hiding this comment.
Extremely helpful to have when creating unit tests.
| jest.spyOn(api, 'default').mockResolvedValue(responseMessage); | ||
| jest.spyOn(api, 'fetchChatResponse').mockResolvedValue(responseMessage); |
There was a problem hiding this comment.
This test update, and the similar ones below, are due to moving the fetchChatResponse() from being a default export to be a named one.
| test('calls useMessageHistory() hook', () => { | ||
| render(<Xpert courseId={courseId} contentToolsEnabled={false} unitId={unitId} />, { preloadedState: initialState }); | ||
|
|
||
| expect(useMessageHistory).toHaveBeenCalledWith(courseId); | ||
| }); |
There was a problem hiding this comment.
This test is to confirm that Xpert is calling the new hook.
| })); | ||
| }); | ||
|
|
||
| it('should call the endpoint and process the results', async () => { |
There was a problem hiding this comment.
Can we think of other test cases here? Maybe error conditions?
There was a problem hiding this comment.
Well.. the fail conditions are handled on the thunk usage, so it is contemplated. On the api module side any error would just throw.
There was a problem hiding this comment.
I was having in mind that any error, whether an endpoint failure or a parsing error (payload structure) would result on not loading the chat history, like a fallback for the current default behavior.
Let me know if you want us to contemplate some custom error messaging for the user.
rijuma
changed the title
Added implementation for fetching the chat history when rendering Xpert Learning Assistant.
Additional updates
test:watch script, very useful to use when working on unit tests.caniuse-litedatabase.Demo
Screen.Recording.2024-11-12.at.17.14.36.mov