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

Rely on Prismic filters to fetch pages based on tagged siteSection #11283

Merged
merged 8 commits into from
Oct 22, 2024
4 changes: 2 additions & 2 deletions .buildkite/urls/expected_200_urls.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
/events/racial-difference-in-the-anglophone-caribbean
/exhibitions/tranquillity
/guides/archives-at-wellcome-collection
/pages/the-history-and-context-of-our-collections
/collections/the-history-and-context-of-our-collections
/projects/regarding-forests-touring-exhibition
/seasons/on-happiness
/series/the-disabled-lockdown-experience
Expand All @@ -76,7 +76,7 @@
# This is the RawMinds page, which has an ID very similar to the
# Schools page, but it shouldn't be redirected. See the router in
# the content app, where we enable case-sensitive routing.
/pages/youth-projects
/get-involved/youth-projects

# This page won't serve anything especially useful, but we want to
# make sure it won't throw a 500 error
Expand Down
2 changes: 1 addition & 1 deletion common/services/prismic/link-resolver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ it('resolves exhibition guides to /guides/exhibitions/{id}', () => {

test.each([
{ doc: { type: 'articles', uid: '1' }, path: '/articles/1' },
{ doc: { type: 'pages', uid: '1' }, path: '/pages/1' },
{ doc: { type: 'pages', uid: '1' }, path: '/1' },
{ doc: { type: 'not a thing', uid: '1' }, path: '/' },
])('$doc resolves to $path', ({ doc, path }) => {
expect(linkResolver(doc)).toBe(path);
Expand Down
11 changes: 11 additions & 0 deletions common/services/prismic/link-resolver.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { SiteSection } from '@weco/common/views/components/PageLayout/PageLayout';

import { isContentType } from './content-types';

type Props = {
Expand All @@ -13,10 +15,12 @@ type DataProps = {
type: string;
};
};
siteSection: SiteSection;
};

function linkResolver(doc: Props | DataProps): string {
const { uid, type } = doc;

if (!uid) return '/';
if (type === 'webcomics') return `/articles/${uid}`;
if (type === 'webcomic-series') return `/series/${uid}`;
Expand All @@ -41,6 +45,13 @@ function linkResolver(doc: Props | DataProps): string {
}
}

if (type === 'pages') {
if ('siteSection' in doc) {
return `${doc.siteSection}/${uid}`;
}
return `/${uid}`;
}

if (isContentType(type)) {
return `/${type}/${uid}`;
}
Expand Down
2 changes: 1 addition & 1 deletion content/webapp/components/EventsSearchResults/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ const EventsSearchResults: FunctionComponent<Props> = ({ events }: Props) => {
return (
<CardOuter
key={event.id}
href={linkResolver({ uid: event.uid, type: 'events' })}
href={linkResolver({ ...event, type: 'events' })}
>
<CardImageWrapper>
{croppedImage && (
Expand Down
2 changes: 1 addition & 1 deletion content/webapp/components/FeaturedCard/FeaturedCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export function convertItemToFeaturedCardProps(
},
labels: item.labels,
link: {
url: linkResolver({ uid: item.uid, type: item.type }),
url: linkResolver(item),
text: item.title,
},
};
Expand Down
2 changes: 1 addition & 1 deletion content/webapp/components/StoriesGrid/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ const StoriesGrid: FunctionComponent<Props> = ({
<StoryWrapper
key={article.id}
as="a"
href={linkResolver({ uid: article.uid, type: 'articles' })}
href={linkResolver({ ...article, type: 'articles' })}
$isDetailed={isDetailed}
data-testid="story-search-result"
>
Expand Down
1 change: 1 addition & 0 deletions content/webapp/pages/[uid].tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const getServerSideProps: GetServerSideProps<
return page.getServerSideProps({
...context,
query: { pageId: context.query.uid },
params: { siteSection: 'orphan' },
});
};

Expand Down
26 changes: 8 additions & 18 deletions content/webapp/pages/pages/[pageId].tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export const getServerSideProps: GetServerSideProps<
const { pageId } = context.query;
const siteSection = toMaybeString(context.params?.siteSection);

if (!looksLikePrismicId(pageId)) {
if (!looksLikePrismicId(pageId) || !siteSection) {
return { notFound: true };
}
const client = createClient(context);
Expand All @@ -120,25 +120,15 @@ export const getServerSideProps: GetServerSideProps<
? context.resolvedUrl
: undefined;

const pageDocument = await fetchPage(client, pageId);
const pageDocument = await fetchPage(
client,
pageId,
isSiteSection(siteSection) || siteSection === 'orphan'
Copy link
Contributor

Choose a reason for hiding this comment

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

would it simplify things to add 'orphan' to the siteSectionList?

Copy link
Contributor Author

@rcantin-w rcantin-w Oct 21, 2024

Choose a reason for hiding this comment

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

I wondered the same, but I felt like "orphan" not being a main menu option, it might be nice to only allow it in the URL logic and not as an actual valid option everywhere that type is used. Do you think that makes sense or would you rather feel like making it a valid option across makes more sense @davidpmccormick ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed I've left comments to explain the logic eb6dec4

? siteSection
: undefined
);

if (isNotUndefined(pageDocument)) {
// If it does not have a tag and the route hasn't specified one either,
// it's an orphan, continue rendering.
const pageTagHasSection = pageDocument.tags.find(t => isSiteSection(t));
if (siteSection || pageTagHasSection) {
// If it does, it has to be a valid one
const tagIsValidSiteSection = isSiteSection(siteSection);
// and the same one passed by the route page.
const isSameSectionAsRoute = pageDocument.tags.find(
t => t === siteSection
);

// otherwise return not found. else, continue rendering.
if (!tagIsValidSiteSection || !isSameSectionAsRoute)
return { notFound: true };
}

const serverData = await getServerData(context);

const page = transformPage(pageDocument);
Expand Down
2 changes: 1 addition & 1 deletion content/webapp/services/prismic/fetch/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ type FetchEventsQueryParams = {
availableOnline?: boolean;
page?: number;
pageSize?: number;
orderings?: (prismic.Ordering | string)[];
orderings?: prismic.Ordering[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

string is deprecated as a value here

};

const startField = 'my.events.times.startDateTime';
Expand Down
7 changes: 6 additions & 1 deletion content/webapp/services/prismic/fetch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { PaginatedResults } from '@weco/common/services/prismic/types';
import { deserialiseDates as deserialiseJsonDates } from '@weco/common/utils/json';
import { toMaybeString } from '@weco/common/utils/routes';
import { isString } from '@weco/common/utils/type-guards';
import { SiteSection } from '@weco/common/views/components/PageLayout/PageLayout';

export type GetServerSidePropsPrismicClient = {
type: 'GetServerSidePropsPrismicClient';
Expand Down Expand Up @@ -153,7 +154,8 @@ export function fetcher<Document extends prismic.PrismicDocument>(

getByUid: async (
{ client }: GetServerSidePropsPrismicClient,
uid: string
uid: string,
siteSection?: SiteSection | 'orphan'
): Promise<Document | undefined> => {
try {
const primaryContentType = toMaybeString(contentType);
Expand All @@ -165,6 +167,9 @@ export function fetcher<Document extends prismic.PrismicDocument>(
uid,
{
fetchLinks,
filters: siteSection
? [prismic.filter.any('document.tags', [siteSection])]
: [],
}
);

Expand Down
6 changes: 4 additions & 2 deletions content/webapp/services/prismic/fetch/pages.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as prismic from '@prismicio/client';

import { PagesDocument as RawPagesDocument } from '@weco/common/prismicio-types';
import { SiteSection } from '@weco/common/views/components/PageLayout/PageLayout';
import { labelsFields } from '@weco/content/services/prismic/fetch-links';
import {
articleFormatsFetchLinks,
Expand Down Expand Up @@ -59,11 +60,12 @@ const pagesFetcher = fetcher<RawPagesDocument>(['pages'], fetchLinks);
export const fetchPages = pagesFetcher.getByType;
export const fetchPage = async (
client: GetServerSidePropsPrismicClient,
id: string
id: string,
siteSection?: SiteSection | 'orphan'
): Promise<RawPagesDocument | undefined> => {
// #11240 once redirects are in place we should only fetch by uid
const pageDocument =
(await pagesFetcher.getByUid(client, id)) ||
(await pagesFetcher.getByUid(client, id, siteSection)) ||
(await pagesFetcher.getById(client, id));

return pageDocument;
Expand Down
2 changes: 0 additions & 2 deletions content/webapp/services/prismic/transformers/pages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ export function transformPage(document: RawPagesDocument): Page {
})
: [];

// TODO (tagging): This is just for now, we will be implementing a proper site tagging
// strategy for this later
const siteSections = headerLinks.map(link => link.siteSection);
const siteSection = document.tags.find(tag =>
siteSections.includes(tag as SiteSection)
Expand Down
2 changes: 1 addition & 1 deletion content/webapp/services/prismic/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ export type StructuredSearchQuery = {
tags: string[];
tag: string[];
pageSize: number;
orderings: string[];
orderings: prismic.Ordering[];
// content type specific
'article-series': string[];
};
Expand Down
6 changes: 6 additions & 0 deletions content/webapp/types/card.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { getCrop, ImageType } from '@weco/common/model/image';
import linkResolver from '@weco/common/services/prismic/link-resolver';
import { SiteSection } from '@weco/common/views/components/PageLayout/PageLayout';
import { ArticleBasic } from '@weco/content/types/articles';
import { Book } from '@weco/content/types/books';
import { EventSeries } from '@weco/content/types/event-series';
Expand All @@ -26,6 +27,7 @@ export type Card = {
image?: ImageType;
link?: string;
order?: number;
siteSection?: SiteSection;
};

export function convertItemToCardProps(
Expand Down Expand Up @@ -60,13 +62,16 @@ export function convertItemToCardProps(
id: item.id,
uid: item.uid,
type: item.type,
siteSection: 'siteSection' in item ? item.siteSection : undefined,
data: { relatedDocument: item.relatedDocument },
}
: {
id: item.id,
uid: item.uid,
type: item.type,
siteSection: 'siteSection' in item ? item.siteSection : undefined,
};

return {
type: 'card',
format: format as never, // TODO: This is now warning for use of any, need to specify type correctly
Expand Down Expand Up @@ -96,5 +101,6 @@ export function convertItemToCardProps(
}
: undefined,
link: (item.promo && item.promo.link) || linkResolver(linkData),
siteSection: 'siteSection' in item ? item.siteSection : undefined,
};
}
2 changes: 1 addition & 1 deletion playwright/test/helpers/contexts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ const mediaOffice = async (
page: Page
): Promise<void> => {
await context.addCookies(requiredCookies);
await gotoWithoutCache(`${baseUrl}/pages/WuxrKCIAAP9h3hmw`, page); // alias is /press but it doesn't work locally
await gotoWithoutCache(`${baseUrl}/about-us/media-office`, page);
};

const isMobile = (page: Page): boolean =>
Expand Down
4 changes: 2 additions & 2 deletions playwright/test/homepage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ test('(2) | Cookie banner displays on first visit from anywhere, except the cook
await gotoWithoutCache(`${baseUrl}/visit-us`, page);
await expect(cookieBanner).toBeAttached();

await gotoWithoutCache(`${baseUrl}/cookie-policy`, page);
await gotoWithoutCache(`${baseUrl}/about-us/cookie-policy`, page);
await expect(cookieBanner).not.toBeAttached();
});

test('(3) | Cookie banner only displays if CookieControl cookie has not already been set', async ({
context,
page,
}) => {
await gotoWithoutCache(`${baseUrl}/pages/WuxrKCIAAP9h3hmw`, page);
await gotoWithoutCache(`${baseUrl}/about-us/media-office`, page);
const cookieBanner = await page.getByLabel('Our website uses cookies');
await expect(cookieBanner).toBeAttached();

Expand Down