FIX: Avoid double-counting pageviews when navigating with loading spinner (#23107)

104baab5 fixed double-counted pageviews for the initial page load. Under the default 'loading slider' implementation, that resolved all the known problems.

However, under the 'loading spinner', there is an additional problem. In 'spinner' mode, each navigation within the JS app involves transitioning to an intermediate 'loading' route. Previously, this intermediate state was being treated as a separate page by the app, and so any ajax requests fired during it would be counted as a distinct pageview. One known case of this is the `/presence/get` request which is made when logged-in users visit a topic.

This commit updates the logic to ignore 'intermediate' transitions, and introduces regression tests for both the 'spinner' and 'slider' modes.

app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js

@@ -65,10 +65,17 @@ 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();
+    // transition.from will be null on initial boot transition, which is already tracked as a pageview via the HTML request
+    if (!transition.from) {
+      return;
     }
+
+    // Ignore intermediate transitions (e.g. loading substates)
+    if (transition.isIntermediate) {
+      return;
+    }
+
+    trackNextAjaxAsPageview();
   },
 
   teardown() {

app/assets/javascripts/discourse/tests/acceptance/page-tracking-test.js

@@ -2,38 +2,124 @@ 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";
+import { getOwner } from "@ember/application";
+import { ajax } from "discourse/lib/ajax";
+
+const trackViewHeaderName = "Discourse-Track-View";
+
+function setupPretender(server, helper) {
+  server.get("/fake-analytics-endpoint", (request) => {
+    if (request.requestHeaders[trackViewHeaderName]) {
+      throw "Fake analytics endpoint was called with track-view header";
+    }
+    return helper.response({});
+  });
+}
+
+function setupFakeAnalytics(ref) {
+  getOwner(ref)
+    .lookup("service:router")
+    .on("routeDidChange", () => ajax("/fake-analytics-endpoint"));
+}
+
+function assertRequests({ assert, tracked, untracked, message }) {
+  let trackedCount = 0,
+    untrackedCount = 0;
+
+  const requests = pretender.handledRequests;
+  requests.forEach((request) => {
+    if (request.requestHeaders[trackViewHeaderName]) {
+      trackedCount++;
+    } else {
+      untrackedCount++;
+    }
+  });
+
+  assert.strictEqual(trackedCount, tracked, `${message} (tracked)`);
+  assert.strictEqual(untrackedCount, untracked, `${message} (untracked)`);
+
+  pretender.handledRequests = [];
+}
+
+acceptance("Page tracking - loading slider", function (needs) {
+  needs.user();
+  needs.pretender(setupPretender);
 
-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"
-    );
+    setupFakeAnalytics(this);
+
+    assertRequests({
+      assert,
+      tracked: 0,
+      untracked: 0,
+      message: "no requests 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"
-    );
+    assertRequests({
+      assert,
+      tracked: 0,
+      untracked: 2,
+      message: "no ajax tracked for initial page load",
+    });
 
     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"
-    );
+    assertRequests({
+      assert,
+      tracked: 1,
+      untracked: 1,
+      message: "tracked one pageview for reloading latest",
+    });
+
+    await visit("/t/-/280");
+    assertRequests({
+      assert,
+      tracked: 1,
+      untracked: 1,
+      message: "tracked one pageview for navigating to topic",
+    });
+  });
+});
+
+acceptance("Page tracking - loading spinner", function (needs) {
+  needs.user();
+  needs.pretender(setupPretender);
+  needs.settings({
+    page_loading_indicator: "spinner",
+  });
+
+  test("sets the discourse-track-view header correctly", async function (assert) {
+    setupFakeAnalytics(this);
+
+    assertRequests({
+      assert,
+      tracked: 0,
+      untracked: 0,
+      message: "no requests before app boot",
+    });
+
+    await visit("/");
+    assertRequests({
+      assert,
+      tracked: 0,
+      untracked: 2,
+      message: "no ajax tracked for initial page load",
+    });
+
+    await click("#site-logo");
+    assertRequests({
+      assert,
+      tracked: 1,
+      untracked: 1,
+      message: "tracked one pageview for reloading latest",
+    });
+
+    await visit("/t/-/280");
+    assertRequests({
+      assert,
+      tracked: 1,
+      untracked: 1,
+      message: "tracked one pageview for navigating to topic",
+    });
   });
 });