feat: use backend GET endpoint to determine whether to show Learning Assistant #41
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
- Coverage 82.35% 81.36% -0.99%
==========================================
Files 13 13
Lines 204 220 +16
Branches 27 30 +3
==========================================
+ Hits 168 179 +11
- Misses 34 39 +5
Partials 2 2 ☔ View full report in Codecov by Sentry. |
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-161-dont-render-when-disabled
branch
2 times, most recently
from
ad2715e to
a070130
Compare
…Assistant This commit uses a new GET endpoint published on the Learning Assistant backend to determine whether the Learning Assistant feature is enabled. If the features is not enabled, the Learning Assistant is not rendered, and vice-versa. This is an alternative to using the edx-platform Courseware API to determine whether the feature is enabled. The reason this decision was made was so as not to introduce 2U-specific code into the platform in the form of calls to the learning-assistant plugin and it's models and APIs. This allows the Learning Assistant frontend to encapsulate calls to the backend instead. Please see edx/learning-assistant#63 for more details.
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-161-dont-render-when-disabled
branch
from
a070130 to
84bbd84
Compare
| const data = await fetchLearningAssistantEnabled(courseId); | ||
| dispatch(setIsEnabled(data.enabled)); | ||
| } catch (error) { | ||
| dispatch(setApiError()); |
There was a problem hiding this comment.
I was considering ensuring that the Learning Assistant isn't rendered when this API call fails (i.e. adding a condition to the render function), but I don't think it's necessary. If the API calls fails, isEnabled in the Redux store will remain as the default false, so the Learning Assistant won't be rendered.
I could add individual states for the two API calls to be sure I know which API call failed. I'm happy to do that, but I don't feel that it's necessary at this time. Let me know if you disagree.
There was a problem hiding this comment.
Your approach makes sense to me!
| @@ -19,4 +19,12 @@ async function fetchChatResponse(courseId, messageList, unitId) { | |||
| return data; | |||
| } | |||
|
|
|||
| async function fetchLearningAssistantEnabled(courseId) { | |||
There was a problem hiding this comment.
We don't have testing for everything in this repository - just happy path. So I'm okay with these being uncovered. We should probably bump up the test coverage at some point, though.
| @@ -19,4 +19,12 @@ async function fetchChatResponse(courseId, messageList, unitId) { | |||
| return data; | |||
| } | |||
|
|
|||
| async function fetchLearningAssistantEnabled(courseId) { | |||
| @@ -12,6 +12,7 @@ export const learningAssistantSlice = createSlice({ | |||
| conversationId: uuidv4(), | |||
| disclosureAcknowledged: false, | |||
| sidebarIsOpen: false, | |||
| isEnabled: false, | |||
| const data = await fetchLearningAssistantEnabled(courseId); | ||
| dispatch(setIsEnabled(data.enabled)); | ||
| } catch (error) { | ||
| dispatch(setApiError()); |
There was a problem hiding this comment.
Your approach makes sense to me!
| @@ -76,6 +88,9 @@ test('clicking the call to action opens the sidebar', async () => { | |||
|
|
|||
| render(<Xpert courseId={courseId} contentToolsEnabled={false} />, { preloadedState: initialState }); | |||
|
|
|||
| // wait for button to appear | |||
| await screen.findByTestId('message-button'); | |||
There was a problem hiding this comment.
[question] so you've just added this check to make sure that the LA is loaded before testing?
There was a problem hiding this comment.
Yes, exactly! I've added an async API call that must complete before the LA is loaded in this commit, so I have to use async React Testing Library utilities to wait for that API call to complete. Before this change, all the tests would fail 🥲, because the test runner was trying to run the rest of the test (e.g. user actions and assertions) before the mock API call "completed" (i.e. the mock promise resolved). This line tells it to wait by saying something like "wait until you can find this element in the DOM". Once that button is in the DOM, we know that the LA has rendered and we can proceed with the rest of the test.
MichaelRoytman
deleted the
michaelroytman/COSMO-161-dont-render-when-disabled
branch
Description
Jira: COSMO-161
Prerequisite: edx/learning-assistant#63 must be merged and deployed before this pull request is merged and deployed.
This commit uses a new GET endpoint published on the Learning Assistant backend to determine whether the Learning Assistant feature is enabled. If the features is not enabled, the Learning Assistant is not rendered, and vice-versa.
This is an alternative to using the
edx-platformCourseware API to determine whether the feature is enabled. The reason this decision was made was so as not to introduce 2U-specific code into the platform in the form of calls to thelearning-assistantplugin and it's models and APIs. This allows the Learning Assistant frontend to encapsulate calls to the backend instead.Please see edx/learning-assistant#63 for more details.