docs: add ADR on supporting learner credit with EMET #686

adamstankiewicz · 2023-03-02T14:17:31Z

Description

ADR for migrating to new learner credit (preview)

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

codecov · 2023-03-02T15:16:03Z

Codecov Report

Patch coverage: 26.47% and project coverage change: -0.44 ⚠️

Comparison is base (cd34ea1) 81.73% compared to head (7f45c38) 81.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #686      +/-   ##
==========================================
- Coverage   81.73%   81.30%   -0.44%     
==========================================
  Files         284      288       +4     
  Lines        5618     5649      +31     
  Branches     1355     1365      +10     
==========================================
+ Hits         4592     4593       +1     
- Misses       1001     1032      +31     
+ Partials       25       24       -1

Impacted Files	Coverage Δ
src/components/skills-quiz/TopSkillsOverview.jsx	`8.57% <8.57%> (ø)`
src/components/skills-quiz/SimilarJobs.jsx	`10.00% <10.00%> (ø)`
src/components/skills-quiz/IndustryDropdown.jsx	`100.00% <100.00%> (ø)`
src/components/skills-quiz/SelectedJobSkills.jsx	`100.00% <100.00%> (ø)`
src/components/skills-quiz/SkillsQuizStepper.jsx	`74.07% <100.00%> (-2.53%)`	⬇️
src/components/skills-quiz/constants.js	`100.00% <100.00%> (ø)`

... and 21 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pwnage101 · 2023-04-18T01:30:38Z

docs/decisions/0008-migrating-to-new-learner-credit.md

+* Poll against `enterprise-subsidy` API for the status of the returned transaction UUID until the transaction is in a non-pending state (e.g., "committed").
+  * If in a "committed" state, redirect learner to courseware URL for OCM courses to begin learning.
+  * If in an "error" state, we will ensure appropriate messaging is displayed and an option to retry.


Currently the create transaction endpoint (POST /api/v1/transactions/) is synchronous.

https://github.com/openedx/enterprise-subsidy/blob/54bee5fb78367f2df72cd0fc809ae621dec88b36/enterprise_subsidy/apps/api/v1/views/transaction.py#L406-L407

Returns 201 (and state will be "committed"), or some other state (in which case the transaction may not get created, or it ended up in "failed"). Either way, after the response is sent, the transaction is in a terminal state if it exists.

Should we be changing this enterprise-subsidy endpoint to be async, or change this ADR to assume that it is synchronous?

I guess what makes the API call synchronous or asynchronous is really just whether the returned transaction is "committed"/"failed" or "pending", which doesn't really change the fundamental shape of the endpoint. Request args and status codes would not change when switching from synchronous to asynchronous mode. In other words, the frontend logic could be:

Make a POST request to the redeem endpoint for the redeemable access policy returned by can_redeem. This returns the transaction payload from enterprise-subsidy.

If the transaction has state "committed":

redirect learner to courseware URL for OCM courses to begin learning.

If the transaction has state "failed":

ensure appropriate messaging is displayed and an option to retry.

If the transaction has state "pending":

Poll against enterprise-subsidy API for the status of the returned transaction UUID until the transaction is in a non-pending state (e.g., "committed").

If in a "committed" state, redirect learner to courseware URL for OCM courses to begin learning.

If in an "failed" state, we will ensure appropriate messaging is displayed and an option to retry.

This frontend flexibility will allow us to defer architecture changes to this create endpoint until a later milestone.

What you're describing is exactly the intent of this ADR and this PR.

While the first EMET release is synchronous as you called out (will always return state: committed), we're opting to future-proof the frontend to account for when the transaction state may not be synchronous such that when state: 'pending' or state: 'failed' is returned, the frontend should handle it appropriately already.

pwnage101 · 2023-04-18T01:50:29Z

docs/decisions/0008-migrating-to-new-learner-credit.md

+
+In order to support existing enrollments that were redeemed outside of the EMET system (e.g., subscription license), the new `CourseRunCard` component (via `CourseRunCards`) will continue to rely on parsing the learner's existing `EnterpriseCourseEnrollments`, data available and used by the course page today, to understand whether the learner has an existing enterprise enrollment that was subsidized outside of the EMET system.
+
+## Consequences


