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

[NASAA-203/204] - Create AmpExperiment component, add top stories experiment to PS articles #12048

Merged
merged 27 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
9a2290e
Create AmpExperiment component
hotinglok Oct 9, 2024
cdd54dd
Update tests
hotinglok Oct 9, 2024
bdd3edd
Implement front-end changes
hotinglok Oct 9, 2024
a215780
Refactor various logic
hotinglok Oct 10, 2024
a4b4188
Fix urn bug
hotinglok Oct 14, 2024
b11f47a
Refactor various logic, add tests
hotinglok Oct 14, 2024
739314b
Refactor block insertion logic, add article page tests
hotinglok Oct 14, 2024
db2b3ff
Update block insertion tests
hotinglok Oct 14, 2024
8f709d0
Merge branch 'latest' into nasaa-create-amp-experiment
hotinglok Oct 14, 2024
bcc34e5
Fix urn retrieval function
hotinglok Oct 15, 2024
6aae312
Minor cleanup
hotinglok Oct 15, 2024
3be6ae5
Minor cleanup
hotinglok Oct 15, 2024
cfe5254
Rename, move, update various files and variables
hotinglok Oct 15, 2024
3e46a76
Apply suggestions from code review
hotinglok Oct 15, 2024
1725aa5
Use id instead of pathname for urn
hotinglok Oct 15, 2024
1c4c59a
fix confflicts
hotinglok Oct 15, 2024
4689617
fix tests
hotinglok Oct 15, 2024
cf7a1f1
Linting
hotinglok Oct 15, 2024
68d5932
Refactor helper functions
hotinglok Oct 15, 2024
1674d2e
Fix test
hotinglok Oct 15, 2024
43f903c
Fix accidentally removed check in getExperimentTopStoriesBlocks()
hotinglok Oct 16, 2024
92a84f8
Linting
hotinglok Oct 16, 2024
5bacd6f
Update src/app/components/AmpExperiment/index.tsx
hotinglok Oct 16, 2024
a031332
Rename getExperimentTopStoriesBlocks to getExperimentTopStories
hotinglok Oct 17, 2024
31fbb08
Add article length condition to getExperimentTopStories()
hotinglok Oct 17, 2024
42ad63a
Update helpers test
hotinglok Oct 17, 2024
bf3c63e
Merge branch 'latest' into nasaa-create-amp-experiment
hotinglok Oct 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions src/app/components/AmpExperiment/index.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import React from 'react';
import { render, waitFor } from '../react-testing-library-with-providers';
import AmpExperiment from './index';

const experimentConfig = {
someExperiment: {
variants: {
control: 33,
variant_1: 33,
variant_2: 33,
},
},
};

const multipleExperimentConfig = {
aExperiment: {
variants: {
control: 33,
variant_1: 33,
variant_2: 33,
},
},
bExperiment: {
variants: {
control: 33,
variant_1: 33,
variant_2: 33,
},
},
};

describe('Amp experiment container on Amp pages', () => {
it('should render an amp-experiment with the expected config', async () => {
const { container } = render(
<AmpExperiment experimentConfig={experimentConfig} />,
);
expect(container.querySelector('amp-experiment')).toBeInTheDocument();
expect(container).toMatchInlineSnapshot(`
<div>
<amp-experiment>
<script
type="application/json"
>
{"someExperiment":{"variants":{"control":33,"variant_1":33,"variant_2":33}}}
</script>
</amp-experiment>
</div>
`);
});

it('should render an amp-experiment with the expected config when multiple experiments are running at the same time', async () => {
const { container } = render(
<AmpExperiment experimentConfig={multipleExperimentConfig} />,
);
expect(container.querySelector('amp-experiment')).toBeInTheDocument();
expect(container).toMatchInlineSnapshot(`
<div>
<amp-experiment>
<script
type="application/json"
>
{"aExperiment":{"variants":{"control":33,"variant_1":33,"variant_2":33}},"bExperiment":{"variants":{"control":33,"variant_1":33,"variant_2":33}}}
</script>
</amp-experiment>
</div>
`);
});

it(`should add amp-experiment extension script to page head`, async () => {
render(<AmpExperiment experimentConfig={experimentConfig} />);

await waitFor(() => {
const scripts = Array.from(document.querySelectorAll('head script'));

expect(scripts).toEqual(
expect.arrayContaining([
expect.objectContaining({
src: `https://cdn.ampproject.org/v0/amp-experiment-0.1.js`,
}),
]),
);

expect(scripts).toHaveLength(1);
});
});
});
50 changes: 50 additions & 0 deletions src/app/components/AmpExperiment/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/** @jsx jsx */
/* @jsxFrag React.Fragment */
import { jsx } from '@emotion/react';
import React from 'react';
import { Helmet } from 'react-helmet';

type Variant = string;
type Experiment = string;
type trafficAllocationPercentage = number;

type AmpExperimentConfig = {
[key: Experiment]: {
sticky?: boolean;
consentNotificationId?: string;
variants: {
[key: Variant]: trafficAllocationPercentage;
hotinglok marked this conversation as resolved.
Show resolved Hide resolved
};
};
};

