diff --git a/app/assets/javascripts/discourse/app/lib/page-tracker.js b/app/assets/javascripts/discourse/app/lib/page-tracker.js index 76a937f195a..3cb60f36ff5 100644 --- a/app/assets/javascripts/discourse/app/lib/page-tracker.js +++ b/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 = diff --git a/app/assets/javascripts/discourse/app/lib/url.js b/app/assets/javascripts/discourse/app/lib/url.js index 94a015236ec..1b2fd5b7bf4 100644 --- a/app/assets/javascripts/discourse/app/lib/url.js +++ b/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 = [ diff --git a/app/assets/javascripts/discourse/app/routes/topic-by-slug-or-id.js b/app/assets/javascripts/discourse/app/routes/topic-by-slug-or-id.js index f2805aedd1a..459fce0904a 100644 --- a/app/assets/javascripts/discourse/app/routes/topic-by-slug-or-id.js +++ b/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); }, }); diff --git a/app/assets/javascripts/discourse/app/routes/topic.js b/app/assets/javascripts/discourse/app/routes/topic.js index d0bfaa7d840..0fb34de9fd5 100644 --- a/app/assets/javascripts/discourse/app/routes/topic.js +++ b/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; }, diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js index cbe8c5c7393..0bdfa70f49f 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js +++ b/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 += `\nLink 1`; + firstPost.cooked += `\nLink 2`; + firstPost.cooked += `\nLink 3`; + firstPost.cooked += `\nLink 3`; + + 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")); + }); +});