-
Notifications
You must be signed in to change notification settings - Fork 19
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: fetch restricted runs and display if unrestricted for redeemable catalog #1155
base: master
Are you sure you want to change the base?
Conversation
0c2eb59
to
3a39614
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1155 +/- ##
==========================================
- Coverage 88.41% 88.40% -0.02%
==========================================
Files 399 399
Lines 8502 8526 +24
Branches 2088 2095 +7
==========================================
+ Hits 7517 7537 +20
- Misses 942 946 +4
Partials 43 43 ☔ View full report in Codecov by Sentry. |
const availableCourseRuns = courseMetadata.availableCourseRuns.filter(r => isRunUnrestricted({ | ||
restrictedRunsAllowed, | ||
courseMetadata, | ||
courseRunKey: r.key, | ||
applicableCatalogUuid, | ||
})); |
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.
[curious] I am wondering if we can pass courseMetadata
directly to parse out the courseRunKey
directly in the helper function from availableCourseRuns
. That way we wouldn't need to pass a specific courseRunKey
.
Also if course metadata could be transformed in the useCourseMetadata
hook, that would be ideal.
The reason being that courseMetadata
would need to parse restricted runs for every usage of the hook versus abstracting that logic direct direct in useCourseMetadata
.
The complexity comes from determining the applicableCatalogUuid
downstream. A polarizing solution would be to allow a optional argument into the useCourseMetadata
hook to pass in applicableCatalogUuid
and if that argument is passed, we can determine whether we display the unrestricted runs. If the argument is not passed, we can probably default on not showing the restricted runs.
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 to this suggestion. I think if we can keep the restrictedRunsAllowed
logic encapsulated within the useCourseMetadata
hook, that would be preferable to avoid needing to consider restrictedRunsAllowed
anywhere that reads
The complexity comes from determining the applicableCatalogUuid downstream.
From the useCourseMetadata
hook's perspective, I don't think it necessarily needs to know the applicableCatalogUuid
; could it simply allow any restricted run(s) as configured by restrictedRunsAllowed
, regardless of an applicable subsidy's catalog? For example, the availableCourseRuns
returned by useCourseMetadata
today do not guarantee the learner has a subsidy covering each of the course runs; the availableCourseRuns
is a list of potentially subsidized, enrollable course runs.
If courseMetadata.availableCourseRuns
returned all available course runs, including restricted (as configured/exposed via restrictedRunsAllowed
), I feel CourseRunCards
could still cross-check any restricted runs returned via courseMetadata.availableCourseRuns
against the associated catalogs for those restricted runs and the catalogUuid
associated with the returned userSubsidyApplicableToCourse
, if needed. Something like:
const {
userSubsidyApplicableToCourse,
missingUserSubsidyReason,
} = useUserSubsidyApplicableToCourse();
const {
data: {
catalogList,
restrictedRunsAllowed,
},
} = useEnterpriseCustomerContainsContent([courseKey]);
const { data: courseMetadata } = useCourseMetadata();
// Exclude configured restricted runs from courseMetadata.availableCourseRuns that are *not* covered by the `userSubsidyApplicableToCourse.catalogUuid`.
const availableCourseRunsWithSubsidizedRestrictedRuns = courseMetadata.availableCourseRuns.filter(r => {
const restrictedRunsForCourse = restrictedRunsAllowed[r.course];
if (!restrictedRunsForCourse?.[r.key]) { return true; } // course run is not restricted
// determine whether applicable subsidy covers (configured) restricted run
return restrictedRunsForCourse[r.key].includes(userSubsidyApplicableToCourse.catalogUuid);
});
// ...
tl;dr; useCourseMetadata
would become responsible for determining which restricted runs (if any) to include in its returned .availableCourseRuns
(regardless of an applicable subsidy's catalogUuid
); then, code paths like here in CourseRunCards
that care about the redeemability of the course runs, could filter the .availableCourseRuns
by restricted runs that are contained by the userSubsidyApplicableToCourse.catalogUuid
.
[thought question] That said, I might also consider the possible implications of rendering a restricted run's CourseRunCard
for which the learner has no applicable subsidy; e.g., might we want the course run card for the (configured) restricted run to appear, but with a disabled enroll CTA and message about the lack of applicable subsidy?
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.
In today's round of updates, I updated useCourseMetadata() to perform an initial pass at filtering the runs based only on restrictedRunsAllowed, but agnostic of the applicable catalog. I'll be honest, it seemed like the Right Thing to do, but I don't actually think it would have any effect on the resulting behavior.
I am intrigued, however, by Hamzah's polarizing proposal to add an optional argument into the useCourseMetadata()
hook to pass in catalogUuid
, as that would theoretically help centralize the filtering logic. Why am I getting the sense that this is taboo?
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 think passing a catalogUuid
to use as a filter within useCourseMetadata
could work; my biggest concern is just coupling useCourseMetadata
to any subsidy-related queries within the hook itself. Passing in the applicable catalogUuid
seems reasonable, since it's not composing useUserSubsidyApplicableToCourse
within useCourseMetadata
directly, which is how I may have (incorrectly) interpreted the above suggestion originally.
src/components/app/data/hooks/useCourseRedemptionEligibility.js
Outdated
Show resolved
Hide resolved
const availableCourseRuns = courseMetadata.availableCourseRuns.filter(r => isRunUnrestricted({ | ||
restrictedRunsAllowed, | ||
courseMetadata, | ||
courseRunKey: r.key, | ||
applicableCatalogUuid, | ||
})); |
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 to this suggestion. I think if we can keep the restrictedRunsAllowed
logic encapsulated within the useCourseMetadata
hook, that would be preferable to avoid needing to consider restrictedRunsAllowed
anywhere that reads
The complexity comes from determining the applicableCatalogUuid downstream.
From the useCourseMetadata
hook's perspective, I don't think it necessarily needs to know the applicableCatalogUuid
; could it simply allow any restricted run(s) as configured by restrictedRunsAllowed
, regardless of an applicable subsidy's catalog? For example, the availableCourseRuns
returned by useCourseMetadata
today do not guarantee the learner has a subsidy covering each of the course runs; the availableCourseRuns
is a list of potentially subsidized, enrollable course runs.
If courseMetadata.availableCourseRuns
returned all available course runs, including restricted (as configured/exposed via restrictedRunsAllowed
), I feel CourseRunCards
could still cross-check any restricted runs returned via courseMetadata.availableCourseRuns
against the associated catalogs for those restricted runs and the catalogUuid
associated with the returned userSubsidyApplicableToCourse
, if needed. Something like:
const {
userSubsidyApplicableToCourse,
missingUserSubsidyReason,
} = useUserSubsidyApplicableToCourse();
const {
data: {
catalogList,
restrictedRunsAllowed,
},
} = useEnterpriseCustomerContainsContent([courseKey]);
const { data: courseMetadata } = useCourseMetadata();
// Exclude configured restricted runs from courseMetadata.availableCourseRuns that are *not* covered by the `userSubsidyApplicableToCourse.catalogUuid`.
const availableCourseRunsWithSubsidizedRestrictedRuns = courseMetadata.availableCourseRuns.filter(r => {
const restrictedRunsForCourse = restrictedRunsAllowed[r.course];
if (!restrictedRunsForCourse?.[r.key]) { return true; } // course run is not restricted
// determine whether applicable subsidy covers (configured) restricted run
return restrictedRunsForCourse[r.key].includes(userSubsidyApplicableToCourse.catalogUuid);
});
// ...
tl;dr; useCourseMetadata
would become responsible for determining which restricted runs (if any) to include in its returned .availableCourseRuns
(regardless of an applicable subsidy's catalogUuid
); then, code paths like here in CourseRunCards
that care about the redeemability of the course runs, could filter the .availableCourseRuns
by restricted runs that are contained by the userSubsidyApplicableToCourse.catalogUuid
.
[thought question] That said, I might also consider the possible implications of rendering a restricted run's CourseRunCard
for which the learner has no applicable subsidy; e.g., might we want the course run card for the (configured) restricted run to appear, but with a disabled enroll CTA and message about the lack of applicable subsidy?
5dfda3a
to
87d0126
Compare
…e catalog ENT-9360
623a0af
to
c2220e9
Compare
c2220e9
to
9910b83
Compare
I massively altered the direction of this PR to reflect fundamental design changes. Most importantly, instead of relying on a customized contains_content_items API endpoint to supply a mapping of which run is allowed for which catalog, then performing all the restricted runs business logic in the frontend (i.e. business logic "at the edge"), we are instead pivoting to performing that same logic in the enterprise-access can-redeem layer and letting the frontend be dumb and just ask can-redeem if a restricted run should be visible. |
ENT-9360