From b3c1e5631a873a71e4343b1373acaa03343562da Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Thu, 17 Oct 2024 12:43:43 -0400 Subject: [PATCH] feat: take into account enrollments in dashboard -> search route redirects for new users (#1208) --- src/components/app/routes/data/utils.js | 54 ++++--- .../app/routes/loaders/rootLoader.ts | 10 +- .../routes/loaders/tests/rootLoader.test.jsx | 56 +------- .../dashboard/data/dashboardLoader.test.jsx | 134 ++++++++++++++++-- .../dashboard/data/dashboardLoader.ts | 24 +++- 5 files changed, 171 insertions(+), 107 deletions(-) diff --git a/src/components/app/routes/data/utils.js b/src/components/app/routes/data/utils.js index 1ffef3878f..0190454061 100644 --- a/src/components/app/routes/data/utils.js +++ b/src/components/app/routes/data/utils.js @@ -130,26 +130,7 @@ export async function ensureEnterpriseAppData({ queryClient.ensureQueryData(queryNotices()), ); } - const enterpriseAppData = await Promise.all(enterpriseAppDataQueries); - return enterpriseAppData; -} - -/** - * Determines whether the user is visiting the dashboard for the first time. - * @param {URL} requestUrl - The request URL. - * @returns {boolean} - Whether the user is visiting the dashboard for the first time. - */ -function isFirstDashboardPageVisit(requestUrl) { - // Check whether the request URL matches the dashboard page route. If not, return early. - const isDashboardRoute = matchPath('/:enterpriseSlug', requestUrl.pathname); - if (!isDashboardRoute) { - return false; - } - - // User is visiting the dashboard for the first time if the 'has-user-visited-learner-dashboard' - // localStorage item is not set. - const hasUserVisitedDashboard = localStorage.getItem('has-user-visited-learner-dashboard'); - return !hasUserVisitedDashboard; + await Promise.all(enterpriseAppDataQueries); } /** @@ -161,22 +142,37 @@ function isFirstDashboardPageVisit(requestUrl) { * * @param {Object} params - The parameters object. * @param {string} params.enterpriseSlug - The enterprise slug. - * @param {Object} params.enterpriseAppData - The enterprise app data. - * @param {URL} params.requestUrl - The request URL. + * @param {Array} params.enterpriseCourseEnrollments - The enterprise course enrollments. + * @param {Object} params.redeemablePolicies - The redeemable policies. */ -export function redirectToSearchPageForNewUser({ enterpriseSlug, enterpriseAppData, requestUrl }) { - const isFirstDashboardVisit = isFirstDashboardPageVisit(requestUrl); - if (!isFirstDashboardVisit) { +export function redirectToSearchPageForNewUser({ + enterpriseSlug, + enterpriseCourseEnrollments, + redeemablePolicies, +}) { + // If the user has already visited the dashboard, return early. + if (localStorage.getItem('has-user-visited-learner-dashboard')) { return; } - // Set the localStorage item to indicate that the user has visited the learner dashboard. + // Otherwise, set the localStorage item to indicate that the user has visited the dashboard. localStorage.setItem('has-user-visited-learner-dashboard', true); - // Check whether the user has any assignments for display. If not, redirect to the search page. - const redeemablePolicies = enterpriseAppData[1]; + // If the current URL does not match the dashboard, return early. This covers the use + // case where user may be on a non-dashboard route (e.g., search) and then explicitly + // navigates to the dashboard route. If the user is not already on the dashboard route, + // we do not want to trigger a redirect to the search page as the user explicitly requested + // to view the dashboard. + const isCurrentUrlMatchingDashboard = matchPath('/:enterpriseSlug', global.location.pathname); + if (!isCurrentUrlMatchingDashboard) { + return; + } + + // Check whether user has any assignments for display or active enterprise course + // enrollments. If not, redirect to the search page. const { hasAssignmentsForDisplay } = redeemablePolicies.learnerContentAssignments; - if (!hasAssignmentsForDisplay) { + const hasEnterpriseCourseEnrollments = enterpriseCourseEnrollments.length > 0; + if (!(hasAssignmentsForDisplay || hasEnterpriseCourseEnrollments)) { throw redirect(generatePath('/:enterpriseSlug/search', { enterpriseSlug })); } } diff --git a/src/components/app/routes/loaders/rootLoader.ts b/src/components/app/routes/loaders/rootLoader.ts index e6a9dd7803..8cf9a8e5d3 100644 --- a/src/components/app/routes/loaders/rootLoader.ts +++ b/src/components/app/routes/loaders/rootLoader.ts @@ -3,7 +3,6 @@ import { ensureAuthenticatedUser, ensureEnterpriseAppData, redirectToRemoveTrailingSlash, - redirectToSearchPageForNewUser, ensureActiveEnterpriseCustomerUser, } from '../data'; @@ -62,7 +61,7 @@ const makeRootLoader: Types.MakeRouteLoaderFunctionWithQueryClient = function ma } // Fetch all enterprise app data. - const enterpriseAppData = await ensureEnterpriseAppData({ + await ensureEnterpriseAppData({ enterpriseCustomer, allLinkedEnterpriseCustomerUsers, userId, @@ -71,13 +70,6 @@ const makeRootLoader: Types.MakeRouteLoaderFunctionWithQueryClient = function ma requestUrl, }); - // Redirect user to search page, for first-time users with no assignments. - redirectToSearchPageForNewUser({ - enterpriseSlug: enterpriseSlug as string, - enterpriseAppData, - requestUrl, - }); - // Redirect to the same URL without a trailing slash, if applicable. redirectToRemoveTrailingSlash(requestUrl); diff --git a/src/components/app/routes/loaders/tests/rootLoader.test.jsx b/src/components/app/routes/loaders/tests/rootLoader.test.jsx index a66c47ca67..16d9817658 100644 --- a/src/components/app/routes/loaders/tests/rootLoader.test.jsx +++ b/src/components/app/routes/loaders/tests/rootLoader.test.jsx @@ -1,7 +1,5 @@ -import { useEffect } from 'react'; import { screen, waitFor } from '@testing-library/react'; import { when } from 'jest-when'; -import { Outlet, useLocation } from 'react-router-dom'; import '@testing-library/jest-dom/extend-expect'; import { renderWithRouterProvider } from '../../../../../utils/tests'; @@ -40,19 +38,9 @@ const mockQueryClient = { ensureQueryData: jest.fn().mockResolvedValue(), }; -let locationPathname; -const ComponentWithLocation = ({ children }) => { - const { pathname } = useLocation(); - useEffect(() => { - locationPathname = pathname; - }, [pathname]); - return children || null; -}; - describe('rootLoader', () => { beforeEach(() => { jest.clearAllMocks(); - localStorage.clear(); ensureAuthenticatedUser.mockResolvedValue(mockAuthenticatedUser); extractEnterpriseCustomer.mockResolvedValue(mockEnterpriseCustomer); }); @@ -108,7 +96,6 @@ describe('rootLoader', () => { { enterpriseCustomer: mockEnterpriseCustomerTwo }, ], isStaffUser: false, - shouldRedirectToSearch: false, }, { enterpriseSlug: mockEnterpriseCustomerTwo.slug, @@ -119,7 +106,6 @@ describe('rootLoader', () => { { enterpriseCustomer: mockEnterpriseCustomerTwo }, ], isStaffUser: false, - shouldRedirectToSearch: true, }, { enterpriseSlug: mockEnterpriseCustomerTwo.slug, @@ -129,7 +115,6 @@ describe('rootLoader', () => { { enterpriseCustomer: mockEnterpriseCustomer }, ], isStaffUser: false, - shouldRedirectToSearch: false, }, { enterpriseSlug: mockEnterpriseCustomerTwo.slug, @@ -139,7 +124,6 @@ describe('rootLoader', () => { { enterpriseCustomer: mockEnterpriseCustomer }, ], isStaffUser: false, - shouldRedirectToSearch: true, }, { enterpriseSlug: mockEnterpriseCustomerTwo.slug, @@ -149,7 +133,6 @@ describe('rootLoader', () => { { enterpriseCustomer: mockEnterpriseCustomer }, ], isStaffUser: true, - shouldRedirectToSearch: false, }, { enterpriseSlug: mockEnterpriseCustomerTwo.slug, @@ -159,7 +142,6 @@ describe('rootLoader', () => { { enterpriseCustomer: mockEnterpriseCustomer }, ], isStaffUser: true, - shouldRedirectToSearch: true, }, ])('ensures all requisite root loader queries are resolved with an active enterprise customer user (%s)', async ({ isStaffUser, @@ -167,7 +149,6 @@ describe('rootLoader', () => { enterpriseCustomer, activeEnterpriseCustomer, allLinkedEnterpriseCustomerUsers, - shouldRedirectToSearch, }) => { const enterpriseLearnerQuery = queryEnterpriseLearner(mockAuthenticatedUser.username, enterpriseSlug); const enterpriseLearnerQueryTwo = queryEnterpriseLearner(mockAuthenticatedUser.username, enterpriseCustomer.slug); @@ -197,13 +178,10 @@ describe('rootLoader', () => { const mockRedeemablePolicies = { redeemablePolicies: [], learnerContentAssignments: { - hasAssignmentsForDisplay: !shouldRedirectToSearch, + hasAssignmentsForDisplay: false, }, }; - const redeemablePoliciesQuery = queryRedeemablePolicies({ - enterpriseUuid: enterpriseCustomer.uuid, - lmsUserId: 3, - }); + const redeemablePoliciesQuery = queryRedeemablePolicies({ enterpriseUuid: enterpriseCustomer.uuid, lmsUserId: 3 }); when(mockQueryClient.ensureQueryData).calledWith( expect.objectContaining({ queryKey: redeemablePoliciesQuery.queryKey, @@ -223,15 +201,9 @@ describe('rootLoader', () => { ).mockResolvedValue(mockSubscriptionsData); renderWithRouterProvider({ - path: '/:enterpriseSlug/*', - element: , + path: '/:enterpriseSlug', + element:
, loader: makeRootLoader(mockQueryClient), - children: [ - { - path: 'search', - element: , - }, - ], }, { initialEntries: [`/${enterpriseSlug}`], }); @@ -246,31 +218,11 @@ describe('rootLoader', () => { } else { expect(mockQueryClient.ensureQueryData).toHaveBeenCalledTimes(2); } - } else if (shouldRedirectToSearch) { - // queries are executed again when redirecting to search - expect(mockQueryClient.ensureQueryData).toHaveBeenCalledTimes(18); } else { expect(mockQueryClient.ensureQueryData).toHaveBeenCalledTimes(9); } }); - function getExpectedSlugPath() { - if (enterpriseSlug === activeEnterpriseCustomer?.slug) { - return enterpriseSlug; - } - if (isLinked || isStaffUser) { - return enterpriseCustomer.slug; - } - return activeEnterpriseCustomer.slug; - } - const expectedCustomerPath = getExpectedSlugPath(); - // Assert that the expected number of queries were made. - if (shouldRedirectToSearch) { - expect(locationPathname).toEqual(`/${expectedCustomerPath}/search`); - } else { - expect(locationPathname).toEqual(`/${expectedCustomerPath}`); - } - // Enterprise learner query expect(mockQueryClient.ensureQueryData).toHaveBeenCalledWith( expect.objectContaining({ diff --git a/src/components/dashboard/data/dashboardLoader.test.jsx b/src/components/dashboard/data/dashboardLoader.test.jsx index e13bdbf035..007514da81 100644 --- a/src/components/dashboard/data/dashboardLoader.test.jsx +++ b/src/components/dashboard/data/dashboardLoader.test.jsx @@ -1,4 +1,5 @@ import { screen } from '@testing-library/react'; +import { when } from 'jest-when'; import '@testing-library/jest-dom/extend-expect'; import { renderWithRouterProvider } from '../../../utils/tests'; @@ -8,6 +9,7 @@ import { queryEnterpriseCourseEnrollments, queryEnterprisePathwaysList, queryEnterpriseProgramsList, + queryRedeemablePolicies, } from '../../app/data'; import { ensureAuthenticatedUser } from '../../app/routes/data'; @@ -21,7 +23,7 @@ jest.mock('../../app/data', () => ({ })); const mockEnterpriseId = 'test-enterprise-uuid'; -extractEnterpriseCustomer.mockResolvedValue({ uuid: mockEnterpriseId }); +const mockEnterpriseSlug = 'test-enterprise-slug'; const mockQueryClient = { ensureQueryData: jest.fn().mockResolvedValue({}), @@ -30,40 +32,136 @@ const mockQueryClient = { describe('dashboardLoader', () => { beforeEach(() => { jest.clearAllMocks(); + localStorage.clear(); ensureAuthenticatedUser.mockResolvedValue({ userId: 3 }); + extractEnterpriseCustomer.mockResolvedValue({ uuid: mockEnterpriseId, slug: mockEnterpriseSlug }); }); it('does nothing with unauthenticated users', async () => { ensureAuthenticatedUser.mockResolvedValue(null); renderWithRouterProvider({ path: '/:enterpriseSlug', - element:
hello world
, + element:
, loader: makeDashboardLoader(mockQueryClient), }, [ { - initialEntries: ['/test-enterprise-slug'], + initialEntries: [`/${mockEnterpriseSlug}`], }, ]); - expect(await screen.findByText('hello world')).toBeInTheDocument(); - + expect(await screen.findByTestId('dashboard')).toBeInTheDocument(); expect(mockQueryClient.ensureQueryData).not.toHaveBeenCalled(); }); - it('ensures the requisite dashboard data is resolved', async () => { - renderWithRouterProvider({ - path: '/:enterpriseSlug', - element:
hello world
, - loader: makeDashboardLoader(mockQueryClient), - }, [ + it.each([ + { + hasAssignmentsForDisplay: false, + hasEnterpriseCourseEnrollments: false, + currentPageRoute: `/${mockEnterpriseSlug}`, + hasVisitedDashboardBefore: false, + shouldRedirectToSearch: true, + }, + { + hasAssignmentsForDisplay: false, + hasEnterpriseCourseEnrollments: false, + currentPageRoute: `/${mockEnterpriseSlug}`, + hasVisitedDashboardBefore: true, + shouldRedirectToSearch: false, + }, + { + hasAssignmentsForDisplay: false, + hasEnterpriseCourseEnrollments: false, + currentPageRoute: `/${mockEnterpriseSlug}/search`, + hasVisitedDashboardBefore: false, + shouldRedirectToSearch: false, + }, + { + hasAssignmentsForDisplay: true, + hasEnterpriseCourseEnrollments: false, + currentPageRoute: `/${mockEnterpriseSlug}`, + hasVisitedDashboardBefore: false, + shouldRedirectToSearch: false, + }, + { + hasAssignmentsForDisplay: false, + hasEnterpriseCourseEnrollments: true, + currentPageRoute: `/${mockEnterpriseSlug}`, + hasVisitedDashboardBefore: false, + shouldRedirectToSearch: false, + }, + { + hasAssignmentsForDisplay: true, + hasEnterpriseCourseEnrollments: true, + currentPageRoute: `/${mockEnterpriseSlug}`, + hasVisitedDashboardBefore: false, + shouldRedirectToSearch: false, + }, + ])('ensures the requisite dashboard data is resolved (%s)', async ({ + hasAssignmentsForDisplay, + hasEnterpriseCourseEnrollments, + currentPageRoute, + hasVisitedDashboardBefore, + shouldRedirectToSearch, + }) => { + // Mock global.location.pathname + const mockLocation = { + pathname: currentPageRoute, + }; + jest.spyOn(global, 'location', 'get').mockReturnValue(mockLocation); + + // Mock localStorage, if necessary + if (hasVisitedDashboardBefore) { + localStorage.setItem('has-user-visited-learner-dashboard', true); + } + + // Mock redeemable policies query + const mockRedeemablePolicies = { + learnerContentAssignments: { + hasAssignmentsForDisplay, + }, + }; + const redeemablePoliciesQuery = queryRedeemablePolicies({ enterpriseUuid: mockEnterpriseId, lmsUserId: 3 }); + when(mockQueryClient.ensureQueryData).calledWith( + expect.objectContaining({ + queryKey: redeemablePoliciesQuery.queryKey, + }), + ).mockResolvedValue(mockRedeemablePolicies); + + // Mock enterprise course enrollments query + const mockEnterpriseCourseEnrollments = []; + if (hasEnterpriseCourseEnrollments) { + mockEnterpriseCourseEnrollments.push({ courseId: 'test-course-id' }); + } + when(mockQueryClient.ensureQueryData).calledWith( + expect.objectContaining({ + queryKey: queryEnterpriseCourseEnrollments(mockEnterpriseId).queryKey, + }), + ).mockResolvedValue(mockEnterpriseCourseEnrollments); + + renderWithRouterProvider( { - initialEntries: ['/test-enterprise-slug'], + path: '/:enterpriseSlug', + element:
, + loader: makeDashboardLoader(mockQueryClient), }, - ]); + { + initialEntries: [`/${mockEnterpriseSlug}`], + routes: [ + { + path: '/:enterpriseSlug/search', + element:
, + }, + ], + }, + ); - expect(await screen.findByText('hello world')).toBeInTheDocument(); + if (shouldRedirectToSearch) { + expect(await screen.findByTestId('search')).toBeInTheDocument(); + } else { + expect(await screen.findByTestId('dashboard')).toBeInTheDocument(); + } - expect(mockQueryClient.ensureQueryData).toHaveBeenCalledTimes(3); + expect(mockQueryClient.ensureQueryData).toHaveBeenCalledTimes(4); expect(mockQueryClient.ensureQueryData).toHaveBeenCalledWith( expect.objectContaining({ queryKey: queryEnterpriseCourseEnrollments(mockEnterpriseId).queryKey, @@ -76,6 +174,12 @@ describe('dashboardLoader', () => { queryFn: expect.any(Function), }), ); + expect(mockQueryClient.ensureQueryData).toHaveBeenCalledWith( + expect.objectContaining({ + queryKey: queryRedeemablePolicies({ enterpriseUuid: mockEnterpriseId, lmsUserId: 3 }).queryKey, + queryFn: expect.any(Function), + }), + ); expect(mockQueryClient.ensureQueryData).toHaveBeenCalledWith( expect.objectContaining({ queryKey: queryEnterprisePathwaysList(mockEnterpriseId).queryKey, diff --git a/src/components/dashboard/data/dashboardLoader.ts b/src/components/dashboard/data/dashboardLoader.ts index 1e9d981ad2..98d0d5dd74 100644 --- a/src/components/dashboard/data/dashboardLoader.ts +++ b/src/components/dashboard/data/dashboardLoader.ts @@ -1,9 +1,10 @@ -import { ensureAuthenticatedUser } from '../../app/routes/data'; +import { ensureAuthenticatedUser, redirectToSearchPageForNewUser } from '../../app/routes/data'; import { extractEnterpriseCustomer, queryEnterpriseCourseEnrollments, queryEnterprisePathwaysList, queryEnterpriseProgramsList, + queryRedeemablePolicies, } from '../../app/data'; type DashboardRouteParams = Types.RouteParams & { @@ -31,8 +32,27 @@ const makeDashboardLoader: Types.MakeRouteLoaderFunctionWithQueryClient = functi authenticatedUser, enterpriseSlug, }); - await Promise.all([ + + const loadEnrollmentsPoliciesAndRedirectForNewUsers = Promise.all([ queryClient.ensureQueryData(queryEnterpriseCourseEnrollments(enterpriseCustomer.uuid)), + queryClient.ensureQueryData(queryRedeemablePolicies({ + enterpriseUuid: enterpriseCustomer.uuid, + lmsUserId: authenticatedUser.userId, + })), + ]).then((responses) => { + const enterpriseCourseEnrollments = responses[0]; + const redeemablePolicies = responses[1]; + + // Redirect user to search page, for first-time users with no enrollments and/or assignments. + redirectToSearchPageForNewUser({ + enterpriseSlug: enterpriseSlug as string, + enterpriseCourseEnrollments, + redeemablePolicies, + }); + }); + + await Promise.all([ + loadEnrollmentsPoliciesAndRedirectForNewUsers, queryClient.ensureQueryData(queryEnterpriseProgramsList(enterpriseCustomer.uuid)), queryClient.ensureQueryData(queryEnterprisePathwaysList(enterpriseCustomer.uuid)), ]);