-
Notifications
You must be signed in to change notification settings - Fork 23
fix: prioritize the redemption over request #1398
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1398 +/- ##
=======================================
Coverage 90.35% 90.35%
=======================================
Files 419 419
Lines 9475 9475
Branches 2210 2210
=======================================
Hits 8561 8561
Misses 880 880
Partials 34 34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
if (userSubsidyApplicableToCourse) { | ||
// If a user has a direct subsidy (i.e., an assignment), show the "Enroll" button. | ||
// This takes priority over any "Request" functionality. | ||
if (externalCourseEnrollmentUrl) { | ||
return ( | ||
<ToExecutiveEducation2UEnrollment enrollmentUrl={externalCourseEnrollmentUrl} /> | ||
); | ||
} | ||
// User is not enrolled, but has a redeemable subsidy access policy applicable to the course. | ||
return ( | ||
<Stack gap={2}> | ||
<StatefulEnroll | ||
contentKey={contentKey} | ||
onClick={handleRedeemClick} | ||
onSuccess={handleRedemptionSuccess} | ||
onError={handleRedeemError} | ||
options={{ | ||
trackSearchConversionEventName: EVENT_NAMES.sucessfulEnrollment, | ||
}} | ||
/> | ||
<RedemptionStatusText redemptionStatus={redemptionStatus} /> | ||
</Stack> | ||
); | ||
} |
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.
Just one concern over giving priority to redemption over request, we need to verify what happens in case of Subscriptions B&R?
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.
Also, what if we simply add this additional condition to the existing code:
Original:
if (userCanRequestSubsidyForCourse) {
// User can request a subsidy for the course, but is not enrolled so
// hide the "Enroll" CTA in favor of the "Request enrollment" CTA below
// the course run cards.
return null;
}
Proposed:
if (userCanRequestSubsidyForCourse && !userSubsidyApplicableToCourse) {
// User can request a subsidy for the course, and no existing subsidy is applicable to the course so
// hide the "Enroll" CTA in favor of the "Request enrollment" CTA below
// the course run cards.
return null;
}
b5ef507
to
3b3ec33
Compare
Description
Previously, the enrollment logic was hiding the "Enroll" button if a learner was eligible to request a subsidy, even when they also had a direct, redeemable subsidy (like a course assignment) available. This prevented learners from immediately enrolling in courses they already had access to.
This change adjusts the priority of the enrollment actions. The logic now checks for any applicable, redeemable subsidy before it checks for the ability to request one.
As a result, if a learner has any subsidy that allows for direct enrollment, the "Enroll" button will now be correctly displayed, taking precedence over the "Request" flow.
For all changes
Only if submitting a visual change