From 699e67f4e4d267f98b1c782a6ebe0ef86d5ec50a Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Thu, 15 Sep 2022 15:48:22 +0200 Subject: [PATCH] Added `email_track_clicks` setting (#15409) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://github.com/TryGhost/Team/issues/1900 refs https://github.com/TryGhost/Team/issues/1901 - Defaults to the same value as the current email_track_opens setting for existing installations, otherwise defaults to true - Had to use a custom migration because the `addSetting` helper doesn't support using an existing setting as current value - Added a minimal UI to change the setting, but this still needs some design magic 🪄✨ - Link replacement is disabled if `email_track_clicks` is disabled. In the future we might consider to still do parial additions, such as source attribution and maybe redirects (to discuss). --- .../app/components/settings/newsletters.hbs | 24 +++++++++ .../app/components/settings/newsletters.js | 9 ++++ ghost/admin/app/models/setting.js | 1 + ghost/admin/mirage/fixtures/settings.js | 1 + .../utils/serializers/input/settings.js | 1 + .../server/data/migrations/utils/settings.js | 4 +- ...14-12-46-add-email-track-clicks-setting.js | 8 +++ .../default-settings/default-settings.json | 8 +++ .../server/services/link-replacement/index.js | 7 +-- .../admin/__snapshots__/settings.test.js.snap | 30 ++++++++++- .../core/test/e2e-api/admin/settings.test.js | 6 +-- .../regression/models/model_settings.test.js | 2 +- .../unit/server/data/exporter/index.test.js | 2 +- .../unit/server/data/schema/integrity.test.js | 2 +- .../test/utils/fixtures/default-settings.json | 8 +++ .../link-replacement/lib/link-replacement.js | 17 +++++- .../test/link-replacement.test.js | 52 ++++++++++++++++++- 17 files changed, 166 insertions(+), 16 deletions(-) create mode 100644 ghost/core/core/server/data/migrations/versions/5.15/2022-09-14-12-46-add-email-track-clicks-setting.js diff --git a/ghost/admin/app/components/settings/newsletters.hbs b/ghost/admin/app/components/settings/newsletters.hbs index 282d02e729..ef43962d0b 100644 --- a/ghost/admin/app/components/settings/newsletters.hbs +++ b/ghost/admin/app/components/settings/newsletters.hbs @@ -162,6 +162,30 @@ + + {{#if this.feature.emailClicks }} +
+
+
+

Enable newsletter click tracking

+

Track how many members are clicking on links in your emails

+
+
+ +
+
+
+ {{/if}} {{/if}} diff --git a/ghost/admin/app/components/settings/newsletters.js b/ghost/admin/app/components/settings/newsletters.js index 0d9af64cc7..f0ddb7bdb8 100644 --- a/ghost/admin/app/components/settings/newsletters.js +++ b/ghost/admin/app/components/settings/newsletters.js @@ -9,6 +9,7 @@ const EU = {flag: '🇪🇺', name: 'EU', baseUrl: 'https://api.eu.mailgun.net/v export default class Newsletters extends Component { @service config; @service settings; + @service feature; // set recipientsSelectValue as a static property because within this // component's lifecycle it's not always derived from the settings values. @@ -69,6 +70,14 @@ export default class Newsletters extends Component { this.settings.set('emailTrackOpens', !this.settings.get('emailTrackOpens')); } + @action + toggleEmailTrackClicks(event) { + if (event) { + event.preventDefault(); + } + this.settings.set('emailTrackClicks', !this.settings.get('emailTrackClicks')); + } + @action toggleEmailNewsletterEnabled(event) { if (event) { diff --git a/ghost/admin/app/models/setting.js b/ghost/admin/app/models/setting.js index 19e7d3aa7d..44b83f830d 100644 --- a/ghost/admin/app/models/setting.js +++ b/ghost/admin/app/models/setting.js @@ -43,6 +43,7 @@ export default Model.extend(ValidationEngine, { mailgunDomain: attr('string'), mailgunBaseUrl: attr('string'), emailTrackOpens: attr('boolean'), + emailTrackClicks: attr('boolean'), portalButton: attr('boolean'), portalName: attr('boolean'), portalPlans: attr('json-string'), diff --git a/ghost/admin/mirage/fixtures/settings.js b/ghost/admin/mirage/fixtures/settings.js index 073c6c2cb4..b863380b9f 100644 --- a/ghost/admin/mirage/fixtures/settings.js +++ b/ghost/admin/mirage/fixtures/settings.js @@ -86,6 +86,7 @@ export default [ setting('email', 'mailgun_api_key', null), setting('email', 'mailgun_base_url', null), setting('email', 'email_track_opens', 'true'), + setting('email', 'email_track_clicks', 'true'), setting('email', 'email_verification_required', 'false'), // AMP diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js b/ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js index 4152e57c7f..29f48c11fa 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js @@ -47,6 +47,7 @@ const EDITABLE_SETTINGS = [ 'mailgun_domain', 'mailgun_base_url', 'email_track_opens', + 'email_track_clicks', 'amp', 'amp_gtag_id', 'slack_url', diff --git a/ghost/core/core/server/data/migrations/utils/settings.js b/ghost/core/core/server/data/migrations/utils/settings.js index 2244937067..d0dafbdc4f 100644 --- a/ghost/core/core/server/data/migrations/utils/settings.js +++ b/ghost/core/core/server/data/migrations/utils/settings.js @@ -32,9 +32,7 @@ function addSetting({key, value, type, group}) { group, type, created_at: now, - created_by: MIGRATION_USER, - updated_at: now, - updated_by: MIGRATION_USER + created_by: MIGRATION_USER }); }, async function down(connection) { diff --git a/ghost/core/core/server/data/migrations/versions/5.15/2022-09-14-12-46-add-email-track-clicks-setting.js b/ghost/core/core/server/data/migrations/versions/5.15/2022-09-14-12-46-add-email-track-clicks-setting.js new file mode 100644 index 0000000000..b90fbcd3ac --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/5.15/2022-09-14-12-46-add-email-track-clicks-setting.js @@ -0,0 +1,8 @@ +const {addSetting} = require('../../utils'); + +module.exports = addSetting({ + key: 'email_track_clicks', + value: 'true', + type: 'boolean', + group: 'email' +}); diff --git a/ghost/core/core/server/data/schema/default-settings/default-settings.json b/ghost/core/core/server/data/schema/default-settings/default-settings.json index 533c0a123d..67d64ba48d 100644 --- a/ghost/core/core/server/data/schema/default-settings/default-settings.json +++ b/ghost/core/core/server/data/schema/default-settings/default-settings.json @@ -360,6 +360,14 @@ }, "type": "boolean" }, + "email_track_clicks": { + "defaultValue": "true", + "validations": { + "isEmpty": false, + "isIn": [["true", "false"]] + }, + "type": "boolean" + }, "email_verification_required": { "defaultValue": "false", "validations": { diff --git a/ghost/core/core/server/services/link-replacement/index.js b/ghost/core/core/server/services/link-replacement/index.js index e9e63c6f94..5f906089fe 100644 --- a/ghost/core/core/server/services/link-replacement/index.js +++ b/ghost/core/core/server/services/link-replacement/index.js @@ -1,5 +1,3 @@ -const urlUtils = require('../../../shared/url-utils'); - class LinkReplacementServiceWrapper { init() { if (this.service) { @@ -9,13 +7,16 @@ class LinkReplacementServiceWrapper { // Wire up all the dependencies const LinkReplacementService = require('@tryghost/link-replacement'); + const urlUtils = require('../../../shared/url-utils'); + const settingsCache = require('../../../shared/settings-cache'); // Expose the service this.service = new LinkReplacementService({ linkRedirectService: require('../link-redirection').service, linkClickTrackingService: require('../link-click-tracking').service, attributionService: require('../member-attribution').service, - urlUtils + urlUtils, + settingsCache }); } } diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap index 474c647cef..2fe96fcef6 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap @@ -208,6 +208,10 @@ Object { "key": "email_track_opens", "value": true, }, + Object { + "key": "email_track_clicks", + "value": true, + }, Object { "key": "email_verification_required", "value": false, @@ -547,6 +551,10 @@ Object { "key": "email_track_opens", "value": true, }, + Object { + "key": "email_track_clicks", + "value": true, + }, Object { "key": "email_verification_required", "value": false, @@ -619,7 +627,7 @@ exports[`Settings API Edit Can edit a setting 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": "3408", + "content-length": "3450", "content-type": "application/json; charset=utf-8", "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, "vary": "Origin, Accept-Encoding", @@ -836,6 +844,10 @@ Object { "key": "email_track_opens", "value": true, }, + Object { + "key": "email_track_clicks", + "value": true, + }, Object { "key": "email_verification_required", "value": false, @@ -1124,6 +1136,10 @@ Object { "key": "email_track_opens", "value": true, }, + Object { + "key": "email_track_clicks", + "value": true, + }, Object { "key": "email_verification_required", "value": false, @@ -1417,6 +1433,10 @@ Object { "key": "email_track_opens", "value": true, }, + Object { + "key": "email_track_clicks", + "value": true, + }, Object { "key": "email_verification_required", "value": false, @@ -1705,6 +1725,10 @@ Object { "key": "email_track_opens", "value": true, }, + Object { + "key": "email_track_clicks", + "value": true, + }, Object { "key": "email_verification_required", "value": false, @@ -2056,6 +2080,10 @@ Object { "key": "email_track_opens", "value": true, }, + Object { + "key": "email_track_clicks", + "value": true, + }, Object { "key": "email_verification_required", "value": false, diff --git a/ghost/core/test/e2e-api/admin/settings.test.js b/ghost/core/test/e2e-api/admin/settings.test.js index 84464804dd..05f113c081 100644 --- a/ghost/core/test/e2e-api/admin/settings.test.js +++ b/ghost/core/test/e2e-api/admin/settings.test.js @@ -7,7 +7,7 @@ const {stringMatching, anyEtag, anyUuid, anyStringNumber} = matchers; const models = require('../../../core/server/models'); const {anyErrorId} = matchers; -const CURRENT_SETTINGS_COUNT = 67; +const CURRENT_SETTINGS_COUNT = 68; const settingsMatcher = {}; @@ -27,9 +27,9 @@ const matchSettingsArray = (length) => { settingsArray[25] = publicHashSettingMatcher; } - if (length > 56) { + if (length > 57) { // Item at index 56 is the lab settings, which changes as we add and remove features - settingsArray[56] = labsSettingMatcher; + settingsArray[57] = labsSettingMatcher; } return settingsArray; diff --git a/ghost/core/test/regression/models/model_settings.test.js b/ghost/core/test/regression/models/model_settings.test.js index 9ce3872130..ab3008a8ff 100644 --- a/ghost/core/test/regression/models/model_settings.test.js +++ b/ghost/core/test/regression/models/model_settings.test.js @@ -5,7 +5,7 @@ const db = require('../../../core/server/data/db'); // Stuff we are testing const models = require('../../../core/server/models'); -const SETTINGS_LENGTH = 77; +const SETTINGS_LENGTH = 78; describe('Settings Model', function () { before(models.init); diff --git a/ghost/core/test/unit/server/data/exporter/index.test.js b/ghost/core/test/unit/server/data/exporter/index.test.js index 112f2e4d25..a00d776a97 100644 --- a/ghost/core/test/unit/server/data/exporter/index.test.js +++ b/ghost/core/test/unit/server/data/exporter/index.test.js @@ -234,7 +234,7 @@ describe('Exporter', function () { // NOTE: if default settings changed either modify the settings keys blocklist or increase allowedKeysLength // This is a reminder to think about the importer/exporter scenarios ;) - const allowedKeysLength = 70; + const allowedKeysLength = 71; totalKeysLength.should.eql(SETTING_KEYS_BLOCKLIST.length + allowedKeysLength); }); }); diff --git a/ghost/core/test/unit/server/data/schema/integrity.test.js b/ghost/core/test/unit/server/data/schema/integrity.test.js index 5ef199756c..a330dedd85 100644 --- a/ghost/core/test/unit/server/data/schema/integrity.test.js +++ b/ghost/core/test/unit/server/data/schema/integrity.test.js @@ -37,7 +37,7 @@ describe('DB version integrity', function () { // Only these variables should need updating const currentSchemaHash = '9cc4c1dae2237d960081d77aa4a528cc'; const currentFixturesHash = '8cf221f0ed930ac1fe8030a58e60d64b'; - const currentSettingsHash = 'd54210758b7054e2174fd34aa2320ad7'; + const currentSettingsHash = '2978a5684a2d5fcf089f61f5d368a0c0'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01'; // If this test is failing, then it is likely a change has been made that requires a DB version bump, diff --git a/ghost/core/test/utils/fixtures/default-settings.json b/ghost/core/test/utils/fixtures/default-settings.json index b46bb8eb46..39d9ca248c 100644 --- a/ghost/core/test/utils/fixtures/default-settings.json +++ b/ghost/core/test/utils/fixtures/default-settings.json @@ -360,6 +360,14 @@ }, "type": "boolean" }, + "email_track_clicks": { + "defaultValue": "true", + "validations": { + "isEmpty": false, + "isIn": [["true", "false"]] + }, + "type": "boolean" + }, "email_verification_required": { "defaultValue": "false", "validations": { diff --git a/ghost/link-replacement/lib/link-replacement.js b/ghost/link-replacement/lib/link-replacement.js index 4b89529b5a..7ac76f3f8d 100644 --- a/ghost/link-replacement/lib/link-replacement.js +++ b/ghost/link-replacement/lib/link-replacement.js @@ -24,6 +24,11 @@ * @prop {(...parts: string[]) => string} urlJoin */ +/** + * @typedef {object} SettingsCache + * @prop {(key: string, options?: any) => any} get + */ + class LinkReplacementService { /** @type ILinkRedirectService */ #linkRedirectService; @@ -33,6 +38,8 @@ class LinkReplacementService { #attributionService; /** @type UrlUtils */ #urlUtils; + /** @type SettingsCache */ + #settingsCache; /** * @param {object} deps @@ -40,12 +47,14 @@ class LinkReplacementService { * @param {ILinkClickTrackingService} deps.linkClickTrackingService * @param {IAttributionService} deps.attributionService * @param {UrlUtils} deps.urlUtils + * @param {SettingsCache} deps.settingsCache */ constructor(deps) { this.#linkRedirectService = deps.linkRedirectService; this.#linkClickTrackingService = deps.linkClickTrackingService; this.#attributionService = deps.attributionService; this.#urlUtils = deps.urlUtils; + this.#settingsCache = deps.settingsCache; } /** @@ -68,11 +77,12 @@ class LinkReplacementService { 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); + const enableTracking = this.#settingsCache.get('email_track_clicks'); // 1. Add attribution url = this.#attributionService.addEmailSourceAttributionTracking(url, newsletter); - if (isSite) { + if (isSite && enableTracking) { // Only add attribution links to our own site (except for the newsletter referrer) url = this.#attributionService.addPostAttributionTracking(url, post); } @@ -81,7 +91,10 @@ class LinkReplacementService { const redirect = await this.#linkRedirectService.addRedirect(url); // 3. Add click tracking by members - if (isSite) { + // Note: we can always add the tracking params (even when isSite === false) + // because they are added to the redirect and not the destination URL + + if (enableTracking) { return this.#linkClickTrackingService.addTrackingToRedirect(redirect, '--uuid--'); } diff --git a/ghost/link-replacement/test/link-replacement.test.js b/ghost/link-replacement/test/link-replacement.test.js index c6632edaa1..a14c674395 100644 --- a/ghost/link-replacement/test/link-replacement.test.js +++ b/ghost/link-replacement/test/link-replacement.test.js @@ -5,16 +5,26 @@ const assert = require('assert'); const LinkReplacementService = require('../lib/link-replacement'); describe('LinkReplacementService', function () { + it('exported', function () { + assert.equal(require('../index'), LinkReplacementService); + }); + describe('isSiteDomain', function () { const serviceWithout = new LinkReplacementService({ urlUtils: { urlFor: () => 'http://localhost:2368' + }, + settingsCache: { + get: () => true } }); const serviceWith = new LinkReplacementService({ urlUtils: { urlFor: () => 'http://localhost:2368/dir' + }, + settingsCache: { + get: () => true } }); @@ -32,6 +42,12 @@ describe('LinkReplacementService', function () { assert(!serviceWithout.isSiteDomain(new URL('https://google.com/path'))); assert(!serviceWith.isSiteDomain(new URL('https://google.com/dir/path'))); }); + + it('returns false if not on same subdirectory', function () { + assert(!serviceWith.isSiteDomain(new URL('http://localhost:2368/different-dir'))); + // Check if the matching is not dumb and only matches at the start + assert(!serviceWith.isSiteDomain(new URL('http://localhost:2368/different/dir'))); + }); }); describe('replacing links', function () { @@ -59,6 +75,34 @@ describe('LinkReplacementService', function () { url.searchParams.append('attribution_id', post.id); return url; } + }, + settingsCache: { + get: () => true + } + }); + + const disabledService = 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; + } + }, + settingsCache: { + get: () => false } }); @@ -79,8 +123,14 @@ describe('LinkReplacementService', function () { 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 () { + it('does not add attribution 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?m=--uuid--'); + assert(redirectSpy.calledOnceWithExactly(new URL('http://external.domain/dir/path?rel=newsletter'))); + }); + + it('does not add attribution or member tracking if click tracking is disabled', async function () { + const replaced = await disabledService.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'))); });