Skip to content
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

Closed
wants to merge 9 commits into from

Conversation

pwnage101
Copy link
Contributor

ENT-9360

@pwnage101 pwnage101 force-pushed the pwnage101/ENT-9360 branch 3 times, most recently from 0c2eb59 to 3a39614 Compare August 16, 2024 19:43
Copy link

codecov bot commented Aug 16, 2024

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.43%. Comparing base (7bef328) to head (39977db).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...s/app/data/hooks/useCourseRedemptionEligibility.js 75.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1155      +/-   ##
==========================================
+ Coverage   88.41%   88.43%   +0.01%     
==========================================
  Files         399      399              
  Lines        8502     8548      +46     
  Branches     2088     2105      +17     
==========================================
+ Hits         7517     7559      +42     
- Misses        942      945       +3     
- Partials       43       44       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 36 to 41
const availableCourseRuns = courseMetadata.availableCourseRuns.filter(r => isRunUnrestricted({
restrictedRunsAllowed,
courseMetadata,
courseRunKey: r.key,
applicableCatalogUuid,
}));
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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/utils.js Outdated Show resolved Hide resolved
src/components/app/data/utils.js Outdated Show resolved Hide resolved
src/components/course/course-header/CourseRunCards.jsx Outdated Show resolved Hide resolved
Comment on lines 36 to 41
const availableCourseRuns = courseMetadata.availableCourseRuns.filter(r => isRunUnrestricted({
restrictedRunsAllowed,
courseMetadata,
courseRunKey: r.key,
applicableCatalogUuid,
}));
Copy link
Member

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?

src/components/course/course-header/CourseRunCards.jsx Outdated Show resolved Hide resolved
src/components/app/data/utils.js Outdated Show resolved Hide resolved
@pwnage101 pwnage101 force-pushed the pwnage101/ENT-9360 branch 2 times, most recently from 623a0af to c2220e9 Compare October 21, 2024 04:54
@pwnage101
Copy link
Contributor Author

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.

@pwnage101
Copy link
Contributor Author

I'm starting a new PR to carry this work forward with a clean comment slate and new ENT ticket number (ENT-9410):
#1220

@pwnage101 pwnage101 closed this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants