From 05215734af00b199e1163904f12e43a17f3345ee Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Thu, 28 Sep 2023 12:54:16 +0200 Subject: [PATCH] Added recommend back URL (#18382) refs https://github.com/TryGhost/Product/issues/3958 - Disabled automatic network retries for external site lookups (=> timed out to 5s in every situation because it returned 404 when a site doesn't implement the Ghost api) - Disabled representing a modal when it is already present on hash changes - Added support for search params in modals - Handle `?url` search param in the addRecommendationModal --- apps/admin-x-settings/playwright.config.ts | 2 +- .../admin-x-ds/global/form/URLTextField.tsx | 2 +- .../src/api/external-ghost-site.ts | 3 +- .../components/providers/RoutingProvider.tsx | 8 +- .../AddRecommendationModal.tsx | 77 +++++++++++++++---- apps/admin-x-settings/src/utils/api/hooks.ts | 13 +++- .../recommendation-emails.test.js.snap | 4 +- .../src/IncomingRecommendationService.ts | 2 +- ghost/staff-service/lib/StaffServiceEmails.js | 4 + .../recommendation-received.hbs | 2 +- 10 files changed, 89 insertions(+), 28 deletions(-) diff --git a/apps/admin-x-settings/playwright.config.ts b/apps/admin-x-settings/playwright.config.ts index bca2d5eed4..c25994f1fc 100644 --- a/apps/admin-x-settings/playwright.config.ts +++ b/apps/admin-x-settings/playwright.config.ts @@ -14,7 +14,7 @@ export default defineConfig({ /* Retry on CI only */ retries: process.env.CI ? 2 : 0, /* Hardcode to use all cores in CI */ - workers: process.env.CI ? '100%' : undefined, + workers: process.env.CI ? '100%' : (process.env.PLAYWRIGHT_SLOWMO ? 1 : undefined), /* Reporter to use. See https://playwright.dev/docs/test-reporters */ reporter: 'html', /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ diff --git a/apps/admin-x-settings/src/admin-x-ds/global/form/URLTextField.tsx b/apps/admin-x-settings/src/admin-x-ds/global/form/URLTextField.tsx index ddb0685698..94cf983b94 100644 --- a/apps/admin-x-settings/src/admin-x-ds/global/form/URLTextField.tsx +++ b/apps/admin-x-settings/src/admin-x-ds/global/form/URLTextField.tsx @@ -3,7 +3,7 @@ import TextField, {TextFieldProps} from './TextField'; import validator from 'validator'; import {useFocusContext} from '../../providers/DesignSystemProvider'; -const formatUrl = (value: string, baseUrl?: string) => { +export const formatUrl = (value: string, baseUrl?: string) => { let url = value.trim(); if (!url) { diff --git a/apps/admin-x-settings/src/api/external-ghost-site.ts b/apps/admin-x-settings/src/api/external-ghost-site.ts index 4e231c5d57..b6465eab84 100644 --- a/apps/admin-x-settings/src/api/external-ghost-site.ts +++ b/apps/admin-x-settings/src/api/external-ghost-site.ts @@ -31,7 +31,8 @@ export const useExternalGhostSite = () => { const result = await fetchApi(url, { method: 'GET', credentials: 'omit', // Allow CORS wildcard, - timeout: 5000 + timeout: 5000, + retry: false }); // We need to validate all data types here for extra safety diff --git a/apps/admin-x-settings/src/components/providers/RoutingProvider.tsx b/apps/admin-x-settings/src/components/providers/RoutingProvider.tsx index a95d19bb45..eab18b4b03 100644 --- a/apps/admin-x-settings/src/components/providers/RoutingProvider.tsx +++ b/apps/admin-x-settings/src/components/providers/RoutingProvider.tsx @@ -30,7 +30,8 @@ export const RouteContext = createContext({ export type RoutingModalProps = { pathName: string; - params?: Record + params?: Record, + searchParams?: URLSearchParams } const modalPaths: {[key: string]: ModalName} = { @@ -85,6 +86,7 @@ const handleNavigation = (currentRoute: string | undefined) => { let url = new URL(hash, domain); const pathName = getHashPath(url.pathname); + const searchParams = url.searchParams; if (pathName) { const [, currentModalName] = Object.entries(modalPaths).find(([modalPath]) => matchRoute(currentRoute || '', modalPath)) || []; @@ -93,9 +95,9 @@ const handleNavigation = (currentRoute: string | undefined) => { return { pathName, changingModal: modalName && modalName !== currentModalName, - modal: (path && modalName) ? + modal: (path && modalName) ? // we should consider adding '&& modalName !== currentModalName' here, but this breaks tests import('./routing/modals').then(({default: modals}) => { - NiceModal.show(modals[modalName] as ModalComponent, {pathName, params: matchRoute(pathName, path)}); + NiceModal.show(modals[modalName] as ModalComponent, {pathName, params: matchRoute(pathName, path), searchParams}); }) : undefined }; diff --git a/apps/admin-x-settings/src/components/settings/site/recommendations/AddRecommendationModal.tsx b/apps/admin-x-settings/src/components/settings/site/recommendations/AddRecommendationModal.tsx index cb7b1b490d..fb1ae20ed2 100644 --- a/apps/admin-x-settings/src/components/settings/site/recommendations/AddRecommendationModal.tsx +++ b/apps/admin-x-settings/src/components/settings/site/recommendations/AddRecommendationModal.tsx @@ -8,8 +8,10 @@ import useForm from '../../../../hooks/useForm'; import useRouting from '../../../../hooks/useRouting'; import {AlreadyExistsError} from '../../../../utils/errors'; import {EditOrAddRecommendation, RecommendationResponseType, useGetRecommendationByUrl} from '../../../../api/recommendations'; +import {LoadingIndicator} from '../../../../admin-x-ds/global/LoadingIndicator'; import {RoutingModalProps} from '../../../providers/RoutingProvider'; import {dismissAllToasts, showToast} from '../../../../admin-x-ds/global/Toast'; +import {formatUrl} from '../../../../admin-x-ds/global/form/URLTextField'; import {trimSearchAndHash} from '../../../../utils/url'; import {useExternalGhostSite} from '../../../../api/external-ghost-site'; import {useGetOembed} from '../../../../api/oembed'; @@ -19,7 +21,11 @@ interface AddRecommendationModalProps { animate?: boolean } -const AddRecommendationModal: React.FC = ({recommendation, animate}) => { +const doFormatUrl = (url: string) => { + return formatUrl(url).save; +}; + +const AddRecommendationModal: React.FC = ({searchParams, recommendation, animate}) => { const [enterPressed, setEnterPressed] = useState(false); const modal = useModal(); const {updateRoute} = useRouting(); @@ -27,10 +33,18 @@ const AddRecommendationModal: React.FC infinite loop + // updateRoute('recommendations/add?url=' + encodeURIComponent(updatedRecommendation.url)); + NiceModal.show(AddRecommendationModalConfirm, { animate: false, recommendation: updatedRecommendation @@ -108,11 +126,16 @@ const AddRecommendationModal: React.FC { + const onOk = React.useCallback(async () => { if (saveState === 'saving') { // Already saving return; @@ -120,7 +143,9 @@ const AddRecommendationModal: React.FC { + if (showLoadingView && !didInitialSubmit.current) { + didInitialSubmit.current = true; + onOk(); + } + }, [showLoadingView, onOk]); useEffect(() => { if (enterPressed) { - saveForm(); + onOk(); setEnterPressed(false); // Reset for future use } // eslint-disable-next-line react-hooks/exhaustive-deps }, [formState]); - const formatUrl = (url: string) => { - if (!url.startsWith('http://') && !url.startsWith('https://')) { - url = `https://${url}`; - } - return url; - }; + if (showLoadingView) { + return { + // Closed without saving: reset route + updateRoute('recommendations'); + }} + animate={animate ?? true} + backDropClick={false} + cancelLabel='' + okLabel='' + size='sm' + > + + ; + } return { @@ -158,7 +205,7 @@ const AddRecommendationModal: React.FC

You can recommend any site your audience will find valuable, not just those published on Ghost.

updateForm(state => ({...state, url: formatUrl(formState.url)}))} + onBlur={() => updateForm(state => ({...state, url: doFormatUrl(formState.url)}))} onChange={(e) => { clearError?.('url'); updateForm(state => ({...state, url: e.target.value})); @@ -180,7 +227,7 @@ const AddRecommendationModal: React.FC { if (e.key === 'Enter') { e.preventDefault(); - updateForm(state => ({...state, url: formatUrl(formState.url)})); + updateForm(state => ({...state, url: doFormatUrl(formState.url)})); setEnterPressed(true); } }} diff --git a/apps/admin-x-settings/src/utils/api/hooks.ts b/apps/admin-x-settings/src/utils/api/hooks.ts index 5ad4a4153c..65635773ae 100644 --- a/apps/admin-x-settings/src/utils/api/hooks.ts +++ b/apps/admin-x-settings/src/utils/api/hooks.ts @@ -27,6 +27,7 @@ interface RequestOptions { }; credentials?: 'include' | 'omit' | 'same-origin'; timeout?: number; + retry?: boolean; } export const useFetchApi = () => { @@ -34,7 +35,7 @@ export const useFetchApi = () => { const sentryDSN = useSentryDSN(); // eslint-disable-next-line @typescript-eslint/no-explicit-any - return async (endpoint: string | URL, options: RequestOptions = {}) => { + return async (endpoint: string | URL, options: RequestOptions = {}): Promise => { // By default, we set the Content-Type header to application/json const defaultHeaders: Record = { 'app-pragma': 'no-cache', @@ -56,6 +57,7 @@ export const useFetchApi = () => { // 1. Server Unreachable error from the browser (code 0 or TypeError), typically from short internet blips // 2. Maintenance error from Ghost, upgrade in progress so API is temporarily unavailable let attempts = 0; + let shouldRetry = options.retry === true || options.retry === undefined; let retryingMs = 0; const startTime = Date.now(); const maxRetryingMs = 15_000; @@ -75,7 +77,7 @@ export const useFetchApi = () => { return data; }; - while (true) { + while (attempts === 0 || shouldRetry) { try { const response = await fetch(endpoint, { headers: { @@ -97,7 +99,7 @@ export const useFetchApi = () => { } catch (error) { retryingMs = Date.now() - startTime; - if (import.meta.env.MODE !== 'development' && retryableErrors.some(errorClass => error instanceof errorClass) && retryingMs <= maxRetryingMs) { + if (shouldRetry && (import.meta.env.MODE !== 'development' && retryableErrors.some(errorClass => error instanceof errorClass) && retryingMs <= maxRetryingMs)) { await new Promise((resolve) => { setTimeout(resolve, retryPeriods[attempts] || retryPeriods[retryPeriods.length - 1]); }); @@ -122,6 +124,11 @@ export const useFetchApi = () => { throw newError; }; } + + // Used for type checking + // this can't happen, but TS isn't smart enough to undeerstand that the loop will never exit without an error or return + // because of shouldRetry + attemps usage combination + return undefined as never; }; }; diff --git a/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap b/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap index 23af02bac2..95ffc8e9b6 100644 --- a/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap +++ b/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap @@ -366,7 +366,7 @@ exports[`Incoming Recommendation Emails Sends an email if we receive a recommend - +
Recommend back Recommend back
@@ -563,7 +563,7 @@ exports[`Incoming Recommendation Emails Sends an email if we receive a recommend - +
Recommend back Recommend back
diff --git a/ghost/recommendations/src/IncomingRecommendationService.ts b/ghost/recommendations/src/IncomingRecommendationService.ts index 08ecc1a070..d5c56e27ad 100644 --- a/ghost/recommendations/src/IncomingRecommendationService.ts +++ b/ghost/recommendations/src/IncomingRecommendationService.ts @@ -98,7 +98,7 @@ export class IncomingRecommendationService { // Check if we are also recommending this URL const existing = await this.#recommendationService.countRecommendations({ - filter: `url:~^'${url}'` + filter: `url:~'${url}'` }); const recommendingBack = existing > 0; diff --git a/ghost/staff-service/lib/StaffServiceEmails.js b/ghost/staff-service/lib/StaffServiceEmails.js index 516c84d31c..685e8affd7 100644 --- a/ghost/staff-service/lib/StaffServiceEmails.js +++ b/ghost/staff-service/lib/StaffServiceEmails.js @@ -494,6 +494,10 @@ class StaffServiceEmails { } return array.slice(0,limit); }); + + this.Handlebars.registerHelper('encodeURIComponent', function (string) { + return encodeURIComponent(string); + }); } async renderHTML(templateName, data) { diff --git a/ghost/staff-service/lib/email-templates/recommendation-received.hbs b/ghost/staff-service/lib/email-templates/recommendation-received.hbs index 975116d5a7..e9c1855d6e 100644 --- a/ghost/staff-service/lib/email-templates/recommendation-received.hbs +++ b/ghost/staff-service/lib/email-templates/recommendation-received.hbs @@ -59,7 +59,7 @@ {{#if recommendation.recommendingBack}} View recommendations {{else}} - Recommend back + Recommend back {{/if}}