-
Notifications
You must be signed in to change notification settings - Fork 280
WS-2946: Sends Resonance page view events for Arabic, Korean and Marathi #14169
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
base: latest
Are you sure you want to change the base?
Changes from all commits
5d05cd2
f9a5b5c
10cbd5e
77aac20
b4607e4
32491dd
bdc3a34
d1867de
8221d62
9e5f8dd
a5db80f
03913fc
b10bd2f
317627e
9a686c9
987d474
e7458f9
54b09e5
0f1c960
f1957a3
b70bd23
a6a3e8c
f7a2bab
8799bf5
51f4b3a
f89c00a
763aea9
287adb3
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 |
|---|---|---|
|
|
@@ -16,13 +16,54 @@ import { | |
| ATIEventTrackingProps, | ||
| ATIPageTrackingProps, | ||
| ReverbBeaconConfig, | ||
| ResonanceBeaconConfig, | ||
| } from '../types'; | ||
|
|
||
| /* | ||
| * For AMP pages, certain browser and device values are determined | ||
| * https://github.com/ampproject/amphtml/blob/master/spec/amp-var-substitutions.md#device-and-browser | ||
| */ | ||
|
|
||
| const RESONANCE_MODE = { LIVE: 'live', TEST: 'test' } as const; | ||
|
|
||
| export const buildResonanceAnalyticsModel = ({ | ||
| appName, | ||
| contentId, | ||
| contentType, | ||
| language, | ||
| statsDestination, | ||
| siteId, | ||
| hashedId, | ||
| pageIdentifier, | ||
| producerName, | ||
| platform, | ||
| }: ATIPageTrackingProps): ResonanceBeaconConfig => { | ||
| const env = getEnvConfig().SIMORGH_APP_ENV; | ||
|
|
||
| return { | ||
| resonanceProperties: { | ||
| mode: env === 'live' ? RESONANCE_MODE.LIVE : RESONANCE_MODE.TEST, | ||
| }, | ||
| baseProperties: { | ||
| app: { | ||
| name: platform === 'app' ? `${appName}-app` : appName, | ||
| }, | ||
| destination: statsDestination, | ||
| hashedUserId: hashedId ?? undefined, | ||
|
Contributor
Author
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. Resonance types permits hashedUserId to be a string or undefined. However reverb permits the equivalent value |
||
| pageName: pageIdentifier, | ||
| producer: producerName, | ||
| siteId, | ||
| }, | ||
| pageviewProperties: { | ||
| contentId, | ||
| contentType, | ||
| language, | ||
| destination: statsDestination, | ||
| producer: producerName, | ||
| }, | ||
| } as ResonanceBeaconConfig; | ||
|
Contributor
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. Do we need the type casting here? I would have expected typescript to infer this ok from the output type defined at the top.
Contributor
Author
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. Hmm no it's a bit unhappy if I remove it. The types in the shared E.g. Reverb permits That The solution would be typing it ourselves, rather than using the imported types (which I did put a question mark on) |
||
| }; | ||
|
|
||
| export const buildReverbAnalyticsModel = ({ | ||
| appName, | ||
| campaigns, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { addSendStaticBeaconToWindow } from '#app/lib/analyticsUtils/staticATITr | |
| import processClientDeviceAndSendStaticBeacon from '#app/lib/analyticsUtils/staticATITracking/processClientDeviceAndSendStaticBeacon'; | ||
| import * as beacon from '../../../lib/analyticsUtils/sendBeacon'; | ||
| import CanonicalATIAnalytics from '.'; | ||
| import { ReverbBeaconConfig } from '../types'; | ||
| import { ResonanceBeaconConfig, ReverbBeaconConfig } from '../types'; | ||
|
|
||
| describe('Canonical ATI Analytics', () => { | ||
| afterEach(() => { | ||
|
|
@@ -53,6 +53,24 @@ describe('Canonical ATI Analytics', () => { | |
| }, | ||
| } as ReverbBeaconConfig; | ||
|
|
||
| const mockResonanceParams = { | ||
| resonanceProperties: { mode: 'test' }, | ||
| baseProperties: { | ||
| app: { name: 'news-pidgin' }, | ||
| destination: 'statsDestination', | ||
| pageName: 'pidgin.articles.c0000000001o.page', | ||
| producer: 'PIDGIN', | ||
| siteId: 12345, | ||
| }, | ||
| pageviewProperties: { | ||
| contentId: 'urn:bbc:optimo:asset:c0000000001o', | ||
| contentType: 'article', | ||
| language: 'pcm', | ||
| destination: 'statsDestination', | ||
| producer: 'PIDGIN', | ||
| }, | ||
| } as ResonanceBeaconConfig; | ||
|
|
||
| const mockSendBeacon = jest.fn().mockReturnValue('beacon-return-value'); | ||
| // @ts-expect-error - we need to mock these functions to ensure tests are deterministic | ||
| beacon.default = mockSendBeacon; | ||
|
|
@@ -125,6 +143,54 @@ describe('Canonical ATI Analytics', () => { | |
| expect(mockSendBeacon).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should call sendBeacon with resonanceParams when provided', () => { | ||
| jest.spyOn(isOperaProxy, 'default').mockImplementation(() => false); | ||
|
|
||
| act(() => { | ||
| render( | ||
| <CanonicalATIAnalytics | ||
| reverbParams={mockReverbParams} | ||
| resonanceParams={mockResonanceParams} | ||
| />, | ||
| ); | ||
| }); | ||
|
|
||
| expect(mockSendBeacon).toHaveBeenCalledWith( | ||
| mockReverbParams, | ||
| mockResonanceParams, | ||
| ); | ||
| }); | ||
|
|
||
| it('should call sendBeacon with undefined resonanceParams when not provided', () => { | ||
| jest.spyOn(isOperaProxy, 'default').mockImplementation(() => false); | ||
|
|
||
| act(() => { | ||
| render( | ||
| <CanonicalATIAnalytics | ||
| reverbParams={mockReverbParams} | ||
| resonanceParams={undefined} | ||
| />, | ||
| ); | ||
| }); | ||
|
|
||
| expect(mockSendBeacon).toHaveBeenCalledWith(mockReverbParams, undefined); | ||
| }); | ||
|
|
||
| it('should call sendBeacon with null resonanceParams when provided as null', () => { | ||
|
Contributor
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. Just want to check this is the desired behaviour. Would it make more sense for this and the case above to always return either null or undefined?
Contributor
Author
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. Yes I agree this looks a bit strange. My logic was that when the page first loads Resonance (and Reverb) is undefined. But once the params are set, they are an object. The different with Resonance is that they can be null if it's not an applicable service or platform. We don't currently test for when ReverbParams are 'undefined'. But they definitely can be. If I had to bin one of the tests, it would be this one. (Note this is typed here as undefined because this is also shared by the AmpATIAnalytics component. I've made a small tweak of the test to improve readability. |
||
| jest.spyOn(isOperaProxy, 'default').mockImplementation(() => false); | ||
|
|
||
| act(() => { | ||
| render( | ||
| <CanonicalATIAnalytics | ||
| reverbParams={mockReverbParams} | ||
| resonanceParams={null} | ||
| />, | ||
| ); | ||
| }); | ||
|
|
||
| expect(mockSendBeacon).toHaveBeenCalledWith(mockReverbParams, null); | ||
| }); | ||
|
|
||
| it('should render a noscript image for non-JS users', () => { | ||
| const { container } = render( | ||
| <CanonicalATIAnalytics reverbParams={mockReverbParams} />, | ||
|
|
||
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.
As we're renaming things eventually to be more generic, it does seem a little strange that this was always called
atiUrlwhen it doesn't return a URL or even a string.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. Totally. Agree. Have added to https://bbc.atlassian.net/browse/WS-3129