Merge remote-tracking branch 'origin/main' into jlk/ab-testing-deploy-lambda

Jakeii · Jakeii · commit 7dd7c46a4120 · 2025-11-20T14:13:13.000Z
diff --git a/.github/workflows/ab-testing-lambda-ci.yml b/.github/workflows/ab-testing-lambda-ci.yml
@@ -81,9 +81,9 @@ jobs:
           roleArn: ${{ secrets.GU_RIFF_RAFF_ROLE_ARN }}
           githubToken: ${{ secrets.GITHUB_TOKEN }}
           projectName: dotcom:ab-testing-deployment-lambda
-          configPath: ab-testing/cd/cdk.out/deployment-lambda/riff-raff.yaml
+          configPath: ab-testing/cdk/cdk.out/deployment-lambda/riff-raff.yaml
           contentDirectories: |
             ab-testing-deployment-lambda:
               - ab-testing/deploy-lambda/dist/lambda.zip
-            cdk.out/cdk/deployment-lambda:
-              - ab-testing/cdk.out/deployment-lambda
+            cdk.out/deployment-lambda:
+              - ab-testing/cdk/cdk.out/deployment-lambda
diff --git a/ab-testing/config/abTests.ts b/ab-testing/config/abTests.ts
@@ -25,9 +25,10 @@ const ABTests: ABTest[] = [
 		description: "Testing Prebid.js v10 integration on DCR",
 		owners: ["commercial.dev@guardian.co.uk"],
 		status: "ON",
-		expirationDate: "2025-12-30",
+		expirationDate: "2025-12-10",
 		type: "client",
-		audienceSize: 0 / 100,
+		audienceSize: 10 / 100,
+		audienceSpace: "A",
 		groups: ["control", "variant"],
 		shouldForceMetricsCollection: true,
 	},
@@ -44,6 +45,18 @@ const ABTests: ABTest[] = [
 		groups: ["control", "variant"],
 		shouldForceMetricsCollection: true,
 	},
+	{
+		name: "thefilter-product-element",
+		description:
+			"A hold back test to measure uplift of the product element",
+		owners: ["thefilter.dev@guardian.co.uk"],
+		status: "ON",
+		expirationDate: "2025-12-30",
+		type: "server",
+		audienceSize: 0 / 100,
+		groups: ["control", "variant"],
+		shouldForceMetricsCollection: false,
+	},
 ];
 
 const activeABtests = ABTests.filter((test) => test.status === "ON");
diff --git a/ab-testing/config/tsconfig.json b/ab-testing/config/tsconfig.json
@@ -1,3 +1,11 @@
 {
-	"extends": "../tsconfig.json",
+	"extends": "@guardian/tsconfig/tsconfig.json",
+	"compilerOptions": {
+		"noEmit": true,
+		"skipLibCheck": true,
+		"allowImportingTsExtensions": true,
+		"moduleResolution": "nodenext",
+		"module": "nodenext"
+	},
+	"exclude": ["node_modules", "index.ts"]
 }
diff --git a/ab-testing/config/types.ts b/ab-testing/config/types.ts
@@ -4,7 +4,7 @@ type AudienceSpace = Map<string, FastlyTestParams>;
 
 type AllSpace = Map<string, FastlyTestParams[]>;
 
-type Team = "commercial" | "webex";
+type Team = "commercial" | "webex" | "thefilter";
 
 type TestName = `${Team}-${string}`;
 
diff --git a/ab-testing/deploy-lambda/package.json b/ab-testing/deploy-lambda/package.json
@@ -13,24 +13,25 @@
 	},
 	"dependencies": {
 		"@aws-sdk/client-s3": "3.931.0",
+		"@aws-sdk/client-ssm": "3.621.0",
 		"superstruct": "2.0.2"
 	},
 	"devDependencies": {
 		"@guardian/cdk": "62.0.1",
 		"@guardian/eslint-config": "12.0.1",
 		"@guardian/tsconfig": "1.0.1",
 		"@rollup/plugin-commonjs": "29.0.0",
-		"@rollup/plugin-node-resolve": "16.0.3",
 		"@rollup/plugin-json": "6.1.0",
+		"@rollup/plugin-node-resolve": "16.0.3",
 		"@types/aws-lambda": "8.10.158",
 		"@types/node": "22.17.0",
 		"aws-cdk-lib": "2.220.0",
 		"aws-lambda": "1.0.7",
+		"esbuild": "0.27.0",
 		"eslint": "9.39.1",
 		"prettier": "3.0.3",
 		"rollup": "4.53.2",
-		"typescript": "5.9.3",
-		"esbuild": "0.27.0",
-		"rollup-plugin-esbuild": "6.2.1"
+		"rollup-plugin-esbuild": "6.2.1",
+		"typescript": "5.9.3"
 	}
 }
