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

WSTEAM1-816: Set ckns_mvt cookie on all first time visits #12030

Open
wants to merge 22 commits into
base: latest
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 15 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ Each render is passed through a set of HOC's (Higher Order Components) to enhanc
- withData
- withHashChangeHandler

With a selection of page types passed through withOptimizelyProvider, currently [Article](https://github.com/bbc/simorgh/blob/latest/src/app/pages/ArticlePage/index.jsx) and [Story](https://github.com/bbc/simorgh/blob/latest/src/app/pages/StoryPage/index.jsx) pages.
With a selection of page types passed through withOptimizelyProvider, that enables usage of Optimizely in the selected page types.

#### withVariant

Expand Down Expand Up @@ -97,6 +97,20 @@ The withHashChangeHandler HOC is a wrapper applied to all pages that checks for

The withOptimizelyProvider HOC returns components that have been enhanced with access to an Optimizely client, that is used to run our A/B testing. This is done to limit bundle sizes, as we seperate some of our bundles by page type, that means if we're only running A/B testing on certain page types, we can prevent polluting page type bundles with the weight of the SDK library we use for Optimizely.

withOptimizelyProvider should be added as the value of the `handlerBeforeContexts` object key within [applyBasicPageHandlers.js](https://github.com/bbc/simorgh/tree/latest/src/app/pages/utils/applyBasicPageHandlers.js#L8), as the `ckns_mvt` is [set within the UserContext](https://github.com/bbc/simorgh/tree/latest/src/app/contexts/UserContext/index.tsx#L33), so the `withOptimizelyProvider` HOC needs to be applied in the correct order alongside the [withContexts](https://github.com/bbc/simorgh/tree/latest/src/app/pages/utils/applyBasicPageHandlers.js#L13) HOC. This makes the `ckns_mvt` available on first time visits to pass into the `OptimizelyProvider`, along with attributes such as `service`, which is used for determining when Optimizely should enable an experiment.

Example for Article page:
```jsx
import withOptimizelyProvider from '#app/legacy/containers/PageHandlers/withOptimizelyProvider';
import ArticlePage from './ArticlePage';
import applyBasicPageHandlers from '../utils/applyBasicPageHandlers';

export default applyBasicPageHandlers(ArticlePage, {
handlerBeforeContexts: withOptimizelyProvider,
});

```

### Adding a new Page type

When adding a new page type there are several parts required.
Expand Down
2 changes: 1 addition & 1 deletion src/app/components/ATIAnalytics/canonical/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('Canonical ATI Analytics', () => {
});

it('should not send beacon when browser is Opera Mini', () => {
jest.spyOn(isOperaProxy, 'default').mockImplementationOnce(() => true);
jest.spyOn(isOperaProxy, 'default').mockImplementation(() => true);

act(() => {
render(<CanonicalATIAnalytics pageviewParams={mockPageviewParams} />);
Expand Down
72 changes: 72 additions & 0 deletions src/app/contexts/UserContext/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import React, { useContext } from 'react';
import { render } from '@testing-library/react';
import Cookie from 'js-cookie';
import * as onClient from '#app/lib/utilities/onClient';
import * as isOperaProxy from '#app/lib/utilities/isOperaProxy';
import { UserContext, UserContextProvider } from '.';
import { getCookiePolicy, personalisationEnabled } from './cookies';
import * as chartbeat from './Chartbeat';
Expand Down Expand Up @@ -61,4 +64,73 @@ describe('UserContext', () => {
{},
);
});
describe('ckns_mvt cookie', () => {
const cookieSetterSpy = jest.spyOn(Cookie, 'set');
const cookieGetterSpy = jest.spyOn(Cookie, 'get');
const isOperaProxySpy = jest.spyOn(isOperaProxy, 'default');
const onClientSpy = jest.spyOn(onClient, 'default');

beforeEach(() => {
jest.clearAllMocks();
Cookie.remove('ckns_mvt');
});

it('should call cookie logic when not opera mini and is on client', () => {
onClientSpy.mockImplementationOnce(() => true as unknown as Location);
isOperaProxySpy.mockImplementationOnce(() => false);

render(<DummyComponentWithContext />);

expect(cookieGetterSpy).toHaveBeenCalled();
});

it('should not call cookie logic when on opera mini and is on client', () => {
onClientSpy.mockImplementationOnce(() => true as unknown as Location);
isOperaProxySpy.mockImplementationOnce(() => true);

render(<DummyComponentWithContext />);

expect(cookieGetterSpy).not.toHaveBeenCalled();
});

it('should not call cookie logic when not on opera mini and is not on client', () => {
onClientSpy.mockImplementationOnce(() => false);
isOperaProxySpy.mockImplementationOnce(() => false);

render(<DummyComponentWithContext />);

expect(cookieGetterSpy).not.toHaveBeenCalled();
});

it('should not set cookie when ckns_mvt cookie already exists', () => {
onClientSpy.mockImplementationOnce(() => true as unknown as Location);
isOperaProxySpy.mockImplementationOnce(() => false);
Cookie.set('ckns_mvt', 'foo');
cookieSetterSpy.mockClear();

render(<DummyComponentWithContext />);

expect(cookieGetterSpy).toHaveReturnedWith('foo');
expect(cookieSetterSpy).not.toHaveBeenCalled();
});

it('should set cookie when no ckns_mvt cookie exists', () => {
const uuidRegex =
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[089ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
onClientSpy.mockImplementationOnce(() => true as unknown as Location);
isOperaProxySpy.mockImplementationOnce(() => false);
// @ts-expect-error This should be able to be mocked as a string or undefined
cookieGetterSpy.mockImplementationOnce(() => undefined);

render(<DummyComponentWithContext />);

const [[cookieName, cookieValue, cookieOptions]] =
cookieSetterSpy.mock.calls;

expect(cookieValue).toMatch(uuidRegex);
expect(cookieName).toEqual('ckns_mvt');
expect(cookieOptions).toEqual({ expires: 365, path: '/', secure: true });
expect(cookieSetterSpy).toHaveBeenCalledTimes(1);
});
});
});
19 changes: 19 additions & 0 deletions src/app/contexts/UserContext/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import React, {
SetStateAction,
useMemo,
} from 'react';
import { v4 as uuid } from 'uuid';
import Cookie from 'js-cookie';
import onClient from '#app/lib/utilities/onClient';
import isOperaProxy from '#app/lib/utilities/isOperaProxy';
import { getCookiePolicy, personalisationEnabled } from './cookies';
import Chartbeat from './Chartbeat';

Expand All @@ -19,10 +23,25 @@ export const UserContext = React.createContext<UserContextProps>(
{} as UserContextProps,
);

const cknsMvtCookie = () => {
const cookieName = 'ckns_mvt';
const cookieValue = Cookie.get(cookieName);

if (!cookieValue) {
const cookieUuid = uuid();
const expires = 365;
Cookie.set(cookieName, cookieUuid, { expires, path: '/', secure: true });
}
};

export const UserContextProvider = ({ children }: PropsWithChildren) => {
const [cookiePolicy, setCookiePolicy] = useState(getCookiePolicy());
const [chartbeatConfig, sendCanonicalChartbeatBeacon] = useState(null);

if (onClient() && !isOperaProxy()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean that we dont set the cookie for opera mini (based of this and the unit tests)? can you remind me why if so! assuming that we just dont run experiments on opera mini for a weird opera reason

Copy link
Contributor Author

@HarveyPeachey HarveyPeachey Oct 7, 2024

Choose a reason for hiding this comment

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

Actually debated wether to set this on opera mini or not. The reason is because Opera mini extreme mode renders its' pages for the user on their servers, so that means all users using Opera Mini Extreme mode would have the same userId, making testing redundant. Although technically we can set it here as this effectively disables running optimizely for opera mini users anyway, I'll ask product and @andrewscfc to see what they think

cknsMvtCookie();
}

const value = useMemo(
() => ({
cookiePolicy,
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import isLive from '#lib/utilities/isLive';
import onClient from '#lib/utilities/onClient';
import { GEL_GROUP_3_SCREEN_WIDTH_MAX } from '#psammead/gel-foundations/src/breakpoints';
import { getEnvConfig } from '#app/lib/utilities/getEnvConfig';
import Cookie from 'js-cookie';
import isOperaProxy from '#app/lib/utilities/isOperaProxy';
import { ServiceContext } from '../../../../contexts/ServiceContext';
import getOptimizelyUserId from './getOptimizelyUserId';

// 004_brasil_recommendations_experiment
const isCypress = onClient() && window.Cypress;
Expand All @@ -32,10 +33,10 @@ const withOptimizelyProvider = Component => {
let mobile;

const getUserId = () => {
if (disableOptimizely) {
if (disableOptimizely || !onClient() || isOperaProxy()) {
return null;
}
return getOptimizelyUserId();
return Cookie.get('ckns_mvt') ?? null;
};

if (onClient()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import React, { useMemo } from 'react';
import * as optimizelyReactSdk from '@optimizely/react-sdk';
import { render } from '@testing-library/react';
import Cookie from 'js-cookie';
import latin from '../../../../components/ThemeProvider/fontScripts/latin';
import { ServiceContext } from '../../../../contexts/ServiceContext';
import withOptimizelyProvider from '.';

const optimizelyProviderSpy = jest.spyOn(
optimizelyReactSdk.OptimizelyProvider.prototype,
'render',
);

const props = {
bbcOrigin: 'https://www.bbc.com',
id: 'c0000000000o',
Expand Down Expand Up @@ -44,8 +40,32 @@ const TestComponent = () => {

describe('withOptimizelyProvider HOC', () => {
it('should enrich the component with the Optimizely API', () => {
const optimizelyProviderRenderSpy = jest.spyOn(
optimizelyReactSdk.OptimizelyProvider.prototype,
'render',
);

render(<TestComponent />);

expect(optimizelyProviderRenderSpy).toHaveBeenCalledTimes(1);
});

it('should return undefined when ckns_mvt is fetched with Cookie.get', () => {
const cookieGetterSpy = jest.spyOn(Cookie, 'get');

render(<TestComponent />);

expect(cookieGetterSpy).toHaveBeenCalledWith('ckns_mvt');
expect(cookieGetterSpy).toHaveReturnedWith(undefined);
});

it('should return the correct ckns_mvt cookie value from Cookie.get', () => {
const cookieGetterSpy = jest.spyOn(Cookie, 'get');
Cookie.set('ckns_mvt', 'random-uuid');

render(<TestComponent />);

expect(optimizelyProviderSpy).toHaveBeenCalledTimes(1);
expect(cookieGetterSpy).toHaveBeenCalledWith('ckns_mvt');
expect(cookieGetterSpy).toHaveReturnedWith('random-uuid');
});
});
6 changes: 5 additions & 1 deletion src/app/pages/utils/applyBasicPageHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@ import withError from '#containers/PageHandlers/withError';
import withData from '#containers/PageHandlers/withData';
import withHashChangeHandler from '#containers/PageHandlers/withHashChangeHandler';

export default component =>
export default (
component,
{ handlerBeforeContexts = Component => Component } = {},
) =>
pipe(
withData,
withError,
withPageWrapper,
handlerBeforeContexts,
withContexts,
withHashChangeHandler,
)(component);
38 changes: 38 additions & 0 deletions src/app/pages/utils/applyBasicPageHandlers.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import * as pipe from 'ramda/src/pipe';
import WithContexts from '#app/legacy/containers/PageHandlers/withContexts';
import applyBasicPageHandlers from './applyBasicPageHandlers';

jest.mock('ramda/src/pipe', () => {
const originalModule = jest.requireActual('ramda/src/pipe');

return {
__esModule: true,
...originalModule,
default: originalModule,
};
});

describe('applyBasicPageHandlers', () => {
beforeEach(() => {
jest.clearAllMocks();
jest.resetModules();
});

it('should call pipe with function as the argument before the withContexts argument when passed via handlerBeforeContexts', () => {
const component = jest.fn();
const mockBeforeContextsFunction = jest.fn();
const pipeMock = jest.spyOn(pipe, 'default');

applyBasicPageHandlers(component, {
handlerBeforeContexts: mockBeforeContextsFunction,
});

const args = pipeMock.mock.calls[0];
const beforeContextsFunctionArg = args[3];
const WithContextsFunctionArg = args[4];

expect(beforeContextsFunctionArg).toEqual(mockBeforeContextsFunction);
expect(WithContextsFunctionArg).toEqual(WithContexts);
expect(mockBeforeContextsFunction).toHaveBeenCalledTimes(1);
});
});
Loading