From 972c25edc76f3c21398e4a5741c51698d10a8445 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Wed, 14 Sep 2022 21:50:54 +0200 Subject: [PATCH] Wired up member attribution from email clicks (#15407) refs https://github.com/TryGhost/Team/issues/1899 - Added `addEmailAttributionToUrl` method to MemberAttributionService. This adds both the source attribution (`rel=newsletter`) and member attribution (`?attribution_id=123&attribution_type=post`) to a URL. - The URLHistory can now contain a new sort of items: `{type: 'post', id: 'post-id', time: 123}`. - Updated frontend script to read `?attribution_id=123&attribution_type=post` from the URL and add it to the URLHistory + clear it from the URL. - Wired up some external dependencies to LinkReplacementService and added some dummy code. - Increased test coverage of attribution service - Moved all logic that removes the subdirectory from a URL to the UrlTranslator instead of the AttributionBuilder - The UrlTranslator now parses a URLHistoryItem to an object that can be used to build an Attribution instance - Excluded sites with different domain from member id and attribution tracking --- .../member-attribution/member-attribution.js | 27 +++ .../server/services/link-replacement/index.js | 6 +- .../services/member-attribution.test.js | 24 +-- .../link-replacement/lib/link-replacement.js | 60 +++++- ghost/link-replacement/package.json | 2 +- ghost/link-replacement/test/hello.test.js | 10 - .../test/link-replacement.test.js | 107 +++++++++++ .../link-replacement/test/utils/overrides.js | 10 - ghost/member-attribution/lib/attribution.js | 67 ++++--- ghost/member-attribution/lib/history.js | 16 +- ghost/member-attribution/lib/service.js | 37 +++- .../member-attribution/lib/url-translator.js | 41 ++++- .../test/attribution.test.js | 113 +++++++++--- ghost/member-attribution/test/history.test.js | 32 ++++ .../test/url-translator.test.js | 171 ++++++++++++++---- ghost/members-api/lib/controllers/router.js | 4 +- 16 files changed, 583 insertions(+), 144 deletions(-) delete mode 100644 ghost/link-replacement/test/hello.test.js create mode 100644 ghost/link-replacement/test/link-replacement.test.js diff --git a/ghost/core/core/frontend/src/member-attribution/member-attribution.js b/ghost/core/core/frontend/src/member-attribution/member-attribution.js index ac945fba24..0092746e72 100644 --- a/ghost/core/core/frontend/src/member-attribution/member-attribution.js +++ b/ghost/core/core/frontend/src/member-attribution/member-attribution.js @@ -15,6 +15,11 @@ const LIMIT = 15; // "path": "/about/" // }, // { +// "time": 12341234, +// "id": "manually-added-id", +// "type": "post", +// }, +// { // "time": 12341235, // "path": "/welcome/" // } @@ -66,6 +71,28 @@ const LIMIT = 15; history = []; } + // Do we have attributions in the query string? + try { + const url = new URL(window.location.href); + const params = url.searchParams; + if (params.get('attribution_id') && params.get('attribution_type')) { + // Add attribution to history before the current path + history.push({ + time: currentTime, + id: params.get('attribution_id'), + type: params.get('attribution_type') + }); + + // Remove attribution from query string + params.delete('attribution_id'); + params.delete('attribution_type'); + url.search = '?' + params.toString(); + window.history.replaceState({}, '', `${url.pathname}${url.search}${url.hash}`); + } + } catch (error) { + console.error('[Member Attribution] Parsing attribution from querystring failed', error); + } + const currentPath = window.location.pathname; if (history.length === 0 || history[history.length - 1].path !== currentPath) { diff --git a/ghost/core/core/server/services/link-replacement/index.js b/ghost/core/core/server/services/link-replacement/index.js index a2a463e510..e9e63c6f94 100644 --- a/ghost/core/core/server/services/link-replacement/index.js +++ b/ghost/core/core/server/services/link-replacement/index.js @@ -1,3 +1,5 @@ +const urlUtils = require('../../../shared/url-utils'); + class LinkReplacementServiceWrapper { init() { if (this.service) { @@ -11,7 +13,9 @@ class LinkReplacementServiceWrapper { // Expose the service this.service = new LinkReplacementService({ linkRedirectService: require('../link-redirection').service, - linkClickTrackingService: require('../link-click-tracking').service + linkClickTrackingService: require('../link-click-tracking').service, + attributionService: require('../member-attribution').service, + urlUtils }); } } 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 3597a006da..f78d0a57af 100644 --- a/ghost/core/test/e2e-server/services/member-attribution.test.js +++ b/ghost/core/test/e2e-server/services/member-attribution.test.js @@ -21,7 +21,7 @@ describe('Member Attribution Service', function () { const url = urlUtils.createUrl(subdomainRelative, false); const absoluteUrl = urlUtils.createUrl(subdomainRelative, true); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -46,7 +46,7 @@ describe('Member Attribution Service', function () { 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([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -75,7 +75,7 @@ describe('Member Attribution Service', function () { const urlWithoutSubdirectory = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: false}); const absoluteUrl = urlService.getUrlByResourceId(post.id, {absolute: true, withSubdirectory: true}); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -109,7 +109,7 @@ describe('Member Attribution Service', function () { const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true}); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -136,7 +136,7 @@ describe('Member Attribution Service', function () { 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([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -163,7 +163,7 @@ describe('Member Attribution Service', function () { 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([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -200,7 +200,7 @@ describe('Member Attribution Service', function () { const url = urlUtils.createUrl(subdomainRelative, false); const absoluteUrl = urlUtils.createUrl(subdomainRelative, true); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -229,7 +229,7 @@ describe('Member Attribution Service', function () { // Check if we are actually testing with subdirectories should(url).startWith('/subdirectory/'); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -263,7 +263,7 @@ describe('Member Attribution Service', function () { // Check if we are actually testing with subdirectories should(url).startWith('/subdirectory/'); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -296,7 +296,7 @@ describe('Member Attribution Service', function () { const url = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: true}); const urlWithoutSubdirectory = urlService.getUrlByResourceId(post.id, {absolute: false, withSubdirectory: false}); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -324,7 +324,7 @@ describe('Member Attribution Service', function () { const url = urlService.getUrlByResourceId(tag.id, {absolute: false, withSubdirectory: true}); const urlWithoutSubdirectory = urlService.getUrlByResourceId(tag.id, {absolute: false, withSubdirectory: false}); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() @@ -352,7 +352,7 @@ describe('Member Attribution Service', function () { const url = urlService.getUrlByResourceId(author.id, {absolute: false, withSubdirectory: true}); const urlWithoutSubdirectory = urlService.getUrlByResourceId(author.id, {absolute: false, withSubdirectory: false}); - const attribution = memberAttributionService.service.getAttribution([ + const attribution = await memberAttributionService.service.getAttribution([ { path: url, time: Date.now() diff --git a/ghost/link-replacement/lib/link-replacement.js b/ghost/link-replacement/lib/link-replacement.js index 15f3659b82..4b89529b5a 100644 --- a/ghost/link-replacement/lib/link-replacement.js +++ b/ghost/link-replacement/lib/link-replacement.js @@ -1,8 +1,7 @@ /** * @typedef {object} ILinkRedirect * @prop {URL} to - * @prop {from} to - * @prop {from} to + * @prop {URL} from */ /** @@ -12,7 +11,17 @@ /** * @typedef {object} ILinkClickTrackingService - * @prop {(link: ILinkRedirect) => Promise} addTrackingToRedirect + * @prop {(link: ILinkRedirect, uuid: string) => Promise} addTrackingToRedirect + */ + +/** + * @typedef {import('@tryghost/member-attribution/lib/service')} IAttributionService + */ + +/** + * @typedef {object} UrlUtils + * @prop {(context: string, absolute?: boolean) => string} urlFor + * @prop {(...parts: string[]) => string} urlJoin */ class LinkReplacementService { @@ -20,32 +29,63 @@ class LinkReplacementService { #linkRedirectService; /** @type ILinkClickTrackingService */ #linkClickTrackingService; + /** @type IAttributionService */ + #attributionService; + /** @type UrlUtils */ + #urlUtils; /** * @param {object} deps * @param {ILinkRedirectService} deps.linkRedirectService * @param {ILinkClickTrackingService} deps.linkClickTrackingService + * @param {IAttributionService} deps.attributionService + * @param {UrlUtils} deps.urlUtils */ constructor(deps) { this.#linkRedirectService = deps.linkRedirectService; this.#linkClickTrackingService = deps.linkClickTrackingService; + this.#attributionService = deps.attributionService; + this.#urlUtils = deps.urlUtils; + } + + /** + * @private (# doesn't work because this method is being tested) + * Return whether the provided URL is a link to the site + * @param {URL} url + * @returns {boolean} + */ + isSiteDomain(url) { + const siteUrl = new URL(this.#urlUtils.urlFor('home', true)); + if (siteUrl.host === url.host) { + if (url.pathname.startsWith(siteUrl.pathname)) { + return true; + } + return false; + } + return false; } async replaceLink(url, newsletter, post) { // Can probably happen in one call to the MemberAttributionService (but just to make clear what happens here) + const isSite = this.isSiteDomain(url); // 1. Add attribution - // TODO: this should move the the attribution service in the future - url.searchParams.append('rel', newsletter.get('slug') + '-newsletter'); - url.searchParams.append('attribution_id', post.id); - url.searchParams.append('attribution_type', 'post'); + url = this.#attributionService.addEmailSourceAttributionTracking(url, newsletter); + + if (isSite) { + // Only add attribution links to our own site (except for the newsletter referrer) + url = this.#attributionService.addPostAttributionTracking(url, post); + } // 2. Add redirect for link click tracking const redirect = await this.#linkRedirectService.addRedirect(url); - // 3. Add member tracking - const result = await this.#linkClickTrackingService.addTrackingToRedirect(redirect, '--uuid--'); - return result; + // 3. Add click tracking by members + if (isSite) { + return this.#linkClickTrackingService.addTrackingToRedirect(redirect, '--uuid--'); + } + + return redirect.from; } /** diff --git a/ghost/link-replacement/package.json b/ghost/link-replacement/package.json index 330bbe2602..debe985603 100644 --- a/ghost/link-replacement/package.json +++ b/ghost/link-replacement/package.json @@ -7,7 +7,7 @@ "main": "index.js", "scripts": { "dev": "echo \"Implement me!\"", - "test:unit": "NODE_ENV=testing c8 --all --reporter text --reporter cobertura mocha './test/**/*.test.js'", + "test:unit": "NODE_ENV=testing c8 --all --check-coverage --reporter text --reporter cobertura mocha './test/**/*.test.js'", "test": "yarn test:unit", "lint:code": "eslint *.js lib/ --ext .js --cache", "lint": "yarn lint:code && yarn lint:test", diff --git a/ghost/link-replacement/test/hello.test.js b/ghost/link-replacement/test/hello.test.js deleted file mode 100644 index 85d69d1e08..0000000000 --- a/ghost/link-replacement/test/hello.test.js +++ /dev/null @@ -1,10 +0,0 @@ -// Switch these lines once there are useful utils -// const testUtils = require('./utils'); -require('./utils'); - -describe('Hello world', function () { - it('Runs a test', function () { - // TODO: Write me! - 'hello'.should.eql('hello'); - }); -}); diff --git a/ghost/link-replacement/test/link-replacement.test.js b/ghost/link-replacement/test/link-replacement.test.js new file mode 100644 index 0000000000..c6632edaa1 --- /dev/null +++ b/ghost/link-replacement/test/link-replacement.test.js @@ -0,0 +1,107 @@ +// Switch these lines once there are useful utils +// const testUtils = require('./utils'); +const sinon = require('sinon'); +const assert = require('assert'); +const LinkReplacementService = require('../lib/link-replacement'); + +describe('LinkReplacementService', function () { + describe('isSiteDomain', function () { + const serviceWithout = new LinkReplacementService({ + urlUtils: { + urlFor: () => 'http://localhost:2368' + } + }); + + const serviceWith = new LinkReplacementService({ + urlUtils: { + urlFor: () => 'http://localhost:2368/dir' + } + }); + + it('returns true for the root domain', function () { + assert(serviceWithout.isSiteDomain(new URL('http://localhost:2368'))); + assert(serviceWith.isSiteDomain(new URL('http://localhost:2368/dir'))); + }); + + it('returns true for a path along the root domain', function () { + assert(serviceWithout.isSiteDomain(new URL('http://localhost:2368/path'))); + assert(serviceWith.isSiteDomain(new URL('http://localhost:2368/dir/path'))); + }); + + it('returns false for a different domain', function () { + assert(!serviceWithout.isSiteDomain(new URL('https://google.com/path'))); + assert(!serviceWith.isSiteDomain(new URL('https://google.com/dir/path'))); + }); + }); + + describe('replacing links', function () { + const linkRedirectService = { + addRedirect: (to) => { + return Promise.resolve({to, from: 'https://redirected.service/r/ro0sdD92'}); + } + }; + const service = new LinkReplacementService({ + urlUtils: { + urlFor: () => 'http://localhost:2368/dir' + }, + linkRedirectService, + linkClickTrackingService: { + addTrackingToRedirect: (link, uuid) => { + return Promise.resolve(new URL(`${link.from}?m=${uuid}`)); + } + }, + attributionService: { + addEmailSourceAttributionTracking: (url) => { + url.searchParams.append('rel', 'newsletter'); + return url; + }, + addPostAttributionTracking: (url, post) => { + url.searchParams.append('attribution_id', post.id); + return url; + } + } + }); + + let redirectSpy; + + beforeEach(function () { + redirectSpy = sinon.spy(linkRedirectService, 'addRedirect'); + }); + + afterEach(function () { + sinon.restore(); + }); + + describe('replaceLink', function () { + it('returns the redirected URL with uuid', async function () { + const replaced = await service.replaceLink(new URL('http://localhost:2368/dir/path'), {}, {id: 'post_id'}); + assert.equal(replaced.toString(), 'https://redirected.service/r/ro0sdD92?m=--uuid--'); + assert(redirectSpy.calledOnceWithExactly(new URL('http://localhost:2368/dir/path?rel=newsletter&attribution_id=post_id'))); + }); + + it('does not add attribution and member id for external sites', async function () { + const replaced = await service.replaceLink(new URL('http://external.domain/dir/path'), {}, {id: 'post_id'}); + assert.equal(replaced.toString(), 'https://redirected.service/r/ro0sdD92'); + assert(redirectSpy.calledOnceWithExactly(new URL('http://external.domain/dir/path?rel=newsletter'))); + }); + }); + + describe('replaceLinks', function () { + it('Replaces hrefs inside links', async function () { + const html = 'link'; + const expected = 'link'; + + const replaced = await service.replaceLinks(html, {}, {id: 'post_id'}); + assert.equal(replaced, expected); + }); + + it('Ignores invalid links', async function () { + const html = 'link'; + const expected = 'link'; + + const replaced = await service.replaceLinks(html, {}, {id: 'post_id'}); + assert.equal(replaced, expected); + }); + }); + }); +}); diff --git a/ghost/link-replacement/test/utils/overrides.js b/ghost/link-replacement/test/utils/overrides.js index 90203424ee..e69de29bb2 100644 --- a/ghost/link-replacement/test/utils/overrides.js +++ b/ghost/link-replacement/test/utils/overrides.js @@ -1,10 +0,0 @@ -// This file is required before any test is run - -// Taken from the should wiki, this is how to make should global -// Should is a global in our eslint test config -global.should = require('should').noConflict(); -should.extend(); - -// Sinon is a simple case -// Sinon is a global in our eslint test config -global.sinon = require('sinon'); diff --git a/ghost/member-attribution/lib/attribution.js b/ghost/member-attribution/lib/attribution.js index 99876a8d95..84bd22accf 100644 --- a/ghost/member-attribution/lib/attribution.js +++ b/ghost/member-attribution/lib/attribution.js @@ -7,6 +7,7 @@ */ class Attribution { + /** @type {import('./url-translator')} */ #urlTranslator; /** @@ -74,6 +75,9 @@ class Attribution { * Convert a UrlHistory to an attribution object */ class AttributionBuilder { + /** @type {import('./url-translator')} */ + urlTranslator; + /** */ constructor({urlTranslator}) { @@ -93,10 +97,10 @@ class AttributionBuilder { /** * Last Post Algorithm™️ - * @param {UrlHistory} history - * @returns {Attribution} + * @param {import('./history').UrlHistoryArray} history + * @returns {Promise} */ - getAttribution(history) { + async getAttribution(history) { if (history.length === 0) { return this.build({ id: null, @@ -105,51 +109,42 @@ class AttributionBuilder { }); } - // Convert history to subdirectory relative (instead of root relative) - // Note: this is ordered from recent to oldest! - const subdirectoryRelativeHistory = []; - for (const item of history) { - subdirectoryRelativeHistory.push({ - ...item, - path: this.urlTranslator.stripSubdirectoryFromPath(item.path) - }); - } - - // TODO: if something is wrong with the attribution script, and it isn't loading - // we might get out of date URLs - // so we need to check the time of each item and ignore items that are older than 24u here! + // Note: history iterator is ordered from recent to oldest! // Start at the end. Return the first post we find - for (const item of subdirectoryRelativeHistory) { - const typeId = this.urlTranslator.getTypeAndId(item.path); + const resources = []; + for (const item of history) { + const resource = await this.urlTranslator.getResourceDetails(item); - if (typeId && typeId.type === 'post') { - return this.build({ - url: item.path, - ...typeId - }); + if (resource && resource.type === 'post') { + return this.build(resource); + } + + // Store to avoid that we need to look it up again + if (resource) { + resources.push(resource); } } // No post found? - // Try page or tag or author - for (const item of subdirectoryRelativeHistory) { - const typeId = this.urlTranslator.getTypeAndId(item.path); - - if (typeId) { - return this.build({ - url: item.path, - ...typeId - }); + // Return first with an id (page, tag, author) + for (const resource of resources) { + if (resource.id) { + return this.build(resource); } } - // Default to last URL - // In the future we might decide to exclude certain URLs, that can happen here + // No post/page/tag/author found? + // Return the last path that was visited + if (resources.length > 0) { + return this.build(resources[0]); + } + + // We only have history items without a path that have invalid ids return this.build({ id: null, - url: subdirectoryRelativeHistory[0].path, - type: 'url' + url: null, + type: null }); } } diff --git a/ghost/member-attribution/lib/history.js b/ghost/member-attribution/lib/history.js index 2f1c9b21fd..ed5fdfef7e 100644 --- a/ghost/member-attribution/lib/history.js +++ b/ghost/member-attribution/lib/history.js @@ -1,6 +1,8 @@ /** * @typedef {Object} UrlHistoryItem - * @prop {string} path + * @prop {string} [path] + * @prop {string} [id] + * @prop {string} [type] * @prop {number} time */ @@ -8,6 +10,11 @@ * @typedef {UrlHistoryItem[]} UrlHistoryArray */ +/** + * Types allowed to add in the URLHistory manually + */ +const ALLOWED_TYPES = ['post']; + /** * Represents a validated history */ @@ -39,7 +46,12 @@ class UrlHistory { */ static isValidHistory(history) { for (const item of history) { - if (typeof item?.path !== 'string' || !Number.isSafeInteger(item?.time)) { + const isValidIdEntry = typeof item?.id === 'string' && typeof item?.type === 'string' && ALLOWED_TYPES.includes(item.type); + const isValidPathEntry = typeof item?.path === 'string'; + + const isValidEntry = isValidPathEntry || isValidIdEntry; + + if (!isValidEntry || !Number.isSafeInteger(item?.time)) { return false; } } diff --git a/ghost/member-attribution/lib/service.js b/ghost/member-attribution/lib/service.js index 2b5e3f5318..4b815d0f2d 100644 --- a/ghost/member-attribution/lib/service.js +++ b/ghost/member-attribution/lib/service.js @@ -17,11 +17,42 @@ class MemberAttributionService { /** * * @param {import('./history').UrlHistoryArray} historyArray - * @returns {import('./attribution').Attribution} + * @returns {Promise} */ - getAttribution(historyArray) { + async getAttribution(historyArray) { const history = UrlHistory.create(historyArray); - return this.attributionBuilder.getAttribution(history); + return await this.attributionBuilder.getAttribution(history); + } + + /** + * Add some parameters to a URL so that the frontend script can detect this and add the required records + * in the URLHistory. + * @param {URL} url instance that will get updated + * @param {Object} newsletter The newsletter from which a link was clicked + * @returns {URL} + */ + addEmailSourceAttributionTracking(url, newsletter) { + // Create a deep copy + url = new URL(url); + url.searchParams.append('rel', newsletter.get('slug') + '-newsletter'); + return url; + } + + /** + * Add some parameters to a URL so that the frontend script can detect this and add the required records + * in the URLHistory. + * @param {URL} url instance that will get updated + * @param {Object} post The post from which a link was clicked + * @returns {URL} + */ + addPostAttributionTracking(url, post) { + // Create a deep copy + url = new URL(url); + + // Post attribution + url.searchParams.append('attribution_id', post.id); + url.searchParams.append('attribution_type', 'post'); + return url; } /** diff --git a/ghost/member-attribution/lib/url-translator.js b/ghost/member-attribution/lib/url-translator.js index 5398956c96..08fd8ac48b 100644 --- a/ghost/member-attribution/lib/url-translator.js +++ b/ghost/member-attribution/lib/url-translator.js @@ -52,8 +52,45 @@ class UrlTranslator { return url === '/' ? 'homepage' : url; } - getTypeAndId(url) { - const resource = this.urlService.getResource(url); + /** + * Get the resource type and ID from a URLHistory item that was added by the frontend attribution script + * @param {import('./history').UrlHistoryItem} item + * @returns {Promise<{type: string, id: string | null, url: string}|null>} Returns null if the item is invalid + */ + async getResourceDetails(item) { + if (item.type) { + const resource = await this.getResourceById(item.id, item.type); + if (resource) { + return { + type: item.type, + id: item.id, + url: this.getUrlByResourceId(item.id, {absolute: false}) + }; + } + + // Invalid id: ignore + return null; + } + + if (!item.path) { + return null; + } + + const path = this.stripSubdirectoryFromPath(item.path); + return { + type: 'url', + id: null, + ...this.getTypeAndIdFromPath(path), + url: path + }; + } + + /** + * Get the resource type and ID from a path that was visited on the site + * @param {string} path (excluding subdirectory) + */ + getTypeAndIdFromPath(path) { + const resource = this.urlService.getResource(path); if (!resource) { return; } diff --git a/ghost/member-attribution/test/attribution.test.js b/ghost/member-attribution/test/attribution.test.js index 5fd372973b..d677e55c69 100644 --- a/ghost/member-attribution/test/attribution.test.js +++ b/ghost/member-attribution/test/attribution.test.js @@ -12,29 +12,57 @@ describe('AttributionBuilder', function () { now = Date.now(); attributionBuilder = new AttributionBuilder({ urlTranslator: { - getTypeAndId(path) { + getResourceDetails(item) { + if (!item.path) { + if (item.id === 'invalid') { + return null; + } + + return { + id: item.id, + type: item.type, + url: `/${item.type}/${item.id}` + }; + } + + const path = this.stripSubdirectoryFromPath(item.path); + if (path === '/my-post') { return { id: 123, - type: 'post' + type: 'post', + url: path }; } if (path === '/my-page') { return { id: 845, - type: 'page' + type: 'page', + url: path }; } - return; + return { + id: null, + type: 'url', + url: path + }; }, - getResourceById(id) { + getResourceById(id, type) { if (id === 'invalid') { return null; } return { id, - get() { - return 'Title'; + get(prop) { + if (prop === 'title' && type === 'author') { + // Simulate an author doesn't have a title + return undefined; + } + if (id === 'no-props') { + // Simulate a model without properties for branch coverage + return undefined; + } + return prop; } }; }, @@ -57,55 +85,76 @@ describe('AttributionBuilder', function () { }); }); - it('Returns empty if empty history', function () { + it('Returns empty if empty history', async function () { const history = UrlHistory.create([]); - should(attributionBuilder.getAttribution(history)).match({id: null, type: null, url: null}); + should(await attributionBuilder.getAttribution(history)).match({id: null, type: null, url: null}); }); - it('Returns last url', function () { + it('Returns last url', async function () { 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/'}); + should(await attributionBuilder.getAttribution(history)).match({type: 'url', id: null, url: '/test/'}); }); - it('Returns last post', function () { + it('Returns last post', async function () { const history = UrlHistory.create([ {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'}); + should(await attributionBuilder.getAttribution(history)).match({type: 'post', id: 123, url: '/my-post'}); }); - it('Returns last post even when it found pages', function () { + it('Returns last post even when it found pages', async function () { const history = UrlHistory.create([ {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'}); + should(await attributionBuilder.getAttribution(history)).match({type: 'post', id: 123, url: '/my-post'}); }); - it('Returns last page if no posts', function () { + it('Returns last page if no posts', async function () { const history = UrlHistory.create([ {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'}); + should(await attributionBuilder.getAttribution(history)).match({type: 'page', id: 845, url: '/my-page'}); }); - it('Returns all null for invalid histories', function () { - const history = UrlHistory.create('invalid'); - should(attributionBuilder.getAttribution(history)).match({ + it('Returns last post via id', async function () { + const history = UrlHistory.create([ + {path: '/dir/other', time: now + 123}, + {id: '123', type: 'post', time: now + 124}, + {path: '/dir/unknown-page', time: now + 125} + ]); + should(await attributionBuilder.getAttribution(history)).match({type: 'post', id: '123', url: '/post/123'}); + }); + + it('Returns all null if only invalid ids', async function () { + const history = UrlHistory.create([ + {id: 'invalid', type: 'post', time: now + 124}, + {id: 'invalid', type: 'post', time: now + 124} + ]); + should(await attributionBuilder.getAttribution(history)).match({ type: null, id: null, url: null }); }); - it('Returns all null for empty histories', function () { + it('Returns all null for invalid histories', async function () { + const history = UrlHistory.create('invalid'); + should(await attributionBuilder.getAttribution(history)).match({ + type: null, + id: null, + url: null + }); + }); + + it('Returns all null for empty histories', async function () { const history = UrlHistory.create([]); - should(attributionBuilder.getAttribution(history)).match({ + should(await attributionBuilder.getAttribution(history)).match({ type: null, id: null, url: null @@ -117,7 +166,25 @@ describe('AttributionBuilder', function () { type: 'post', id: '123', url: 'https://absolute/dir/path', - title: 'Title' + title: 'title' + }); + }); + + it('Returns author resource', async function () { + should(await attributionBuilder.build({type: 'author', id: '123', url: '/author'}).fetchResource()).match({ + type: 'author', + id: '123', + url: 'https://absolute/dir/path', + title: 'name' + }); + }); + + it('Returns default url title for resource if no title or name', async function () { + should(await attributionBuilder.build({type: 'post', id: 'no-props', url: '/post'}).fetchResource()).match({ + type: 'post', + id: 'no-props', + url: 'https://absolute/dir/path', + title: '/post' }); }); diff --git a/ghost/member-attribution/test/history.test.js b/ghost/member-attribution/test/history.test.js index 16d576acc2..2d639d6bea 100644 --- a/ghost/member-attribution/test/history.test.js +++ b/ghost/member-attribution/test/history.test.js @@ -34,6 +34,29 @@ describe('UrlHistory', function () { }], [{ time: 123 + }], + [{ + time: 123, + type: 'post' + }], + [{ + time: 123, + id: 'id' + }], + [{ + time: 123, + type: 123, + id: 'test' + }], + [{ + time: 123, + type: 'invalid', + id: 'test' + }], + [{ + time: 123, + type: 'post', + id: 123 }] ]; @@ -49,6 +72,11 @@ describe('UrlHistory', function () { [{ time: Date.now(), path: '/test' + }], + [{ + time: Date.now(), + type: 'post', + id: '123' }] ]; for (const input of inputs) { @@ -64,6 +92,10 @@ describe('UrlHistory', function () { }, { time: Date.now() - 1000 * 60 * 60 * 23, path: '/not-old' + }, { + time: Date.now() - 1000 * 60 * 60 * 25, + type: 'post', + id: 'old' }]; const history = UrlHistory.create(input); should(history.history).eql([input[1]]); diff --git a/ghost/member-attribution/test/url-translator.test.js b/ghost/member-attribution/test/url-translator.test.js index fe462eff45..61d06948fa 100644 --- a/ghost/member-attribution/test/url-translator.test.js +++ b/ghost/member-attribution/test/url-translator.test.js @@ -3,6 +3,33 @@ require('./utils'); const UrlTranslator = require('../lib/url-translator'); +const models = { + Post: { + findOne({id}) { + if (id === 'invalid') { + return null; + } + return {id: 'post_id', get: () => 'Title'}; + } + }, + User: { + findOne({id}) { + if (id === 'invalid') { + return null; + } + return {id: 'user_id', get: () => 'Title'}; + } + }, + Tag: { + findOne({id}) { + if (id === 'invalid') { + return null; + } + return {id: 'tag_id', get: () => 'Title'}; + } + } +}; + describe('UrlTranslator', function () { describe('Constructor', function () { it('doesn\'t throw', function () { @@ -10,7 +37,112 @@ describe('UrlTranslator', function () { }); }); - describe('getTypeAndId', function () { + describe('getResourceDetails', function () { + let translator; + before(function () { + translator = new UrlTranslator({ + urlUtils: { + relativeToAbsolute: (t) => { + return 'https://absolute' + t; + }, + absoluteToRelative: (t) => { + return t.replace('https://absolute/with-subdirectory', '').replace('https://absolute', ''); + } + }, + urlService: { + getUrlByResourceId: (id) => { + return '/path/' + id; + }, + getResource: (path) => { + switch (path) { + case '/path/post': return { + config: {type: 'posts'}, + data: {id: 'post'} + }; + case '/path/tag': return { + config: {type: 'tags'}, + data: {id: 'tag'} + }; + case '/path/page': return { + config: {type: 'pages'}, + data: {id: 'page'} + }; + case '/path/author': return { + config: {type: 'authors'}, + data: {id: 'author'} + }; + } + } + }, + models + }); + }); + + it('returns posts for explicit items', async function () { + should(await translator.getResourceDetails({id: 'my-post', type: 'post', time: 123})).eql({ + type: 'post', + id: 'my-post', + url: '/path/my-post' + }); + }); + + it('returns null if explicit resource not found', async function () { + should(await translator.getResourceDetails({id: 'invalid', type: 'post', time: 123})).eql(null); + }); + + it('returns null for invalid item', async function () { + should(await translator.getResourceDetails({time: 123})).eql(null); + }); + + it('returns url type if no path not matching a resource', async function () { + should(await translator.getResourceDetails({path: '/test', time: 123})).eql({ + type: 'url', + id: null, + url: '/test' + }); + }); + + it('strips subdirectory for url types', async function () { + should(await translator.getResourceDetails({path: '/with-subdirectory/test', time: 123})).eql({ + type: 'url', + id: null, + url: '/test' + }); + }); + + it('returns post type if matching resource', async function () { + should(await translator.getResourceDetails({path: '/with-subdirectory/path/post', time: 123})).eql({ + type: 'post', + id: 'post', + url: '/path/post' + }); + }); + + it('returns page type if matching resource', async function () { + should(await translator.getResourceDetails({path: '/with-subdirectory/path/page', time: 123})).eql({ + type: 'page', + id: 'page', + url: '/path/page' + }); + }); + }); + + describe('getUrlTitle', function () { + let translator; + before(function () { + translator = new UrlTranslator({}); + }); + + it('returns homepage', function () { + should(translator.getUrlTitle('/')).eql('homepage'); + }); + + it('returns url', function () { + should(translator.getUrlTitle('/url')).eql('/url'); + }); + }); + + describe('getTypeAndIdFromPath', function () { let translator; before(function () { translator = new UrlTranslator({ @@ -40,35 +172,35 @@ describe('UrlTranslator', function () { }); it('returns posts', function () { - should(translator.getTypeAndId('/post')).eql({ + should(translator.getTypeAndIdFromPath('/post')).eql({ type: 'post', id: 'post' }); }); it('returns pages', function () { - should(translator.getTypeAndId('/page')).eql({ + should(translator.getTypeAndIdFromPath('/page')).eql({ type: 'page', id: 'page' }); }); it('returns authors', function () { - should(translator.getTypeAndId('/author')).eql({ + should(translator.getTypeAndIdFromPath('/author')).eql({ type: 'author', id: 'author' }); }); it('returns tags', function () { - should(translator.getTypeAndId('/tag')).eql({ + should(translator.getTypeAndIdFromPath('/tag')).eql({ type: 'tag', id: 'tag' }); }); it('returns undefined', function () { - should(translator.getTypeAndId('/other')).eql(undefined); + should(translator.getTypeAndIdFromPath('/other')).eql(undefined); }); }); @@ -81,32 +213,7 @@ describe('UrlTranslator', function () { return '/path'; } }, - models: { - Post: { - findOne({id}) { - if (id === 'invalid') { - return null; - } - return {id: 'post_id', get: () => 'Title'}; - } - }, - User: { - findOne({id}) { - if (id === 'invalid') { - return null; - } - return {id: 'user_id', get: () => 'Title'}; - } - }, - Tag: { - findOne({id}) { - if (id === 'invalid') { - return null; - } - return {id: 'tag_id', get: () => 'Title'}; - } - } - } + models }); }); diff --git a/ghost/members-api/lib/controllers/router.js b/ghost/members-api/lib/controllers/router.js index 0818ccd87c..70e7978856 100644 --- a/ghost/members-api/lib/controllers/router.js +++ b/ghost/members-api/lib/controllers/router.js @@ -212,7 +212,7 @@ module.exports = class RouterController { const urlHistory = metadata.urlHistory; delete metadata.urlHistory; - const attribution = this._memberAttributionService.getAttribution(urlHistory); + const attribution = await this._memberAttributionService.getAttribution(urlHistory); // Don't set null properties if (attribution.id) { @@ -412,7 +412,7 @@ module.exports = class RouterController { tokenData.reqIp = req.ip; } // Save attribution data in the tokenData - tokenData.attribution = this._memberAttributionService.getAttribution(req.body.urlHistory); + tokenData.attribution = await this._memberAttributionService.getAttribution(req.body.urlHistory); await this._sendEmailWithMagicLink({email, tokenData, requestedType: emailType, referrer: referer}); }