From b537d591b361915498abb582926c00a77044dfe2 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 11 Jan 2022 10:37:37 +0000 Subject: [PATCH] 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 27211ee7bbf661991cbf8a69f98d490be233ae26. - 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. --- .../discourse/app/lib/page-tracker.js | 4 ++ .../javascripts/discourse/app/lib/url.js | 2 +- .../app/routes/topic-by-slug-or-id.js | 17 ++----- .../javascripts/discourse/app/routes/topic.js | 3 +- .../discourse/tests/acceptance/topic-test.js | 49 ++++++++++++++++++- 5 files changed, 59 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/page-tracker.js b/app/assets/javascripts/discourse/app/lib/page-tracker.js index 76a937f195..3cb60f36ff 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 94a015236e..1b2fd5b7bf 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 f2805aedd1..459fce0904 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 d0bfaa7d84..0fb34de9fd 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 cbe8c5c739..0bdfa70f49 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")); + }); +});