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-205] - Send top stories experiment analytics events #12075

Open
wants to merge 42 commits into
base: latest
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
7625d2d
Add amp-analytics to AmpExperiment
hotinglok Oct 21, 2024
ac4cd70
Add tests
hotinglok Oct 21, 2024
ef8f7f1
Remove commented out snapshot
hotinglok Oct 21, 2024
a7cd6af
Pass down & use producerId
hotinglok Oct 21, 2024
6905317
Make selectors for event triggers more robust
hotinglok Oct 22, 2024
5607655
Update selectors to account for secondary column section
hotinglok Oct 22, 2024
30f746f
Refactor scripts in AmpExperiment to use same component
hotinglok Oct 22, 2024
558bb5e
Cleanup of tests/types
hotinglok Oct 22, 2024
fc995db
Add conditions to account for short articles
hotinglok Oct 23, 2024
9c42c11
Refactor block insertion logic, exclude articles too short
hotinglok Oct 24, 2024
34973c6
Add variant to pageview only for pages impacted by experiment
hotinglok Oct 24, 2024
150f137
Refactor ATIData to accept ampExperimentName, add tests
hotinglok Oct 27, 2024
b723306
Fix test
hotinglok Oct 28, 2024
2b79825
Add more test assets
hotinglok Oct 28, 2024
cae1f67
Add CPS asset
hotinglok Oct 28, 2024
4766c64
Remove unnecessary undefined type from ampExperimentName
hotinglok Oct 28, 2024
1792eb4
Minor cleanup
hotinglok Oct 28, 2024
c24cdec
Update analytics config type
hotinglok Oct 28, 2024
d97435f
Update data attribute name
hotinglok Oct 28, 2024
6a14fe1
Add position of top stories section to view events, add cymrufyw asse…
hotinglok Oct 28, 2024
5e92f4c
Add position to click events
hotinglok Oct 29, 2024
4cd546e
Fix click event selectors
hotinglok Oct 29, 2024
85e6a69
Fix page view event, renamed variables
hotinglok Oct 29, 2024
5eac132
Add buildATIPageTrackPath() test
hotinglok Oct 30, 2024
3295412
Add MVT test property
hotinglok Oct 30, 2024
2f64abe
Update snapshots
hotinglok Oct 30, 2024
f585e09
Fix missed prop name update
hotinglok Oct 30, 2024
f3d828a
Add valid sport asset
hotinglok Oct 30, 2024
42f3c53
Merge branch 'latest' into nasaa-amp-experiment-analytics
hotinglok Oct 30, 2024
4439bf6
Refactor block insertion to add more blocks
hotinglok Oct 30, 2024
a2040e3
Add other variants & update tests
hotinglok Oct 31, 2024
a6a7619
Add sick typescript refactor
hotinglok Nov 4, 2024
8c67eba
Remove comments in insertBlockAtPosition()
hotinglok Nov 4, 2024
c49dc8d
Add types, refactor css
hotinglok Nov 4, 2024
745e9dd
Refactor types in helpers
hotinglok Nov 4, 2024
e0d0b7a
Fix conflicts
hotinglok Nov 5, 2024
ff62b71
Update src/app/pages/ArticlePage/experimentTopStories/helpers.tsx
hotinglok Nov 5, 2024
5819495
Linting
hotinglok Nov 5, 2024
b04d1ab
Merge branch 'nasaa-amp-experiment-analytics' into nasaa-amp-experime…
hotinglok Nov 6, 2024
b5e725d
Merge pull request #12119 from bbc/nasaa-amp-experiment-more-variants
hotinglok Nov 12, 2024
82475ce
Merge branch 'latest' into nasaa-amp-experiment-analytics
hotinglok Nov 12, 2024
be4d6cb
Update bundle size limits
hotinglok Nov 12, 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
4 changes: 2 additions & 2 deletions scripts/bundleSize/bundleSizeConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
* We are allowing a variance of -5 on `MIN_SIZE` and +5 on `MAX_SIZE` to avoid the need for frequent changes, as bundle sizes can fluctuate
*/

