Added email_track_clicks setting (#15409)

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).
2024-12-23 19:02:29 +03:00 · 2022-09-15 15:48:22 +02:00 · 2022-09-15 15:48:22 +02:00 · 699e67f4e4
commit 699e67f4e4
parent de9d63bb9f
17 changed files with 166 additions and 16 deletions
--- a/ghost/admin/app/components/settings/newsletters.hbs
+++ b/ghost/admin/app/components/settings/newsletters.hbs
@ -162,6 +162,30 @@
                    </div>
                </div>
            </div>
+
+            {{#if this.feature.emailClicks }}
+                <div class="gh-expandable-block">
+                    <div class="gh-expandable-header">
+                        <div>
+                            <h4 class="gh-expandable-title">Enable newsletter click tracking</h4>
+                            <p class="gh-expandable-description">Track how many members are clicking on links in your emails</p>
+                        </div>
+                        <div class="for-switch">
+                            <label class="switch" data-test-label="email-track-opens">
+                                <input
+                                    id="email-track-clicks"
+                                    type="checkbox"
+                                    checked={{this.settings.emailTrackClicks}}
+                                    class="gh-input"
+                                    {{on "change" this.toggleEmailTrackClicks}}
+                                    data-test-checkbox="email-track-clicks"
+                                >
+                                <span class="input-toggle-component mt1"></span>
+                            </label>
+                        </div>
+                    </div>
+                </div>
+            {{/if}}
        </div>
    </section>
 {{/if}}
--- 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) {
--- 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'),
--- 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
--- 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',
--- 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) {
--- 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
@ -0,0 +1,8 @@
+const {addSetting} = require('../../utils');
+
+module.exports = addSetting({
+    key: 'email_track_clicks',
+    value: 'true',
+    type: 'boolean',
+    group: 'email'
+});
--- 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": {
--- 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
        });
    }
 }
--- 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,
--- 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;
--- 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);
--- 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);
        });
    });
--- 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,
--- 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": {
--- 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--');
        }

--- 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')));
            });