One other consequence related to the point made in the rejected alternatives:

This results in enterprise business logic embedded in frontend code for as long as it takes for us to migrate or age-off old subsidies.

Good callout, will add!

pwnage101 · 2023-04-18T01:55:58Z

docs/decisions/0008-migrating-to-new-learner-credit.md

+
+The metadata pertinent to learner credit that gets exposed by `UserSubsidyContext` is fetched via the [`useEnterpriseOffers`](https://github.com/openedx/frontend-app-learner-portal-enterprise/blob/5034f5ab170589a923c223cfe238112eff48f5c4/src/components/enterprise-user-subsidy/enterprise-offers/data/hooks.js#L11) React hook.
+
+It currently relies on the system-wide feature flag `FEATURE_ENROLL_WITH_ENTERPRISE_OFFERS` and the enterprise customer configuration flag `enableLearnerPortalOffers` to both be truthy. To support the new learner credit system, we will not be relying `enableLearnerPortalOffers`. This boolean flag on the customer configuration is currently used to detemrine whether we should attempt to make API calls to retrieve any learner credit data (such that we can avoid making API calls if a customer doesn't rely on learner credit). The recommendation for the new learner credit system is to no longer rely on `enableLearnerPortalOffers` and always make the API calls even if the customer doesn't utilize learner credit. The performance impacts should largely be mitigated by the fact that we make API calls to fetch all subsidies in parallel rather than waterfall.


[curious] To be clear, this doesn't mean we're removing the enableLearnerPortalOffers flag, right? Will it still be used to switch old offers on and off?

If not (i.e. we will remove the per-enterprise enableLearnerPortalOffers flag), does that represent an unmitigated performance impact in the case where it was previously False, but also the enterprise has no redeemable subsidy in new learner credit?

[curious] To be clear, this doesn't mean we're removing the enableLearnerPortalOffers flag, right? Will it still be used to switch old offers on and off?

Correct, no current plans to remove the enableLearnerPortalOffers quite yet (and probably not until we no longer need to support legacy enterprise offers). It may still be used to switch old offers on/off for the time being.

I believe I was trying to communicate here that we won't be relying on a customer config flag to determine whether or not to call the can-redeem API endpoint; we will always call it even if that customer has no EMET-compatible subsidies.

pwnage101 · 2023-04-18T02:06:05Z

docs/decisions/0008-migrating-to-new-learner-credit.md

+`EnterpriseOffersSummaryCard` either displays a generic message for max global spend or a user-specific message for max user spend remaining. It does not currently support any enrollment limits (e.g., `maxGlobalApplications`). In the case of max user spend remaining, we sum all remaining user balance for all available learner credit. The expiration date shown is from the learner credit that expires first.
+
+#### Search
+This page route similarly relies on the data provided by the `UserSubsidyContext` in order to display an alert to inform learners of low/no balance remaining. The rationale for this alert in the UX is to help proactively make learners are aware they may not have enough funds available to cover the cost of all content returned in the search results.


This context about alerting on low or no balance remaining seems uncovered by the decisions section. What should we do when new learner credit subsidies have low or no balance? How should "low" balance be defined, since we're returning raw balance amounts instead of booleans now? In the name of removing business logic from the frontend, should we change the policy API to return booleans that indicate low balance?

Great questions! The context about the alerting for low/no balance remaining was written before I realized we don't actually need to focus on it for the initial EMET release given we're only tackling use cases where the learners are coming from an external LMS. Given that, the dashboard/search pages are not accessible to learners coming from an LMS so the implications for the dashboard/search page are not being accounted for in the initial EMET release. That's probably why the "Decisions" section doesn't really cover it.

That said, you bring up a great point around whether the API should be making the determination of "low" or "no" balance remaining. The existing business logic in the frontend for this are capturing in the following functions:

isOfferLowOnBalance (source)

isOfferOutOfBalance (source)

I'd be in favor of eventually getting this in to the API layer to further remove business logic from the frontend, but I don't think we necessarily need to take action on it until EMET has a requirement to support non-LMS use cases, too.

Thanks! My only suggestion then would be to add a placeholder section/paragraph in decisions to explicitly defer decisions about low balance UI

Agreed. Will update!

adamstankiewicz added 2 commits March 2, 2023 09:35

docs: add ADR on supporting learner credit with EMET

8ee738e

docs: update ADR title

44786fc

adamstankiewicz force-pushed the ags/adr-learner-credit-emet branch from b99f95c to 44786fc Compare March 2, 2023 14:36

adamstankiewicz added 2 commits March 2, 2023 09:57

docs: add more words

b1b11e6

docs: describe GET call to retrieve policies/subsidies available to user

3b2ca13

adamstankiewicz added the emet label Apr 7, 2023

adamstankiewicz added 2 commits April 7, 2023 14:02

fix: add more details

40632c6

fix: add more details

1b4a202

adamstankiewicz marked this pull request as ready for review April 7, 2023 18:07

adamstankiewicz added 2 commits April 7, 2023 14:07

fix: update ADR date

c8096bc

fix: another push

7f45c38

pwnage101 reviewed Apr 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add ADR on supporting learner credit with EMET #686

docs: add ADR on supporting learner credit with EMET #686

adamstankiewicz commented Mar 2, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

pwnage101 Apr 18, 2023 •

edited

Loading

pwnage101 Apr 18, 2023 •

edited

Loading

adamstankiewicz Apr 19, 2023

pwnage101 Apr 18, 2023

adamstankiewicz Apr 19, 2023

pwnage101 Apr 18, 2023 •

edited

Loading

adamstankiewicz Apr 19, 2023

pwnage101 Apr 18, 2023 •

edited

Loading

adamstankiewicz Apr 19, 2023

pwnage101 Apr 20, 2023 •

edited

Loading

adamstankiewicz Apr 21, 2023


		In order to support existing enrollments that were redeemed outside of the EMET system (e.g., subscription license), the new `CourseRunCard` component (via `CourseRunCards`) will continue to rely on parsing the learner's existing `EnterpriseCourseEnrollments`, data available and used by the course page today, to understand whether the learner has an existing enterprise enrollment that was subsidized outside of the EMET system.

		## Consequences


		The metadata pertinent to learner credit that gets exposed by `UserSubsidyContext` is fetched via the [`useEnterpriseOffers`](https://github.com/openedx/frontend-app-learner-portal-enterprise/blob/5034f5ab170589a923c223cfe238112eff48f5c4/src/components/enterprise-user-subsidy/enterprise-offers/data/hooks.js#L11) React hook.

		It currently relies on the system-wide feature flag `FEATURE_ENROLL_WITH_ENTERPRISE_OFFERS` and the enterprise customer configuration flag `enableLearnerPortalOffers` to both be truthy. To support the new learner credit system, we will not be relying `enableLearnerPortalOffers`. This boolean flag on the customer configuration is currently used to detemrine whether we should attempt to make API calls to retrieve any learner credit data (such that we can avoid making API calls if a customer doesn't rely on learner credit). The recommendation for the new learner credit system is to no longer rely on `enableLearnerPortalOffers` and always make the API calls even if the customer doesn't utilize learner credit. The performance impacts should largely be mitigated by the fact that we make API calls to fetch all subsidies in parallel rather than waterfall.

docs: add ADR on supporting learner credit with EMET #686

Are you sure you want to change the base?

docs: add ADR on supporting learner credit with EMET #686

Conversation

adamstankiewicz commented Mar 2, 2023 • edited Loading

Description

For all changes

Only if submitting a visual change

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

pwnage101 Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

pwnage101 Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz Apr 19, 2023

Choose a reason for hiding this comment

pwnage101 Apr 18, 2023

Choose a reason for hiding this comment

adamstankiewicz Apr 19, 2023

Choose a reason for hiding this comment

pwnage101 Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz Apr 19, 2023

Choose a reason for hiding this comment

pwnage101 Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz Apr 19, 2023

Choose a reason for hiding this comment

pwnage101 Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz Apr 21, 2023

Choose a reason for hiding this comment

adamstankiewicz commented Mar 2, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

pwnage101 Apr 18, 2023 •

edited

Loading

pwnage101 Apr 18, 2023 •

edited

Loading

pwnage101 Apr 18, 2023 •

edited

Loading

pwnage101 Apr 18, 2023 •

edited

Loading

pwnage101 Apr 20, 2023 •

edited

Loading