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: Add API support for audit upgradable course via learner credit #1101

Merged
merged 19 commits into from
Jul 1, 2024

Conversation

brobro10000
Copy link
Contributor

@brobro10000 brobro10000 commented Jun 17, 2024

Introduces API integration for learner credit based redemptions for the audit -> upgrade enrollment flow.
Refactors related API calls in the useCourseUpgradeData to React Query. This allows us to remove the class based CourseService component.

Conditionally calls APIs based on subsidy redeemability for course upgrades.
Ex. If a subscription based subsidy is applicable for the upgrade flow, it will not call the coupons or can-redeem API.

Suspends the CourseSection with a fallback skeleton component for longer API calls. (see video)
https://github.com/openedx/frontend-app-learner-portal-enterprise/assets/82611798/a4f20b46-8399-4084-9f6a-1dee359e4466

Tickets: https://2u-internal.atlassian.net/browse/ENT-9068 , https://2u-internal.atlassian.net/browse/ENT-9062

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)
  • Ensure English strings are marked for translation. See documentation for more details.

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

@brobro10000 brobro10000 force-pushed the hu/ent-9068 branch 3 times, most recently from aac4272 to eecb25c Compare June 17, 2024 19:34
// export const ENTERPRISE_OFFER_SUBSIDY_TYPE = 'enterpriseOffer';
// export const LEARNER_CREDIT_SUBSIDY_TYPE = 'learnerCredit';

export const PROMISE_FULFILLED = 'fulfilled';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused variable

@brobro10000 brobro10000 force-pushed the hu/ent-9068 branch 2 times, most recently from 16d4fba to 3871d96 Compare June 18, 2024 20:50
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

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

Project coverage is 86.35%. Comparing base (4bffff7) to head (78bd5f2).

Files Patch % Lines
...oard/main-content/course-enrollments/data/hooks.js 88.88% 3 Missing ⚠️
...s/app/data/hooks/useCanUpgradeWithLearnerCredit.js 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1101      +/-   ##
==========================================
+ Coverage   86.04%   86.35%   +0.31%     
==========================================
  Files         378      378              
  Lines        7803     7785      -18     
  Branches     1872     1909      +37     
==========================================
+ Hits         6714     6723       +9     
+ Misses       1035     1010      -25     
+ Partials       54       52       -2     

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

@brobro10000 brobro10000 marked this pull request as ready for review June 20, 2024 15:47
@brobro10000 brobro10000 force-pushed the hu/ent-9068 branch 2 times, most recently from e15f6e9 to 04ca824 Compare June 20, 2024 16:34
@brobro10000 brobro10000 force-pushed the hu/ent-9068 branch 4 times, most recently from 4e2a404 to ef972a5 Compare June 21, 2024 14:27
Comment on lines -2 to -21
export const COURSE_MODES = {
VERIFIED: 'verified',
PROFESSIONAL: 'professional',
NO_ID_PROFESSIONAL: 'no-id-professional',
AUDIT: 'audit',
HONOR: 'honor',
EXECUTIVE_EDUCATION: 'executive-education',
PAID_EXECUTIVE_EDUCATION: 'paid-executive-education',
UNPAID_EXECUTIVE_EDUCATION: 'unpaid-executive-education',
EXECUTIVE_EDUCATION_2U: 'executive-education-2u',
CREDIT_VERIFIED_AUDIT: 'credit-verified-audit',
VERIFIED_AUDIT: 'verified-audit',
};

export const EXECUTIVE_EDUCATION_COURSE_MODES = [
COURSE_MODES.EXECUTIVE_EDUCATION,
COURSE_MODES.PAID_EXECUTIVE_EDUCATION,
COURSE_MODES.UNPAID_EXECUTIVE_EDUCATION,
COURSE_MODES.EXECUTIVE_EDUCATION_2U,
];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed duplicate constants that existed at the src level directory. Refactored usage of COURSE_MODES to COURSE_MODES_MAP

Copy link
Contributor

@kiram15 kiram15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me imho!

expect(result).toBeNull();
} else {
expect(logError).toHaveBeenCalledTimes(1);
expect(logError).toHaveBeenCalledWith(new Error('Request failed with status code 404'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused on how logError would be called if the status code is 404 when logerror is under a check that makes sure it isn't that.

    if (getErrorResponseStatusCode(error) !== 404) {
      logError(error);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a great question. So for the getErrorResponseStatusCode function, were directly mocking the return value of the tests defined httpStatusCode. I am not 100% sure how the status code errors are set. I think the main intent of the test and others like it is to show that we are logging an error only on status codes that aren't 404s. But I'll look into a better strategy to more accurately represent this and other mock API calls with the correct rejected httpStatusCode. 👍🏽

COUPON_CODE_SUBSIDY_TYPE,
COURSE_AVAILABILITY_MAP,
emptyRedeemableLearnerCreditPolicies, ENTERPRISE_OFFER_SUBSIDY_TYPE, LEARNER_CREDIT_SUBSIDY_TYPE,
LICENSE_SUBSIDY_TYPE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the ticket, I expected to see an API call to license-subsidy that should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple parts of the PR that addresses that ask.
The first is the consuming hook which called courseService.fetchUserLicenseSubsidy().
We are removing this call to transform existing data from the useSubscriptions hook which contains all the necessary data that came from the previous courseService.fetchUserLicenseSubsidy() api call. That also allowed us to remove the CourseService class component altogether since no other components were consuming the component.
Screenshot 2024-06-27 at 12 32 28 PM

@brobro10000 brobro10000 merged commit 7212f81 into master Jul 1, 2024
7 checks passed
@brobro10000 brobro10000 deleted the hu/ent-9068 branch July 1, 2024 14:06
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.

None yet

4 participants