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

HarveyPeachey · 2024-10-04T11:02:59Z

Resolves JIRA WSTEAM1-816

Overall changes

Moves the logic for setting the ckns_mvt cookie to the user context so it's set everywhere in our application.

Code changes

Refactors applyBasicPageHandlers to cater for the order of the Optmizely provider needing the WithContexts HOC to be be cakked ub the right order, so ckns_mvt is guaranteed to be set before being passed to the Optmizely client.
Moves logic from withOptimizelyProvider for setting the ckns_mvt cookie.
Updates readme with new way to apply the withOptimizelyProvider HOC

Testing

Check the cookie is being set properly everywhere

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

…816-set-ckns-mvt-all-first-visits

emilysaffron · 2024-10-07T14:53:39Z

src/app/contexts/UserContext/index.tsx

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

+  if (onClient() && !isOperaProxy()) {


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

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

karinathomasbbc · 2024-10-07T15:31:35Z

README.md

@@ -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


Could we reword this slightly as it is a wee bit confusing?

This should be used by using the handlerBeforeContexts object key within [applyBasicPageHandlers.js]

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?

Yeah I can make it more succint

karinathomasbbc · 2024-10-07T15:32:27Z

README.md

@@ -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


Suggested change

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

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 available on first time visits and set as the Optimizely User Id

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?

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

README.md

karinathomasbbc · 2024-10-07T15:36:41Z

src/app/contexts/UserContext/index.test.tsx

+      expect(cookieGetterSpy).not.toHaveBeenCalled();
+    });
+
+    it('should not set cookie when ckns_mvt cookie exists', () => {


Suggested change

it('should not set cookie when ckns_mvt cookie exists', () => {

it('should not set cookie when ckns_mvt cookie already exists', () => {

I am not seeing any references to the ckns_mvt in this test - do we need to have an explicit mock for this (rather than just foo?) e.g. we did a form of cookie mocking here:

simorgh/src/app/lib/utilities/setCookie/index.test.jsx

Line 17 in 52633ff

setCookie({ name: 'test', value: '111' });

wonder if this might be helpful i.e. allow us to pretend that ckns_mvt is already set?

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

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

karinathomasbbc

Thanks for making the doc & code changes!

adjusts withOptimizelyProvider usage

00f9388

HarveyPeachey self-assigned this Oct 4, 2024

HarveyPeachey added 3 commits October 7, 2024 10:18

adds extra test case

219fd9d

adds check to have been called once

6206a1e

moves cookie ckns_mvt setter logic to user context

598104c

HarveyPeachey changed the title ~~adjusts withOptimizelyProvider usage~~ WSTEAM1-816: Set ckns_mvt cookie on all first time visits Oct 7, 2024

HarveyPeachey added 6 commits October 7, 2024 14:08

fixes order of userId check

8d7697e

adjust applyBasicPageHandlers api

d559889

Merge branch 'latest' of https://github.com/bbc/simorgh into WSTEAM1-…

25c8e56

…816-set-ckns-mvt-all-first-visits

updates readme

f3aafd4

removes test comments

119e7ad

readme updates

4e2f19c

HarveyPeachey marked this pull request as ready for review October 7, 2024 13:44

HarveyPeachey added 4 commits October 7, 2024 14:50

removes console log

e7d13b5

rearranges pipe

03e761e

updates readme

6cc12a1

updates links

1e34a11

emilysaffron approved these changes Oct 7, 2024

View reviewed changes

updates failing test

84ab860

karinathomasbbc reviewed Oct 7, 2024

View reviewed changes

HarveyPeachey and others added 6 commits October 8, 2024 08:30

Update README.md

0999ce6

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

Update README.md

e528410

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

Update src/app/contexts/UserContext/index.test.tsx

98aaa6a

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

updates test to be more explicit with ckns_mvt

624087d

updates readme for added clarity

7a392d5

more readme updates

041beb7

HarveyPeachey requested a review from karinathomasbbc October 8, 2024 07:55

removes optimizely from article pages as no testing is happening

9fc0d50

karinathomasbbc approved these changes Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HarveyPeachey commented Oct 4, 2024 •

edited

Loading

emilysaffron Oct 7, 2024

HarveyPeachey Oct 7, 2024 •

edited

Loading

karinathomasbbc Oct 7, 2024 •

edited

Loading

HarveyPeachey Oct 8, 2024

karinathomasbbc Oct 7, 2024

karinathomasbbc Oct 7, 2024

HarveyPeachey Oct 8, 2024 •

edited

Loading

karinathomasbbc Oct 7, 2024

karinathomasbbc Oct 7, 2024

HarveyPeachey Oct 8, 2024 •

edited

Loading

karinathomasbbc left a comment

		@@ -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

	it('should not set cookie when ckns_mvt cookie exists', () => {
	it('should not set cookie when ckns_mvt cookie already exists', () => {

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

Are you sure you want to change the base?

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

Conversation

HarveyPeachey commented Oct 4, 2024 • edited Loading

Overall changes

Code changes

Testing

Helpful Links

emilysaffron Oct 7, 2024

Choose a reason for hiding this comment

HarveyPeachey Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

karinathomasbbc Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

HarveyPeachey Oct 8, 2024

Choose a reason for hiding this comment

karinathomasbbc Oct 7, 2024

Choose a reason for hiding this comment

karinathomasbbc Oct 7, 2024

Choose a reason for hiding this comment

HarveyPeachey Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

karinathomasbbc Oct 7, 2024

Choose a reason for hiding this comment

karinathomasbbc Oct 7, 2024

Choose a reason for hiding this comment

HarveyPeachey Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

karinathomasbbc left a comment

Choose a reason for hiding this comment

HarveyPeachey commented Oct 4, 2024 •

edited

Loading

HarveyPeachey Oct 7, 2024 •

edited

Loading

karinathomasbbc Oct 7, 2024 •

edited

Loading

HarveyPeachey Oct 8, 2024 •

edited

Loading

HarveyPeachey Oct 8, 2024 •

edited

Loading