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: Integrate BFF calls with loader and hooks #1227

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

brobro10000
Copy link
Member

@brobro10000 brobro10000 commented Nov 25, 2024

Integrates the BFF API in dashboard related API calls related to enterpriseCourseEnrollments and subscriptions.
Utilize the feature flag from the MFE config for BFF API eligibility based on the enterpriseCustomer.uuid. Currently, only a single stage customer is enabled.
Determines if the user is on the dashboard route /:enterpriseSlug before making a to the BFF loader.
Determines if the user is feature flagged to use the BFF API as a source of truth for enterpriseCourseEnrollments and subscriptions.
If the user is not eligible to use the BFF API, fallback to the existing custom query hooks passed into useBFF.
Updates the dashboardLoader to conditionally use either the BFF API or the existing enterpriseCourseEnrollments query.
Updates the root loader to determine if the user is on the dashboard route and conditionally use either the BFF API or the existing subscriptionsQuery

Pending cleanup and test updates.

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-9636-2 branch 3 times, most recently from 3ba3978 to b720344 Compare November 25, 2024 20:34
src/components/app/data/hooks/useBFF.js Outdated Show resolved Hide resolved
src/components/app/data/utils.js Outdated Show resolved Hide resolved
src/components/app/data/queries/utils.js Outdated Show resolved Hide resolved
src/components/app/data/queries/utils.js Outdated Show resolved Hide resolved
src/components/app/routes/data/utils.js Outdated Show resolved Hide resolved
src/components/dashboard/DashboardPage.jsx Outdated Show resolved Hide resolved
src/components/dashboard/data/dashboardLoader.ts Outdated Show resolved Hide resolved
src/components/dashboard/data/dashboardLoader.ts Outdated Show resolved Hide resolved
src/index.jsx Outdated Show resolved Hide resolved
src/components/app/data/hooks/useBFF.js Outdated Show resolved Hide resolved
src/components/app/routes/data/utils.js Outdated Show resolved Hide resolved
src/components/app/routes/loaders/rootLoader.ts Outdated Show resolved Hide resolved
src/components/dashboard/data/dashboardLoader.ts Outdated Show resolved Hide resolved
src/types.d.ts Show resolved Hide resolved
src/components/app/data/hooks/useSubscriptions.js Outdated Show resolved Hide resolved
Comment on lines -181 to -182
queryClient,
subscriptionsQuery,
Copy link
Member

Choose a reason for hiding this comment

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

nit: looks like these args need to be removed from the parent function activateOrAutoApplySubscriptionLicense, too, if no longer used (same where this parent function is called).

return subscriptionsData;
return transformSubscriptionsData({
customerAgreement,
subscriptionLicensesByStatus: baseLicensesByStatus,
Copy link
Member

Choose a reason for hiding this comment

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

Given this function doesn't really utilize subscriptionLicensesByStatus here, i.e. when not relying on the BFF data, I believe it could be omitted? It'll already fallback to baseLicensesByStatus based on the logic within the transform function, no?

src/types.d.ts Outdated Show resolved Hide resolved
* @returns {Function|null} The BFF query function to use for the current route, or null if no match is found.
*/
export function resolveBFFQuery(pathname, options = {}) {
// Define route patterns and their corresponding query functions
Copy link
Member

Choose a reason for hiding this comment

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

nit: this comment technically refers to routeToBFFQueryMap below; should it be co-located?

course_run_url: string;
resume_course_run_url?: string;
is_revoked: boolean;
due_dates: DueDate[]
Copy link
Member

Choose a reason for hiding this comment

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

nit: semi-colon at the end for parity with other fields.

const { authenticatedUser } = useContext(AppContext);
const userFirstName = authenticatedUser?.name?.split(' ').shift();

const [shouldShowLicenseActivationSuccessMessage, , close] = useToggle(!!sessionStorage.getItem('shouldShowActivationSuccessMessage'));
Copy link
Member

Choose a reason for hiding this comment

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

Name mismatch in the session storage key, shouldShowLicenseActivationSuccessMessage is used now. Optionally consider creating a constant to re-use if it makes it less prone to mismatches?

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.

2 participants