From 0f7b1567a34cc135626b503a5b0111da9c3abad6 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Wed, 2 Oct 2024 07:06:09 -0500 Subject: [PATCH] =?UTF-8?q?Reverted=20"=F0=9F=8C=90=20Updated=20Portal=20e?= =?UTF-8?q?rror=20handling=20to=20be=20i18n-friendly=20(#21176)"=20(#21184?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no ref - this caused some troubles with error representation and changed defaulting behavior Going back to the drawing board on this one. I've been working on a larger scale refactor so that this could be a hook, which feels much more appropriate, though a much more substantial change. --- apps/portal/src/actions.js | 4 +- .../src/components/pages/FeedbackPage.js | 4 +- apps/portal/src/data-attributes.js | 6 +- apps/portal/src/tests/unit/errors.test.js | 93 ------------------- apps/portal/src/utils/api.js | 6 +- apps/portal/src/utils/errors.js | 68 ++++++-------- 6 files changed, 36 insertions(+), 145 deletions(-) delete mode 100644 apps/portal/src/tests/unit/errors.test.js diff --git a/apps/portal/src/actions.js b/apps/portal/src/actions.js index 22e5d82026..96cc784d9a 100644 --- a/apps/portal/src/actions.js +++ b/apps/portal/src/actions.js @@ -1,5 +1,5 @@ import setupGhostApi from './utils/api'; -import {getMessageFromError} from './utils/errors'; +import {HumanReadableError} from './utils/errors'; import {createPopupNotification, getMemberEmail, getMemberName, getProductCadenceFromPrice, removePortalLinkFromUrl, getRefDomain} from './utils/helpers'; function switchPage({data, state}) { @@ -90,7 +90,7 @@ async function signin({data, api, state}) { action: 'signin:failed', popupNotification: createPopupNotification({ type: 'signin:failed', autoHide: false, closeable: true, state, status: 'error', - message: getMessageFromError(e, 'Failed to log in, please try again') + message: HumanReadableError.getMessageFromError(e, 'Failed to log in, please try again') }) }; } diff --git a/apps/portal/src/components/pages/FeedbackPage.js b/apps/portal/src/components/pages/FeedbackPage.js index 8cad18b038..1720ac52d4 100644 --- a/apps/portal/src/components/pages/FeedbackPage.js +++ b/apps/portal/src/components/pages/FeedbackPage.js @@ -4,7 +4,7 @@ import {ReactComponent as ThumbDownIcon} from '../../images/icons/thumbs-down.sv import {ReactComponent as ThumbUpIcon} from '../../images/icons/thumbs-up.svg'; import {ReactComponent as ThumbErrorIcon} from '../../images/icons/thumbs-error.svg'; import setupGhostApi from '../../utils/api'; -import {getMessageFromError} from '../../utils/errors'; +import {HumanReadableError} from '../../utils/errors'; import ActionButton from '../common/ActionButton'; import CloseButton from '../common/CloseButton'; import LoadingPage from './LoadingPage'; @@ -317,7 +317,7 @@ export default function FeedbackPage() { await sendFeedback({siteUrl: site.url, uuid, key, postId, score: selectedScore}, api); setScore(selectedScore); } catch (e) { - const text = getMessageFromError(e, t('There was a problem submitting your feedback. Please try again a little later.')); + const text = HumanReadableError.getMessageFromError(e, t('There was a problem submitting your feedback. Please try again a little later.')); setError(text); } setLoading(false); diff --git a/apps/portal/src/data-attributes.js b/apps/portal/src/data-attributes.js index 3b7d700eb8..9f4e9daefe 100644 --- a/apps/portal/src/data-attributes.js +++ b/apps/portal/src/data-attributes.js @@ -1,6 +1,6 @@ /* eslint-disable no-console */ import {getCheckoutSessionDataFromPlanAttribute, getUrlHistory} from './utils/helpers'; -import {getErrorFromApiResponse, getMessageFromError} from './utils/errors'; +import {HumanReadableError} from './utils/errors'; export function formSubmitHandler({event, form, errorEl, siteUrl, submitHandler}) { form.removeEventListener('submit', submitHandler); @@ -77,14 +77,14 @@ export function formSubmitHandler({event, form, errorEl, siteUrl, submitHandler} if (res.ok) { form.classList.add('success'); } else { - return getErrorFromApiResponse(res).then((e) => { + return HumanReadableError.fromApiResponse(res).then((e) => { throw e; }); } }).catch((err) => { if (errorEl) { // This theme supports a custom error element - errorEl.innerText = getMessageFromError(err, 'There was an error sending the email, please try again'); + errorEl.innerText = HumanReadableError.getMessageFromError(err, 'There was an error sending the email, please try again'); } form.classList.add('error'); }); diff --git a/apps/portal/src/tests/unit/errors.test.js b/apps/portal/src/tests/unit/errors.test.js deleted file mode 100644 index 7b707097be..0000000000 --- a/apps/portal/src/tests/unit/errors.test.js +++ /dev/null @@ -1,93 +0,0 @@ -import React from 'react'; -import {render} from '@testing-library/react'; -import AppContext from '../../AppContext'; -import {GetMessage, getErrorFromApiResponse, getMessageFromError} from '../../utils/errors'; - -// Mock AppContext -jest.mock('../../AppContext', () => ({ - __esModule: true, - default: React.createContext({t: jest.fn()}) -})); - -describe('GetMessage', () => { - it('returns translated message when t function is available', () => { - const tMock = jest.fn().mockImplementation(msg => `translated: ${msg}`); - const {getByText} = render( - - - - ); - expect(getByText('translated: Hello')).toBeInTheDocument(); - expect(tMock).toHaveBeenCalledWith('Hello'); - }); - - it('returns original message when t function is not available', () => { - const {getByText} = render( - - - - ); - expect(getByText('Hello')).toBeInTheDocument(); - }); -}); - -describe('getErrorFromApiResponse', () => { - // These tests are a little goofy because we're not testing the translation, we're testing that the function - // returns an Error with the correct message. We're not testing the message itself because that's handled in the - // getMessage function. And this doesn't run in react so the react component gets odd. - it('returns Error with translated message for 400 status', async () => { - const res = { - status: 400, - json: jest.fn().mockResolvedValue({errors: [{message: 'Bad Request'}]}) - }; - const error = await getErrorFromApiResponse(res); - expect(error).toBeInstanceOf(Error); - }); - - // These tests are a little goofy because we're not testing the translation, we're testing that the function - // returns an Error with the correct message. We're not testing the message itself because that's handled in the - // getMessage function. And this doesn't run in react so the react component gets odd. - it('returns Error with translated message for 429 status', async () => { - const res = { - status: 429, - json: jest.fn().mockResolvedValue({errors: [{message: 'Too Many Requests'}]}) - }; - const error = await getErrorFromApiResponse(res); - expect(error).toBeInstanceOf(Error); - }); - - it('returns undefined for other status codes', async () => { - const res = {status: 200}; - const error = await getErrorFromApiResponse(res); - expect(error).toBeUndefined(); - }); - - it('returns undefined when json parsing fails', async () => { - const res = { - status: 400, - json: jest.fn().mockRejectedValue(new Error('JSON parse error')) - }; - const error = await getErrorFromApiResponse(res); - expect(error).toBeUndefined(); - }); -}); - -describe('getMessageFromError', () => { - it('returns GetMessage component with default message when provided', () => { - const result = getMessageFromError(new Error('Test error'), 'Default message'); - const {getByText} = render({result}); - expect(getByText('Default message')).toBeInTheDocument(); - }); - - it('returns GetMessage component with error message for Error instance', () => { - const result = getMessageFromError(new Error('Test error')); - const {getByText} = render({result}); - expect(getByText('Test error')).toBeInTheDocument(); - }); - - it('returns GetMessage component with error for non-Error objects', () => { - const result = getMessageFromError('String error'); - const {getByText} = render({result}); - expect(getByText('String error')).toBeInTheDocument(); - }); -}); \ No newline at end of file diff --git a/apps/portal/src/utils/api.js b/apps/portal/src/utils/api.js index 2c283cb311..9d8f32e082 100644 --- a/apps/portal/src/utils/api.js +++ b/apps/portal/src/utils/api.js @@ -1,4 +1,4 @@ -import {getErrorFromApiResponse} from './errors'; +import {HumanReadableError} from './errors'; import {transformApiSiteData, transformApiTiersData, getUrlHistory} from './helpers'; function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}) { @@ -159,7 +159,7 @@ function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}) { if (res.ok) { return res.json(); } else { - throw (await getErrorFromApiResponse(res)) ?? new Error('Failed to save feedback'); + throw (await HumanReadableError.fromApiResponse(res)) ?? new Error('Failed to save feedback'); } } }; @@ -291,7 +291,7 @@ function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}) { return 'Success'; } else { // Try to read body error message that is human readable and should be shown to the user - const humanError = await getErrorFromApiResponse(res); + const humanError = await HumanReadableError.fromApiResponse(res); if (humanError) { throw humanError; } diff --git a/apps/portal/src/utils/errors.js b/apps/portal/src/utils/errors.js index e6d70fd12d..7f695160c3 100644 --- a/apps/portal/src/utils/errors.js +++ b/apps/portal/src/utils/errors.js @@ -1,48 +1,32 @@ -import {useContext} from 'react'; -import AppContext from '../AppContext'; - -/** - * A component that gets the internationalized (i18n) message from the app context. - * @param {{message: string}} props - The props object containing the message to be translated. - * @returns {string} The translated message if AppContext.t is available, otherwise the original message. - */ -export const GetMessage = ({message}) => { - const {t} = useContext(AppContext); - return t ? t(message) : message; -}; - -/** - * Creates a human-readable error from an API response. - * @param {Response} res - The API response object. - * @returns {Promise} A promise that resolves to an Error if one can be created, or undefined otherwise. - */ -export async function getErrorFromApiResponse(res) { - // Bad request + Too many requests - if (res.status === 400 || res.status === 429) { - try { - const json = await res.json(); - if (json.errors && Array.isArray(json.errors) && json.errors.length > 0 && json.errors[0].message) { - return new Error(); +export class HumanReadableError extends Error { + /** + * Returns whether this response from the server is a human readable error and should be shown to the user. + * @param {Response} res + * @returns {HumanReadableError|undefined} + */ + static async fromApiResponse(res) { + // Bad request + Too many requests + if (res.status === 400 || res.status === 429) { + try { + const json = await res.json(); + if (json.errors && Array.isArray(json.errors) && json.errors.length > 0 && json.errors[0].message) { + return new HumanReadableError(json.errors[0].message); + } + } catch (e) { + // Failed to decode: ignore + return false; } - } catch (e) { - // Failed to decode: ignore - return undefined; } } - return undefined; -} -/** - * Creates a human-readable error message from an error object. - * @param {Error} error - The error object. - * @param {string} defaultMessage - The default message to use if a human-readable message can't be extracted. - * @returns {React.ReactElement} A React element containing the human-readable error message. - */ -export function getMessageFromError(error, defaultMessage) { - if (defaultMessage) { - return ; - } else if (error instanceof Error) { - return ; + /** + * Only output the message of an error if it is a human readable error and should be exposed to the user. + * Otherwise it returns a default generic message. + */ + static getMessageFromError(error, defaultMessage) { + if (error instanceof HumanReadableError) { + return error.message; + } + return defaultMessage; } - return ; }