From 104baab557e418a6d521a2d43b659b82661ac74b Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Tue, 18 Jul 2023 09:58:01 +0100
Subject: [PATCH] FIX: Do not track first AJAX request as a pageview (#22661)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When the app boots, Ember fires a `routeWillChange` event. This was causing us to set the `_trackView` flag in our ajax library, which would cause the next request to have the `Discourse-Track-View` header, despite not being relevant to the page view. Depending on the plugins/themes installed, this could lead to 'double counting' of pageviews. (because the initial HTML request is also counted as a page view)

This commit updates the the logic to ignore the first transition (by checking `transition.from`), and also introduces an acceptance test for the behaviour.

Co-authored-by: Régis Hanol <regis@hanol.fr>
---
 .../instance-initializers/page-tracking.js    | 12 +++++-
 .../javascripts/discourse/app/lib/ajax.js     |  6 ++-
 .../tests/acceptance/page-tracking-test.js    | 39 +++++++++++++++++++
 3 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 app/assets/javascripts/discourse/tests/acceptance/page-tracking-test.js

diff --git a/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js b/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js
index 5a0a1ffe56e..804cfd26d6d 100644
--- a/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js
+++ b/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js
@@ -3,7 +3,7 @@ import {
   resetPageTracking,
   startPageTracking,
 } from "discourse/lib/page-tracker";
-import { viewTrackingRequired } from "discourse/lib/ajax";
+import { resetAjax, trackNextAjaxAsPageview } from "discourse/lib/ajax";
 
 export default {
   after: "inject-objects",
@@ -11,7 +11,7 @@ export default {
   initialize(owner) {
     // Tell our AJAX system to track a page transition
     const router = owner.lookup("router:main");
-    router.on("routeWillChange", viewTrackingRequired);
+    router.on("routeWillChange", this.handleRouteWillChange);
 
     let appEvents = owner.lookup("service:app-events");
     let documentTitle = owner.lookup("service:document-title");
@@ -64,7 +64,15 @@ export default {
     }
   },
 
+  handleRouteWillChange(transition) {
+    // will be null on initial boot transition, which is already tracked as a pageview via the HTML request
+    if (transition.from) {
+      trackNextAjaxAsPageview();
+    }
+  },
+
   teardown() {
     resetPageTracking();
+    resetAjax();
   },
 };
diff --git a/app/assets/javascripts/discourse/app/lib/ajax.js b/app/assets/javascripts/discourse/app/lib/ajax.js
index 4ec61986c91..e33b0e4a61f 100644
--- a/app/assets/javascripts/discourse/app/lib/ajax.js
+++ b/app/assets/javascripts/discourse/app/lib/ajax.js
@@ -15,10 +15,14 @@ export function setTransientHeader(key, value) {
   _transientHeader = { key, value };
 }
 
-export function viewTrackingRequired() {
+export function trackNextAjaxAsPageview() {
   _trackView = true;
 }
 
+export function resetAjax() {
+  _trackView = false;
+}
+
 export function setLogoffCallback(cb) {
   _logoffCallback = cb;
 }
diff --git a/app/assets/javascripts/discourse/tests/acceptance/page-tracking-test.js b/app/assets/javascripts/discourse/tests/acceptance/page-tracking-test.js
new file mode 100644
index 00000000000..bcbfedaa28f
--- /dev/null
+++ b/app/assets/javascripts/discourse/tests/acceptance/page-tracking-test.js
@@ -0,0 +1,39 @@
+import { acceptance } from "discourse/tests/helpers/qunit-helpers";
+import { click, visit } from "@ember/test-helpers";
+import { test } from "qunit";
+import pretender from "discourse/tests/helpers/create-pretender";
+
+acceptance("Page tracking", function () {
+  test("sets the discourse-track-view header correctly", async function (assert) {
+    const trackViewHeaderName = "Discourse-Track-View";
+    assert.strictEqual(
+      pretender.handledRequests.length,
+      0,
+      "no requests logged before app boot"
+    );
+
+    await visit("/");
+    assert.strictEqual(
+      pretender.handledRequests.length,
+      1,
+      "one request logged during app boot"
+    );
+    assert.strictEqual(
+      pretender.handledRequests[0].requestHeaders[trackViewHeaderName],
+      undefined,
+      "does not track view for ajax before a transition has taken place"
+    );
+
+    await click("#site-logo");
+    assert.strictEqual(
+      pretender.handledRequests.length,
+      2,
+      "second request logged during next transition"
+    );
+    assert.strictEqual(
+      pretender.handledRequests[1].requestHeaders[trackViewHeaderName],
+      "true",
+      "sends track-view header for subsequent requests"
+    );
+  });
+});