From bca2a5637924f7936aa7c85994e49736877f7ab0 Mon Sep 17 00:00:00 2001 From: Marcin Gosz Date: Mon, 6 Jul 2026 13:47:55 +0100 Subject: [PATCH 1/2] excluding html from error description, using error header for logging --- .../src/components/SecureSignup.island.tsx | 11 ++- .../newsletterSignupFailureDetails.test.ts | 83 +++++++++++++++---- .../src/lib/newsletterSignupFailureDetails.ts | 40 +++++++-- .../src/lib/useNewsletterSignupForm.ts | 11 ++- 4 files changed, 111 insertions(+), 34 deletions(-) diff --git a/dotcom-rendering/src/components/SecureSignup.island.tsx b/dotcom-rendering/src/components/SecureSignup.island.tsx index 9fabe6cfc84..c9e2608f799 100644 --- a/dotcom-rendering/src/components/SecureSignup.island.tsx +++ b/dotcom-rendering/src/components/SecureSignup.island.tsx @@ -26,7 +26,7 @@ import { } from '../lib/newsletter-marketing-opt-in'; import { getErrorType, - getResponseErrorDescription, + getResponseFailureDetails, } from '../lib/newsletterSignupFailureDetails'; import { EVENT_DESCRIPTION_TO_ACTION, @@ -394,11 +394,10 @@ export const SecureSignup = ({ if (!response.ok) { trackingDetails.responseStatus = response.status; - const errorDescription = - await getResponseErrorDescription(response); - if (errorDescription) { - trackingDetails.errorDescription = errorDescription; - } + Object.assign( + trackingDetails, + await getResponseFailureDetails(response), + ); } sendTracking( diff --git a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts index 3f95f264738..88ae60f45b9 100644 --- a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts +++ b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts @@ -1,47 +1,96 @@ import { getErrorType, - getResponseErrorDescription, + getResponseFailureDetails, } from './newsletterSignupFailureDetails'; -const mockResponse = (text: string): Response => +const mockResponse = ( + text: string, + additionalHeaders: Record = {}, +): Response => ({ text: jest.fn().mockResolvedValue(text), + headers: { + get: jest + .fn() + .mockImplementation( + (headerName: string) => + additionalHeaders[headerName.toLowerCase()] ?? null, + ), + }, }) as unknown as Response; describe('newsletterSignupFailureDetails', () => { - describe('getResponseErrorDescription', () => { + describe('getResponseFailureDetails', () => { it('returns undefined for an empty response body', async () => { - const response = mockResponse(' '); + const response = mockResponse(' ', { + 'content-type': 'text/plain', + }); - await expect(getResponseErrorDescription(response)).resolves.toBe( - undefined, + await expect(getResponseFailureDetails(response)).resolves.toEqual( + {}, ); }); it('trims and collapses whitespace in the response body', async () => { - const response = mockResponse(' too\n\n many\t spaces '); + const response = mockResponse(' too\n\n many\t spaces ', { + 'content-type': 'text/plain', + }); - await expect(getResponseErrorDescription(response)).resolves.toBe( - 'too many spaces', - ); + await expect(getResponseFailureDetails(response)).resolves.toEqual({ + errorDescription: 'too many spaces', + }); + }); + + it('truncates the response body to 60 characters', async () => { + const longMessage = 'a'.repeat(70); + const response = mockResponse(longMessage, { + 'content-type': 'text/plain', + }); + + await expect(getResponseFailureDetails(response)).resolves.toEqual({ + errorDescription: 'a'.repeat(60), + }); }); - it('truncates the response body to 120 characters', async () => { - const longMessage = 'a'.repeat(130); - const response = mockResponse(longMessage); + it('returns the error code header when present', async () => { + const response = mockResponse('ignored body', { + 'content-type': 'text/plain', + 'email-signup-error-code': 'already-subscribed', + }); + + await expect(getResponseFailureDetails(response)).resolves.toEqual({ + errorCode: 'already-subscribed', + }); + expect(response.text).not.toHaveBeenCalled(); + }); + + it('returns undefined for html error pages based on content type', async () => { + const response = mockResponse( + 'ErrorOops', + { 'content-type': 'text/html; charset=utf-8' }, + ); - await expect(getResponseErrorDescription(response)).resolves.toBe( - 'a'.repeat(120), + await expect(getResponseFailureDetails(response)).resolves.toEqual( + {}, ); }); it('returns undefined when reading the response body throws', async () => { const response = { text: jest.fn().mockRejectedValue(new Error('read failed')), + headers: { + get: jest + .fn() + .mockImplementation((headerName: string) => + headerName.toLowerCase() === 'content-type' + ? 'text/plain' + : null, + ), + }, } as unknown as Response; - await expect(getResponseErrorDescription(response)).resolves.toBe( - undefined, + await expect(getResponseFailureDetails(response)).resolves.toEqual( + {}, ); }); }); diff --git a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts index 401641ae1d5..233bcf254ba 100644 --- a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts +++ b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts @@ -1,12 +1,42 @@ -export const getResponseErrorDescription = async ( +const MAX_FAILURE_DETAIL_LENGTH = 60; +const ERROR_CODE_HEADER = 'Email-Signup-Error-Code'; + +export type NewsletterSignupFailureDetails = { + errorCode?: string; + errorDescription?: string; +}; + +export const getResponseFailureDetails = async ( response: Response, -): Promise => { +): Promise => { try { + const errorCode = response.headers + .get(ERROR_CODE_HEADER) + ?.trim() + .substring(0, MAX_FAILURE_DETAIL_LENGTH); + + if (errorCode) { + return { errorCode }; + } + + const contentType = response.headers.get('content-type')?.toLowerCase(); + + if (contentType?.includes('text/html')) { + return {}; + } + const responseText = (await response.text()).trim(); - if (!responseText) return; - return responseText.replace(/\s+/g, ' ').substring(0, 120); + if (!responseText) return {}; + + const normalisedResponseText = responseText.replace(/\s+/g, ' '); + return { + errorDescription: normalisedResponseText.substring( + 0, + MAX_FAILURE_DETAIL_LENGTH, + ), + }; } catch { - return; + return {}; } }; diff --git a/dotcom-rendering/src/lib/useNewsletterSignupForm.ts b/dotcom-rendering/src/lib/useNewsletterSignupForm.ts index 54a868bd8e5..cc48cda4915 100644 --- a/dotcom-rendering/src/lib/useNewsletterSignupForm.ts +++ b/dotcom-rendering/src/lib/useNewsletterSignupForm.ts @@ -12,7 +12,7 @@ import { } from './newsletter-marketing-opt-in'; import { getErrorType, - getResponseErrorDescription, + getResponseFailureDetails, } from './newsletterSignupFailureDetails'; import { EVENT_DESCRIPTION_TO_ACTION, @@ -379,11 +379,10 @@ export const useNewsletterSignupForm = ( if (!response.ok) { trackingDetails.responseStatus = response.status; - const errorDescription = - await getResponseErrorDescription(response); - if (errorDescription) { - trackingDetails.errorDescription = errorDescription; - } + Object.assign( + trackingDetails, + await getResponseFailureDetails(response), + ); } sendTracking( From 208a1a4148239f8ecb496752d0f820dc6c540edb Mon Sep 17 00:00:00 2001 From: Marcin Gosz Date: Mon, 6 Jul 2026 15:12:47 +0100 Subject: [PATCH 2/2] lint fix --- .../newsletterSignupFailureDetails.test.ts | 43 +++++++++---------- .../src/lib/newsletterSignupFailureDetails.ts | 4 +- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts index 88ae60f45b9..0a002ea213a 100644 --- a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts +++ b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.test.ts @@ -3,20 +3,21 @@ import { getResponseFailureDetails, } from './newsletterSignupFailureDetails'; +type MockHeaders = Partial>; + +const mockHeaders = (headers: MockHeaders) => ({ + get: jest.fn().mockImplementation((headerName: string) => { + return headers[headerName.toLowerCase()] ?? null; + }), +}); + const mockResponse = ( text: string, - additionalHeaders: Record = {}, + additionalHeaders: MockHeaders = {}, ): Response => ({ text: jest.fn().mockResolvedValue(text), - headers: { - get: jest - .fn() - .mockImplementation( - (headerName: string) => - additionalHeaders[headerName.toLowerCase()] ?? null, - ), - }, + headers: mockHeaders(additionalHeaders), }) as unknown as Response; describe('newsletterSignupFailureDetails', () => { @@ -53,15 +54,19 @@ describe('newsletterSignupFailureDetails', () => { }); it('returns the error code header when present', async () => { - const response = mockResponse('ignored body', { - 'content-type': 'text/plain', - 'email-signup-error-code': 'already-subscribed', - }); + const textMock = jest.fn().mockResolvedValue('ignored body'); + const response = { + text: textMock, + headers: mockHeaders({ + 'content-type': 'text/plain', + 'email-signup-error-code': 'already-subscribed', + }), + } as unknown as Response; await expect(getResponseFailureDetails(response)).resolves.toEqual({ errorCode: 'already-subscribed', }); - expect(response.text).not.toHaveBeenCalled(); + expect(textMock).not.toHaveBeenCalled(); }); it('returns undefined for html error pages based on content type', async () => { @@ -78,15 +83,7 @@ describe('newsletterSignupFailureDetails', () => { it('returns undefined when reading the response body throws', async () => { const response = { text: jest.fn().mockRejectedValue(new Error('read failed')), - headers: { - get: jest - .fn() - .mockImplementation((headerName: string) => - headerName.toLowerCase() === 'content-type' - ? 'text/plain' - : null, - ), - }, + headers: mockHeaders({ 'content-type': 'text/plain' }), } as unknown as Response; await expect(getResponseFailureDetails(response)).resolves.toEqual( diff --git a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts index 233bcf254ba..0aaf46ae3a9 100644 --- a/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts +++ b/dotcom-rendering/src/lib/newsletterSignupFailureDetails.ts @@ -15,13 +15,13 @@ export const getResponseFailureDetails = async ( ?.trim() .substring(0, MAX_FAILURE_DETAIL_LENGTH); - if (errorCode) { + if (errorCode !== undefined && errorCode !== '') { return { errorCode }; } const contentType = response.headers.get('content-type')?.toLowerCase(); - if (contentType?.includes('text/html')) { + if (contentType?.includes('text/html') === true) { return {}; }