From 45ee032e6a97456297af8b51ef6f093c3cc82e0a Mon Sep 17 00:00:00 2001 From: "Adolfo R. Brandes" Date: Tue, 22 Sep 2026 17:53:46 -0300 Subject: [PATCH] feat: resolve routes by role and link to them in the client Closes #317 and #319. Co-Authored-By: Claude --- docs/how_tos/migrate-frontend-app.md | 12 ++- runtime/index.ts | 6 +- runtime/routing/utils.test.ts | 82 ++++++++++++++++- runtime/routing/utils.ts | 75 +++++++++++++++- shell/Logo.test.tsx | 61 ++++++++++--- shell/Logo.tsx | 7 +- shell/header/anonymous-menu/LoginButton.tsx | 3 +- .../header/anonymous-menu/RegisterButton.tsx | 3 +- shell/header/anonymous-menu/utils.ts | 19 ---- shell/menus/LinkMenuItem.test.tsx | 87 +++++++++++++++++++ shell/menus/LinkMenuItem.tsx | 16 ++-- 11 files changed, 319 insertions(+), 52 deletions(-) delete mode 100644 shell/header/anonymous-menu/utils.ts create mode 100644 shell/menus/LinkMenuItem.test.tsx diff --git a/docs/how_tos/migrate-frontend-app.md b/docs/how_tos/migrate-frontend-app.md index dbbe109d..8fc6d5ef 100644 --- a/docs/how_tos/migrate-frontend-app.md +++ b/docs/how_tos/migrate-frontend-app.md @@ -738,7 +738,7 @@ Other configuration is now optional, and many values have been given sensible de URL Config changes ------------------ -Note that the .env files and env.config.js files also include a number of URLs for various micro-frontends and services. These URLs should now be expressed as part of the `apps` config as route roles, and used in code via `getUrlForRouteRole()`. Or as externalRoutes. +Note that the .env files and env.config.js files also include a number of URLs for various micro-frontends and services. These URLs should now be expressed as part of the `apps` config as route roles, or as `externalRoutes`, and resolved in code via `resolveRouteByRole()`. It substitutes route params, drops a trailing splat, and reports whether the result is a path in this site or an external URL, so a link can stay in the client when it is. `getLinkProps()` turns any URL into the props for a component that accepts `as`, a react-router `Link` for a path in this site and a plain anchor otherwise. ```js // Creating a route role with for 'example' in an App @@ -754,8 +754,14 @@ const app: App = { }], }; -// Using the role in code to link to the page -const examplePageUrl = getUrlForRouteRole('example'); +// Using the role in code to link to the page: { url: '/example', isInternal: true }, or null +// when no app or external route provides it +const examplePage = resolveRouteByRole('example'); + +// Rendering the link, so that a route in this site navigates without a page load +{examplePage && ( + +)} ``` App-specific config values diff --git a/runtime/index.ts b/runtime/index.ts index c0ea2574..018a2de0 100644 --- a/runtime/index.ts +++ b/runtime/index.ts @@ -127,9 +127,13 @@ export { export { authenticatedLoader, + getLinkProps, getUrlByRouteRole, - isRoleRouteObject + isInternalUrl, + isRoleRouteObject, + resolveRouteByRole } from './routing'; +export type { LinkProps, ResolvedRoute } from './routing/utils'; export { clearAllSubscriptions, diff --git a/runtime/routing/utils.test.ts b/runtime/routing/utils.test.ts index 55e3b3bb..83511f52 100644 --- a/runtime/routing/utils.test.ts +++ b/runtime/routing/utils.test.ts @@ -1,4 +1,6 @@ -import { getUrlByRouteRole } from './utils'; +import { Link } from 'react-router-dom'; + +import { getLinkProps, getUrlByRouteRole, isInternalUrl, resolveRouteByRole } from './utils'; import { getSiteConfig } from '../config'; jest.mock('../config'); @@ -60,3 +62,81 @@ describe('getUrlByRouteRole', () => { expect(getUrlByRouteRole('nonexistent')).toBeNull(); }); }); + +describe('isInternalUrl', () => { + it('accepts a path in this site', () => { + expect(isInternalUrl('/account')).toBe(true); + expect(isInternalUrl('/')).toBe(true); + }); + + it('rejects absolute and protocol-relative URLs', () => { + expect(isInternalUrl('https://example.com/account')).toBe(false); + expect(isInternalUrl('//example.com/account')).toBe(false); + expect(isInternalUrl('mailto:help@example.com')).toBe(false); + }); +}); + +describe('resolveRouteByRole', () => { + beforeEach(() => { + mockGetSiteConfig.mockReturnValue({ + apps: [{ + appId: 'test-app', + routes: [ + { path: '/account', handle: { roles: ['account'] } }, + { path: '/gradebook/:courseId', handle: { roles: ['gradebook'] } }, + { path: '/learning/:courseId/:unitId?', handle: { roles: ['learning'] } }, + { path: '/learner-dashboard/*', handle: { roles: ['dashboard'] } }, + { path: '/*', handle: { roles: ['catch-all'] } }, + ], + }], + externalRoutes: [{ + role: 'profile', + url: 'https://apps.example.com/profile/', + }], + } as any); + }); + + it('resolves an app route to a path in this site', () => { + expect(resolveRouteByRole('account')).toEqual({ url: '/account', isInternal: true }); + }); + + it('substitutes route params, optional ones included', () => { + expect(resolveRouteByRole('gradebook', { courseId: 'course-v1:edX+DemoX+Demo_Course' })) + .toEqual({ url: '/gradebook/course-v1:edX+DemoX+Demo_Course', isInternal: true }); + expect(resolveRouteByRole('learning', { courseId: 'c1', unitId: 'u1' })) + .toEqual({ url: '/learning/c1/u1', isInternal: true }); + }); + + it('drops an optional param that is not given', () => { + expect(resolveRouteByRole('learning', { courseId: 'c1' })).toEqual({ url: '/learning/c1', isInternal: true }); + }); + + it('throws when a required param is not given', () => { + expect(() => resolveRouteByRole('gradebook')).toThrow('Missing ":courseId" param'); + }); + + it('drops a trailing splat', () => { + expect(resolveRouteByRole('dashboard')).toEqual({ url: '/learner-dashboard', isInternal: true }); + expect(resolveRouteByRole('catch-all')).toEqual({ url: '/', isInternal: true }); + }); + + it('resolves an external route to its URL as configured', () => { + expect(resolveRouteByRole('profile', { courseId: 'c1' })) + .toEqual({ url: 'https://apps.example.com/profile/', isInternal: false }); + }); + + it('returns null when nothing provides the role', () => { + expect(resolveRouteByRole('nonexistent')).toBeNull(); + }); +}); + +describe('getLinkProps', () => { + it('links a path in this site through react-router', () => { + expect(getLinkProps('/account')).toEqual({ as: Link, to: '/account' }); + }); + + it('links anything else through a plain anchor', () => { + expect(getLinkProps('https://example.com/account')).toEqual({ href: 'https://example.com/account' }); + expect(getLinkProps('//example.com/account')).toEqual({ href: '//example.com/account' }); + }); +}); diff --git a/runtime/routing/utils.ts b/runtime/routing/utils.ts index f34c6f4e..ca7c5382 100644 --- a/runtime/routing/utils.ts +++ b/runtime/routing/utils.ts @@ -1,4 +1,6 @@ -import { RouteObject } from 'react-router'; +import { ElementType } from 'react'; +import { generatePath, RouteObject } from 'react-router'; +import { Link } from 'react-router-dom'; import { RoleRouteObject } from '../../types'; import { getSiteConfig } from '../config'; @@ -50,3 +52,74 @@ export function getUrlByRouteRole(role: string) { export function isRoleRouteObject(match: RouteObject): match is RoleRouteObject { return match.handle !== undefined && 'roles' in match.handle; } + +/** + * Whether `url` is a path within this site, one react-router can navigate to, rather than an + * absolute or protocol-relative URL. + * + * @param {string} url + * @returns {boolean} + */ +export function isInternalUrl(url: string): boolean { + return url.startsWith('/') && !url.startsWith('//'); +} + +export interface ResolvedRoute { + /** The path or URL to link to. */ + url: string; + /** Whether `url` is a path in this site, so a react-router `Link` can navigate to it. */ + isInternal: boolean; +} + +/** + * Resolves the route the site provides for `role`, if any, ready to link to: the path of an + * installed app's route, or the URL of an external route, as `getUrlByRouteRole` finds it. An app + * route's path has its params filled from `params` and a trailing splat dropped, as react-router's + * `generatePath` does, so a missing required param throws; an external URL is returned as + * configured. `isInternal` tells the two apart. + * + * ``` + * const gradebook = resolveRouteByRole('org.openedx.frontend.role.gradebook', { courseId }); + * if (gradebook?.isInternal) { + * return ; + * } + * return ; + * ``` + * + * @param {string} role + * @param {Object.} [params] Values for the route's params, by name. + * @returns {ResolvedRoute|null} `null` when no app or external route provides the role. + */ +export function resolveRouteByRole(role: string, params: Record = {}): ResolvedRoute | null { + const path = getUrlByRouteRole(role); + if (path === null) { + return null; + } + const isInternal = isInternalUrl(path); + return { url: isInternal ? generatePath(path, params) : path, isInternal }; +} + +export interface LinkProps { + as?: ElementType; + to?: string; + href?: string; +} + +/** + * Props for linking to `url` from a component that accepts `as`, such as Paragon's `Button`, + * `Hyperlink`, `NavLink` or `Dropdown.Item`: a react-router `Link` for a path in this site, so the + * navigation stays in the client, and a plain anchor `href` for anything else. + * + * ``` + * + * ``` + * + * @param {string} url + * @returns {LinkProps} + */ +export function getLinkProps(url: string): LinkProps { + if (isInternalUrl(url)) { + return { as: Link, to: url }; + } + return { href: url }; +} diff --git a/shell/Logo.test.tsx b/shell/Logo.test.tsx index f3f86c52..c921bb23 100644 --- a/shell/Logo.test.tsx +++ b/shell/Logo.test.tsx @@ -1,14 +1,31 @@ import '@testing-library/jest-dom'; -import { render } from '@testing-library/react'; +import { ReactElement } from 'react'; +import { fireEvent, render, screen } from '@testing-library/react'; +import { MemoryRouter, useLocation } from 'react-router-dom'; import { getSiteConfig, mergeSiteConfig, setSiteConfig } from '../runtime/config'; import { SiteConfig } from '../types'; +import { homeRole } from './constants'; import Logo from './Logo'; +function LocationDisplay() { + const { pathname } = useLocation(); + return
{pathname}
; +} + +function renderWithRouter(ui: ReactElement, initialEntry = '/elsewhere') { + return render( + + {ui} + + + ); +} + describe('Logo component', () => { let originalConfig: SiteConfig; beforeEach(() => { - // Shallow clone is sufficient here since we only modify headerLogoImageUrl, a top-level field. + // Shallow clone is sufficient here since we only modify top-level fields. originalConfig = { ...getSiteConfig() }; }); @@ -17,39 +34,55 @@ describe('Logo component', () => { }); it('renders the image with default URL and links to / when no props are provided', async () => { - const { getByRole } = render(); - const image = getByRole('img'); + renderWithRouter(); + const image = screen.getByRole('img'); expect(image).toHaveAttribute('src', 'https://edx-cdn.org/v3/default/logo.svg'); - const link = getByRole('link'); + const link = screen.getByRole('link'); expect(link).toHaveAttribute('href', '/'); }); it('renders the image with provided imageUrl and links to / by default', async () => { const testUrl = 'https://example.com/test-logo.svg'; - const { getByRole } = render(); - const image = getByRole('img'); + renderWithRouter(); + const image = screen.getByRole('img'); expect(image).toHaveAttribute('src', testUrl); - const link = getByRole('link'); + const link = screen.getByRole('link'); expect(link).toHaveAttribute('href', '/'); }); it('renders the image with headerLogoImageUrl when set in site config', async () => { const configLogoUrl = 'https://example.com/config-logo.svg'; mergeSiteConfig({ headerLogoImageUrl: configLogoUrl }); - const { getByRole } = render(); - const image = getByRole('img'); + renderWithRouter(); + const image = screen.getByRole('img'); expect(image).toHaveAttribute('src', configLogoUrl); - const link = getByRole('link'); + const link = screen.getByRole('link'); expect(link).toHaveAttribute('href', '/'); }); it('renders the image wrapped in a Hyperlink when destinationUrl is provided', async () => { const testDestinationUrl = 'https://example.com'; - const { getByRole } = render(); - const link = getByRole('link'); + renderWithRouter(); + const link = screen.getByRole('link'); expect(link).toHaveAttribute('href', testDestinationUrl); - const image = getByRole('img'); + const image = screen.getByRole('img'); expect(image).toBeInTheDocument(); expect(image).toHaveAttribute('src', 'https://edx-cdn.org/v3/default/logo.svg'); }); + + it('navigates to the home route in the client when an app provides one', async () => { + mergeSiteConfig({ + apps: [{ + appId: 'org.openedx.frontend.app.logoTest', + routes: [{ path: '/learner-dashboard/*', handle: { roles: [homeRole] } }], + }], + }); + renderWithRouter(); + + const link = screen.getByRole('link'); + expect(link).toHaveAttribute('href', '/learner-dashboard'); + + fireEvent.click(link); + expect(screen.getByTestId('location')).toHaveTextContent('/learner-dashboard'); + }); }); diff --git a/shell/Logo.tsx b/shell/Logo.tsx index 905e8c35..5ddc4df5 100644 --- a/shell/Logo.tsx +++ b/shell/Logo.tsx @@ -1,7 +1,7 @@ import { IntlProvider } from 'react-intl'; import { Hyperlink, Image } from '@openedx/paragon'; import { getSiteConfig } from '../runtime/config'; -import { getUrlByRouteRole } from '../runtime/routing'; +import { getLinkProps, resolveRouteByRole } from '../runtime/routing'; import { homeRole } from './constants'; interface LogoProps { @@ -11,7 +11,7 @@ interface LogoProps { export default function Logo({ imageUrl = getSiteConfig().headerLogoImageUrl ?? 'https://edx-cdn.org/v3/default/logo.svg', - destinationUrl = getUrlByRouteRole(homeRole) || '/' + destinationUrl = resolveRouteByRole(homeRole)?.url ?? '/' }: LogoProps) { const image = ( @@ -21,9 +21,10 @@ export default function Logo({ return image; } + // A path in this site is a react-router Link, so the navigation stays in the client. return ( - + {image} diff --git a/shell/header/anonymous-menu/LoginButton.tsx b/shell/header/anonymous-menu/LoginButton.tsx index 0a2b1508..20f78148 100644 --- a/shell/header/anonymous-menu/LoginButton.tsx +++ b/shell/header/anonymous-menu/LoginButton.tsx @@ -1,8 +1,7 @@ import { Button } from '@openedx/paragon'; -import { getUrlByRouteRole, useSiteConfig, useIntl } from '../../../runtime'; +import { getLinkProps, getUrlByRouteRole, useSiteConfig, useIntl } from '../../../runtime'; import { loginRole } from '../../constants'; import messages from '../../Shell.messages'; -import { getLinkProps } from './utils'; export default function LoginButton({ ...props }) { const config = useSiteConfig(); diff --git a/shell/header/anonymous-menu/RegisterButton.tsx b/shell/header/anonymous-menu/RegisterButton.tsx index 7d1aad17..f709c674 100644 --- a/shell/header/anonymous-menu/RegisterButton.tsx +++ b/shell/header/anonymous-menu/RegisterButton.tsx @@ -1,9 +1,8 @@ import { Button } from '@openedx/paragon'; -import { getUrlByRouteRole, useSiteConfig, useIntl } from '../../../runtime'; +import { getLinkProps, getUrlByRouteRole, useSiteConfig, useIntl } from '../../../runtime'; import { registerRole } from '../../constants'; import messages from '../../Shell.messages'; -import { getLinkProps } from './utils'; export default function RegisterButton({ ...props }) { const config = useSiteConfig(); diff --git a/shell/header/anonymous-menu/utils.ts b/shell/header/anonymous-menu/utils.ts deleted file mode 100644 index 202a0b9e..00000000 --- a/shell/header/anonymous-menu/utils.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { ElementType } from 'react'; -import { Link } from 'react-router-dom'; - -interface LinkProps { - as?: ElementType; - to?: string; - href?: string; -} - -/** - * Builds the props needed to link to a URL, keeping navigation inside the - * client when the URL is a route in this site rather than an external one. - */ -export function getLinkProps(url: string): LinkProps { - if (url.startsWith('/')) { - return { as: Link, to: url }; - } - return { href: url }; -} diff --git a/shell/menus/LinkMenuItem.test.tsx b/shell/menus/LinkMenuItem.test.tsx new file mode 100644 index 00000000..56c023bd --- /dev/null +++ b/shell/menus/LinkMenuItem.test.tsx @@ -0,0 +1,87 @@ +import '@testing-library/jest-dom'; +import { ComponentProps } from 'react'; +import { fireEvent, render, screen } from '@testing-library/react'; +import { MemoryRouter, useLocation } from 'react-router-dom'; + +import { mergeSiteConfig } from '../../runtime'; +import { IntlProvider } from '../../runtime/i18n'; +import LinkMenuItem from './LinkMenuItem'; + +const accountRole = 'org.openedx.frontend.role.linkMenuItemTest.account'; +const profileRole = 'org.openedx.frontend.role.linkMenuItemTest.profile'; +const externalUrl = 'https://apps.example.com/profile/'; + +mergeSiteConfig({ + apps: [{ + appId: 'org.openedx.frontend.app.linkMenuItemTest', + routes: [{ path: '/account/*', handle: { roles: [accountRole] } }], + }], + externalRoutes: [{ role: profileRole, url: externalUrl }], +}); + +function LocationDisplay() { + const { pathname } = useLocation(); + return
{pathname}
; +} + +function renderItem(props: Partial>, initialEntry = '/home') { + return render( + + + + + + + ); +} + +// jsdom cannot navigate, so keep a plain anchor's click from reaching it. +function clickWithoutNavigating(link: HTMLElement) { + link.addEventListener('click', (event) => event.preventDefault()); + fireEvent.click(link); +} + +const variants = ['hyperlink', 'navLink', 'navDropdownItem', 'dropdownItem'] as const; + +describe('LinkMenuItem', () => { + describe.each(variants)('as %s', (variant) => { + it('navigates to a route an app provides without leaving the client', () => { + renderItem({ role: accountRole, variant }); + + const link = screen.getByRole('link', { name: 'Account' }); + expect(link).toHaveAttribute('href', '/account'); + + fireEvent.click(link); + expect(screen.getByTestId('location')).toHaveTextContent('/account'); + }); + + it('links to an external route with a plain anchor', () => { + renderItem({ role: profileRole, variant }); + + const link = screen.getByRole('link', { name: 'Account' }); + expect(link).toHaveAttribute('href', externalUrl); + + clickWithoutNavigating(link); + expect(screen.getByTestId('location')).toHaveTextContent('/home'); + }); + }); + + it('treats a URL given directly the same way', () => { + renderItem({ url: '/help', variant: 'navLink' }); + + fireEvent.click(screen.getByRole('link', { name: 'Account' })); + expect(screen.getByTestId('location')).toHaveTextContent('/help'); + }); + + it('marks the nav link active on its own route', () => { + renderItem({ role: accountRole, variant: 'navLink' }, '/account/'); + + expect(screen.getByRole('link', { name: 'Account' })).toHaveClass('active'); + }); + + it('renders nothing when no app or external route provides the role', () => { + renderItem({ role: 'org.openedx.frontend.role.linkMenuItemTest.missing', variant: 'dropdownItem' }); + + expect(screen.queryByRole('link')).not.toBeInTheDocument(); + }); +}); diff --git a/shell/menus/LinkMenuItem.tsx b/shell/menus/LinkMenuItem.tsx index 08e32ca3..993eb436 100644 --- a/shell/menus/LinkMenuItem.tsx +++ b/shell/menus/LinkMenuItem.tsx @@ -2,7 +2,7 @@ import { Dropdown, Hyperlink, NavDropdown, NavLink } from '@openedx/paragon'; import { useIntl } from 'react-intl'; import { useLocation } from 'react-router-dom'; -import { getUrlByRouteRole } from '../../runtime/routing'; +import { getLinkProps, resolveRouteByRole } from '../../runtime/routing'; import { MenuItemName } from '../../types'; @@ -24,7 +24,7 @@ export default function LinkMenuItem({ label, role, url, variant = 'hyperlink' } let finalUrl: string | null | undefined; if (role !== undefined) { - finalUrl = getUrlByRouteRole(role); + finalUrl = resolveRouteByRole(role)?.url; } else if (url !== undefined) { finalUrl = url; } @@ -35,27 +35,31 @@ export default function LinkMenuItem({ label, role, url, variant = 'hyperlink' } return null; } + // A path in this site is a react-router Link, so the navigation stays in the client; anything + // else is a plain anchor. + const linkProps = getLinkProps(finalUrl); + if (variant === 'hyperlink') { return ( - + {finalLabel} ); } else if (variant === 'navLink') { return ( - + {finalLabel} ); } else if (variant === 'navDropdownItem') { return ( - + {finalLabel} ); } else if (variant === 'dropdownItem') { return ( - + {finalLabel} );