-
Notifications
You must be signed in to change notification settings - Fork 214
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: update gating for chat component #1550
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1550 +/- ##
=======================================
Coverage 89.81% 89.81%
=======================================
Files 325 325
Lines 5587 5598 +11
Branches 1384 1357 -27
=======================================
+ Hits 5018 5028 +10
- Misses 553 555 +2
+ Partials 16 15 -1 ☔ View full report in Codecov by Sentry. |
src/courseware/course/chat/Chat.jsx
Outdated
courseId={courseId} | ||
contentToolsEnabled={contentToolsEnabled} | ||
unitId={unitId} | ||
isUpgradEligible={isUpgradeEligible} |
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.
nit: small typo, guessing you meant to name this isUpgradeEligible
?
src/courseware/course/chat/Chat.jsx
Outdated
@@ -42,19 +47,30 @@ const Chat = ({ | |||
|
|||
const shouldDisplayChat = ( | |||
enabled | |||
&& (hasVerifiedEnrollment || isStaff) // display only to verified learners or staff | |||
&& (hasValidEnrollment || isStaff) |
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.
I like this rename, makes more sense since what counts as "verified" is kinda blurred by the enrollments we chose to use
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.
+1!
@@ -175,6 +175,7 @@ initialize({ | |||
CHAT_RESPONSE_URL: process.env.CHAT_RESPONSE_URL || null, | |||
PRIVACY_POLICY_URL: process.env.PRIVACY_POLICY_URL || null, | |||
SHOW_UNGRADED_ASSIGNMENT_PROGRESS: process.env.SHOW_UNGRADED_ASSIGNMENT_PROGRESS || false, | |||
ENABLE_XPERT_AUDIT: process.env.ENABLE_XPERT_AUDIT || false, |
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.
Codecov seems to be unhappy with these lines not being covered, not sure we need (or are planning on) testing on these though?
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.
These aren't normally tested as part of unit tests!
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.
Looks great!
@@ -48,6 +48,13 @@ export const VERIFIED_MODES = [ | |||
'paid-bootcamp', | |||
] as const satisfies readonly string[]; | |||
|
|||
export const AUDIT_MODES = [ |
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.
question: this is just pulled from here right? https://github.com/openedx/edx-platform/blob/394a459dec831b9d306f4c1a09b7d9ece1a97388/common/djangoapps/course_modes/models.py#L208 Looks good!
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.
Yes!
src/courseware/course/chat/Chat.jsx
Outdated
&& ( | ||
VERIFIED_MODES.includes(enrollmentMode) | ||
// audit learners are only eligible if Xpert has been explicitly enabled for audit | ||
|| (AUDIT_MODES.includes(enrollmentMode) && getConfig().ENABLE_XPERT_AUDIT) |
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.
nice!
src/courseware/course/chat/Chat.jsx
Outdated
@@ -42,19 +47,30 @@ const Chat = ({ | |||
|
|||
const shouldDisplayChat = ( | |||
enabled | |||
&& (hasVerifiedEnrollment || isStaff) // display only to verified learners or staff | |||
&& (hasValidEnrollment || isStaff) |
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.
+1!
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.
👍
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.
LGTM
COSMO-574
Enrollment based gating for the chat feature should be toggle-able by a frontend setting. This PR introduces the
ENABLE_XPERT_AUDIT
setting which is used to determine whether or not audit learners should have access to the chat tool.Additionally, a new prop should be passed to the
Xpert
component, which can be used to determine whether or not a given learner is eligible to upgrade in a specific course.This PR should not be merged until after edx/frontend-lib-learning-assistant#71 has been merged and a new version of the library has been released.