-
Notifications
You must be signed in to change notification settings - Fork 222
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
base: latest
Are you sure you want to change the base?
Changes from 15 commits
00f9388
219fd9d
6206a1e
598104c
8d7697e
d559889
25c8e56
f3aafd4
119e7ad
4e2f19c
e7d13b5
03e761e
6cc12a1
1e34a11
84ab860
0999ce6
e528410
98aaa6a
624087d
7a392d5
041beb7
9fc0d50
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 | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
||||||
|
@@ -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. | ||||||
|
||||||
This should be used by using 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 [withContexts](https://github.com/bbc/simorgh/tree/latest/src/app/pages/utils/applyBasicPageHandlers.js#L13) HOC, so the `ckns_mvt` is availible to be used on first time visits to be used as the Optimizely User Id | ||||||
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.
Suggested change
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. A more general question - should we use links to the latest version of the code, or a fixed commit ID? Especially once the functionality changes, and the latest code no longer reflects the docs? 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. Yeah I deliberated on this, either way if the code changes both would become out of date, I thought the latest branch would be better as hopefully a reader can see it's changed based on the context of the readme, but if we link to a fixed commit ID, the code could be completely different from what is actually in the codebase. Happy to change this to a fixed commit ID as I'm not fussed |
||||||
|
||||||
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, { | ||||||
lastHandler: withOptimizelyProvider, | ||||||
HarveyPeachey marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}); | ||||||
|
||||||
``` | ||||||
|
||||||
### Adding a new Page type | ||||||
|
||||||
When adding a new page type there are several parts required. | ||||||
|
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'; | ||||||||
|
@@ -61,4 +64,71 @@ 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(); | ||||||||
}); | ||||||||
|
||||||||
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 exists', () => { | ||||||||
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.
Suggested change
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 am not seeing any references to the
ckns_mvt is already set?
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. Yeah I took this approach as we're only really bothered if Cookie.set is called, but I agree it does make it more clear if ckns_mvt is set properly. I've updated the test to reflect this, clearing the setter mock before we render the user context so it doesn't mess with the expect |
||||||||
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(() => 'foo'); | ||||||||
|
||||||||
render(<DummyComponentWithContext />); | ||||||||
|
||||||||
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); | ||||||||
}); | ||||||||
}); | ||||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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()) { | ||
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. 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 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. 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, | ||
|
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
import withOptimizelyProvider from '#app/legacy/containers/PageHandlers/withOptimizelyProvider'; | ||
import ArticlePage from './ArticlePage'; | ||
import applyBasicPageHandlers from '../utils/applyBasicPageHandlers'; | ||
|
||
export default applyBasicPageHandlers(ArticlePage); | ||
export default applyBasicPageHandlers(ArticlePage, { | ||
handlerBeforeContexts: withOptimizelyProvider, | ||
}); |
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); | ||
}); | ||
}); |
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.
Could we reword this slightly as it is a wee bit confusing?
What should be used? We could repeat whatever it is for clarity.
Could we also avoid used & using twice in the same sentence if possible?
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.
Yeah I can make it more succint