-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
feat: Fetches chat history when loading Xpert #64
Changes from all commits
5646e47
14870d3
ee2c068
586b83a
6498378
cf367ac
b5a41cf
ba81740
a410855
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,5 +30,15 @@ async function fetchLearningAssistantEnabled(courseId) { | |
return data; | ||
} | ||
|
||
export default fetchChatResponse; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no need for mixing default and named exports on these type of modules. |
||
export { fetchLearningAssistantEnabled }; | ||
async function fetchLearningAssistantMessageHistory(courseId) { | ||
const url = new URL(`${getConfig().CHAT_RESPONSE_URL}/${courseId}/history`); | ||
|
||
const { data } = await getAuthenticatedHttpClient().get(url.href); | ||
return data; | ||
} | ||
|
||
export { | ||
fetchChatResponse, | ||
fetchLearningAssistantEnabled, | ||
fetchLearningAssistantMessageHistory, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/* eslint-disable no-import-assign */ | ||
import * as auth from '@edx/frontend-platform/auth'; | ||
|
||
import { fetchLearningAssistantMessageHistory } from './api'; | ||
|
||
jest.mock('@edx/frontend-platform/auth'); | ||
|
||
const CHAT_RESPONSE_URL = 'https://some.url/endpoint'; | ||
jest.mock('@edx/frontend-platform', () => ({ | ||
getConfig: () => ({ CHAT_RESPONSE_URL }), | ||
CHAT_RESPONSE_URL, | ||
})); | ||
|
||
describe('API', () => { | ||
afterEach(() => { | ||
jest.restoreAllMocks(); | ||
}); | ||
|
||
describe('fetchLearningAssistantMessageHistory()', () => { | ||
const fakeCourseId = 'course-v1:edx+test+23'; | ||
const apiPayload = [ | ||
{ | ||
role: 'user', | ||
content: 'Marco', | ||
timestamp: '2024-11-04T19:05:07.403363Z', | ||
}, | ||
{ | ||
role: 'assistant', | ||
content: 'Polo', | ||
timestamp: '2024-11-04T19:05:21.357636Z', | ||
}, | ||
]; | ||
|
||
const fakeGet = jest.fn(async () => ({ | ||
data: apiPayload, | ||
catch: () => {}, | ||
})); | ||
|
||
beforeEach(() => { | ||
auth.getAuthenticatedHttpClient = jest.fn(() => ({ | ||
get: fakeGet, | ||
})); | ||
}); | ||
|
||
it('should call the endpoint and process the results', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we think of other test cases here? Maybe error conditions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
const response = await fetchLearningAssistantMessageHistory(fakeCourseId); | ||
|
||
expect(response).toEqual(apiPayload); | ||
expect(fakeGet).toHaveBeenCalledTimes(1); | ||
expect(fakeGet).toHaveBeenCalledWith(`${CHAT_RESPONSE_URL}/${fakeCourseId}/history`); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
import { fetchLearningAssistantMessageHistory } from './api'; | ||
|
||
import { getLearningAssistantMessageHistory } from './thunks'; | ||
|
||
jest.mock('./api'); | ||
|
||
describe('Thunks unit tests', () => { | ||
const dispatch = jest.fn(); | ||
|
||
afterEach(() => jest.resetAllMocks()); | ||
|
||
describe('getLearningAssistantMessageHistory()', () => { | ||
const fakeCourseId = 'course-v1:edx+test+23'; | ||
|
||
describe('when returning results', () => { | ||
const apiResponse = [ | ||
{ | ||
role: 'user', | ||
content: 'Marco', | ||
timestamp: '2024-11-04T19:05:07.403363Z', | ||
}, | ||
{ | ||
role: 'assistant', | ||
content: 'Polo', | ||
timestamp: '2024-11-04T19:05:21.357636Z', | ||
}, | ||
]; | ||
|
||
beforeEach(() => { | ||
fetchLearningAssistantMessageHistory.mockResolvedValue(apiResponse); | ||
}); | ||
|
||
it('should set the loading state, fetch, parse and set the messages and remove the loading state', async () => { | ||
await getLearningAssistantMessageHistory(fakeCourseId)(dispatch); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(1, { | ||
type: 'learning-assistant/setApiIsLoading', | ||
payload: true, | ||
}); | ||
|
||
expect(fetchLearningAssistantMessageHistory).toHaveBeenCalledWith(fakeCourseId); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(2, { | ||
type: 'learning-assistant/setMessageList', | ||
payload: { | ||
messageList: apiResponse.map(({ timestamp, ...msg }) => ({ | ||
...msg, | ||
timestamp: new Date(timestamp), // Parse ISO time to Date() | ||
})), | ||
}, | ||
}); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(3, { | ||
type: 'learning-assistant/setDisclosureAcknowledged', | ||
payload: true, | ||
}); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(4, { | ||
type: 'learning-assistant/setApiIsLoading', | ||
payload: false, | ||
}); | ||
}); | ||
}); | ||
|
||
describe('when returning no messages', () => { | ||
const apiResponse = []; | ||
|
||
beforeEach(() => { | ||
fetchLearningAssistantMessageHistory.mockResolvedValue(apiResponse); | ||
}); | ||
|
||
it('should only set and remove the loading state', async () => { | ||
await getLearningAssistantMessageHistory(fakeCourseId)(dispatch); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(1, { | ||
type: 'learning-assistant/setApiIsLoading', | ||
payload: true, | ||
}); | ||
|
||
expect(fetchLearningAssistantMessageHistory).toHaveBeenCalledWith(fakeCourseId); | ||
|
||
expect(dispatch).not.toHaveBeenCalledWith( | ||
expect.objectContaining({ type: 'learning-assistant/setMessageList' }), | ||
); | ||
|
||
expect(dispatch).not.toHaveBeenCalledWith( | ||
expect.objectContaining({ type: 'learning-assistant/setDisclosureAcknowledged' }), | ||
); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(2, { | ||
type: 'learning-assistant/setApiIsLoading', | ||
payload: false, | ||
}); | ||
}); | ||
}); | ||
|
||
describe('when throwing on fetching', () => { | ||
beforeEach(() => { | ||
fetchLearningAssistantMessageHistory.mockRejectedValue('Whoopsie!'); | ||
}); | ||
|
||
it('should only set and remove the loading state', async () => { | ||
await getLearningAssistantMessageHistory(fakeCourseId)(dispatch); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(1, { | ||
type: 'learning-assistant/setApiIsLoading', | ||
payload: true, | ||
}); | ||
|
||
expect(fetchLearningAssistantMessageHistory).toHaveBeenCalledWith(fakeCourseId); | ||
|
||
expect(dispatch).not.toHaveBeenCalledWith( | ||
expect.objectContaining({ type: 'learning-assistant/setMessageList' }), | ||
); | ||
|
||
expect(dispatch).not.toHaveBeenCalledWith( | ||
expect.objectContaining({ type: 'learning-assistant/setDisclosureAcknowledged' }), | ||
); | ||
|
||
expect(dispatch).toHaveBeenNthCalledWith(2, { | ||
type: 'learning-assistant/setApiIsLoading', | ||
payload: false, | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/* eslint-disable import/prefer-default-export */ | ||
export { useMessageHistory } from './message-history'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* eslint-disable import/prefer-default-export */ | ||
import { useEffect } from 'react'; | ||
import { useDispatch, useSelector } from 'react-redux'; | ||
import { getLearningAssistantMessageHistory } from '../data/thunks'; | ||
|
||
export const useMessageHistory = (courseId) => { | ||
const dispatch = useDispatch(); | ||
const { isEnabled } = useSelector(state => state.learningAssistant); | ||
|
||
useEffect(() => { | ||
if (!courseId || !isEnabled) { return; } | ||
|
||
dispatch(getLearningAssistantMessageHistory(courseId)); | ||
}, [dispatch, isEnabled, courseId]); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import { renderHook } from '@testing-library/react-hooks'; | ||
|
||
import { useSelector } from 'react-redux'; | ||
import { useMessageHistory } from './message-history'; | ||
import { getLearningAssistantMessageHistory } from '../data/thunks'; | ||
|
||
const mockDispatch = jest.fn(); | ||
jest.mock('react-redux', () => ({ | ||
...jest.requireActual('react-redux'), | ||
useSelector: jest.fn(), | ||
useDispatch: () => mockDispatch, | ||
})); | ||
|
||
const getLearningAssistantMessageHistorySignature = { getLearningAssistantMessageHistory: 'getLearningAssistantMessageHistory' }; | ||
jest.mock('../data/thunks', () => ({ | ||
getLearningAssistantMessageHistory: jest.fn().mockReturnValue(getLearningAssistantMessageHistorySignature), | ||
})); | ||
|
||
describe('Learning Assistant Message History Hooks', () => { | ||
afterEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
describe('useMessageHistory()', () => { | ||
let hook; | ||
const fakeCourseId = 'course-v1:edx+test+23'; | ||
|
||
const renderTestHook = (courseId, isEnabled) => { | ||
const mockedStoreState = { learningAssistant: { isEnabled } }; | ||
useSelector.mockImplementation(selector => selector(mockedStoreState)); | ||
hook = renderHook(() => useMessageHistory(courseId)); | ||
return hook; | ||
}; | ||
|
||
it('should dispatch getLearningAssistantMessageHistory() with the chat history', () => { | ||
renderTestHook(fakeCourseId, true); | ||
|
||
expect(mockDispatch).toHaveBeenCalledTimes(1); | ||
expect(mockDispatch).toHaveBeenCalledWith(getLearningAssistantMessageHistorySignature); | ||
expect(getLearningAssistantMessageHistory).toHaveBeenCalledWith(fakeCourseId); | ||
}); | ||
|
||
it('should NOT dispatch getLearningAssistantMessageHistory() when disabled', () => { | ||
renderTestHook(fakeCourseId, false); | ||
|
||
expect(mockDispatch).not.toHaveBeenCalled(); | ||
expect(getLearningAssistantMessageHistory).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('should NOT dispatch getLearningAssistantMessageHistory() with no courseId', () => { | ||
renderTestHook(null, true); | ||
|
||
expect(mockDispatch).not.toHaveBeenCalled(); | ||
expect(getLearningAssistantMessageHistory).not.toHaveBeenCalled(); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extremely helpful to have when creating unit tests.