-
Notifications
You must be signed in to change notification settings - Fork 225
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
base: latest
Are you sure you want to change the base?
Changes from 9 commits
7625d2d
ac4cd70
ef8f7f1
a7cd6af
6905317
5607655
30f746f
558bb5e
fc995db
9c42c11
34973c6
150f137
b723306
2b79825
cae1f67
4766c64
1792eb4
c24cdec
d97435f
6a14fe1
5e92f4c
4cd546e
85e6a69
5eac132
3295412
2f64abe
f585e09
f3d828a
42f3c53
4439bf6
a2040e3
a6a7619
8c67eba
c49dc8d
745e9dd
e0d0b7a
ff62b71
5819495
b04d1ab
b5e725d
82475ce
be4d6cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,6 +109,7 @@ export interface ATIEventTrackingProps { | |
advertiserID?: string; | ||
url?: string; | ||
detailedPlacement?: string; | ||
variant?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per comment above - is this an experiment variant or a dual script variant? |
||
} | ||
|
||
export interface ATIPageTrackingProps { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,8 +18,14 @@ type AmpExperimentConfig = { | |
}; | ||
}; | ||
|
||
type AmpAnalyticsConfig = { | ||
requests: Record<string, unknown>; | ||
triggers: Record<string, unknown>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question (non-blocking) Can this be typed any more strictly? I can see that we set base, clicks, trackClicks which we could encode here too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated this a bit with just the things that we've used for the experiment. There's a lot of other optional properties that I've not included, but those can be added whenever they're needed. |
||
}; | ||
|
||
type AmpExperimentProps = { | ||
[key: Experiment]: AmpExperimentConfig; | ||
experimentConfig: AmpExperimentConfig; | ||
analyticsConfig?: AmpAnalyticsConfig; | ||
}; | ||
|
||
const AmpHead = () => ( | ||
|
@@ -32,17 +38,31 @@ const AmpHead = () => ( | |
</Helmet> | ||
); | ||
|
||
const AmpExperiment = ({ experimentConfig }: AmpExperimentProps) => { | ||
const AmpScript = ({ config }: { config: Record<string, unknown> }) => { | ||
hotinglok marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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> | ||
)} | ||
</> | ||
); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,14 @@ | ||
import { getExperimentTopStories } from './helpers'; | ||
import { | ||
getExperimentAnalyticsConfig, | ||
getExperimentTopStories, | ||
} from './helpers'; | ||
import { topStoriesList } from '../PagePromoSections/TopStoriesSection/fixture/index'; | ||
|
||
jest.mock('../../../lib/analyticsUtils', () => ({ | ||
...jest.requireActual('../../../lib/analyticsUtils'), | ||
getAtUserId: jest.fn().mockReturnValue('123-456-789'), | ||
})); | ||
|
||
Comment on lines
+7
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block is needed because |
||
describe('AMP top stories experiment', () => { | ||
const mockTextBlock = { | ||
type: 'text', | ||
|
@@ -16,8 +24,6 @@ describe('AMP top stories experiment', () => { | |
}; | ||
}; | ||
|
||
const blocksShortLength = [mockTextBlock]; | ||
|
||
const blocksEvenLength = [ | ||
mockTextBlock, | ||
mockTextBlock, | ||
|
@@ -63,26 +69,67 @@ describe('AMP top stories experiment', () => { | |
}, | ||
); | ||
|
||
const blocksShortLength = [mockTextBlock]; | ||
const blocksLongEvenLength = [ | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
]; | ||
const blocksLongOddLength = [ | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
]; | ||
|
||
const expectedBlocksEvenLength = [ | ||
mockTextBlock, | ||
mockTextBlock, | ||
expectedExperimentTopStoriesBlock(2), | ||
mockTextBlock, | ||
expectedExperimentTopStoriesBlock(3), | ||
mockTextBlock, | ||
]; | ||
const expectedBlocksOddLength = [ | ||
mockTextBlock, | ||
expectedExperimentTopStoriesBlock(1), | ||
mockTextBlock, | ||
mockTextBlock, | ||
expectedExperimentTopStoriesBlock(3), | ||
]; | ||
const expectedBlocksLongEvenLength = [ | ||
mockTextBlock, | ||
mockTextBlock, | ||
expectedExperimentTopStoriesBlock(2), | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
]; | ||
|
||
const expectedBlocksLongOddLength = [ | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
expectedExperimentTopStoriesBlock(3), | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
mockTextBlock, | ||
]; | ||
|
||
it.each` | ||
testType | inputBlocks | expectedOutput | ||
${'even'} | ${blocksEvenLength} | ${expectedBlocksEvenLength} | ||
${'odd'} | ${blocksOddLength} | ${expectedBlocksOddLength} | ||
testType | inputBlocks | expectedOutput | ||
${'4'} | ${blocksEvenLength} | ${expectedBlocksEvenLength} | ||
${'3'} | ${blocksOddLength} | ${expectedBlocksOddLength} | ||
${'even'} | ${blocksLongEvenLength} | ${expectedBlocksLongEvenLength} | ||
${'odd'} | ${blocksLongOddLength} | ${expectedBlocksLongOddLength} | ||
`( | ||
'should insert experimentTopStories block into blocks array in the correct position when blocks.length is $testType', | ||
'should insert experimentTopStories block into blocks array in the correct position when blocks.length is $testType.', | ||
({ inputBlocks, expectedOutput }) => { | ||
const { transformedBlocks } = getExperimentTopStories({ | ||
blocks: inputBlocks, | ||
|
@@ -106,4 +153,71 @@ describe('AMP top stories experiment', () => { | |
expect(transformedBlocks).toBe(blocksShortLength); | ||
}); | ||
}); | ||
|
||
describe('getExperimentAnalyticsConfig()', () => { | ||
process.env.SIMORGH_ATI_BASE_URL = 'http://foobar.com?'; | ||
|
||
const PS_NEWS_DESTINATION_ID = 598285; | ||
const PS_NEWS_TEST_DESTINATION_ID = 598286; | ||
const PS_NEWS_GNL_DESTINATION_ID = 598287; | ||
const PS_NEWS_GNL_TEST_DESINTATION_ID = 598288; | ||
const PS_SPORT_DESTINATION_ID = 598310; | ||
const PS_SPORT_TEST_DESTINATION_ID = 598311; | ||
const PS_SPORT_GNL_DESTINATION_ID = 598308; | ||
const PS_SPORT_GNL_TEST_DESTINATION_ID = 598309; | ||
const NEWS_PRODUCER_ID = 64; | ||
const SPORT_PRODUCER_ID = 85; | ||
|
||
it.each` | ||
service | env | destinationId | gnlId | producerId | ||
${'news'} | ${'live'} | ${PS_NEWS_DESTINATION_ID} | ${PS_NEWS_GNL_DESTINATION_ID} | ${NEWS_PRODUCER_ID} | ||
${'news'} | ${'test'} | ${PS_NEWS_TEST_DESTINATION_ID} | ${PS_NEWS_GNL_TEST_DESINTATION_ID} | ${NEWS_PRODUCER_ID} | ||
${'sport'} | ${'live'} | ${PS_SPORT_DESTINATION_ID} | ${PS_SPORT_GNL_DESTINATION_ID} | ${SPORT_PRODUCER_ID} | ||
${'sport'} | ${'test'} | ${PS_SPORT_TEST_DESTINATION_ID} | ${PS_SPORT_GNL_TEST_DESTINATION_ID} | ${SPORT_PRODUCER_ID} | ||
`( | ||
'should create the analytics config with the correct parameters for $service on $env.', | ||
({ env, service, destinationId, gnlId, producerId }) => { | ||
const analyticsConfig = getExperimentAnalyticsConfig({ | ||
env, | ||
service, | ||
atiAnalyticsProducerId: producerId, | ||
}); | ||
expect(analyticsConfig).toMatchInlineSnapshot(` | ||
{ | ||
"requests": { | ||
"topStoriesClick": "http://foobar.com?idclient=123-456-789&s=$IF($EQUALS($MATCH(\${ampGeo}, gbOrUnknown, 0), gbOrUnknown), ${destinationId}, ${gnlId})&s2=${producerId}&p=SOURCE_URL&r=\${screenWidth}x\${screenHeight}x\${screenColorDepth}&re=\${availableScreenWidth}x\${availableScreenHeight}&hl=\${timestamp}&lng=\${browserLanguage}&atc=PUB-[article]-[top-stories-promo]-[topStoriesExperiment:VARIANT(topStoriesExperiment)]-[]-[SOURCE_URL]-[]-[]-[]&type=AT", | ||
"topStoriesView": "http://foobar.com?idclient=123-456-789&s=$IF($EQUALS($MATCH(\${ampGeo}, gbOrUnknown, 0), gbOrUnknown), ${destinationId}, ${gnlId})&s2=${producerId}&p=SOURCE_URL&r=\${screenWidth}x\${screenHeight}x\${screenColorDepth}&re=\${availableScreenWidth}x\${availableScreenHeight}&hl=\${timestamp}&lng=\${browserLanguage}&ati=PUB-[article]-[top-stories-section]-[topStoriesExperiment:VARIANT(topStoriesExperiment)]-[]-[SOURCE_URL]-[]-[]-[]&type=AT", | ||
}, | ||
"triggers": { | ||
"trackTopStoriesClick": { | ||
"on": "click", | ||
"request": "topStoriesClick", | ||
"selector": "a[aria-labelledby*='top-stories-promo']", | ||
}, | ||
"trackTopStoriesDesktopView": { | ||
"on": "visible", | ||
"request": "topStoriesView", | ||
"visibilitySpec": { | ||
"continuousTimeMin": 200, | ||
"selector": "div[data-experiment='position:secondaryColumn'] > section[aria-labelledby='top-stories-heading']", | ||
"totalTimeMin": 500, | ||
"visiblePercentageMin": 20, | ||
}, | ||
}, | ||
"trackTopStoriesView": { | ||
"on": "visible", | ||
"request": "topStoriesView", | ||
"visibilitySpec": { | ||
"continuousTimeMin": 200, | ||
"selector": "div[data-experiment='position:articleBody'] > section[aria-labelledby='top-stories-heading']", | ||
"totalTimeMin": 500, | ||
"visiblePercentageMin": 20, | ||
}, | ||
}, | ||
}, | ||
} | ||
`); | ||
}, | ||
); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confusingly, we use the term
variant
for services with dual scripts e.g. serbian with cyrillic script has a variant = cyrIf this is not the intention of the term variant here, can we please rename it to something else, to avoid confusion - perhaps
experimentVariant
or something similar?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this as well, but had a look at the utility and apparently it was set for A/B testing 3 years ago
simorgh/src/app/lib/analyticsUtils/index.js
Line 272 in 275b924
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've added this here to use as the experiment variant in our use-case because
getEventInfo()
uses this as a variant for A/B testing, it just wasn't previously passed in here.As the value just needs to be in the right place in the string, I'll rename it in there too and remove the comment to make it clearer