From 4718171b1ddd5b164a2cde4c5ec74df671077a9e Mon Sep 17 00:00:00 2001 From: "Fabien \"egg\" O'Carroll" Date: Thu, 25 Aug 2022 12:21:41 -0400 Subject: [PATCH] Removed out of date history items from UrlHistory In case there is an issue with the filtering of items in our client side attribution script, we also check for and remove out of date items here. This ensures that we do not erroneously attribute signups or conversions to webpages from more than 24h ago. --- .../create-stripe-checkout-session.test.js | 4 +- .../e2e-api/members/send-magic-link.test.js | 2 +- .../services/member-attribution.test.js | 62 +++++++++---------- ghost/member-attribution/lib/history.js | 12 +++- .../test/attribution.test.js | 22 ++++--- ghost/member-attribution/test/history.test.js | 12 ++++ 6 files changed, 69 insertions(+), 45 deletions(-) diff --git a/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js b/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js index 70be213a7c..f678177b72 100644 --- a/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js +++ b/ghost/core/test/e2e-api/members/create-stripe-checkout-session.test.js @@ -116,7 +116,7 @@ describe('Create Stripe Checkout Session', function () { urlHistory: [ { path: '/test', - time: 123 + time: Date.now() } ] } @@ -160,7 +160,7 @@ describe('Create Stripe Checkout Session', function () { urlHistory: [ { path: url, - time: 123 + time: Date.now() } ] } diff --git a/ghost/core/test/e2e-api/members/send-magic-link.test.js b/ghost/core/test/e2e-api/members/send-magic-link.test.js index 9fba77554b..95ad4837db 100644 --- a/ghost/core/test/e2e-api/members/send-magic-link.test.js +++ b/ghost/core/test/e2e-api/members/send-magic-link.test.js @@ -100,7 +100,7 @@ describe('sendMagicLink', function () { urlHistory: [ { path: '/test-path', - time: 123 + time: Date.now() } ] }) diff --git a/ghost/core/test/e2e-server/services/member-attribution.test.js b/ghost/core/test/e2e-server/services/member-attribution.test.js index bab38a3b28..3597a006da 100644 --- a/ghost/core/test/e2e-server/services/member-attribution.test.js +++ b/ghost/core/test/e2e-server/services/member-attribution.test.js @@ -24,7 +24,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -32,7 +32,7 @@ describe('Member Attribution Service', function () { url: subdomainRelative, type: 'url' })); - + (await attribution.fetchResource()).should.match(({ id: null, url: absoluteUrl, @@ -45,11 +45,11 @@ describe('Member Attribution Service', function () { const id = fixtureManager.get('posts', 0).id; const post = await models.Post.where('id', id).fetch({require: true}); const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true}); - + const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -57,9 +57,9 @@ describe('Member Attribution Service', function () { url, type: 'post' })); - + const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true}); - + (await attribution.fetchResource()).should.match(({ id: post.id, url: absoluteUrl, @@ -78,7 +78,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); @@ -101,18 +101,18 @@ describe('Member Attribution Service', function () { await models.Post.edit({status: 'published'}, {id}); }); - + it('resolves pages', async function () { const id = fixtureManager.get('posts', 5).id; const post = await models.Post.where('id', id).fetch({require: true}); should(post.get('type')).eql('page'); - + const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true}); - + const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -120,9 +120,9 @@ describe('Member Attribution Service', function () { url, type: 'page' })); - + const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true}); - + (await attribution.fetchResource()).should.match(({ id: post.id, url: absoluteUrl, @@ -130,16 +130,16 @@ describe('Member Attribution Service', function () { title: post.get('title') })); }); - + it('resolves tags', async function () { const id = fixtureManager.get('tags', 0).id; const tag = await models.Tag.where('id', id).fetch({require: true}); const url = urlService.getUrlByResourceId(tag.id, {absolute: false, withSubdirectory: true}); - + const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -147,9 +147,9 @@ describe('Member Attribution Service', function () { url, type: 'tag' })); - + const absoluteUrl = urlService.getUrlByResourceId(tag.id, {absolute: true, withSubdirectory: true}); - + (await attribution.fetchResource()).should.match(({ id: tag.id, url: absoluteUrl, @@ -157,16 +157,16 @@ describe('Member Attribution Service', function () { title: tag.get('name') })); }); - + it('resolves authors', async function () { const id = fixtureManager.get('users', 0).id; const author = await models.User.where('id', id).fetch({require: true}); const url = urlService.getUrlByResourceId(author.id, {absolute: false, withSubdirectory: true}); - + const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -174,9 +174,9 @@ describe('Member Attribution Service', function () { url, type: 'author' })); - + const absoluteUrl = urlService.getUrlByResourceId(author.id, {absolute: true, withSubdirectory: true}); - + (await attribution.fetchResource()).should.match(({ id: author.id, url: absoluteUrl, @@ -190,7 +190,7 @@ describe('Member Attribution Service', function () { beforeEach(function () { configUtils.set('url', 'https://siteurl.com/subdirectory/'); }); - + afterEach(function () { configUtils.restore(); }); @@ -203,7 +203,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -211,7 +211,7 @@ describe('Member Attribution Service', function () { url: subdomainRelative, type: 'url' })); - + (await attribution.fetchResource()).should.match(({ id: null, url: absoluteUrl, @@ -232,7 +232,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); @@ -266,7 +266,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); @@ -299,7 +299,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -327,7 +327,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ @@ -355,7 +355,7 @@ describe('Member Attribution Service', function () { const attribution = memberAttributionService.service.getAttribution([ { path: url, - time: 123 + time: Date.now() } ]); attribution.should.match(({ diff --git a/ghost/member-attribution/lib/history.js b/ghost/member-attribution/lib/history.js index ed644042af..2f1c9b21fd 100644 --- a/ghost/member-attribution/lib/history.js +++ b/ghost/member-attribution/lib/history.js @@ -59,8 +59,18 @@ class UrlHistory { return new UrlHistory([]); } - return new UrlHistory(urlHistory); + const now = Date.now(); + const filteredHistory = urlHistory.filter((item) => { + return now - item.time < this.MAX_AGE; + }); + + return new UrlHistory(filteredHistory); } + + /** + * @private + */ + static MAX_AGE = 1000 * 60 * 60 * 24; } module.exports = UrlHistory; diff --git a/ghost/member-attribution/test/attribution.test.js b/ghost/member-attribution/test/attribution.test.js index 3b1738583f..5fd372973b 100644 --- a/ghost/member-attribution/test/attribution.test.js +++ b/ghost/member-attribution/test/attribution.test.js @@ -6,8 +6,10 @@ const AttributionBuilder = require('../lib/attribution'); describe('AttributionBuilder', function () { let attributionBuilder; + let now; before(function () { + now = Date.now(); attributionBuilder = new AttributionBuilder({ urlTranslator: { getTypeAndId(path) { @@ -61,33 +63,33 @@ describe('AttributionBuilder', function () { }); it('Returns last url', function () { - const history = UrlHistory.create([{path: '/dir/not-last', time: 123}, {path: '/dir/test/', time: 123}]); + const history = UrlHistory.create([{path: '/dir/not-last', time: now + 123}, {path: '/dir/test/', time: now + 123}]); should(attributionBuilder.getAttribution(history)).match({type: 'url', id: null, url: '/test/'}); }); it('Returns last post', function () { const history = UrlHistory.create([ - {path: '/dir/my-post', time: 123}, - {path: '/dir/test', time: 124}, - {path: '/dir/unknown-page', time: 125} + {path: '/dir/my-post', time: now + 123}, + {path: '/dir/test', time: now + 124}, + {path: '/dir/unknown-page', time: now + 125} ]); should(attributionBuilder.getAttribution(history)).match({type: 'post', id: 123, url: '/my-post'}); }); it('Returns last post even when it found pages', function () { const history = UrlHistory.create([ - {path: '/dir/my-post', time: 123}, - {path: '/dir/my-page', time: 124}, - {path: '/dir/unknown-page', time: 125} + {path: '/dir/my-post', time: now + 123}, + {path: '/dir/my-page', time: now + 124}, + {path: '/dir/unknown-page', time: now + 125} ]); should(attributionBuilder.getAttribution(history)).match({type: 'post', id: 123, url: '/my-post'}); }); it('Returns last page if no posts', function () { const history = UrlHistory.create([ - {path: '/dir/other', time: 123}, - {path: '/dir/my-page', time: 124}, - {path: '/dir/unknown-page', time: 125} + {path: '/dir/other', time: now + 123}, + {path: '/dir/my-page', time: now + 124}, + {path: '/dir/unknown-page', time: now + 125} ]); should(attributionBuilder.getAttribution(history)).match({type: 'page', id: 845, url: '/my-page'}); }); diff --git a/ghost/member-attribution/test/history.test.js b/ghost/member-attribution/test/history.test.js index 91316633fa..16d576acc2 100644 --- a/ghost/member-attribution/test/history.test.js +++ b/ghost/member-attribution/test/history.test.js @@ -56,4 +56,16 @@ describe('UrlHistory', function () { should(history.history).eql(input); } }); + + it('removes entries older than 24 hours', function () { + const input = [{ + time: Date.now() - 1000 * 60 * 60 * 25, + path: '/old' + }, { + time: Date.now() - 1000 * 60 * 60 * 23, + path: '/not-old' + }]; + const history = UrlHistory.create(input); + should(history.history).eql([input[1]]); + }); });