From 5c699e438440a58c0700820e726c28a183945423 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 1 Feb 2023 12:39:23 -0300 Subject: [PATCH] DEV: Pass messageId as a dynamic segment instead of a query param (#20013) * DEV: Rnemae channel path to just c Also swap the channel id and channel slug params to be consistent with core. * linting * channel_path * Drop slugify helper and channel route without slug * Request slug and route models through the channel model if possible * DEV: Pass messageId as a dynamic segment instead of a query param * Ensure change is backwards-compatible * drop query param from oneboxes * Correctly extract channelId from routes * Better route organization using siblings for regular and near-message * Ensures sessions are unique even when using parallelism * prevents didReceiveAttrs to clear input mid test * we disable animations in capybara so sometimes the message was barely showing * adds wait * ensures finished loading * is it causing more harm than good? * this check is slowing things for no reason * actually target the button * more resilient select chat message * apply similar fix to bookmark * fix --------- Co-authored-by: Joffrey JAFFEUX --- .../app/jobs/regular/chat_notify_mentioned.rb | 2 +- .../javascripts/discourse/chat-route-map.js | 21 ++++--- .../components/chat-channel-card.hbs | 2 +- .../discourse/components/chat-channel-row.hbs | 2 +- .../discourse/components/chat-composer.js | 1 + .../discourse/components/chat-drawer.js | 56 ++++++++++--------- .../discourse/components/chat-message.js | 2 +- .../components/chat-selection-manager.hbs | 11 +++- .../components/chat-selection-manager.js | 10 +++- .../components/reviewable-chat-message.hbs | 9 ++- .../discourse/controllers/chat-channel.js | 1 + .../discourse/helpers/format-chat-date.js | 4 +- .../discourse/initializers/chat-sidebar.js | 4 +- .../discourse/initializers/chat-user-menu.js | 4 +- .../routes/chat-channel-from-params.js | 18 ++++++ .../discourse/routes/chat-channel-legacy.js | 11 +++- .../routes/chat-channel-near-message.js | 41 ++++++++++++++ .../discourse/routes/chat-channel.js | 29 +++------- .../discourse/routes/chat-message.js | 6 +- .../javascripts/discourse/routes/chat.js | 8 +-- .../javascripts/discourse/services/chat.js | 23 +++++--- ...index.hbs => chat-channel-from-params.hbs} | 0 .../discourse/templates/chat-channel-info.hbs | 2 +- .../templates/chat-channel-near-message.hbs | 1 + .../chat-invitation-notification-item.js | 2 +- .../widgets/chat-mention-notification-item.js | 2 +- .../lib/discourse-markdown/chat-transcript.js | 2 +- plugins/chat/plugin.rb | 25 ++------- .../spec/integration/post_chat_quote_spec.rb | 12 ++-- .../regular/chat_notify_mentioned_spec.rb | 4 +- plugins/chat/spec/models/chat_message_spec.rb | 2 +- plugins/chat/spec/plugin_spec.rb | 9 ++- .../chat/spec/system/bookmark_message_spec.rb | 8 ++- .../spec/system/navigating_to_message_spec.rb | 12 ++-- plugins/chat/spec/system/transcript_spec.rb | 23 ++++---- .../user_menu_notifications/sidebar_spec.rb | 8 +-- .../chat/spec/system/visit_channel_spec.rb | 2 +- .../unit/helpers/format-chat-date-test.js | 5 +- .../chat-invitation-notification-item-test.js | 2 +- .../chat-mention-notification-item-test.js | 6 +- spec/rails_helper.rb | 2 +- spec/support/system_helpers.rb | 13 ++++- 42 files changed, 238 insertions(+), 169 deletions(-) create mode 100644 plugins/chat/assets/javascripts/discourse/routes/chat-channel-from-params.js create mode 100644 plugins/chat/assets/javascripts/discourse/routes/chat-channel-near-message.js rename plugins/chat/assets/javascripts/discourse/templates/{chat-channel-index.hbs => chat-channel-from-params.hbs} (100%) create mode 100644 plugins/chat/assets/javascripts/discourse/templates/chat-channel-near-message.hbs diff --git a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb index d6fa48e3320..b10083f5ff0 100644 --- a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb +++ b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb @@ -65,7 +65,7 @@ module Jobs username: @creator.username, tag: Chat::ChatNotifier.push_notification_tag(:mention, @chat_channel.id), excerpt: @chat_message.push_notification_excerpt, - post_url: "#{@chat_channel.relative_url}?messageId=#{@chat_message.id}", + post_url: "#{@chat_channel.relative_url}/#{@chat_message.id}", } translation_prefix = diff --git a/plugins/chat/assets/javascripts/discourse/chat-route-map.js b/plugins/chat/assets/javascripts/discourse/chat-route-map.js index 2ffee549611..d303778eaee 100644 --- a/plugins/chat/assets/javascripts/discourse/chat-route-map.js +++ b/plugins/chat/assets/javascripts/discourse/chat-route-map.js @@ -5,17 +5,16 @@ export default function () { path: "/channel/:channelId/:channelTitle", }); - this.route( - "channel", - { path: "/c/:channelTitle/:channelId/" }, - function () { - this.route("info", { path: "/info" }, function () { - this.route("about", { path: "/about" }); - this.route("members", { path: "/members" }); - this.route("settings", { path: "/settings" }); - }); - } - ); + this.route("channel", { path: "/c/:channelTitle/:channelId" }, function () { + this.route("from-params", { path: "/" }); + this.route("near-message", { path: "/:messageId" }); + + this.route("info", { path: "/info" }, function () { + this.route("about", { path: "/about" }); + this.route("members", { path: "/members" }); + this.route("settings", { path: "/settings" }); + }); + }); this.route("draft-channel", { path: "/draft-channel" }); this.route("browse", { path: "/browse" }, function () { diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs index a7c4b39bb4f..0bc79af6c6e 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs @@ -10,7 +10,7 @@ >
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-row.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-row.hbs index 3d1e442ae8d..07d94994d75 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-row.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-row.hbs @@ -1,5 +1,5 @@ { - this.appEvents.trigger( - "chat-live-pane:highlight-message", - route.queryParams.messageId - ); - }; - } - switch (route.name) { case "chat": this.set("view", LIST_VIEW); @@ -231,25 +220,42 @@ export default Component.extend({ this.set("view", DRAFT_CHANNEL_VIEW); this.appEvents.trigger("chat:float-toggled", false); return; - case "chat.channel": - return this._openChannel(route, highlightCb); + case "chat.channel.from-params": + return this._openChannel( + route.parent.params.channelId, + this._highlightCb(route.queryParams.messageId) + ); + case "chat.channel.near-message": + return this._openChannel( + route.parent.params.channelId, + this._highlightCb(route.params.messageId) + ); case "chat.channel-legacy": - return this._openChannel(route, highlightCb); + return this._openChannel( + route.params.channelId, + this._highlightCb(route.queryParams.messageId) + ); } }, - _openChannel(route, afterRenderFunc = null) { - return this.chatChannelsManager - .find(route.params.channelId) - .then((channel) => { - this.chat.setActiveChannel(channel); - this.set("view", CHAT_VIEW); - this.appEvents.trigger("chat:float-toggled", false); + _highlightCb(messageId) { + if (messageId) { + return () => { + this.appEvents.trigger("chat-live-pane:highlight-message", messageId); + }; + } + }, - if (afterRenderFunc) { - schedule("afterRender", afterRenderFunc); - } - }); + _openChannel(channelId, afterRenderFunc = null) { + return this.chatChannelsManager.find(channelId).then((channel) => { + this.chat.setActiveChannel(channel); + this.set("view", CHAT_VIEW); + this.appEvents.trigger("chat:float-toggled", false); + + if (afterRenderFunc) { + schedule("afterRender", afterRenderFunc); + } + }); }, @action diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.js b/plugins/chat/assets/javascripts/discourse/components/chat-message.js index 4d504c2e2cc..c5ad03d6048 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.js @@ -786,7 +786,7 @@ export default Component.extend({ const { protocol, host } = window.location; let url = getURL( - `/chat/c/-/${this.details.chat_channel_id}?messageId=${this.message.id}` + `/chat/c/-/${this.details.chat_channel_id}/${this.message.id}` ); url = url.indexOf("/") === 0 ? protocol + "//" + host + url : url; clipboardCopy(url); diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.hbs index 66b616894ba..a01163126db 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.hbs @@ -1,4 +1,9 @@ -
+
- {{#if this.showChatQuoteSuccess}} + {{#if this.showChatCopySuccess}}
{{i18n "chat.quote.copy_success"}}
{{/if}} -
\ No newline at end of file +
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.js b/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.js index a9c8887318d..5556ee7841d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-selection-manager.js @@ -16,7 +16,8 @@ export default class AdminCustomizeColorsShowController extends Component { tagName = ""; chatChannel = null; selectedMessageIds = null; - showChatQuoteSuccess = false; + chatCopySuccess = false; + showChatCopySuccess = false; cancelSelecting = null; canModerate = false; @@ -100,12 +101,15 @@ export default class AdminCustomizeColorsShowController extends Component { @action async copyMessages() { try { + this.set("chatCopySuccess", false); + if (!isTesting()) { // clipboard API throws errors in tests await clipboardCopyAsync(this.generateQuote); + this.set("chatCopySuccess", true); } - this.set("showChatQuoteSuccess", true); + this.set("showChatCopySuccess", true); schedule("afterRender", () => { const element = document.querySelector(".chat-selection-message"); @@ -114,7 +118,7 @@ export default class AdminCustomizeColorsShowController extends Component { return; } - this.set("showChatQuoteSuccess", false); + this.set("showChatCopySuccess", false); }); }); } catch (error) { diff --git a/plugins/chat/assets/javascripts/discourse/components/reviewable-chat-message.hbs b/plugins/chat/assets/javascripts/discourse/components/reviewable-chat-message.hbs index 004e907d703..8401719457d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/reviewable-chat-message.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/reviewable-chat-message.hbs @@ -1,8 +1,11 @@
diff --git a/plugins/chat/assets/javascripts/discourse/controllers/chat-channel.js b/plugins/chat/assets/javascripts/discourse/controllers/chat-channel.js index 75d4a3b4ee9..0e4cade32a2 100644 --- a/plugins/chat/assets/javascripts/discourse/controllers/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/controllers/chat-channel.js @@ -5,6 +5,7 @@ import { inject as service } from "@ember/service"; export default class ChatChannelController extends Controller { @service chat; + // Backwards-compatibility queryParams = ["messageId"]; @action diff --git a/plugins/chat/assets/javascripts/discourse/helpers/format-chat-date.js b/plugins/chat/assets/javascripts/discourse/helpers/format-chat-date.js index 58fc361097d..4573ed60484 100644 --- a/plugins/chat/assets/javascripts/discourse/helpers/format-chat-date.js +++ b/plugins/chat/assets/javascripts/discourse/helpers/format-chat-date.js @@ -14,9 +14,7 @@ registerUnbound("format-chat-date", function (message, details, mode) { let url = ""; if (details) { - url = getURL( - `/chat/c/-/${details.chat_channel_id}?messageId=${message.id}` - ); + url = getURL(`/chat/c/-/${details.chat_channel_id}/${message.id}`); } let title = date.format(I18n.t("dates.long_with_year")); diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js index 1cb408dbaba..6098649435d 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js @@ -52,7 +52,7 @@ export default { } get route() { - return "chat.channel"; + return "chat.channel.from-params"; } get models() { @@ -215,7 +215,7 @@ export default { } get route() { - return "chat.channel"; + return "chat.channel.from-params"; } get models() { diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js index 484d511dade..ed19441f31f 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js @@ -26,7 +26,7 @@ export default { }); return `/chat/c/${slug || "-"}/${ this.notification.data.chat_channel_id - }?messageId=${this.notification.data.chat_message_id}`; + }/${this.notification.data.chat_message_id}`; } get linkTitle() { @@ -61,7 +61,7 @@ export default { }); return `/chat/c/${slug || "-"}/${ this.notification.data.chat_channel_id - }?messageId=${this.notification.data.chat_message_id}`; + }/${this.notification.data.chat_message_id}`; } get linkTitle() { diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-from-params.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-from-params.js new file mode 100644 index 00000000000..134c7f14b11 --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-from-params.js @@ -0,0 +1,18 @@ +import DiscourseRoute from "discourse/routes/discourse"; +import { inject as service } from "@ember/service"; + +export default class ChatChannelFromParamsRoute extends DiscourseRoute { + @service router; + + async model() { + return this.modelFor("chat-channel"); + } + + afterModel(model) { + const { channelTitle } = this.paramsFor("chat.channel"); + + if (channelTitle !== model.slugifiedTitle) { + this.router.replaceWith("chat.channel.from-params", ...model.routeModels); + } + } +} diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-legacy.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-legacy.js index 29c5104d189..b256fc639d6 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-legacy.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-legacy.js @@ -9,8 +9,13 @@ export default class ChatChannelLegacyRoute extends DiscourseRoute { this.routeName ); - this.router.replaceWith("chat.channel", channelTitle, channelId, { - queryParams: { messageId }, - }); + this.router.replaceWith( + "chat.channel.from-params", + channelTitle, + channelId, + { + queryParams: { messageId }, + } + ); } } diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-near-message.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-near-message.js new file mode 100644 index 00000000000..b400a7fb133 --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-near-message.js @@ -0,0 +1,41 @@ +import DiscourseRoute from "discourse/routes/discourse"; +import { inject as service } from "@ember/service"; +import { action } from "@ember/object"; +import { schedule } from "@ember/runloop"; + +export default class ChatChannelNearMessage extends DiscourseRoute { + @service chat; + @service router; + + async model() { + return this.modelFor("chat-channel"); + } + + afterModel(model) { + const { messageId } = this.paramsFor(this.routeName); + const { channelTitle } = this.paramsFor("chat.channel"); + + if (channelTitle !== model.slugifiedTitle) { + this.router.replaceWith( + "chat.channel.near-message", + ...model.routeModels, + messageId + ); + } + } + + @action + didTransition() { + this.controllerFor("chat-channel").set("messageId", null); + + const { messageId } = this.paramsFor(this.routeName); + const { channelId } = this.paramsFor("chat.channel"); + + if (channelId && messageId) { + schedule("afterRender", () => { + this.chat.openChannelAtMessage(channelId, messageId); + }); + } + return true; + } +} diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js index 209a207045d..daacc3bb74c 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js @@ -1,12 +1,10 @@ import DiscourseRoute from "discourse/routes/discourse"; import { inject as service } from "@ember/service"; -import { action } from "@ember/object"; -import { schedule } from "@ember/runloop"; export default class ChatChannelRoute extends DiscourseRoute { + @service chatChannelsManager; @service chat; @service router; - @service chatChannelsManager; async model(params) { return this.chatChannelsManager.find(params.channelId); @@ -15,24 +13,15 @@ export default class ChatChannelRoute extends DiscourseRoute { afterModel(model) { this.chat.setActiveChannel(model); - const { channelTitle, messageId } = this.paramsFor(this.routeName); + const { messageId } = this.paramsFor(this.routeName); - if (channelTitle !== model.slugifiedTitle) { - this.router.replaceWith("chat.channel.index", ...model.routeModels, { - queryParams: { messageId }, - }); + // messageId query param backwards-compatibility + if (messageId) { + this.router.replaceWith( + "chat.channel.near-message", + ...model.routeModels, + messageId + ); } } - - @action - didTransition() { - const { channelId, messageId } = this.paramsFor(this.routeName); - if (channelId && messageId) { - schedule("afterRender", () => { - this.chat.openChannelAtMessage(channelId, messageId); - this.controller.set("messageId", null); // clear the query param - }); - } - return true; - } } diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-message.js b/plugins/chat/assets/javascripts/discourse/routes/chat-message.js index 810822e1897..c03258f76a3 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-message.js @@ -10,12 +10,10 @@ export default class ChatMessageRoute extends DiscourseRoute { return ajax(`/chat/message/${params.messageId}.json`) .then((response) => { this.transitionTo( - "chat.channel", + "chat.channel.near-message", response.chat_channel_title, response.chat_channel_id, - { - queryParams: { messageId: params.messageId }, - } + params.messageId ); }) .catch(() => this.replaceWith("/404")); diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat.js b/plugins/chat/assets/javascripts/discourse/routes/chat.js index a5cf28488a9..aa8dde44a3f 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat.js @@ -21,8 +21,9 @@ export default class ChatRoute extends DiscourseRoute { } const INTERCEPTABLE_ROUTES = [ - "chat.channel.index", "chat.channel", + "chat.channel.from-params", + "chat.channel.near-message", "chat.channel-legacy", "chat", "chat.index", @@ -37,10 +38,7 @@ export default class ChatRoute extends DiscourseRoute { transition.abort(); let URL = transition.intent.url; - if ( - transition.targetName.startsWith("chat.channel") || - transition.targetName.startsWith("chat.channel-legacy") - ) { + if (transition.targetName.startsWith("chat.channel")) { URL ??= this.router.urlFor( transition.targetName, ...transition.intent.contexts diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index 4d6ad20f44e..0cdde90a6a3 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat.js @@ -277,7 +277,8 @@ export default class Chat extends Service { async _openFoundChannelAtMessage(channel, messageId = null) { if ( - this.router.currentRouteName === "chat.channel.index" && + (this.router.currentRouteName === "chat.channel.from-params" || + this.router.currentRouteName === "chat.channel.near-message") && this.activeChannel?.id === channel.id ) { this.setActiveChannel(channel); @@ -292,14 +293,18 @@ export default class Chat extends Service { this.site.mobileView || this.chatStateManager.isFullPagePreferred ) { - const queryParams = messageId ? { messageId } : {}; - - return this.router.transitionTo( - "chat.channel", - channel.slugifiedTitle, - channel.id, - { queryParams } - ); + if (messageId) { + return this.router.transitionTo( + "chat.channel.near-message", + ...channel.routeModels, + messageId + ); + } else { + return this.router.transitionTo( + "chat.channel.from-params", + ...channel.routeModels + ); + } } else { this._fireOpenFloatAppEvent(channel, messageId); return Promise.resolve(); diff --git a/plugins/chat/assets/javascripts/discourse/templates/chat-channel-index.hbs b/plugins/chat/assets/javascripts/discourse/templates/chat-channel-from-params.hbs similarity index 100% rename from plugins/chat/assets/javascripts/discourse/templates/chat-channel-index.hbs rename to plugins/chat/assets/javascripts/discourse/templates/chat-channel-from-params.hbs diff --git a/plugins/chat/assets/javascripts/discourse/templates/chat-channel-info.hbs b/plugins/chat/assets/javascripts/discourse/templates/chat-channel-info.hbs index e390f0a194c..86979de4d28 100644 --- a/plugins/chat/assets/javascripts/discourse/templates/chat-channel-info.hbs +++ b/plugins/chat/assets/javascripts/discourse/templates/chat-channel-info.hbs @@ -12,7 +12,7 @@ {{else}} \ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/widgets/chat-invitation-notification-item.js b/plugins/chat/assets/javascripts/discourse/widgets/chat-invitation-notification-item.js index f85ceb6c5c3..b9498312ef9 100644 --- a/plugins/chat/assets/javascripts/discourse/widgets/chat-invitation-notification-item.js +++ b/plugins/chat/assets/javascripts/discourse/widgets/chat-invitation-notification-item.js @@ -38,7 +38,7 @@ createWidgetFrom(DefaultNotificationItem, "chat-invitation-notification-item", { title: data.chat_channel_title, slug: data.chat_channel_slug, }); - return `/chat/c/${slug || "-"}/${data.chat_channel_id}?messageId=${ + return `/chat/c/${slug || "-"}/${data.chat_channel_id}/${ data.chat_message_id }`; }, diff --git a/plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js b/plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js index 2cb6f373f7b..7efebdb2834 100644 --- a/plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js +++ b/plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js @@ -48,7 +48,7 @@ const chatNotificationItem = { title: data.chat_channel_title, slug: data.chat_channel_slug, }); - return `/chat/c/${slug || "-"}/${data.chat_channel_id}?messageId=${ + return `/chat/c/${slug || "-"}/${data.chat_channel_id}/${ data.chat_message_id }`; }, diff --git a/plugins/chat/assets/javascripts/lib/discourse-markdown/chat-transcript.js b/plugins/chat/assets/javascripts/lib/discourse-markdown/chat-transcript.js index d853a045408..1263a4b919a 100644 --- a/plugins/chat/assets/javascripts/lib/discourse-markdown/chat-transcript.js +++ b/plugins/chat/assets/javascripts/lib/discourse-markdown/chat-transcript.js @@ -122,7 +122,7 @@ const chatTranscriptRule = { } else { let linkToken = state.push("link_open", "a", 1); linkToken.attrs = [ - ["href", `${channelLink}?messageId=${messageIdStart}`], + ["href", `${channelLink}/${messageIdStart}`], ["title", messageTimeStart], ]; diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 9edf33895c7..329d07209ed 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -265,16 +265,8 @@ after_initialize do if Oneboxer.respond_to?(:register_local_handler) Oneboxer.register_local_handler("chat/chat") do |url, route| - queryParams = - begin - CGI.parse(URI.parse(url).query) - rescue StandardError - {} - end - messageId = queryParams["messageId"]&.first - - if messageId.present? - message = ChatMessage.find_by(id: messageId) + if route[:message_id].present? + message = ChatMessage.find_by(id: route[:message_id]) next if !message chat_channel = message.chat_channel @@ -334,16 +326,8 @@ after_initialize do if InlineOneboxer.respond_to?(:register_local_handler) InlineOneboxer.register_local_handler("chat/chat") do |url, route| - queryParams = - begin - CGI.parse(URI.parse(url).query) - rescue StandardError - {} - end - messageId = queryParams["messageId"]&.first - - if messageId.present? - message = ChatMessage.find_by(id: messageId) + if route[:message_id].present? + message = ChatMessage.find_by(id: route[:message_id]) next if !message chat_channel = message.chat_channel @@ -655,6 +639,7 @@ after_initialize do base_c_route = "/c/:channel_title/:channel_id" get base_c_route => "chat#respond", :as => "channel" + get "#{base_c_route}/:message_id" => "chat#respond" %w[info info/about info/members info/settings].each do |route| get "#{base_c_route}/#{route}" => "chat#respond" diff --git a/plugins/chat/spec/integration/post_chat_quote_spec.rb b/plugins/chat/spec/integration/post_chat_quote_spec.rb index ce14c313443..71457ccde1b 100644 --- a/plugins/chat/spec/integration/post_chat_quote_spec.rb +++ b/plugins/chat/spec/integration/post_chat_quote_spec.rb @@ -41,7 +41,7 @@ describe "chat bbcode quoting in posts" do
martin
- +
@@ -63,7 +63,7 @@ describe "chat bbcode quoting in posts" do
martin
- +
#Cool Cats Club @@ -87,7 +87,7 @@ describe "chat bbcode quoting in posts" do
martin
- +
#Cool Cats Club @@ -137,7 +137,7 @@ describe "chat bbcode quoting in posts" do
martin
- +
#Cool Cats Club @@ -242,7 +242,7 @@ martin
#{message1.user.username}
- +
##{channel.name} @@ -256,7 +256,7 @@ martin
#{message2.user.username}
- +
##{channel.name} diff --git a/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb b/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb index 5fe55080999..99db2226233 100644 --- a/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb @@ -215,7 +215,7 @@ describe Jobs::ChatNotifyMentioned do ) expect(desktop_notification.data[:excerpt]).to eq(message.push_notification_excerpt) expect(desktop_notification.data[:post_url]).to eq( - "/chat/c/#{public_channel.slug}/#{public_channel.id}?messageId=#{message.id}", + "/chat/c/#{public_channel.slug}/#{public_channel.id}/#{message.id}", ) end @@ -229,7 +229,7 @@ describe Jobs::ChatNotifyMentioned do username: user_1.username, tag: Chat::ChatNotifier.push_notification_tag(:mention, public_channel.id), excerpt: message.push_notification_excerpt, - post_url: "/chat/c/#{public_channel.slug}/#{public_channel.id}?messageId=#{message.id}", + post_url: "/chat/c/#{public_channel.slug}/#{public_channel.id}/#{message.id}", translated_title: payload_translated_title, }, ) diff --git a/plugins/chat/spec/models/chat_message_spec.rb b/plugins/chat/spec/models/chat_message_spec.rb index fd775b984aa..5f42e77f583 100644 --- a/plugins/chat/spec/models/chat_message_spec.rb +++ b/plugins/chat/spec/models/chat_message_spec.rb @@ -138,7 +138,7 @@ describe ChatMessage do
chatbbcodeuser
- +
diff --git a/plugins/chat/spec/plugin_spec.rb b/plugins/chat/spec/plugin_spec.rb index 6a23fe258de..b90f1861e64 100644 --- a/plugins/chat/spec/plugin_spec.rb +++ b/plugins/chat/spec/plugin_spec.rb @@ -155,10 +155,9 @@ describe Chat do end it "renders messages" do - results = - InlineOneboxer.new(["#{chat_url}?messageId=#{chat_message.id}"], skip_cache: true).process + results = InlineOneboxer.new(["#{chat_url}/#{chat_message.id}"], skip_cache: true).process expect(results).to be_present - expect(results[0][:url]).to eq("#{chat_url}?messageId=#{chat_message.id}") + expect(results[0][:url]).to eq("#{chat_url}/#{chat_message.id}") expect(results[0][:title]).to eq( "Message ##{chat_message.id} by #{chat_message.user.username} – ##{chat_channel.name}", ) @@ -197,7 +196,7 @@ describe Chat do end it "renders messages" do - expect(Oneboxer.preview("#{chat_url}?messageId=#{chat_message.id}")).to match_html <<~HTML + expect(Oneboxer.preview("#{chat_url}/#{chat_message.id}")).to match_html <<~HTML
@@ -207,7 +206,7 @@ describe Chat do
#{user.username}
- #{chat_message.created_at} + #{chat_message.created_at}
diff --git a/plugins/chat/spec/system/bookmark_message_spec.rb b/plugins/chat/spec/system/bookmark_message_spec.rb index 2b843360958..e8afe05c918 100644 --- a/plugins/chat/spec/system/bookmark_message_spec.rb +++ b/plugins/chat/spec/system/bookmark_message_spec.rb @@ -52,8 +52,14 @@ RSpec.describe "Bookmark message", type: :system, js: true do context "when mobile", mobile: true do it "allows to bookmark a message" do chat.visit_channel(category_channel_1) + expect(channel).to have_no_loading_skeleton - channel.message_by_id(message_1.id).click(delay: 0.5) + i = 0.5 + try_until_success(timeout: 20) do + channel.message_by_id(message_1.id).click(delay: i) + first(".bookmark-btn") + i += 0.1 + end find(".bookmark-btn").click bookmark_modal.fill_name("Check this out later") diff --git a/plugins/chat/spec/system/navigating_to_message_spec.rb b/plugins/chat/spec/system/navigating_to_message_spec.rb index 3b5fe12398d..9dae4fe8b81 100644 --- a/plugins/chat/spec/system/navigating_to_message_spec.rb +++ b/plugins/chat/spec/system/navigating_to_message_spec.rb @@ -26,7 +26,7 @@ RSpec.describe "Navigating to message", type: :system, js: true do Fabricate( :post, topic: topic_1, - raw: "#{link}", + raw: "#{link}", ) end @@ -45,7 +45,7 @@ RSpec.describe "Navigating to message", type: :system, js: true do Fabricate( :chat_message, chat_channel: channel_1, - message: "[#{link}](/chat/c/-/#{channel_1.id}?messageId=#{first_message.id})", + message: "[#{link}](/chat/c/-/#{channel_1.id}/#{first_message.id})", ) end @@ -77,7 +77,7 @@ RSpec.describe "Navigating to message", type: :system, js: true do Fabricate( :chat_message, chat_channel: channel_2, - message: "[#{link}](/chat/c/-/#{channel_1.id}?messageId=#{first_message.id})", + message: "[#{link}](/chat/c/-/#{channel_1.id}/#{first_message.id})", ) channel_2.add(current_user) end @@ -94,7 +94,7 @@ RSpec.describe "Navigating to message", type: :system, js: true do context "when navigating directly to a message link" do it "highglights the correct message" do - visit("/chat/c/-/#{channel_1.id}?messageId=#{first_message.id}") + visit("/chat/c/-/#{channel_1.id}/#{first_message.id}") expect(page).to have_css( ".chat-message-container.highlighted[data-id='#{first_message.id}']", @@ -111,7 +111,7 @@ RSpec.describe "Navigating to message", type: :system, js: true do Fabricate( :post, topic: topic_1, - raw: "#{link}", + raw: "#{link}", ) end @@ -130,7 +130,7 @@ RSpec.describe "Navigating to message", type: :system, js: true do Fabricate( :chat_message, chat_channel: channel_1, - message: "[#{link}](/chat/c/-/#{channel_1.id}?messageId=#{first_message.id})", + message: "[#{link}](/chat/c/-/#{channel_1.id}/#{first_message.id})", ) end diff --git a/plugins/chat/spec/system/transcript_spec.rb b/plugins/chat/spec/system/transcript_spec.rb index ad5e75212b5..08809cec57a 100644 --- a/plugins/chat/spec/system/transcript_spec.rb +++ b/plugins/chat/spec/system/transcript_spec.rb @@ -26,12 +26,12 @@ RSpec.describe "Quoting chat message transcripts", type: :system, js: true do end def select_message_mobile(message) - if page.has_css?(".chat-message-container.selecting-messages") - chat_channel_page.message_by_id(message.id).find(".chat-message-selector").click - else - chat_channel_page.message_by_id(message.id).click(delay: 0.5) - find(".chat-message-action-item[data-id=\"selectMessage\"]").click + i = 0.5 + try_until_success(timeout: 20) do + chat_channel_page.message_by_id(message.id).click(delay: i) + first(".chat-message-action-item[data-id=\"selectMessage\"]") end + find(".chat-message-action-item[data-id=\"selectMessage\"] button").click end def cdp_allow_clipboard_access! @@ -62,15 +62,15 @@ RSpec.describe "Quoting chat message transcripts", type: :system, js: true do selector = case button when "quote" - "#chat-quote-btn" + "chat-quote-btn" when "copy" - "#chat-copy-btn" + "chat-copy-btn" when "cancel" - "#chat-cancel-selection-btn" + "chat-cancel-selection-btn" when "move" - "#chat-move-to-channel-btn" + "chat-move-to-channel-btn" end - within(".chat-selection-management-buttons") { find(selector).click } + find_button(selector, disabled: false, wait: 5).click end def copy_messages_to_clipboard(messages) @@ -78,7 +78,7 @@ RSpec.describe "Quoting chat message transcripts", type: :system, js: true do messages.each { |message| select_message_desktop(message) } expect(chat_channel_page).to have_selection_management click_selection_button("copy") - expect(page).to have_content("Chat quote copied to clipboard") + expect(page).to have_selector(".chat-copy-success") clip_text = read_clipboard expect(clip_text.chomp).to eq(generate_transcript(messages, current_user)) clip_text @@ -227,6 +227,7 @@ RSpec.describe "Quoting chat message transcripts", type: :system, js: true do it "first navigates to the channel's category before opening the topic composer with the quote prefilled", mobile: true do chat_page.visit_channel(chat_channel_1) + expect(chat_channel_page).to have_no_loading_skeleton select_message_mobile(message_1) diff --git a/plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb b/plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb index 6adb92ab675..b52f70fd4c5 100644 --- a/plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb +++ b/plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb @@ -59,7 +59,7 @@ RSpec.describe "User menu notifications | sidebar", type: :system, js: true do end expect(find("#quick-access-chat-notifications")).to have_link( I18n.t("js.notifications.popup.direct_message_chat_mention.direct"), - href: "/chat/c/#{other_user.username}/#{dm_channel_1.id}?messageId=#{message.id}", + href: "/chat/c/#{other_user.username}/#{dm_channel_1.id}/#{message.id}", ) end end @@ -100,7 +100,7 @@ RSpec.describe "User menu notifications | sidebar", type: :system, js: true do identifier: "@#{group.name}", channel: channel_1.name, ), - href: "/chat/c/#{channel_1.slug}/#{channel_1.id}?messageId=#{message.id}", + href: "/chat/c/#{channel_1.slug}/#{channel_1.id}/#{message.id}", ) end end @@ -126,7 +126,7 @@ RSpec.describe "User menu notifications | sidebar", type: :system, js: true do expect(find("#quick-access-chat-notifications")).to have_link( I18n.t("js.notifications.popup.chat_mention.direct", channel: channel_1.name), - href: "/chat/c/#{channel_1.slug}/#{channel_1.id}?messageId=#{message.id}", + href: "/chat/c/#{channel_1.slug}/#{channel_1.id}/#{message.id}", ) end end @@ -153,7 +153,7 @@ RSpec.describe "User menu notifications | sidebar", type: :system, js: true do identifier: "@all", channel: channel_1.name, ), - href: "/chat/c/#{channel_1.slug}/#{channel_1.id}?messageId=#{message.id}", + href: "/chat/c/#{channel_1.slug}/#{channel_1.id}/#{message.id}", ) end end diff --git a/plugins/chat/spec/system/visit_channel_spec.rb b/plugins/chat/spec/system/visit_channel_spec.rb index 1fe73fad66b..2a7a45fa40b 100644 --- a/plugins/chat/spec/system/visit_channel_spec.rb +++ b/plugins/chat/spec/system/visit_channel_spec.rb @@ -69,7 +69,7 @@ RSpec.describe "Visit channel", type: :system, js: true do context "when loading a non existing message of a channel" do it "shows an error" do - visit("/chat/c/-/#{category_channel_1.id}?messageId=-999") + visit("/chat/c/-/#{category_channel_1.id}/-999") expect(page).to have_content(I18n.t("not_found")) end diff --git a/plugins/chat/test/javascripts/unit/helpers/format-chat-date-test.js b/plugins/chat/test/javascripts/unit/helpers/format-chat-date-test.js index e6fe904aabd..474dba68dcc 100644 --- a/plugins/chat/test/javascripts/unit/helpers/format-chat-date-test.js +++ b/plugins/chat/test/javascripts/unit/helpers/format-chat-date-test.js @@ -12,9 +12,6 @@ module("Discourse Chat | Unit | Helpers | format-chat-date", function (hooks) { this.set("message", { id: 1 }); await render(hbs`{{format-chat-date this.message this.details}}`); - assert.equal( - query(".chat-time").getAttribute("href"), - "/chat/c/-/1?messageId=1" - ); + assert.equal(query(".chat-time").getAttribute("href"), "/chat/c/-/1/1"); }); }); diff --git a/plugins/chat/test/javascripts/widgets/chat-invitation-notification-item-test.js b/plugins/chat/test/javascripts/widgets/chat-invitation-notification-item-test.js index 862cfaeada6..16b16fd3585 100644 --- a/plugins/chat/test/javascripts/widgets/chat-invitation-notification-item-test.js +++ b/plugins/chat/test/javascripts/widgets/chat-invitation-notification-item-test.js @@ -45,7 +45,7 @@ module( query(".chat-invitation a").getAttribute("href"), `/chat/c/${slugifyChannel({ title: data.chat_channel_title, - })}/${data.chat_channel_id}?messageId=${data.chat_message_id}` + })}/${data.chat_channel_id}/${data.chat_message_id}` ); }); } diff --git a/plugins/chat/test/javascripts/widgets/chat-mention-notification-item-test.js b/plugins/chat/test/javascripts/widgets/chat-mention-notification-item-test.js index 62603da1be1..d640ba8fccf 100644 --- a/plugins/chat/test/javascripts/widgets/chat-mention-notification-item-test.js +++ b/plugins/chat/test/javascripts/widgets/chat-mention-notification-item-test.js @@ -54,7 +54,7 @@ module( query(".chat-invitation a").getAttribute("href"), `/chat/c/${slugifyChannel({ title: data.chat_channel_title, - })}/${data.chat_channel_id}?messageId=${data.chat_message_id}` + })}/${data.chat_channel_id}/${data.chat_message_id}` ); }); } @@ -93,7 +93,7 @@ module( query(".chat-invitation a").getAttribute("href"), `/chat/c/${slugifyChannel({ title: data.chat_channel_title, - })}/${data.chat_channel_id}?messageId=${data.chat_message_id}` + })}/${data.chat_channel_id}/${data.chat_message_id}` ); }); } @@ -132,7 +132,7 @@ module( query(".chat-invitation a").getAttribute("href"), `/chat/c/${slugifyChannel({ title: data.chat_channel_title, - })}/${data.chat_channel_id}?messageId=${data.chat_message_id}` + })}/${data.chat_channel_id}/${data.chat_message_id}` ); }); } diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 22009af6faa..c822777e709 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -245,7 +245,7 @@ RSpec.configure do |config| end Capybara.threadsafe = true - Capybara.disable_animation = true + Capybara.disable_animation = false Capybara.configure do |capybara_config| capybara_config.server_host = "localhost" diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb index 028483bf886..336584a231c 100644 --- a/spec/support/system_helpers.rb +++ b/spec/support/system_helpers.rb @@ -32,7 +32,9 @@ module SystemHelpers start ||= Time.zone.now backoff ||= frequency yield - rescue RSpec::Expectations::ExpectationNotMetError + rescue RSpec::Expectations::ExpectationNotMetError, + Capybara::ExpectationNotMet, + Capybara::ElementNotFound raise if Time.zone.now >= start + timeout.seconds sleep backoff backoff += frequency @@ -67,8 +69,15 @@ module SystemHelpers ENV["TZ"] = timezone - Capybara.using_session(timezone) { freeze_time(&example) } + using_session(timezone) { freeze_time(&example) } ENV["TZ"] = previous_browser_timezone end + + # When using parallelism, Capybara's `using_session` method can cause + # intermittent failures as two sessions can be created with the same name + # in different tests and be run at the same time. + def using_session(name, &block) + Capybara.using_session(name.to_s + self.method_name, &block) + end end