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: UI updates expired workflow #1264

Conversation

zwidekalanga
Copy link
Contributor

For all changes

Ticket: ENT-9047
Figma: https://www.figma.com/design/gm4yVDt3TixtEi7aD6LETz/Learner-Credit-Expiration?node-id=489-19375&m=dev

  • 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

@zwidekalanga zwidekalanga force-pushed the zwidekalanga/ui-updates-expired-workflow branch 10 times, most recently from 312f886 to 88449b4 Compare July 15, 2024 11:51
Copy link
Member

@brobro10000 brobro10000 left a comment

Choose a reason for hiding this comment

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

I would look at renaming the usages of isEnded to something a bit less ambiguous.

@zwidekalanga zwidekalanga force-pushed the zwidekalanga/ui-updates-expired-workflow branch 2 times, most recently from 80f5ad7 to 89949e5 Compare July 17, 2024 09:51
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.41%. Comparing base (45f7fbc) to head (f9faee6).

Files Patch % Lines
...rner-credit-management/BudgetDetailAssignments.jsx 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1264      +/-   ##
==========================================
+ Coverage   85.40%   85.41%   +0.01%     
==========================================
  Files         541      542       +1     
  Lines       11966    11991      +25     
  Branches     2519     2563      +44     
==========================================
+ Hits        10219    10242      +23     
- Misses       1689     1690       +1     
- Partials       58       59       +1     

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

@zwidekalanga zwidekalanga force-pushed the zwidekalanga/ui-updates-expired-workflow branch 5 times, most recently from eb0345b to f1ad334 Compare July 18, 2024 19:08
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

Overall, this is looking good. I left several comments, but most are smaller, nit-level comments around simplifying the checks around BUDGET_STATUSES. Given the check for retired/expired budget status is fairly commonplace now, I do wonder if it's worth creating a small util function (along the lines of isBudgetExpiredOrRetired) or otherwise to use instead of re-implementing the same conditional in numerous places.

Beyond that, there were a couple other small suggestions as well including a clarifying question around a TODO comment about abstracting status higher up I'd be curious to learn more about.

@zwidekalanga zwidekalanga force-pushed the zwidekalanga/ui-updates-expired-workflow branch from f1ad334 to dfc5fef Compare July 19, 2024 15:15
@zwidekalanga zwidekalanga force-pushed the zwidekalanga/ui-updates-expired-workflow branch from dfc5fef to f9faee6 Compare July 19, 2024 15:21
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

LGTM! ✅ I'll merge this a bit later this morning, and help verify on stage (will let you know when it's deployed to stage).

@adamstankiewicz adamstankiewicz merged commit e6d56d7 into openedx:master Jul 22, 2024
6 checks passed
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