From 7a40ab52fb8f3c580974bd07be83f03a6848b006 Mon Sep 17 00:00:00 2001 From: Sag Date: Thu, 14 Mar 2024 11:36:28 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20adding=20recommendation?= =?UTF-8?q?=20when=20oembed=20fails=20(#19861)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://linear.app/tryghost/issue/ENG-750 - when adding a recommendation, we fetch the recommended site's metadata - before this change, if the metadata fetch failed for some reason, we'd show an error and block the recommendation from being added - after this change, we use fallback values if the metadata fails to fetch, instead of blocking the recommendation from being added. We use the site domain as the title and leave the rest empty (no favicon, no description) - this change also means we are not checking whether a site exists or not for the publisher anymore. It’s then up to the publisher to make sure they don’t enter broken URLs --- .../recommendations.test.js.snap | 33 +++++++++++++++++++ .../e2e-api/admin/recommendations.test.js | 27 +++++++++++++++ .../src/RecommendationMetadataService.ts | 1 + .../src/RecommendationService.ts | 17 +++++++++- .../test/RecommendationService.test.ts | 14 ++++++++ 5 files changed, 91 insertions(+), 1 deletion(-) diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap index 76a2963aab..ed337e9849 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap @@ -2217,6 +2217,39 @@ Object { } `; +exports[`Recommendations Admin API check Returns nullified values if site fails to fetch 1: [body] 1`] = ` +Object { + "recommendations": Array [ + Object { + "created_at": null, + "description": null, + "excerpt": null, + "favicon": null, + "featured_image": null, + "id": null, + "one_click_subscribe": false, + "title": null, + "updated_at": null, + "url": "https://dogpictures.com/", + }, + ], +} +`; + +exports[`Recommendations Admin API check Returns nullified values if site fails to fetch 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "214", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-cache-invalidate": "/*", + "x-powered-by": "Express", +} +`; + exports[`Recommendations Admin API delete Can delete recommendation 1: [body] 1`] = `Object {}`; exports[`Recommendations Admin API delete Can delete recommendation 2: [headers] 1`] = ` diff --git a/ghost/core/test/e2e-api/admin/recommendations.test.js b/ghost/core/test/e2e-api/admin/recommendations.test.js index 699aa84c10..af40c51b31 100644 --- a/ghost/core/test/e2e-api/admin/recommendations.test.js +++ b/ghost/core/test/e2e-api/admin/recommendations.test.js @@ -703,6 +703,33 @@ describe('Recommendations Admin API', function () { assert.equal(body.recommendations[0].favicon, 'https://dogpictures.com/favicon.ico'); assert.equal(body.recommendations[0].one_click_subscribe, true); }); + + it('Returns nullified values if site fails to fetch', async function () { + nock('https://dogpictures.com') + .get('/') + .reply(404); + + const {body} = await agent.post('recommendations/check/') + .body({ + recommendations: [{ + url: 'https://dogpictures.com' + }] + }) + .expectStatus(200) + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }) + .matchBodySnapshot({}); + + assert.equal(body.recommendations[0].title, null); + assert.equal(body.recommendations[0].url, 'https://dogpictures.com/'); + assert.equal(body.recommendations[0].description, null); + assert.equal(body.recommendations[0].excerpt, null); + assert.equal(body.recommendations[0].featured_image, null); + assert.equal(body.recommendations[0].favicon, null); + assert.equal(body.recommendations[0].one_click_subscribe, false); + }); }); describe('delete', function () { diff --git a/ghost/recommendations/src/RecommendationMetadataService.ts b/ghost/recommendations/src/RecommendationMetadataService.ts index 5006c584f1..c45f9d81d6 100644 --- a/ghost/recommendations/src/RecommendationMetadataService.ts +++ b/ghost/recommendations/src/RecommendationMetadataService.ts @@ -114,6 +114,7 @@ export class RecommendationMetadataService { // Use the oembed service to fetch metadata const oembed = await this.#oembedService.fetchOembedDataFromUrl(url.toString(), 'mention'); + return { title: oembed?.metadata?.title || null, excerpt: oembed?.metadata?.description || null, diff --git a/ghost/recommendations/src/RecommendationService.ts b/ghost/recommendations/src/RecommendationService.ts index 8326d54be8..e3dc23d914 100644 --- a/ghost/recommendations/src/RecommendationService.ts +++ b/ghost/recommendations/src/RecommendationService.ts @@ -150,7 +150,22 @@ export class RecommendationService { return existing.plain; } - const metadata = await this.recommendationMetadataService.fetch(url); + let metadata; + try { + metadata = await this.recommendationMetadataService.fetch(url); + } catch (e) { + logging.error('[Recommendations] Failed to fetch metadata for url ' + url, e); + + return { + url: url, + title: undefined, + excerpt: undefined, + featuredImage: undefined, + favicon: undefined, + oneClickSubscribe: false + }; + } + return { url: url, title: metadata.title ?? undefined, diff --git a/ghost/recommendations/test/RecommendationService.test.ts b/ghost/recommendations/test/RecommendationService.test.ts index c7af99693f..0cbe361802 100644 --- a/ghost/recommendations/test/RecommendationService.test.ts +++ b/ghost/recommendations/test/RecommendationService.test.ts @@ -280,6 +280,20 @@ describe('RecommendationService', function () { url: new URL('http://localhost/newone') }); }); + + it('Returns undefined recommendation metadata if metadata fails to fetch', async function () { + fetchMetadataStub.rejects(new Error('Metadata failed to fetch')); + + const response = await service.checkRecommendation(new URL('http://localhost/newone')); + assert.deepEqual(response, { + title: undefined, + excerpt: undefined, + featuredImage: undefined, + favicon: undefined, + oneClickSubscribe: false, + url: new URL('http://localhost/newone') + }); + }); }); describe('updateRecommendationsEnabledSetting', function () {