diff --git a/ab-testing/frontend/src/lib/components/OphanLink.svelte b/ab-testing/frontend/src/lib/components/OphanLink.svelte
@@ -8,6 +8,7 @@
 
 <a
 	href={`https://dashboard.ophan.co.uk/graph/breakdown?day=today&ab=${testName}`}
+	target="_blank"
 >
 	graph
 </a>
diff --git a/ab-testing/tsconfig.json b/ab-testing/tsconfig.json
diff --git a/dotcom-rendering/docs/development/ab-testing-in-dcr.md b/dotcom-rendering/docs/development/ab-testing-in-dcr.md
@@ -119,7 +119,7 @@ When your PR is merged, the A/B test will be automatically deployed to Fastly an
 
 #### Naming Conventions
 
-A/B tests should be prefixed with the team associated with the test, for example `webex-example-test`. This helps to identify the team responsible for the test and is enforce by typescript validation, you can inspect & edit the allowed team name definitions [here](https://github.com/guardian/dotcom-rendering/blob/main/ab-testing/types.ts#L9).
+A/B tests should be prefixed with the team associated with the test, for example `webex-example-test`. This helps to identify the team responsible for the test and is enforced by typescript validation, you can inspect & edit the allowed team name definitions [here](https://github.com/guardian/dotcom-rendering/blob/main/ab-testing/types.ts#L9).
 
 #### Test Size and Groups
 
@@ -135,7 +135,7 @@ All requests are processed by Fastly at the edge, however, A/B testing of server
 
 Ensure that the `type` field is set to either `client` or `server` to indicate the type of test so that server side tests can be cached correctly, and client side tests are not splitting the cache unnecessarily.
 
-There's a limit of the number of concurrent server-side tests that can be run, enforce by the validation script, so it's important to use client-side tests where possible.
+There's a limit of the number of concurrent server-side tests that can be run, enforced by the validation script, so it's important to use client-side tests where possible.
 
 #### Test Expiration
 
@@ -178,13 +178,13 @@ const someComponent = () => {
 	const abTests = useBetaAB();
 
 	// Am I in the test at all?
-	const isInTest = abTests?.isUserInTest('AbTestTest') ?? false;
+	const isInTest = abTests?.isUserInTest('webex-example-test') ?? false;
 
 	const isInControlGroup =
-		(abTests?.isUserInTestGroup('AbTestTest', 'control') ?? false);
+		(abTests?.isUserInTestGroup('webex-example-test', 'control') ?? false);
 
 	const isInVariantGroup =
-	abTests?.isUserInTestGroup('AbTestTest', 'variant') ?? false;
+	abTests?.isUserInTestGroup('webex-example-test', 'variant') ?? false;
 
 	if (isInControlGroup) {
 		return (
@@ -223,16 +223,16 @@ const abTests = useBetaAB();
 // Get all of the user's server/client-side A/B test participations
 const abTestParticipations = abTests?.getParticipations(); // EG. { commercial-dev-client-side-test: 'variant', commercial-dev-server-side-test: 'variant' }
 
-// Is user in the AbTestTest test (any cohort)
-const isInTest = abTests?.isUserInTest('AbTestTest') ?? false;
+// Is user in the webex-example-test test (any cohort)
+const isInTest = abTests?.isUserInTest('webex-example-test') ?? false;
 
-// Is user in the AbTestTest test (control cohort)
+// Is user in the webex-example-test test (control cohort)
 const isInControlGroup =
-	abTests?.isUserInTestGroup('AbTestTest', 'control') ?? false;
+	abTests?.isUserInTestGroup('webex-example-test', 'control') ?? false;
 
-// Is user in the AbTestTest test (variant cohort)
+// Is user in the webex-example-test test (variant cohort)
 const isInVariantGroup =
-	abTests?.isUserInTestGroup('AbTestTest', 'variant') ?? false;
+	abTests?.isUserInTestGroup('webex-example-test', 'variant') ?? false;
 ```
 
 #### On the Client
diff --git a/dotcom-rendering/package.json b/dotcom-rendering/package.json
@@ -38,7 +38,7 @@
 		"@guardian/identity-auth": "6.0.1",
 		"@guardian/identity-auth-frontend": "8.1.0",
 		"@guardian/libs": "26.1.0",
-		"@guardian/ophan-tracker-js": "2.6.2",
+		"@guardian/ophan-tracker-js": "2.6.3",
 		"@guardian/react-crossword": "11.1.0",
 		"@guardian/shimport": "1.0.2",
 		"@guardian/source": "11.3.0",
diff --git a/dotcom-rendering/src/components/ProductCardButtons.tsx b/dotcom-rendering/src/components/ProductCardButtons.tsx
@@ -24,9 +24,11 @@ export const ProductCardButtons = ({
 					minimisePadding={true}
 					priority={index === 0 ? 'primary' : 'tertiary'}
 					fullwidth={true}
-					data-component={`${
-						dataComponent ?? 'product-card-button'
-					}-${index}`}
+					dataComponent={
+						dataComponent
+							? `${dataComponent}-${index + 1}`
+							: undefined
+					}
 				/>
 			);
 		})}
diff --git a/dotcom-rendering/src/components/ProductCardImage.tsx b/dotcom-rendering/src/components/ProductCardImage.tsx
@@ -39,6 +39,7 @@ export const ProductCardImage = ({
 					href={url}
 					target="_blank"
 					rel="noopener noreferrer"
+					data-link-name="product image link"
 					// this is needed to override global style
 					// html:not(.src-focus-disabled) *:focus
 					// it has specificity(0, 2, 1) so we need (0, 3, 0)
diff --git a/dotcom-rendering/src/components/ProductCardInline.tsx b/dotcom-rendering/src/components/ProductCardInline.tsx
@@ -182,6 +182,7 @@ export const ProductCardInline = ({
 
 	return (
 		<div
+			data-component="product-card-inline"
 			css={[
 				isCardOnly ? productCard : showcaseCard,
 				shouldShowLeftColCard && !isCardOnly && hideFromWide,
@@ -213,7 +214,7 @@ export const ProductCardInline = ({
 			<div css={buttonWrapper}>
 				<ProductCardButtons
 					productCtas={productCtas}
-					dataComponent={'inline-product-card-buttons'}
+					dataComponent={'inline-product-card-button'}
 				/>
 			</div>
 			{hasCustomAttributes && (
diff --git a/dotcom-rendering/src/components/ProductCardLeftCol.tsx b/dotcom-rendering/src/components/ProductCardLeftCol.tsx
@@ -66,7 +66,7 @@ const buttonContainer = css`
 	padding-bottom: ${space[6]}px;
 	min-width: 100%;
 	display: grid;
-	row-gap: ${space[1]}px;
+	row-gap: ${space[3]}px;
 `;
 const customAttributesContainer = css`
 	border-top: 1px solid ${palette('--section-border')};
@@ -97,7 +97,7 @@ export const ProductCardLeftCol = ({
 	lowestPrice,
 }: LeftColProductCardProps) => {
 	return (
-		<div css={card}>
+		<div data-component="product-card-left-col" css={card}>
 			<ProductCardImage
 				format={format}
 				image={image}
@@ -121,7 +121,7 @@ export const ProductCardLeftCol = ({
 			<div css={buttonContainer}>
 				<ProductCardButtons
 					productCtas={productCtas}
-					dataComponent={'left-col-product-card-buttons'}
+					dataComponent={'left-col-product-card-button'}
 				/>
 			</div>
 			{customAttributes.length > 0 && (
diff --git a/dotcom-rendering/src/components/ProductElement.tsx b/dotcom-rendering/src/components/ProductElement.tsx
@@ -4,6 +4,7 @@ import type { ReactNode } from 'react';
 import type { ArticleFormat } from '../lib/articleFormat';
 import { parseHtml } from '../lib/domUtils';
 import type { NestedArticleElement } from '../lib/renderElement';
+import { useBetaAB } from '../lib/useAB';
 import type { ProductBlockElement } from '../types/content';
 import { ProductCardInline } from './ProductCardInline';
 import { ProductCardLeftCol } from './ProductCardLeftCol';
@@ -39,20 +40,32 @@ export const ProductElement = ({
 	format: ArticleFormat;
 	shouldShowLeftColCard: boolean;
 }) => {
+	const abTests = useBetaAB();
+	const isInHoldBackTestVariant =
+		abTests?.isUserInTestGroup('thefilter-product-element', 'variant') ??
+		false;
+
 	const showContent =
 		product.displayType === 'InlineOnly' ||
 		product.displayType === 'InlineWithProductCard';
+	// In the hold back test variant, if the product element has a display type of ProductCardOnly,
+	// we should still render the product card. This is because there may not be any suitable
+	// nested content to render, which could result in some unintended display issues. For the
+	// InlineWithProductCard display type, we won't render the cards in the hold back test.
 	const showProductCard =
 		product.displayType === 'ProductCardOnly' ||
-		product.displayType === 'InlineWithProductCard';
+		(!isInHoldBackTestVariant &&
+			product.displayType === 'InlineWithProductCard');
 	return (
 		<>
 			{showContent && (
 				<Content
 					product={product}
 					format={format}
 					ArticleElementComponent={ArticleElementComponent}
-					shouldShowLeftColCard={shouldShowLeftColCard}
+					shouldShowLeftColCard={
+						shouldShowLeftColCard && !isInHoldBackTestVariant
+					}
 				/>
 			)}
 			{showProductCard && (
diff --git a/dotcom-rendering/src/components/marketing/epics/ContributionsEpic.tsx b/dotcom-rendering/src/components/marketing/epics/ContributionsEpic.tsx
@@ -278,7 +278,7 @@ const ContributionsEpic: ReactComponent<EpicProps> = ({
 
 	const [hasBeenSeen, setNode] = useIsInView({
 		debounce: true,
-		threshold: 0,
+		threshold: 0.4,
 	});
 
 	useEffect(() => {
diff --git a/dotcom-rendering/src/components/marketing/epics/ContributionsLiveblogEpic.tsx b/dotcom-rendering/src/components/marketing/epics/ContributionsLiveblogEpic.tsx
@@ -151,6 +151,7 @@ export const ContributionsLiveblogEpic: ReactComponent<EpicProps> = ({
 
 	const [hasBeenSeen, setNode] = useIsInView({
 		debounce: true,
+		threshold: 0.4,
 	});
 
 	useEffect(() => {
diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts
@@ -88,7 +88,7 @@ const everyCardHasSameSponsor = ([
 /**
  * TODO verify that this is what is necessary for two branding to be equal
  */
-const brandingEqual = (b1: Branding, b2: Branding) => {
+export const brandingEqual = (b1: Branding, b2: Branding): boolean => {
 	return (
 		b1.brandingType?.name === b2.brandingType?.name &&
 		b1.sponsorName === b2.sponsorName
@@ -101,6 +101,33 @@ export const isPaidContentSameBranding = (
 	collectionBranding?.kind === 'paid-content' &&
 	!collectionBranding.hasMultipleBranding;
 
+/**
+ * Determines if cards should have their branding stripped
+ * Returns true if all cards have paid-content branding with the same sponsor
+ *
+ * This can be called with either:
+ * - A CollectionBranding object (when available from decideCollectionBranding)
+ * - Raw cards array (when CollectionBranding is undefined but we still need to check)
+ */
+export const shouldStripBrandingFromCards = (
+	input: CollectionBranding | CardWithBranding[],
+	editionId: EditionId,
+): boolean => {
+	// If we have a CollectionBranding object, use it directly
+	if (!Array.isArray(input)) {
+		return isPaidContentSameBranding(input);
+	}
+
+	const brandingsFromCards = getBrandingFromCards(input, editionId);
+	if (!brandingsFromCards) {
+		return false;
+	}
+
+	const sameSponsor = everyCardHasSameSponsor(brandingsFromCards);
+
+	return sameSponsor;
+};
+
 export const badgeFromBranding = (
 	collectionBranding: CollectionBranding | undefined,
 ): DCRBadgeType | undefined => {
diff --git a/dotcom-rendering/src/model/enhanceCollections.ts b/dotcom-rendering/src/model/enhanceCollections.ts
diff --git a/dotcom-rendering/src/server/dev-index.html b/dotcom-rendering/src/server/dev-index.html
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,11 @@`
`1`	`1`	`{`
`2`		`- "extends": "../tsconfig.json",`
	`2`	`+ "extends": "@guardian/tsconfig/tsconfig.json",`
	`3`	`+ "compilerOptions": {`
	`4`	`+ "noEmit": true,`
	`5`	`+ "skipLibCheck": true,`
	`6`	`+ "allowImportingTsExtensions": true,`
	`7`	`+ "moduleResolution": "nodenext",`
	`8`	`+ "module": "nodenext"`
	`9`	`+ },`
	`10`	`+ "exclude": ["node_modules", "index.ts"]`
`3`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -8,6 +8,7 @@`
`8`	`8`
`9`	`9`	`<a`
`10`	`10`	href={`https://dashboard.ophan.co.uk/graph/breakdown?day=today&ab=${testName}`}
	`11`	`+ target="_blank"`
`11`	`12`	`>`
`12`	`13`	`graph`
`13`	`14`	`</a>`