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: show all enterprise budgets regardless of plan and route correctly #1037

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

jajjibhai008
Copy link
Contributor

@jajjibhai008 jajjibhai008 commented Sep 20, 2023

Description
This PR covers up the following points

  • All budgets of ecommerce offers and learner-credit offers are showing regardless of plan.
  • View Budget button takes the learner to the detail page along with offerId/budgetId
  • Create two new routes
    • for e-commerce /:enterpriseSlug/admin/learner-credit/:offerId
    • for learner_credit budget /:enterpriseSlug/admin/learner-credit/:budgetId
  • Fetch the enrollments on the basis of budget_id if present otherwise, fetch enrollments on the base of offer_id on the detail page
  • The “Budgets” link in the breadcrumbs on the LCM UI relies on react-router’s Link component and avoids a full-page refresh.
  • Ensure dollar amounts are displayed as monetary (e.g., $18,500.25) rather than just floats without any formatting.
  • Ensure LCM is wrapped in a fluid container for more left/right whitespace padding in the page contents. It should be consistent with other page’s throughout the MFE (e.g., see “Highlights” page’s left/right padding).

JIRA -> [ENT-7672, ENT-7673, ENT-7674, ENT-7565]

Here is the updated screen short
screencapture-localhost-1991-test-enterprise-admin-learner-credit-2023-09-25-19_13_50

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
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (e12f09a) 82.90% compared to head (7874784) 82.91%.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           eahmadjaved/ENT-7660    #1037      +/-   ##
========================================================
+ Coverage                 82.90%   82.91%   +0.01%     
========================================================
  Files                       412      415       +3     
  Lines                      8902     8933      +31     
  Branches                   1825     1832       +7     
========================================================
+ Hits                       7380     7407      +27     
- Misses                     1482     1484       +2     
- Partials                     40       42       +2     
Files Coverage Δ
...c/components/EnterpriseApp/EnterpriseAppRoutes.jsx 65.00% <100.00%> (ø)
src/components/EnterpriseApp/data/constants.js 100.00% <100.00%> (ø)
...omponents/EnterpriseSubsidiesContext/data/hooks.js 94.64% <100.00%> (-1.08%) ⬇️
.../learner-credit-management/MultipleBudgetsPage.jsx 81.81% <100.00%> (ø)
...components/learner-credit-management/data/hooks.js 90.76% <100.00%> (+0.60%) ⬆️
...components/learner-credit-management/data/utils.js 96.00% <100.00%> (+3.14%) ⬆️
src/components/learner-credit-management/index.jsx 100.00% <100.00%> (ø)
...nts/learner-credit-management/BudgetDetailPage.jsx 92.85% <92.85%> (ø)
...onents/learner-credit-management/BudgetCard-V2.jsx 80.00% <50.00%> (-20.00%) ⬇️
...earner-credit-management/MultipleBudgetsPicker.jsx 50.00% <0.00%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

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

Copy link
Member

@zamanafzal zamanafzal left a comment

Choose a reason for hiding this comment

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

LGTM

src/components/EnterpriseApp/data/constants.js Outdated Show resolved Hide resolved
src/components/EnterpriseSubsidiesContext/data/hooks.js Outdated Show resolved Hide resolved
src/components/EnterpriseSubsidiesContext/data/hooks.js Outdated Show resolved Hide resolved
src/components/EnterpriseSubsidiesContext/data/hooks.js Outdated Show resolved Hide resolved
src/components/EnterpriseSubsidiesContext/data/hooks.js Outdated Show resolved Hide resolved
src/components/learner-credit-management/Budgetcard-V3.jsx Outdated Show resolved Hide resolved
src/components/learner-credit-management/BudgetCard-V2.jsx Outdated Show resolved Hide resolved
src/components/learner-credit-management/Budgetcard-V3.jsx Outdated Show resolved Hide resolved
src/components/learner-credit-management/Budgetcard-V3.jsx Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jajjibhai008 jajjibhai008 left a comment

Choose a reason for hiding this comment

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

@adamstankiewicz if this PR looks good to you, then please approve this.

@adamstankiewicz
Copy link
Member

@jajjibhai008 I'm seeing a couple, possibly related, prop type warnings when running this PR locally. Do you mind verifying to ensure they are either unrelated to your changes or, if they are related, whether they can be addressed?

