Skip to content

Commit 17ce31d

Browse files
dognose24claude
dognose24
and
claude
authored
Banner: absolutize root-relative hrefs in wp-admin (Odyssey) (#113168)
* Banner: absolutize root-relative hrefs in wp-admin (Odyssey) Root-relative hrefs on Banner/UpsellNudge are Calypso routes, but wp-admin resolves them against the site's own domain - every upgrade nudge rendered in Odyssey Stats linked to a 404 (the SEO preview nudge was patched at its call site in #113144; stats-no-content-banner's /post link is still broken today). Absolutize at the Banner chokepoint - computed defaults, caller-provided hrefs, and the secondary CTA all pass through it - via getCalypsoUrl(), a no-op outside wp-admin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Banner: move toCalypsoHref to lib/url and cover the wiring Review feedback on #113168: - toCalypsoHref isn't a Banner concept, and call sites that can't import from a component (promo-cards, stats-email-detail, and #113144's getUpgradeNudgeHref) need it too. Move it to calypso/lib/url so the ~4 drifted copies in the tree have somewhere to converge. - The wiring itself was untested: cutting toCalypsoHref out of getHref() and secondaryHref left the suite green. Add render tests covering every href Banner emits — computed plans hrefs, caller-provided hrefs, the CTA button, and the secondary CTA — which now fail if the wiring is removed. - getCalypsoUrl() already strips the leading slash, so pass the href straight through instead of concatenating. - Guard on typeof href === 'string' rather than truthiness. - Wrap once where getHref() returns instead of at each branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 8507f51 commit 17ce31d

5 files changed

Lines changed: 183 additions & 9 deletions

File tree