export const MIN_SIZE = 635 - 5;
export const MAX_SIZE = 1175 + 5;
export const MIN_SIZE = 644 - 5;
export const MAX_SIZE = 1183 + 5;
53 changes: 52 additions & 1 deletion src/app/components/ATIAnalytics/atiUrl/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,21 @@ describe('getThingAttributes', () => {
timePublished: 'timePublished',
timeUpdated: 'timeUpdated',
}} | ${'https://www.bbcnewsd73hkzno2ini43t4gblxvycyac5aw4gnv7t2rccijh7745uqd.onion/news'} | ${['s2=producerId', 'p=pageIdentifier', 'x1=[contentId]', 'x3=[appName]', 'x4=[language]', 'x7=[contentType]', 'x11=[timePublished]', 'x12=[timeUpdated]', 'x13=[ldpThingLabels]', 'x14=[ldpThingIds]', 'xto=SEC------', 'product_platform=tor-bbc']}
${{
appName: 'appName',
contentId: 'contentId',
contentType: 'contentType',
language: 'language',
ldpThingIds: 'ldpThingIds',
ldpThingLabels: 'ldpThingLabels',
pageIdentifier: 'pageIdentifier',
pageTitle: 'pageTitle',
platform: 'platform',
producerId: 'producerId',
timePublished: 'timePublished',
timeUpdated: 'timeUpdated',
ampExperimentName: 'someExperiment',
}} | ${'https://www.bbc.com/news'} | ${['s2=producerId', 'p=pageIdentifier', 'x1=[contentId]', 'x3=[appName]', 'x4=[language]', 'x7=[contentType]', 'x11=[timePublished]', 'x12=[timeUpdated]', 'x13=[ldpThingLabels]', 'x14=[ldpThingIds]', 'xto=SEC------', 'mv_test=Google Discover', 'mv_experiment_id=someExperiment', 'mv_creation=VARIANT(someExperiment)']}
`(
'should take in optional props and add them as correct query params',
({ props, currentUrl, expectedValues }) => {
Expand Down Expand Up @@ -244,6 +259,39 @@ describe('buildATIEventTrackUrl', () => {
format: 'format',
url: 'url',
detailedPlacement: 'detailedPlacement',
experimentVariant: 'variant_1',
});

expect(splitUrl(atiEventTrackUrl)).toEqual([
'http://foobar.com',
'idclient=getAtUserId',
's=getDestination',
'p=pageIdentifier',
'r=getScreenInfo',
're=getBrowserViewPort',
'hl=getCurrentTime',
'lng=getDeviceLanguage',
'atc=PUB-[campaignID]-[component]-[variant_1]-[format]-[pageIdentifier]-[detailedPlacement]-[]-[url]',
'type=AT',
]);
});

it('should return the correct url with mvt properties if ampExperimentName is present', () => {
process.env.SIMORGH_ATI_BASE_URL = 'http://foobar.com?';

const atiEventTrackUrl = buildATIEventTrackUrl({
pageIdentifier: 'pageIdentifier',
service: 'news',
platform: 'canonical',
statsDestination: 'statsDestination',
componentName: 'component',
type: 'type',
campaignID: 'campaignID',
format: 'format',
url: 'url',
detailedPlacement: 'detailedPlacement',
experimentVariant: 'variant_1',
ampExperimentName: 'someExperiment',
});

expect(splitUrl(atiEventTrackUrl)).toEqual([
Expand All @@ -255,7 +303,10 @@ describe('buildATIEventTrackUrl', () => {
're=getBrowserViewPort',
'hl=getCurrentTime',
'lng=getDeviceLanguage',
'atc=PUB-[campaignID]-[component]-[]-[format]-[pageIdentifier]-[detailedPlacement]-[]-[url]',
'atc=PUB-[campaignID]-[component]-[variant_1]-[format]-[pageIdentifier]-[detailedPlacement]-[]-[url]',
'mv_test=Google Discover',
'mv_experiment_id=someExperiment',
'mv_creation=variant_1',
'type=AT',
]);
});
Expand Down
54 changes: 54 additions & 0 deletions src/app/components/ATIAnalytics/atiUrl/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export const buildATIPageTrackPath = ({
categoryName,
campaigns,
nationsProducer,
ampExperimentName,
experimentVariant,
}: ATIPageTrackingProps) => {
const href = getHref(platform);
Expand Down Expand Up @@ -215,6 +216,31 @@ export const buildATIPageTrackPath = ({
value: getATIMarketingString(href, campaignType),
wrap: false,
},
...(ampExperimentName
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After fixing conflicts, there was an initial concern that this might be overridden by the elections banner experiment that is currently being done, but as that is only run on mundo/portuguese, this should be ok because experimentVariant won't be defined.

? [
{
key: 'mv_test',
description: 'AMP experiment project name',
value: `Google Discover`,
wrap: false,
disableEncoding: true,
},
{
key: 'mv_experiment_id',
description: 'AMP experiment name',
value: `${ampExperimentName}`,
wrap: false,
disableEncoding: true,
},
{
key: 'mv_creation',
description: 'AMP experiment variant name',
value: `VARIANT(${ampExperimentName})`,
wrap: false,
disableEncoding: true,
},
]
: []),
...(experimentVariant
? [
{
Expand Down Expand Up @@ -269,6 +295,8 @@ export const buildATIEventTrackUrl = ({
advertiserID,
url,
detailedPlacement,
experimentVariant,
ampExperimentName,
}: ATIEventTrackingProps) => {
// on AMP, variable substitutions are used in the value and they cannot be
// encoded: https://github.com/ampproject/amphtml/blob/master/spec/amp-var-substitutions.md
Expand Down Expand Up @@ -340,10 +368,36 @@ export const buildATIEventTrackUrl = ({
advertiserID,
url,
detailedPlacement,
experimentVariant,
}),
wrap: false,
disableEncoding: true,
},
...(ampExperimentName
? [
{
key: 'mv_test',
description: 'AMP experiment project name',
value: `Google Discover`,
wrap: false,
disableEncoding: true,
},
{
key: 'mv_experiment_id',
description: 'AMP experiment name',
value: `${ampExperimentName}`,
wrap: false,
disableEncoding: true,
},
{
key: 'mv_creation',
description: 'AMP experiment variant name',
value: `${experimentVariant}`,
wrap: false,
disableEncoding: true,
},
]
: []),
];

return `${getEnvConfig().SIMORGH_ATI_BASE_URL}${getAtiUrl(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,27 @@ describe('implementation of buildPageATIParams and buildPageATIUrl', () => {

expect(parsedATIURLParams).toEqual(expectedATIURLParams);
});

it('should return ampExperimentName only if it is present in atiData', () => {
const result = buildPageATIParams({
atiData: {
...articlePageAtiData,
ampExperimentName: 'topStoriesExperiment',
},
requestContext: {
...requestContext,
isUK: false,
origin: 'example.com',
pageType: 'article',
previousPath: 'previousPath',
},
serviceContext: { ...serviceContext, service: 'burmese', lang: 'my' },
});
expect(result).toEqual({
...validPageURLParams,
ampExperimentName: 'topStoriesExperiment',
});
});
});

