Install Xpert chatbot from frontend-lib-learning-assistant. #1166
Conversation
MichaelRoytman
force-pushed
the
michaelroytman/MST-2034-install-learning-assistant-lib
branch
from
1cbe62e to
d4333a3
Compare
| @@ -93,10 +93,10 @@ const Course = ({ | |||
| /> | |||
| {shouldDisplayTriggers && ( | |||
| <> | |||
| <ChatTrigger | |||
| <Chat | |||
| enabled={course.learningAssistantEnabled} | |||
There was a problem hiding this comment.
I'd prefer to read these values from the Redux store, but it'll make testing more difficult because I'll have to set up that part of the Redux store, so I've chosen to use props here in the interest of getting this out more quickly.
| enrollmentMode: null, | ||
| }; | ||
|
|
||
| export default injectIntl(Chat); |
There was a problem hiding this comment.
I'm not sure if this is really necessary, since there's no text to internationalize right now.
|
|
||
| const courseId = 'course-v1:edX+DemoX+Demo_Course'; | ||
| let testCases = []; | ||
| let enabledTestCases = []; |
There was a problem hiding this comment.
I tried to refactor the test to generate the test cases instead of declaring them outright. This is because the conditions under which the Xpert chatbot should appear have grown more complex, so it's easier to generate the objects. Let me know if this makes it more confusing to read.
| { store }, | ||
| ); | ||
|
|
||
| // This is rather brittle and depends on the implementation of the Xpert component, but there |
There was a problem hiding this comment.
Any other ideas here?
There was a problem hiding this comment.
Is there an ID that we can query by instead? I'm hesitant to query by button name because that could change
There was a problem hiding this comment.
We could add a data-testid property to the button that we could query for. I'm a little torn, because I think we should be querying by roles like button, because it's a more representative and accessible way of testing. And I wonder if these tests should break if we change the text of the button. But, on the other hand, these tests aren't meant to test how the toggle button looks and behaves.
Actually, now that I think of it, since I'm only rendering the Xpert in the test, there should only be a single button in the DOM, so I could get away with this.
const chat = screen.queryByRole('button');
What do you think of that? I feel like it's more reasonable to assume there is a button in the DOM than a button with a particular "name". If you prefer the test ID, I can work on adding that.
There was a problem hiding this comment.
If it's only rendering the Xpert Component in the test, I like the idea of querying by the button role alone, as opposed to a button with a particular name. That seems like an appropriate solution to me that is definitely less brittle.
| @@ -116,6 +117,7 @@ export async function initializeTestStore(options = {}, overrideStore = true) { | |||
| models: modelsReducer, | |||
| courseware: coursewareReducer, | |||
| courseHome: courseHomeReducer, | |||
| learningAssistant: learningAssistantReducer, | |||
There was a problem hiding this comment.
I'm a little confused why the learningAssistantReducer is necessary, but the tours or recommendations reducers aren't. Without the learningAssistantReducer, other tests were breaking, so I added it.
MichaelRoytman
changed the title
MichaelRoytman
force-pushed
the
michaelroytman/MST-2034-install-learning-assistant-lib
branch
from
d4333a3 to
c88cd44
Compare
|
|
||
| return ( | ||
| <> | ||
| {/* Use a portal to ensure that component overlay does not compete with learning MFE styles. */} |
There was a problem hiding this comment.
Thank you adding this comment!
| { store }, | ||
| ); | ||
|
|
||
| // This is rather brittle and depends on the implementation of the Xpert component, but there |
There was a problem hiding this comment.
Is there an ID that we can query by instead? I'm hesitant to query by button name because that could change
This commit installs the Xpert chatbot feature from the frontend-lib-learning-assistant repository into the frontend-lib-learning application. This component is rendered by the Course component. The component is only rendered when a few conditions are satisfied.
MichaelRoytman
force-pushed
the
michaelroytman/MST-2034-install-learning-assistant-lib
branch
from
c88cd44 to
4ba1cde
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1166 +/- ##
==========================================
- Coverage 87.87% 87.85% -0.03%
==========================================
Files 264 263 -1
Lines 4480 4470 -10
Branches 1124 1122 -2
==========================================
- Hits 3937 3927 -10
Misses 529 529
Partials 14 14
☔ View full report in Codecov by Sentry. |
|
@alangsto I addressed your feedback. Let me know what you think! |
MichaelRoytman
deleted the
michaelroytman/MST-2034-install-learning-assistant-lib
branch
|
Hey there! This just came to my attention. Is there an OEP, ADR, or other documentation (including any community discussions) that describe the intention behind this new functionality? |
Description
This commit installs the Xpert chatbot feature from the frontend-lib-learning-assistant repository into the frontend-lib-learning application.
This component is rendered by the Course component. The component is only rendered when a few conditions are satisfied.
JIRA: MST-2034 (private)
Demo
NOTE: Please ignore the line over the video camera icon. This was fixed in edx/frontend-lib-learning-assistant#12.
MST-2034-Xpert-Demo.mov