Warning: Failed prop type: Invalid prop `offer.id` of type `number` supplied to `BudgetCard`, expected `string`.
    in BudgetCard (created by MultipleBudgetsPicker)
    in MultipleBudgetsPicker (created by MultipleBudgetsPage)
    in div (created by Container)
    in Container (created by Container)
    in Container (created by MultipleBudgetsPage)
    in MultipleBudgetsPage (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in Route (created by LearnerCreditManagementRoutes)
    in LearnerCreditManagementRoutes (created by EnterpriseAppRoutes)
    in Switch (created by EnterpriseAppRoutes)
    in EnterpriseAppRoutes (created by EnterpriseAppContent)
    in EnterpriseAppContent (created by MediaQuery)
    in div (created by MediaQuery)
    in MediaQuery (created by EnterpriseApp)
    in div (created by EnterpriseApp)
    in EnterpriseAppContextProvider (created by EnterpriseApp)
    in EnterpriseApp (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(EnterpriseApp)) (created by AuthenticatedEnterpriseApp)
    in LoginRedirect (created by AuthenticatedEnterpriseApp)
    in AuthenticatedEnterpriseApp (created by Context.Consumer)
    in Route (created by Context.Consumer)
    in Switch (created by Context.Consumer)
    in Route (created by PageRoute)
    in PageRoute (created by AppWrapper)
    in Switch (created by AppWrapper)
    in Router (created by AppProvider)
    in Provider (created by OptionalReduxProvider)
    in OptionalReduxProvider (created by AppProvider)
    in ErrorBoundary (created by AppProvider)
    in IntlProvider (created by AppProvider)
    in AppProvider (created by AppWrapper)
    in AppWrapper
react-jsx-runtime.development.js:124 Warning: Failed prop type: The prop `children` is marked as required in `ForwardRef(_c)`, but its value is `undefined`.
    in ForwardRef(_c) (created by BudgetCard)
    in BudgetCard (created by MultipleBudgetsPicker)
    in div (created by Col)
    in Col (created by MultipleBudgetsPicker)
    in div (created by Row)
    in Row (created by MultipleBudgetsPicker)
    in div (created by ForwardRef(_c))
    in ForwardRef(_c) (created by MultipleBudgetsPicker)
    in MultipleBudgetsPicker (created by MultipleBudgetsPage)
    in div (created by Container)
    in Container (created by Container)
    in Container (created by MultipleBudgetsPage)
    in MultipleBudgetsPage (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in Route (created by LearnerCreditManagementRoutes)
    in LearnerCreditManagementRoutes (created by EnterpriseAppRoutes)
    in Switch (created by EnterpriseAppRoutes)
    in EnterpriseAppRoutes (created by EnterpriseAppContent)
    in EnterpriseAppContent (created by MediaQuery)
    in div (created by MediaQuery)
    in MediaQuery (created by EnterpriseApp)
    in div (created by EnterpriseApp)
    in EnterpriseAppContextProvider (created by EnterpriseApp)
    in EnterpriseApp (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(EnterpriseApp)) (created by AuthenticatedEnterpriseApp)
    in LoginRedirect (created by AuthenticatedEnterpriseApp)
    in AuthenticatedEnterpriseApp (created by Context.Consumer)
    in Route (created by Context.Consumer)
    in Switch (created by Context.Consumer)
    in Route (created by PageRoute)
    in PageRoute (created by AppWrapper)
    in Switch (created by AppWrapper)
    in Router (created by AppProvider)
    in Provider (created by OptionalReduxProvider)
    in OptionalReduxProvider (created by AppProvider)
    in ErrorBoundary (created by AppProvider)
    in IntlProvider (created by AppProvider)
    in AppProvider (created by AppWrapper)
    in AppWrapper

@jajjibhai008 jajjibhai008 force-pushed the eahmadjaved/lcm-revamp branch 2 times, most recently from e719476 to 0398253 Compare September 25, 2023 15:41
@jajjibhai008
Copy link
Contributor Author

jajjibhai008 commented Sep 25, 2023

@jajjibhai008 I'm seeing a couple, possibly related, prop type warnings when running this PR locally. Do you mind verifying to ensure they are either unrelated to your changes or, if they are related, whether they can be addressed?

Warning: Failed prop type: Invalid prop `offer.id` of type `number` supplied to `BudgetCard`, expected `string`.
    in BudgetCard (created by MultipleBudgetsPicker)
    in MultipleBudgetsPicker (created by MultipleBudgetsPage)
    in div (created by Container)
    in Container (created by Container)
    in Container (created by MultipleBudgetsPage)
    in MultipleBudgetsPage (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in Route (created by LearnerCreditManagementRoutes)
    in LearnerCreditManagementRoutes (created by EnterpriseAppRoutes)
    in Switch (created by EnterpriseAppRoutes)
    in EnterpriseAppRoutes (created by EnterpriseAppContent)
    in EnterpriseAppContent (created by MediaQuery)
    in div (created by MediaQuery)
    in MediaQuery (created by EnterpriseApp)
    in div (created by EnterpriseApp)
    in EnterpriseAppContextProvider (created by EnterpriseApp)
    in EnterpriseApp (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(EnterpriseApp)) (created by AuthenticatedEnterpriseApp)
    in LoginRedirect (created by AuthenticatedEnterpriseApp)
    in AuthenticatedEnterpriseApp (created by Context.Consumer)
    in Route (created by Context.Consumer)
    in Switch (created by Context.Consumer)
    in Route (created by PageRoute)
    in PageRoute (created by AppWrapper)
    in Switch (created by AppWrapper)
    in Router (created by AppProvider)
    in Provider (created by OptionalReduxProvider)
    in OptionalReduxProvider (created by AppProvider)
    in ErrorBoundary (created by AppProvider)
    in IntlProvider (created by AppProvider)
    in AppProvider (created by AppWrapper)
    in AppWrapper
react-jsx-runtime.development.js:124 Warning: Failed prop type: The prop `children` is marked as required in `ForwardRef(_c)`, but its value is `undefined`.
    in ForwardRef(_c) (created by BudgetCard)
    in BudgetCard (created by MultipleBudgetsPicker)
    in div (created by Col)
    in Col (created by MultipleBudgetsPicker)
    in div (created by Row)
    in Row (created by MultipleBudgetsPicker)
    in div (created by ForwardRef(_c))
    in ForwardRef(_c) (created by MultipleBudgetsPicker)
    in MultipleBudgetsPicker (created by MultipleBudgetsPage)
    in div (created by Container)
    in Container (created by Container)
    in Container (created by MultipleBudgetsPage)
    in MultipleBudgetsPage (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in Route (created by LearnerCreditManagementRoutes)
    in LearnerCreditManagementRoutes (created by EnterpriseAppRoutes)
    in Switch (created by EnterpriseAppRoutes)
    in EnterpriseAppRoutes (created by EnterpriseAppContent)
    in EnterpriseAppContent (created by MediaQuery)
    in div (created by MediaQuery)
    in MediaQuery (created by EnterpriseApp)
    in div (created by EnterpriseApp)
    in EnterpriseAppContextProvider (created by EnterpriseApp)
    in EnterpriseApp (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(EnterpriseApp)) (created by AuthenticatedEnterpriseApp)
    in LoginRedirect (created by AuthenticatedEnterpriseApp)
    in AuthenticatedEnterpriseApp (created by Context.Consumer)
    in Route (created by Context.Consumer)
    in Switch (created by Context.Consumer)
    in Route (created by PageRoute)
    in PageRoute (created by AppWrapper)
    in Switch (created by AppWrapper)
    in Router (created by AppProvider)
    in Provider (created by OptionalReduxProvider)
    in OptionalReduxProvider (created by AppProvider)
    in ErrorBoundary (created by AppProvider)
    in IntlProvider (created by AppProvider)
    in AppProvider (created by AppWrapper)
    in AppWrapper
  1. I have resolved the first warning that you have mentioned. (reason: when we have e-commerce offer then offerId is in number form and when we have learner_credit offer then we have budgetId in string format)
  2. But I am not sure about the second warning whether it is related to my changes or not. By looking into it, it looks minor and we can ignore this one. I am saying this because I tested the code as thoroughly as I could and I didn't find any problem.
    what do you think?

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! :shipit: Thanks for the work on this :)

@jajjibhai008 jajjibhai008 force-pushed the eahmadjaved/lcm-revamp branch 2 times, most recently from b866982 to f53b2d9 Compare September 27, 2023 07:16
fix: full page refresh issue when clicking 'Budgets', added test coverage and fixed lint issues

feat: show all enterprise budgets regardless of plan and route correctly

fix: moved LCM routes to separate file

fix: resolve spacing and repetitions issue

fix: resolve spacing and repetitions issue

fix: improved test coverage

refactor: improve code efficiency and readability

fix: incorporate adam's suggestions and nits

fix: incorporated adam's feedback

fix: incorporate adam's feedback
@jajjibhai008 jajjibhai008 merged commit 5a6502c into eahmadjaved/ENT-7660 Sep 27, 2023
5 checks passed
@jajjibhai008 jajjibhai008 deleted the eahmadjaved/lcm-revamp branch September 27, 2023 07:49
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.

4 participants