From 7e2d842db2f7a905f6d5e5fe9f57f81f969a68eb Mon Sep 17 00:00:00 2001 From: Sag Date: Thu, 28 Mar 2024 13:54:23 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20keeping=20existing=20att?= =?UTF-8?q?ribution=20in=20recommendations=20(#19945)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/tryghost/issue/ENG-799 - recommendations were being stripped of query parameters and hash fragments before save - in particular, query parameters for attribution such as ?ref were not being stored --- .../AddRecommendationModal.tsx | 2 - .../src/BookshelfRecommendationRepository.ts | 2 +- .../src/InMemoryRecommendationRepository.ts | 2 +- ghost/recommendations/src/Recommendation.ts | 8 --- .../BookshelfRecommendationRepository.test.ts | 64 +++++++++++++++++-- .../test/Recommendation.test.ts | 4 +- 6 files changed, 64 insertions(+), 18 deletions(-) diff --git a/apps/admin-x-settings/src/components/settings/growth/recommendations/AddRecommendationModal.tsx b/apps/admin-x-settings/src/components/settings/growth/recommendations/AddRecommendationModal.tsx index 21e672f020..dc6e7b66b0 100644 --- a/apps/admin-x-settings/src/components/settings/growth/recommendations/AddRecommendationModal.tsx +++ b/apps/admin-x-settings/src/components/settings/growth/recommendations/AddRecommendationModal.tsx @@ -6,7 +6,6 @@ import {EditOrAddRecommendation, useCheckRecommendation} from '@tryghost/admin-x import {ErrorMessages, useForm} from '@tryghost/admin-x-framework/hooks'; import {Form, LoadingIndicator, Modal, TextField, dismissAllToasts, formatUrl, showToast} from '@tryghost/admin-x-design-system'; import {RoutingModalProps, useRouting} from '@tryghost/admin-x-framework/routing'; -import {trimSearchAndHash} from '../../../../utils/url'; interface AddRecommendationModalProps { recommendation?: EditOrAddRecommendation, @@ -60,7 +59,6 @@ const AddRecommendationModal: React.FC { let validatedUrl: URL; validatedUrl = new URL(formState.url); - validatedUrl = trimSearchAndHash(validatedUrl); // Use the hostname as fallback title const defaultTitle = validatedUrl.hostname.replace('www.', ''); diff --git a/ghost/recommendations/src/BookshelfRecommendationRepository.ts b/ghost/recommendations/src/BookshelfRecommendationRepository.ts index 9ba36b6197..b0338c8ee6 100644 --- a/ghost/recommendations/src/BookshelfRecommendationRepository.ts +++ b/ghost/recommendations/src/BookshelfRecommendationRepository.ts @@ -109,7 +109,7 @@ export class BookshelfRecommendationRepository extends BookshelfRepository { return r.url.hostname.replace('www.', '') === url.hostname.replace('www.', '') && - r.url.pathname === url.pathname; + r.url.pathname.replace(/\/$/, '') === url.pathname.replace(/\/$/, ''); }) || null; return existing; diff --git a/ghost/recommendations/src/InMemoryRecommendationRepository.ts b/ghost/recommendations/src/InMemoryRecommendationRepository.ts index 31b5d3fe48..24043ce534 100644 --- a/ghost/recommendations/src/InMemoryRecommendationRepository.ts +++ b/ghost/recommendations/src/InMemoryRecommendationRepository.ts @@ -12,7 +12,7 @@ export class InMemoryRecommendationRepository extends InMemoryRepository { return r.url.hostname.replace('www.', '') === url.hostname.replace('www.', '') && - r.url.pathname === url.pathname; + r.url.pathname.replace(/\/$/, '') === url.pathname.replace(/\/$/, ''); }) || null; return existing; diff --git a/ghost/recommendations/src/Recommendation.ts b/ghost/recommendations/src/Recommendation.ts index 4ff593af7c..f0da1da2da 100644 --- a/ghost/recommendations/src/Recommendation.ts +++ b/ghost/recommendations/src/Recommendation.ts @@ -121,18 +121,10 @@ export class Recommendation { this.description = null; } - this.url = this.cleanURL(this.url); this.createdAt.setMilliseconds(0); this.updatedAt?.setMilliseconds(0); } - cleanURL(url: URL) { - url.search = ''; - url.hash = ''; - - return url; - }; - static create(data: RecommendationCreateData) { const id = data.id ?? ObjectId().toString(); diff --git a/ghost/recommendations/test/BookshelfRecommendationRepository.test.ts b/ghost/recommendations/test/BookshelfRecommendationRepository.test.ts index a36005af24..7577bfa05e 100644 --- a/ghost/recommendations/test/BookshelfRecommendationRepository.test.ts +++ b/ghost/recommendations/test/BookshelfRecommendationRepository.test.ts @@ -114,7 +114,35 @@ describe('BookshelfRecommendationRepository', function () { sinon.assert.calledOnce(stub); }); - it('getByUrl returns if matching hostname', async function () { + it('getByUrl returns null if not matching path', async function () { + const repository = new BookshelfRecommendationRepository({} as any, { + sentry: undefined + }); + const recommendation = Recommendation.create({ + id: 'id', + title: 'title', + description: 'description', + excerpt: 'excerpt', + featuredImage: new URL('https://example.com'), + favicon: new URL('https://example.com'), + url: new URL('https://example.com/other-path'), + oneClickSubscribe: true, + createdAt: new Date('2021-01-01'), + updatedAt: new Date('2021-01-02') + }); + const stub = sinon.stub(repository, 'getAll').returns(Promise.resolve([ + recommendation + ])); + const entity = await repository.getByUrl(new URL('https://www.example.com/path')); + + assert.equal( + entity, + null + ); + sinon.assert.calledOnce(stub); + }); + + it('getByUrl returns if matching hostname and pathname', async function () { const repository = new BookshelfRecommendationRepository({} as any, { sentry: undefined }); @@ -142,7 +170,7 @@ describe('BookshelfRecommendationRepository', function () { sinon.assert.calledOnce(stub); }); - it('getByUrl returns null if not matching path', async function () { + it('getByUrl returns if matching hostname and pathname, but not query params', async function () { const repository = new BookshelfRecommendationRepository({} as any, { sentry: undefined }); @@ -153,7 +181,35 @@ describe('BookshelfRecommendationRepository', function () { excerpt: 'excerpt', featuredImage: new URL('https://example.com'), favicon: new URL('https://example.com'), - url: new URL('https://example.com/other-path'), + url: new URL('https://example.com/path'), + oneClickSubscribe: true, + createdAt: new Date('2021-01-01'), + updatedAt: new Date('2021-01-02') + }); + const stub = sinon.stub(repository, 'getAll').returns(Promise.resolve([ + recommendation + ])); + const entity = await repository.getByUrl(new URL('https://www.example.com/path/?query=param')); + + assert.equal( + entity, + recommendation + ); + sinon.assert.calledOnce(stub); + }); + + it('getByUrl returns if matching hostname and pathname, but not hash fragments', async function () { + const repository = new BookshelfRecommendationRepository({} as any, { + sentry: undefined + }); + const recommendation = Recommendation.create({ + id: 'id', + title: 'title', + description: 'description', + excerpt: 'excerpt', + featuredImage: new URL('https://example.com'), + favicon: new URL('https://example.com'), + url: new URL('https://example.com/path/#section1'), oneClickSubscribe: true, createdAt: new Date('2021-01-01'), updatedAt: new Date('2021-01-02') @@ -165,7 +221,7 @@ describe('BookshelfRecommendationRepository', function () { assert.equal( entity, - null + recommendation ); sinon.assert.calledOnce(stub); }); diff --git a/ghost/recommendations/test/Recommendation.test.ts b/ghost/recommendations/test/Recommendation.test.ts index a88e645d70..f7a8fc2357 100644 --- a/ghost/recommendations/test/Recommendation.test.ts +++ b/ghost/recommendations/test/Recommendation.test.ts @@ -118,7 +118,7 @@ describe('Recommendation', function () { assert.equal(recommendation.description, null); }); - it('removes search and hash params', function () { + it('keeps search and hash params', function () { const recommendation = Recommendation.create({ title: 'Test', description: '', @@ -130,7 +130,7 @@ describe('Recommendation', function () { updatedAt: new Date('2021-01-01T00:00:05Z') }); - assert.equal(recommendation.url.toString(), 'https://example.com/'); + assert.equal(recommendation.url.toString(), 'https://example.com/?query=1#hash'); }); });