describe('Media Article Page', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const buildPageATIParams = ({
producerId,
timePublished,
timeUpdated,
ampExperimentName,
experimentVariant,
} = atiData;

Expand All @@ -50,6 +51,7 @@ export const buildPageATIParams = ({
statsDestination,
timePublished,
timeUpdated,
...(ampExperimentName && { ampExperimentName }),
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Is spreading needed here? If ampExperimentName is undefined, it won't be returned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I left it as is in the object, the key ampExperimentName would still be there even if the value was falsey. As it is currently written, it won't work without the spread operator because it wouldn't have a valid key/pair if it evaluated as false.

...(experimentVariant && { experimentVariant }),
};
};
Expand Down
4 changes: 4 additions & 0 deletions src/app/components/ATIAnalytics/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface ATIData {
producerName?: string | null;
timePublished?: string | null;
timeUpdated?: string | null;
ampExperimentName?: string;
experimentVariant?: string | null;
}

Expand Down Expand Up @@ -110,6 +111,8 @@ export interface ATIEventTrackingProps {
advertiserID?: string;
url?: string;
detailedPlacement?: string;
experimentVariant?: string;
ampExperimentName?: string;
}

export interface ATIPageTrackingProps {
Expand All @@ -132,6 +135,7 @@ export interface ATIPageTrackingProps {
categoryName?: string | null;
campaigns?: { campaignId?: string; campaignName?: string }[] | null;
nationsProducer?: string | null;
ampExperimentName?: string;
experimentVariant?: string | null;
}

Expand Down
46 changes: 45 additions & 1 deletion src/app/components/AmpExperiment/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,22 @@ const multipleExperimentConfig = {
},
};

const analyticsConfig = {
requests: {
base: 'somehost.com',
clicks: 'somehost.com?somequeryparam=somevalue',
},
triggers: {
trackClicks: {
on: 'click',
request: 'clicks',
selector: 'a',
},
},
};

describe('Amp experiment container on Amp pages', () => {
it('should render an amp-experiment with the expected config', async () => {
it('should render an amp-experiment with the expected experiment config', async () => {
const { container } = render(
<AmpExperiment experimentConfig={experimentConfig} />,
);
Expand Down Expand Up @@ -66,6 +80,36 @@ describe('Amp experiment container on Amp pages', () => {
`);
});

it('should render an amp-analytics with the expected analytics config if present', async () => {
const { container } = render(
<AmpExperiment
experimentConfig={experimentConfig}
analyticsConfig={analyticsConfig}
/>,
);
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>
<amp-analytics
type="piano"
>
<script
type="application/json"
>
{"requests":{"base":"somehost.com","clicks":"somehost.com?somequeryparam=somevalue"},"triggers":{"trackClicks":{"on":"click","request":"clicks","selector":"a"}}}
</script>
</amp-analytics>
</div>
`);
});

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

Expand Down
49 changes: 42 additions & 7 deletions src/app/components/AmpExperiment/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,31 @@ type AmpExperimentConfig = {
};
};

type AmpAnalyticsConfig = {
requests: Record<string, string>;
triggers: Record<
string,
{
on: string;
request: string;
selector?: string;
visibilitySpec?: {
selector?: string;
visiblePercentageMin?: number;
totalTimeMin?: number;
continuousTimeMin?: number;
};
}
>;
};

type AmpExperimentProps = {
[key: Experiment]: AmpExperimentConfig;
experimentConfig: AmpExperimentConfig;
analyticsConfig?: AmpAnalyticsConfig;
};

type AmpScriptProps = AmpExperimentConfig | AmpAnalyticsConfig;

const AmpHead = () => (
<Helmet>
<script
Expand All @@ -32,17 +53,31 @@ const AmpHead = () => (
</Helmet>
);

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

const AmpExperiment = ({
experimentConfig,
analyticsConfig,
}: AmpExperimentProps) => {
return (
<>
<AmpHead />
<amp-experiment>
<script
type="application/json"
/* eslint-disable-next-line react/no-danger */
dangerouslySetInnerHTML={{ __html: JSON.stringify(experimentConfig) }}
/>
<AmpScript config={experimentConfig} />
</amp-experiment>
{analyticsConfig && (
<amp-analytics type="piano">
<AmpScript config={analyticsConfig} />
</amp-analytics>
)}
</>
);
};
Expand Down
4 changes: 2 additions & 2 deletions src/app/lib/analyticsUtils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ export const getEventInfo = ({
pageIdentifier = '',
componentName = '',
campaignID = '',
variant = '', // not a service variant - used for A/B testing
experimentVariant = '',
format = '',
detailedPlacement = '',
advertiserID = '',
Expand All @@ -278,7 +278,7 @@ export const getEventInfo = ({
const generalPlacement = pageIdentifier;
const creation = componentName;

return `PUB-[${campaignID}]-[${creation}]-[${variant}]-[${format}]-[${generalPlacement}]-[${detailedPlacement}]-[${advertiserID}]-[${url}]`;
return `PUB-[${campaignID}]-[${creation}]-[${experimentVariant}]-[${format}]-[${generalPlacement}]-[${detailedPlacement}]-[${advertiserID}]-[${url}]`;
};

export const getThingAttributes = (attribute, articleData) => {
Expand Down
2 changes: 1 addition & 1 deletion src/app/lib/analyticsUtils/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ describe('getEventInfo', () => {
format: 'format',
detailedPlacement: 'detailed-placement',
advertiserID: 'mundo',
variant: 'a/b-test',
experimentVariant: 'a/b-test',
};

it('should return url section', () => {
Expand Down
Loading
Loading