FIX: allow slug-less topic URLs to work within the same topic (#15508)

- Update the TOPIC_URL_REGEXP in `lib/url` so that `navigatedToPost` doesn't attempt to handle slug-less URLs. Slugs must contain at least one non-numeric character, so we can use that fact to make the regex more specific. We want slug-less URLs to be routed as a normal Ember transition, so that `topic-by-slug-or-id` can catch them and re-write the URL to include the slug.

- Update the `topic-by-slug-or-id` afterModel to ensure that the Ember router is used to handle the redirect, rather than DiscourseURL. This guarantees that it will function as a redirect (DiscourseURL.routeTo sometimes bypasses the router). This solves the history problem which was worked-around in 27211ee7bb.

- Update routes/topic to recover from aborted transitions gracefully. This means that following an aborted transition, the browser URL continues to be updated with post numbers as the user scrolls down the page.

app/assets/javascripts/discourse/app/lib/page-tracker.js

@@ -23,6 +23,10 @@ export function startPageTracking(router, appEvents, documentTitle) {
     return;
   }
   router.on("routeDidChange", (transition) => {
+    if (transition.isAborted) {
+      return;
+    }
+
     // we occasionally prevent tracking of replaced pages when only query params changed
     // eg: google analytics
     const replacedOnlyQueryParams =

app/assets/javascripts/discourse/app/lib/url.js

@@ -12,7 +12,7 @@ import { setOwner } from "@ember/application";
 import { isTesting } from "discourse-common/config/environment";
 
 const rewrites = [];
-export const TOPIC_URL_REGEXP = /\/t\/([^\/]+)\/(\d+)\/?(\d+)?/;
+export const TOPIC_URL_REGEXP = /\/t\/([^\/]*[^\d\/][^\/]*)\/(\d+)\/?(\d+)?/;
 
 // We can add links here that have server side responses but not client side.
 const SERVER_SIDE_ONLY = [

app/assets/javascripts/discourse/app/routes/topic-by-slug-or-id.js

@@ -1,8 +1,10 @@
 import Topic, { ID_CONSTRAINT } from "discourse/models/topic";
 import DiscourseRoute from "discourse/routes/discourse";
-import DiscourseURL from "discourse/lib/url";
+import { inject as service } from "@ember/service";
 
 export default DiscourseRoute.extend({
+  router: service(),
+
   model(params) {
     if (params.slugOrId.match(ID_CONSTRAINT)) {
       return { url: `/t/topic/${params.slugOrId}` };
@@ -12,17 +14,6 @@ export default DiscourseRoute.extend({
   },
 
   afterModel(result) {
-    // Using { replaceURL: true } to replace the current incomplete URL with
-    // the complete one is working incorrectly.
-    //
-    // Let's consider an example where the user is at /t/-/1. If they click on
-    // a link to /t/2 the expected behavior is to take the user to /t/2 that
-    // will redirect to /t/-/2 and generate a history with two entries: /t/-/1
-    // followed by /t/-/2.
-    //
-    // When { replaceURL: true } is present, the history contains a single
-    // entry /t/-/2. This suggests that `afterModel` is called in the context
-    // of the referrer replacing its entry with the new one.
-    DiscourseURL.routeTo(result.url);
+    this.router.transitionTo(result.url);
   },
 });

app/assets/javascripts/discourse/app/routes/topic.js

@@ -247,10 +247,11 @@ const TopicRoute = DiscourseRoute.extend({
   },
 
   @action
-  willTransition() {
+  willTransition(transition) {
     this._super(...arguments);
     cancel(this.scheduledReplace);
     this.set("isTransitioning", true);
+    transition.catch(() => this.set("isTransitioning", false));
     return true;
   },
 

app/assets/javascripts/discourse/tests/acceptance/topic-test.js

@@ -8,11 +8,19 @@ import {
   selectText,
   visible,
 } from "discourse/tests/helpers/qunit-helpers";
-import { click, fillIn, triggerKeyEvent, visit } from "@ember/test-helpers";
+import {
+  click,
+  currentURL,
+  fillIn,
+  triggerKeyEvent,
+  visit,
+} from "@ember/test-helpers";
 import I18n from "I18n";
 import selectKit from "discourse/tests/helpers/select-kit-helper";
 import { test } from "qunit";
 import { withPluginApi } from "discourse/lib/plugin-api";
+import topicFixtures from "discourse/tests/fixtures/topic";
+import { cloneJSON } from "discourse-common/lib/object";
 
 acceptance("Topic", function (needs) {
   needs.user();
@@ -571,3 +579,42 @@ acceptance("Topic filter replies to post number", function (needs) {
     );
   });
 });
+
+acceptance("Navigating between topics", function (needs) {
+  needs.pretender((server, helper) => {
+    const topicResponse = cloneJSON(topicFixtures["/t/280/1.json"]);
+    const firstPost = topicResponse.post_stream.posts[0];
+    firstPost.cooked += `\n<a class='same-topic-slugless' href='/t/280'>Link 1</a>`;
+    firstPost.cooked += `\n<a class='same-topic-slugless-post' href='/t/280/3'>Link 2</a>`;
+    firstPost.cooked += `\n<a class='diff-topic-slugless' href='/t/281'>Link 3</a>`;
+    firstPost.cooked += `\n<a class='diff-topic-slugless-post' href='/t/281/3'>Link 3</a>`;
+
+    server.get("/t/280.json", () => helper.response(topicResponse));
+    server.get("/t/280/:post_number.json", () =>
+      helper.response(topicResponse)
+    );
+
+    server.get("/t/281.json", () => helper.response(topicResponse));
+    server.get("/t/281/:post_number.json", () =>
+      helper.response(topicResponse)
+    );
+  });
+
+  test("clicking slug-less URLs within the same topic", async function (assert) {
+    await visit("/t/-/280");
+    await click("a.same-topic-slugless");
+    assert.ok(currentURL().includes("/280"));
+
+    await click("a.same-topic-slugless-post");
+    assert.ok(currentURL().includes("/280"));
+  });
+
+  test("clicking slug-less URLs to a different topic", async function (assert) {
+    await visit("/t/-/280");
+    await click("a.diff-topic-slugless");
+    assert.ok(currentURL().includes("/281"));
+
+    await click("a.diff-topic-slugless-post");
+    assert.ok(currentURL().includes("/281"));
+  });
+});