Add data-fronts-test-uuid to card links with an active headline test - #16723
Conversation
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
🚀 Image pushed to AWS ECRImage digest: 🐛 Run the image locallyThe following can be used to run the image locally: # Refer to image using the immutable digest. Find alternatives below.
IMAGE_IDENTIFIER="@sha256:7585a21aa676e4baaa7bddd2a4f8afc84de7744e2eab99f59046e40d015cc33d"
# Refer to image using branch tag
# IMAGE_IDENTIFIER=":branch-ei-add-data-fronts-test-uuid"
# Refer to image using build tag
# IMAGE_IDENTIFIER=":build-31143"
# Set environment variables for the AWS CLI
AWS_PROFILE="<A_PROFILE_FROM_JANUS>"
AWS_DEFAULT_REGION="eu-west-1"
IMAGE_ACCOUNT_ID=$(aws ssm get-parameter --name /organisation/accounts/artifacts --query "Parameter.Value" --output text)
REGISTRY="${IMAGE_ACCOUNT_ID}.dkr.ecr.${AWS_DEFAULT_REGION}.amazonaws.com"
IMAGE="${REGISTRY}/guardian/dotcom-rendering${IMAGE_IDENTIFIER}"
# Login to AWS ECR https://docs.aws.amazon.com/AmazonECR/latest/userguide/registry_auth.html
aws ecr get-login-password | docker login --username AWS --password-stdin $REGISTRY
# Pull the image
docker pull $IMAGE
# Run the image. You'll likely need to set additional flags. See https://docs.docker.com/reference/cli/docker/container/run.
docker run $IMAGE |
| return tests?.find((test) => isActiveEditorialTest(test)); | ||
| }; | ||
|
|
||
| type EditorialTestData = { |
There was a problem hiding this comment.
I'd wondered if, given that article editorial testing will soon be realised, if it would be worthwhile having headline orfront in this type to try and minimise confusion but as this is scoped to the card enhancer, I don't think this is necessary.
There was a problem hiding this comment.
Yes I totally agree - I think we should also look at renaming the EditorialTest type in front.ts. Because that renaming would affect other files and create a bigger diff, I think we could do that in a follow up PR to this one, to keep this scoped to the Ophan tracking change. Does that sound ok to you?
There was a problem hiding this comment.
Yep that sounds great, thanks!
github-actions
Bot
removed
the
run_chromatic
| /** | ||
| * Decide the headline to be shown for a given card. If there is an active editorial test on a card, | ||
| * return the variant headline matching the user test group. Otherwise, return the default headline | ||
| */ | ||
| export const decideHeadline = ( | ||
| faciaCard: FEFrontCard | FESupportingContent, | ||
| serverSideABTests: Record<string, string>, | ||
| isEditorialABTestingEnabled: boolean, | ||
| pageId?: string, | ||
| ): string => { | ||
| const editorialTestData = getEditorialTestData( | ||
| faciaCard, | ||
| serverSideABTests, | ||
| isEditorialABTestingEnabled, | ||
| pageId, | ||
| ); | ||
|
|
||
| if (isUndefined(editorialTestData)) return faciaCard.header.headline; | ||
|
|
||
| return editorialTestData.headline; | ||
| }; | ||
|
|
||
| /** | ||
| * Find the UUID of a headline test if one is running on the card | ||
| */ | ||
| export const findHeadlineTestUuid = ( | ||
| faciaCard: FEFrontCard | FESupportingContent, | ||
| serverSideABTests: Record<string, string>, | ||
| isEditorialABTestingEnabled: boolean, | ||
| pageId?: string, | ||
| ): string | undefined => { | ||
| const editorialTestData = getEditorialTestData( | ||
| faciaCard, | ||
| serverSideABTests, | ||
| isEditorialABTestingEnabled, | ||
| pageId, | ||
| ); | ||
|
|
||
| if (isUndefined(editorialTestData)) return undefined; | ||
|
|
||
| return variantMeta.meta.headline; | ||
| return editorialTestData.uuid; |
There was a problem hiding this comment.
This is a nice refactor! thanks ✨
There was a problem hiding this comment.
Thanks for updating these
|
Seen on PROD (merged by @emma-imber 10 minutes and 15 seconds ago) Please check your changes! |
2 participants
What does this change?
Adds the
data-fronts-test-uuidattribute to links on cards with an active headline test. This follows on from the corresponding Ophan change, which was brought in in this PR.As part of this change, I've refactored the
decideHeadlinefunction inenhanceCardsso that we can share logic with the newfindHeadlineTestUuidfunction. This allows us to unit test the logic for both headlines and UUIDs together, instead duplicating test definitions in the testing file. It also means we can be confident that we only add this tracking parameter to cards that are actually showing a test variant headline.Why?
We need to add this parameter to allow Ophan to track data for the headline tests. By passing this data in, Ophan can distinguish pageviews from different headline test variants to allow us to attribute the data accurately.
How has this change been tested?
The existing unit tests have been expanded to also test for the correct headline test UUID value being returned, in addition to the corresponding variant headline.
Manual testing
I've run this locally and have tested against a code front with many types of containers, to make sure Feature cards, Highlight cards, Front cards & sublinks all have a
data-fronts-test-uuidattribute added when a headline test is running.I also manually tested that Ophan correctly picked up the test ID when clicking one of these test links.