type AmpExperimentProps = {
[key: Experiment]: AmpExperimentConfig;
};

const AmpHead = () => (
<Helmet>
<script
async
custom-element="amp-experiment"
src="https://cdn.ampproject.org/v0/amp-experiment-0.1.js"
/>
</Helmet>
);

const AmpExperiment = ({ experimentConfig }: AmpExperimentProps) => {
return (
<>
<AmpHead />
<amp-experiment>
<script
type="application/json"
/* eslint-disable-next-line react/no-danger */
dangerouslySetInnerHTML={{ __html: JSON.stringify(experimentConfig) }}
/>
</amp-experiment>
</>
);
};

export default AmpExperiment;
7 changes: 7 additions & 0 deletions src/app/components/AmpExperiment/types.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
declare namespace JSX {
interface IntrinsicElements {
'amp-experiment': React.PropsWithChildren<
ScriptHTMLAttributes<HTMLScriptElement>
>;
}
}
20 changes: 18 additions & 2 deletions src/app/pages/ArticlePage/ArticlePage.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,7 @@ export default {
paddingBottom: `${spacings.QUADRUPLE}rem`,
},
}),

topStoriesAndFeaturesSection: ({ spacings, mq }: Theme) =>
featuresSection: ({ spacings, mq }: Theme) =>
css({
marginBottom: `${spacings.TRIPLE}rem`,

Expand All @@ -91,4 +90,21 @@ export default {
padding: `${spacings.DOUBLE}rem`,
},
}),
topStoriesSection: ({ spacings, mq }: Theme) =>
css({
marginBottom: `${spacings.TRIPLE}rem`,
[mq.GROUP_4_MIN_WIDTH]: {
display: 'block',
marginBottom: `${spacings.FULL}rem`,
padding: `${spacings.DOUBLE}rem`,
},
'[amp-x-topStoriesExperiment="show_at_halfway"] &': {
display: 'none',
[mq.GROUP_4_MIN_WIDTH]: {
display: 'block',
marginBottom: `${spacings.FULL}rem`,
padding: `${spacings.DOUBLE}rem`,
},
},
}),
};
33 changes: 28 additions & 5 deletions src/app/pages/ArticlePage/ArticlePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,26 @@ import {
import { ServiceContext } from '../../contexts/ServiceContext';
import RelatedContentSection from '../../components/RelatedContentSection';
import Disclaimer from '../../components/Disclaimer';

import SecondaryColumn from './SecondaryColumn';

import styles from './ArticlePage.styles';
import { ComponentToRenderProps, TimeStampProps } from './types';
import AmpExperiment from '../../components/AmpExperiment';
import {
experimentTopStoriesConfig,
getExperimentTopStoriesBlocks,
ExperimentTopStories,
} from './experimentTopStories/helpers';

const ArticlePage = ({ pageData }: { pageData: Article }) => {
const { isApp, pageType, service } = useContext(RequestContext);
const { isApp, pageType, service, isAmp, id } = useContext(RequestContext);

const {
articleAuthor,
isTrustProjectParticipant,
showRelatedTopics,
brandName,
} = useContext(ServiceContext);

const { enabled: preloadLeadImageToggle } = useToggle('preloadLeadImage');

const {
Expand Down Expand Up @@ -131,6 +137,16 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {
...(isCPS && { pageTitle: `${atiAnalytics.pageTitle} - ${brandName}` }),
};

const topStoriesContent = pageData?.secondaryColumn?.topStories;
const { shouldEnableExperimentTopStories, transformedBlocks } =
getExperimentTopStoriesBlocks({
blocks,
topStoriesContent,
isAmp,
service,
id,
});

const componentsToRender = {
visuallyHiddenHeadline,
headline: headings,
Expand Down Expand Up @@ -174,6 +190,10 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {
<Disclaimer {...props} increasePaddingOnDesktop={false} />
),
podcastPromo: () => (podcastPromoEnabled ? <InlinePodcastPromo /> : null),
experimentTopStories: () =>
topStoriesContent ? (
<ExperimentTopStories topStoriesContent={topStoriesContent} />
) : null,
};

const visuallyHiddenBlock = {
Expand All @@ -183,8 +203,8 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {
};

const articleBlocks = startsWithHeading
? blocks
: [visuallyHiddenBlock, ...blocks];
? transformedBlocks
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused on what's happening here. Does this always add in the experiment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be. This was flagged by a TS error I get on line 102

Right operand of ?? is unreachable because the left operand is never nullish.

If I add something in like the below on line 205 then the unit tests pass

  const blocksToUse = shouldEnableExperimentTopStories
    ? transformedBlocks
    : blocks;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the function now returns either the blocks unchanged or the experimentally changes to the blocks. This should probably be made more clear by updating the name of the function to de-couple it from experimentation. i.e getExperimentTopStoriesBlocks should really be getTopStoriesBlocks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry that was just me being careless. You're right about the fix, I moved around a few things in the last refactor and completely left the check out. Re-added, should make sense now. Also missed some more renaming, but should be good now!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@iainrj I've kept the name getExperimentTopStoriesBlocks since it doesn't get the data for the original TopStoriesSection in the secondary column and because the top stories blocks in the article blocks array have type: experimentTopStories.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice one, thanks :)

: [visuallyHiddenBlock, ...transformedBlocks];

const promoImageBlocks =
pageData?.promo?.images?.defaultPromoImage?.blocks ?? [];
Expand All @@ -206,6 +226,9 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {

return (
<div css={styles.pageWrapper}>
{shouldEnableExperimentTopStories && (
<AmpExperiment experimentConfig={experimentTopStoriesConfig} />
)}
<ATIAnalytics atiData={atiData} />
<ChartbeatAnalytics
sectionName={pageData?.relatedContent?.section?.name}
Expand Down
7 changes: 2 additions & 5 deletions src/app/pages/ArticlePage/SecondaryColumn.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,12 @@ const SecondaryColumn = ({ pageData }: { pageData: Article }) => {
return (
<div css={styles.secondaryColumn}>
{topStoriesContent && (
<div
css={styles.topStoriesAndFeaturesSection}
data-testid="top-stories"
>
<div css={styles.topStoriesSection} data-testid="top-stories">
<TopStoriesSection content={topStoriesContent} />
</div>
)}
{featuresContent && (
<div css={styles.topStoriesAndFeaturesSection} data-testid="features">
<div css={styles.featuresSection} data-testid="features">
<FeaturesAnalysis
content={featuresContent}
parentColumns={{}}
Expand Down
89 changes: 89 additions & 0 deletions src/app/pages/ArticlePage/experimentTopStories/helpers.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { getExperimentTopStoriesBlocks } from './helpers';
import { topStoriesList } from '../PagePromoSections/TopStoriesSection/fixture/index';

describe('AMP top stories experiment', () => {
const mockTextBlock = {
type: 'text',
model: {
blocks: [],
},
};
const expectedExperimentTopStoriesBlock = {
type: 'experimentTopStories',
model: topStoriesList,
id: 'experimentTopStories-1',
};

const blocksEvenLength = [mockTextBlock, mockTextBlock];
const blocksOddLength = [mockTextBlock, mockTextBlock, mockTextBlock];

describe('getExperimentTopStoriesBlocks()', () => {
it('returns shouldEnableExperimentTopStories as true if props match conditions.', () => {
const { shouldEnableExperimentTopStories } =
getExperimentTopStoriesBlocks({
blocks: blocksEvenLength,
topStoriesContent: topStoriesList,
isAmp: true,
id: 'c6v11qzyv8po',
service: 'news',
});
expect(shouldEnableExperimentTopStories).toBe(true);
});

it.each`
testDescription | isAmp | id | service
${'all props are undefined'} | ${false} | ${undefined} | ${undefined}
${'only isAmp is true'} | ${true} | ${undefined} | ${undefined}
${'only pathname is undefined'} | ${true} | ${undefined} | ${'news'}
${'only pathname is defined and valid'} | ${false} | ${'c6v11qzyv8po'} | ${undefined}
${'all props defined but pathname is invalid'} | ${false} | ${'c1231qzyv8po'} | ${undefined}
${'only service is undefined'} | ${true} | ${'c6v11qzyv8po'} | ${undefined}
${'only service is defined and valid'} | ${false} | ${undefined} | ${'news'}
${'all props defined but service is invalid'} | ${true} | ${'c6v11qzyv8po'} | ${'igbo'}
`(

Choose a reason for hiding this comment

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

Impressive I like that

'returns shouldEnableExperimentTopStories as false because $testDescription.',
({ isAmp, id, service }) => {
const { shouldEnableExperimentTopStories } =
getExperimentTopStoriesBlocks({
blocks: blocksEvenLength,
topStoriesContent: topStoriesList,
isAmp,
id,
service,
});

expect(shouldEnableExperimentTopStories).toBe(false);
},
);

const expectedBlocksEvenLength = [
mockTextBlock,
expectedExperimentTopStoriesBlock,
mockTextBlock,
];
const expectedBlocksOddLength = [
mockTextBlock,
expectedExperimentTopStoriesBlock,
mockTextBlock,
mockTextBlock,
];

it.each`
testType | inputBlocks | expectedOutput
${'even'} | ${blocksEvenLength} | ${expectedBlocksEvenLength}
${'odd'} | ${blocksOddLength} | ${expectedBlocksOddLength}
`(
'should insert experimentTopStories block into blocks array in the correct position when blocks.length is $testType',
({ inputBlocks, expectedOutput }) => {
const { transformedBlocks } = getExperimentTopStoriesBlocks({
blocks: inputBlocks,
topStoriesContent: topStoriesList,
isAmp: true,
id: 'c6v11qzyv8po',
service: 'news',
});
expect(transformedBlocks).toEqual(expectedOutput);
},
);
});
});
Loading
Loading