‎client/components/banner/index.jsx‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { connect } from 'react-redux';
1818
import DismissibleCard from 'calypso/blocks/dismissible-card';
1919
import JetpackLogo from 'calypso/components/jetpack-logo';
2020
import TrackComponentView from 'calypso/lib/analytics/track-component-view';
21-
import { addQueryArgs } from 'calypso/lib/url';
21+
import { addQueryArgs, toCalypsoHref } from 'calypso/lib/url';
2222
import { recordTracksEvent } from 'calypso/state/analytics/actions';
2323
import { canCurrentUser } from 'calypso/state/selectors/can-current-user';
2424
import isSiteWPForTeams from 'calypso/state/selectors/is-site-wpforteams';
@@ -107,23 +107,27 @@ export class Banner extends Component {
107107
getHref() {
108108
const { canUserUpgrade, feature, href, plan, siteSlug, customerType } = this.props;
109109

110+
let computedHref = href;
111+
110112
if ( ! href && siteSlug && canUserUpgrade ) {
111-
if ( customerType ) {
112-
return `/plans/${ siteSlug }?customerType=${ customerType }`;
113-
}
114113
const baseUrl = `/plans/${ siteSlug }`;
115-
if ( feature || plan ) {
116-
return addQueryArgs(
114+
115+
if ( customerType ) {
116+
computedHref = `${ baseUrl }?customerType=${ customerType }`;
117+
} else if ( feature || plan ) {
118+
computedHref = addQueryArgs(
117119
{
118120
feature,
119121
plan,
120122
},
121123
baseUrl
122124
);
125+
} else {
126+
computedHref = baseUrl;
123127
}
124-
return baseUrl;
125128
}
126-
return href;
129+
130+
return toCalypsoHref( computedHref );
127131
}
128132

129133
handleClick = ( e ) => {
@@ -305,7 +309,7 @@ export class Banner extends Component {
305309
{ secondaryCallToAction && (
306310
<Button
307311
compact={ compactButton }
308-
href={ secondaryHref }
312+
href={ toCalypsoHref( secondaryHref ) }
309313
onClick={ this.handleSecondaryClick }
310314
primary={ false }
311315
>

‎client/components/banner/test/odyssey-href.jsx‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
/**
2+
* @jest-environment jsdom
3+
*/
4+
jest.mock( '@automattic/calypso-config', () => {
5+
const configApi = () => '';
6+
configApi.isEnabled = jest.fn( ( flag ) => flag === 'is_odyssey' );
7+
return configApi;
8+
} );
9+
10+
jest.mock( 'calypso/state/analytics/actions', () => ( {
11+
recordTracksEvent: jest.fn( () => ( { type: 'ANALYTICS_EVENT_RECORD' } ) ),
12+
} ) );
13+
14+
import { render, screen } from '@testing-library/react';
15+
import { Banner } from '../index';
16+
17+
const props = {
18+
title: 'banner title',
19+
siteSlug: 'example.com',
20+
canUserUpgrade: true,
21+
};
22+
23+
describe( 'Banner hrefs in wp-admin (Odyssey)', () => {
24+
test( 'absolutizes the computed plans href', () => {
25+
const { container } = render( <Banner { ...props } /> );
26+
27+
expect( container.querySelector( 'a' ) ).toHaveAttribute(
28+
'href',
29+
'https://wordpress.com/plans/example.com'
30+
);
31+
} );
32+
33+
test( 'absolutizes the computed plans href with feature and plan args', () => {
34+
const { container } = render(
35+
<Banner { ...props } feature="advanced-seo" plan="business-bundle" />
36+
);
37+
38+
expect( container.querySelector( 'a' ) ).toHaveAttribute(
39+
'href',
40+
'https://wordpress.com/plans/example.com?feature=advanced-seo&plan=business-bundle'
41+
);
42+
} );
43+
44+
test( 'absolutizes the computed plans href with a customerType arg', () => {
45+
const { container } = render( <Banner { ...props } customerType="business" /> );
46+
47+
expect( container.querySelector( 'a' ) ).toHaveAttribute(
48+
'href',
49+
'https://wordpress.com/plans/example.com?customerType=business'
50+
);
51+
} );
52+
53+
test( 'absolutizes a caller-provided href', () => {
54+
const { container } = render( <Banner { ...props } href="/post/example.com" /> );
55+
56+
expect( container.querySelector( 'a' ) ).toHaveAttribute(
57+
'href',
58+
'https://wordpress.com/post/example.com'
59+
);
60+
} );
61+
62+
test( 'absolutizes the call-to-action button href', () => {
63+
render( <Banner { ...props } callToAction="Upgrade" forceHref={ false } /> );
64+
65+
expect( screen.getByRole( 'link', { name: 'Upgrade' } ) ).toHaveAttribute(
66+
'href',
67+
'https://wordpress.com/plans/example.com'
68+
);
69+
} );
70+
71+
test( 'absolutizes the secondary call-to-action href', () => {
72+
render(
73+
<Banner
74+
{ ...props }
75+
callToAction="Upgrade"
76+
secondaryCallToAction="Learn more"
77+
secondaryHref="/support/example.com"
78+
/>
79+
);
80+
81+
expect( screen.getByRole( 'link', { name: 'Learn more' } ) ).toHaveAttribute(
82+
'href',
83+
'https://wordpress.com/support/example.com'
84+
);
85+
} );
86+
87+
test( 'leaves an absolute href untouched', () => {
88+
const { container } = render(
89+
<Banner { ...props } href="https://example.com/wp-admin/admin.php?page=stats" />
90+
);
91+
92+
expect( container.querySelector( 'a' ) ).toHaveAttribute(
93+
'href',
94+
'https://example.com/wp-admin/admin.php?page=stats'
95+
);
96+
} );
97+
} );

‎client/lib/url/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,4 @@ export { addSchemeIfMissing, setUrlScheme } from './scheme-utils';
1212
export { decodeURIIfValid, decodeURIComponentIfValid } from './decode-utils';
1313
export { default as resolveRelativePath } from './resolve-relative-path';
1414
export { pathToUrl } from './path-to-url';
15+
export { default as toCalypsoHref } from './to-calypso-href';

‎client/lib/url/test/to-calypso-href.js‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/**
2+
* @jest-environment jsdom
3+
*/
4+
jest.mock( '@automattic/calypso-config', () => {
5+
const configApi = () => '';
6+
configApi.isEnabled = jest.fn( () => false );
7+
return configApi;
8+
} );
9+
10+
import config from '@automattic/calypso-config';
11+
import toCalypsoHref from '../to-calypso-href';
12+
13+
const enableOdyssey = ( enabled ) =>
14+
config.isEnabled.mockImplementation( ( flag ) => flag === 'is_odyssey' && enabled );
15+
16+
describe( 'toCalypsoHref', () => {
17+
afterEach( () => {
18+
config.isEnabled.mockReset();
19+
config.isEnabled.mockReturnValue( false );
20+
} );
21+
22+
test( 'absolutizes a root-relative Calypso route in wp-admin (Odyssey)', () => {
23+
enableOdyssey( true );
24+
expect( toCalypsoHref( '/post/example.com' ) ).toBe( 'https://wordpress.com/post/example.com' );
25+
expect( toCalypsoHref( '/plans/example.com?feature=advanced-seo&plan=business-bundle' ) ).toBe(
26+
'https://wordpress.com/plans/example.com?feature=advanced-seo&plan=business-bundle'
27+
);
28+
} );
29+
30+
test( 'leaves absolute and protocol-relative URLs untouched in Odyssey', () => {
31+
enableOdyssey( true );
32+
expect( toCalypsoHref( 'https://example.com/wp-admin/upload.php' ) ).toBe(
33+
'https://example.com/wp-admin/upload.php'
34+
);
35+
expect( toCalypsoHref( '//example.com/path' ) ).toBe( '//example.com/path' );
36+
expect( toCalypsoHref( 'admin.php?page=stats' ) ).toBe( 'admin.php?page=stats' );
37+
} );
38+
39+
test( 'passes empty values through in Odyssey', () => {
40+
enableOdyssey( true );
41+
expect( toCalypsoHref( undefined ) ).toBeUndefined();
42+
expect( toCalypsoHref( null ) ).toBeNull();
43+
} );
44+
45+
test( 'is a no-op outside wp-admin', () => {
46+
enableOdyssey( false );
47+
expect( toCalypsoHref( '/post/example.com' ) ).toBe( '/post/example.com' );
48+
} );
49+
} );

‎client/lib/url/to-calypso-href.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import config from '@automattic/calypso-config';
2+
import { getCalypsoUrl } from '@automattic/calypso-url';
3+
4+
/**
5+
* In wp-admin (Odyssey) a root-relative href is a Calypso route, which the browser would
6+
* resolve against the site's own domain — a 404. Point it at Calypso absolutely there;
7+
* getCalypsoUrl() falls back to https://wordpress.com when the current origin isn't a
8+
* Calypso one, as in wp-admin. Everywhere else (Calypso proper, Jetpack Cloud) this is a
9+
* no-op, so it is safe to apply to any href of unknown origin.
10+
* @param href The href to absolutize.
11+
* @returns The href, absolutized against Calypso only when it is a root-relative route in wp-admin.
12+
*/
13+
export default function toCalypsoHref< T >( href: T ): T | string {
14+
if (
15+
typeof href === 'string' &&
16+
href.startsWith( '/' ) &&
17+
! href.startsWith( '//' ) &&
18+
config.isEnabled( 'is_odyssey' )
19+
) {
20+
return getCalypsoUrl( href );
21+
}
22+
return href;
23+
}

0 commit comments

Comments
 (0)