feat: display warning when plan is about to expire #1177

zwidekalanga · 2024-02-27T05:20:57Z

For all changes

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

Only if submitting a visual change

6a54ca9b-86fb-4d50-abc5-91f8fc1de163.mp4

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

adamstankiewicz

Left some comments/suggestions (most notably around removing the need to do as much prop drilling, etc. to pass budgets related data into your BudgetExpiryComponent), and several clarifying questions about the removal of the no longer relevant "Contact representative" modal, and some duplicative functions serving the same purpose.

Note, it'd also be great to include screenshots in the PR description for UI-facing changes such as these to aid reviewers as well (per the PR checklist template).

src/components/BudgetExpiryComponent/BudgetExpiryComponentWrapper.jsx

src/components/Admin/index.jsx

src/components/BudgetExpiryComponent/index.jsx

src/components/learner-credit-management/BudgetDetailPageHeader.jsx

src/components/learner-credit-management/BudgetDetailPageOverviewAvailability.jsx

src/components/learner-credit-management/data/utils.js

src/eventTracking.js

adamstankiewicz

Looks good to me! I just have one clarifying question about the changes in orderBudgets that I might recommend looking into before this merges.

src/components/learner-credit-management/data/utils.js

src/components/Admin/__snapshots__/Admin.test.jsx.snap

src/components/Admin/index.jsx

brobro10000

Attempted to review but some unaddressed PR feedback remaining. Will check tomorrow morning for a quick lookover.

codecov · 2024-04-04T07:33:02Z

Codecov Report

Attention: Patch coverage is 92.62295% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 85.54%. Comparing base (a19fe04) to head (8bc645f).
Report is 2 commits behind head on master.

❗ Current head 8bc645f differs from pull request most recent head eef9e65. Consider uploading reports for the commit eef9e65 to get more accurate results

Files	Patch %	Lines
src/components/BudgetExpiryAlertAndModal/index.jsx	84.00%	4 Missing ⚠️
...BudgetExpiryAlertAndModal/data/hooks/useExpiry.jsx	87.50%	3 Missing ⚠️
src/components/Admin/index.jsx	75.00%	1 Missing ⚠️
...anagement/BudgetDetailPageOverviewAvailability.jsx	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1177      +/-   ##
==========================================
+ Coverage   85.46%   85.54%   +0.07%     
==========================================
  Files         508      514       +6     
  Lines       11074    11169      +95     
  Branches     2329     2348      +19     
==========================================
+ Hits         9464     9554      +90     
- Misses       1566     1571       +5     
  Partials       44       44

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

brobro10000

Approved with 1 nit that needs to be addressed.

src/components/learner-credit-management/data/utils.js

adamstankiewicz · 2024-04-11T12:05:40Z

src/components/Admin/index.jsx

+            <div className="mt-4">
+              <BudgetExpiryAlertAndModal />
+            </div>


nit: if no alert is rendered, it appears (per the Admin.test.jsx.snap snapshot file) this renders an empty div with mt-4. Is this adding extra whitespace to the Learner Progress Report when there is no budget expiry alert?

Perhaps the .mt-4 could be rendered within the div.col below where we render the <h2> heading, which already has a .mt-4 specified all the time.

adamstankiewicz · 2024-04-11T12:08:10Z

src/components/BudgetExpiryAlertAndModal/data/utils.js

+  const thresholdKeys = Object.keys(ExpiryThresholds).sort((a, b) => a - b);
+  const thresholdKey = thresholdKeys.find((key) => durationDiff.asDays() <= key);
+
+  if (thresholdKey === undefined) {


nit: is the check against undefined necessary or could it be simplified to !thresholdKey?

adamstankiewicz · 2024-04-11T12:08:30Z

src/components/BudgetExpiryAlertAndModal/data/utils.js

+export const isPlanApproachingExpiry = (endDateStr) => {
+  const { thresholdKey, threshold } = getExpirationMetadata(endDateStr);
+
+  if (thresholdKey === null) {


nit: is the check against null necessary or could it be simplified to !thresholdKey?

zwidekalanga force-pushed the zwidekalanga/ent-7934 branch from 31c08d6 to ba31b6a Compare March 12, 2024 13:49

zwidekalanga changed the title ~~Display warning when plan is about to expire~~ feat: display warning when plan is about to expire Mar 13, 2024

zwidekalanga force-pushed the zwidekalanga/ent-7934 branch 2 times, most recently from db6d182 to 859753c Compare March 14, 2024 00:38

adamstankiewicz reviewed Mar 15, 2024

View reviewed changes

zwidekalanga force-pushed the zwidekalanga/ent-7934 branch 5 times, most recently from d7ca0cd to 5750f0d Compare March 25, 2024 13:26

zwidekalanga force-pushed the zwidekalanga/ent-7934 branch 2 times, most recently from 8bd8853 to f28856e Compare March 27, 2024 16:21

adamstankiewicz approved these changes Apr 2, 2024

View reviewed changes

src/components/learner-credit-management/data/utils.js Outdated Show resolved Hide resolved

src/components/Admin/__snapshots__/Admin.test.jsx.snap Outdated Show resolved Hide resolved

src/components/Admin/index.jsx Outdated Show resolved Hide resolved

brobro10000 reviewed Apr 3, 2024

View reviewed changes

zwidekalanga force-pushed the zwidekalanga/ent-7934 branch 7 times, most recently from 733a6c5 to 8bc645f Compare April 4, 2024 07:23

brobro10000 approved these changes Apr 4, 2024

View reviewed changes

src/components/learner-credit-management/data/utils.js Show resolved Hide resolved

zwidekalanga force-pushed the zwidekalanga/ent-7934 branch 3 times, most recently from e86e9cb to e972937 Compare April 10, 2024 09:31

feat: display warning when plan is expiring

eef9e65

zwidekalanga force-pushed the zwidekalanga/ent-7934 branch from e972937 to eef9e65 Compare April 10, 2024 11:29

adamstankiewicz approved these changes Apr 11, 2024

View reviewed changes

adamstankiewicz merged commit 220f4fa into openedx:master Apr 11, 2024
4 checks passed

zwidekalanga added a commit to zwidekalanga/frontend-app-admin-portal that referenced this pull request Apr 11, 2024

fix: code styling issues mentioned in the previous PR openedx#1177

97827f2

zwidekalanga mentioned this pull request Apr 11, 2024

fix: code styling issues for expiry logic #1198

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: display warning when plan is about to expire #1177

feat: display warning when plan is about to expire #1177

zwidekalanga commented Feb 27, 2024 •

edited

Loading

adamstankiewicz left a comment

adamstankiewicz left a comment

brobro10000 left a comment

codecov bot commented Apr 4, 2024 •

edited

Loading

brobro10000 left a comment

adamstankiewicz Apr 11, 2024 •

edited

Loading

adamstankiewicz Apr 11, 2024

adamstankiewicz Apr 11, 2024

feat: display warning when plan is about to expire #1177

feat: display warning when plan is about to expire #1177

Conversation

zwidekalanga commented Feb 27, 2024 • edited Loading

For all changes

Only if submitting a visual change

adamstankiewicz left a comment

Choose a reason for hiding this comment

adamstankiewicz left a comment

Choose a reason for hiding this comment

brobro10000 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 4, 2024 • edited Loading

Codecov Report

brobro10000 left a comment

Choose a reason for hiding this comment

adamstankiewicz Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz Apr 11, 2024

Choose a reason for hiding this comment

adamstankiewicz Apr 11, 2024

Choose a reason for hiding this comment

zwidekalanga commented Feb 27, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading

adamstankiewicz Apr 11, 2024 •

edited

Loading