From c6764d8c74634c3928c8dadf36f8786487e80ba5 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Wed, 9 Nov 2022 10:28:31 +1000
Subject: [PATCH] FIX: Automatically generate category channel slugs (#18879)
This commit automatically ensures that category channels
have slugs when they are created or updated based on the
channel name, category name, or existing slug. The behaviour
has been copied from the Category model.
We also include a backfill here with a simplified version
of Slug.for with deduplication to fill the slugs for already
created Category chat channels.
The channel slug is also now used for chat notifications,
and for the UI and navigation for chat. `slugifyChannel`
is still used, but now does the following fallback:
* Uses channel.slug if it is present
* Uses channel.escapedTitle if it is present
* Uses channel.title if it is present
In future we may want to remove this altogether
and always rely on the slug being present, but this
is currently not possible because we are not generating
slugs for DM channels at this point.
---
app/models/category.rb | 2 +-
config/locales/server.en.yml | 1 +
.../chat/app/controllers/chat_controller.rb | 1 +
.../app/jobs/regular/chat_notify_mentioned.rb | 10 +--
.../app/jobs/regular/chat_notify_watching.rb | 2 +-
plugins/chat/app/models/category_channel.rb | 20 +++++-
plugins/chat/app/models/chat_channel.rb | 13 +++-
.../chat/app/models/direct_message_channel.rb | 8 +++
.../serializers/chat_channel_serializer.rb | 1 +
.../discourse/initializers/chat-sidebar.js | 8 +--
.../discourse/initializers/chat-user-menu.js | 28 +++++---
.../discourse/lib/slugify-channel.js | 19 ++++--
.../discourse/routes/chat-channel.js | 2 +-
.../javascripts/discourse/services/chat.js | 4 +-
.../discourse/templates/chat-channel-info.hbs | 4 +-
.../components/chat-channel-card.hbs | 8 +--
.../templates/components/chat-live-pane.hbs | 2 +-
.../templates/components/topic-chat-float.hbs | 2 +-
.../chat-invitation-notification-item.js | 11 ++--
.../widgets/chat-mention-notification-item.js | 11 ++--
plugins/chat/config/locales/server.en.yml | 5 ++
.../20221104054957_backfill_channel_slugs.rb | 62 +++++++++++++++++
.../chat/spec/fabricators/chat_fabricator.rb | 15 ++++-
.../regular/chat_notify_mentioned_spec.rb | 5 +-
.../jobs/regular/chat_notify_watching_spec.rb | 32 +++++++--
.../chat/spec/models/category_channel_spec.rb | 66 +++++++++++++++++++
plugins/chat/spec/models/chat_channel_spec.rb | 25 +++++++
.../models/direct_message_channel_spec.rb | 10 +++
.../requests/chat_channel_controller_spec.rb | 4 +-
.../spec/requests/chat_controller_spec.rb | 5 ++
.../chat/test/javascripts/chat-fixtures.js | 2 +-
.../unit/lib/slugify-channel-test.js | 30 +++++++--
.../chat-invitation-notification-item-test.js | 6 +-
.../chat-mention-notification-item-test.js | 18 ++---
34 files changed, 367 insertions(+), 75 deletions(-)
create mode 100644 plugins/chat/db/post_migrate/20221104054957_backfill_channel_slugs.rb
create mode 100644 plugins/chat/spec/models/chat_channel_spec.rb
diff --git a/app/models/category.rb b/app/models/category.rb
index 0efbb5f3ed8..7e7bd947b02 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -374,7 +374,7 @@ class Category < ActiveRecord::Base
elsif SiteSetting.slug_generation_method == 'ascii' && !CGI.unescape(self.slug).ascii_only?
errors.add(:slug, I18n.t("category.errors.slug_contains_non_ascii_chars"))
elsif duplicate_slug?
- errors.add(:slug, 'is already in use')
+ errors.add(:slug, I18n.t("category.errors.is_already_in_use"))
end
else
# auto slug
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 73b0d3fe785..07b182b805e 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -697,6 +697,7 @@ en:
disallowed_topic_tags: "This topic has tags not allowed by this category: '%{tags}'"
disallowed_tags_generic: "This topic has disallowed tags."
slug_contains_non_ascii_chars: "contains non-ascii characters"
+ is_already_in_use: "is already in use"
cannot_delete:
uncategorized: "This category is special. It is intended as a holding area for topics that have no category; it cannot be deleted."
has_subcategories: "Can't delete this category because it has sub-categories."
diff --git a/plugins/chat/app/controllers/chat_controller.rb b/plugins/chat/app/controllers/chat_controller.rb
index e272188e720..3f78f15b8ef 100644
--- a/plugins/chat/app/controllers/chat_controller.rb
+++ b/plugins/chat/app/controllers/chat_controller.rb
@@ -352,6 +352,7 @@ class Chat::ChatController < Chat::ChatBaseController
message: "chat.invitation_notification",
chat_channel_id: @chat_channel.id,
chat_channel_title: @chat_channel.title(user),
+ chat_channel_slug: @chat_channel.slug,
invited_by_username: current_user.username,
}
data[:chat_message_id] = params[:chat_message_id] if params[:chat_message_id]
diff --git a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb
index 523d3259f2f..d6fa48e3320 100644
--- a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb
+++ b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb
@@ -39,9 +39,10 @@ module Jobs
is_direct_message_channel: @chat_channel.direct_message_channel?,
}
- data[:chat_channel_title] = @chat_channel.title(
- membership.user,
- ) unless @is_direct_message_channel
+ if !@is_direct_message_channel
+ data[:chat_channel_title] = @chat_channel.title(membership.user)
+ data[:chat_channel_slug] = @chat_channel.slug
+ end
return data if identifier_type == :direct_mentions
@@ -64,8 +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/#{@chat_channel.id}/#{@chat_channel.title(membership.user)}?messageId=#{@chat_message.id}",
+ post_url: "#{@chat_channel.relative_url}?messageId=#{@chat_message.id}",
}
translation_prefix =
diff --git a/plugins/chat/app/jobs/regular/chat_notify_watching.rb b/plugins/chat/app/jobs/regular/chat_notify_watching.rb
index c2c2cd8681e..e9b8805e88d 100644
--- a/plugins/chat/app/jobs/regular/chat_notify_watching.rb
+++ b/plugins/chat/app/jobs/regular/chat_notify_watching.rb
@@ -62,7 +62,7 @@ module Jobs
payload = {
username: @creator.username,
notification_type: Notification.types[:chat_message],
- post_url: "/chat/channel/#{@chat_channel.id}/#{@chat_channel.title(user)}",
+ post_url: @chat_channel.relative_url,
translated_title: I18n.t(translation_key, translation_args),
tag: Chat::ChatNotifier.push_notification_tag(:message, @chat_channel.id),
excerpt: @chat_message.push_notification_excerpt,
diff --git a/plugins/chat/app/models/category_channel.rb b/plugins/chat/app/models/category_channel.rb
index 2a2a32246b4..e95c3d5cff8 100644
--- a/plugins/chat/app/models/category_channel.rb
+++ b/plugins/chat/app/models/category_channel.rb
@@ -21,7 +21,23 @@ class CategoryChannel < ChatChannel
name.presence || category.name
end
- def slug
- title.truncate(100).parameterize
+ def generate_auto_slug
+ return if self.slug.present?
+ self.slug = Slug.for(self.title.strip, "")
+ self.slug = "" if duplicate_slug?
+ end
+
+ def ensure_slug_ok
+ # if we don't unescape it first we strip the % from the encoded version
+ slug = SiteSetting.slug_generation_method == "encoded" ? CGI.unescape(self.slug) : self.slug
+ self.slug = Slug.for(slug, "", method: :encoded)
+
+ if self.slug.blank?
+ errors.add(:slug, :invalid)
+ elsif SiteSetting.slug_generation_method == "ascii" && !CGI.unescape(self.slug).ascii_only?
+ errors.add(:slug, I18n.t("chat.category_channel.errors.slug_contains_non_ascii_chars"))
+ elsif duplicate_slug?
+ errors.add(:slug, I18n.t("chat.category_channel.errors.is_already_in_use"))
+ end
end
end
diff --git a/plugins/chat/app/models/chat_channel.rb b/plugins/chat/app/models/chat_channel.rb
index bbcb53e0ae7..560ae2d322b 100644
--- a/plugins/chat/app/models/chat_channel.rb
+++ b/plugins/chat/app/models/chat_channel.rb
@@ -21,6 +21,8 @@ class ChatChannel < ActiveRecord::Base
},
presence: true,
allow_nil: true
+ validate :ensure_slug_ok
+ before_validation :generate_auto_slug
scope :public_channels,
-> {
@@ -74,7 +76,11 @@ class ChatChannel < ActiveRecord::Base
end
def url
- "#{Discourse.base_url}/chat/channel/#{self.id}/-"
+ "#{Discourse.base_url}#{relative_url}"
+ end
+
+ def relative_url
+ "/chat/channel/#{self.id}/#{self.slug || "-"}"
end
def public_channel_title
@@ -109,6 +115,10 @@ class ChatChannel < ActiveRecord::Base
ChatPublisher.publish_channel_status(self)
end
+
+ def duplicate_slug?
+ ChatChannel.where(slug: self.slug).where.not(id: self.id).any?
+ end
end
# == Schema Information
@@ -132,6 +142,7 @@ end
# auto_join_users :boolean default(FALSE), not null
# user_count_stale :boolean default(FALSE), not null
# slug :string
+# type :string
#
# Indexes
#
diff --git a/plugins/chat/app/models/direct_message_channel.rb b/plugins/chat/app/models/direct_message_channel.rb
index 69be58827a6..9d116643d7e 100644
--- a/plugins/chat/app/models/direct_message_channel.rb
+++ b/plugins/chat/app/models/direct_message_channel.rb
@@ -18,4 +18,12 @@ class DirectMessageChannel < ChatChannel
def title(user)
direct_message.chat_channel_title_for_user(self, user)
end
+
+ def ensure_slug_ok
+ true
+ end
+
+ def generate_auto_slug
+ self.slug = nil
+ end
end
diff --git a/plugins/chat/app/serializers/chat_channel_serializer.rb b/plugins/chat/app/serializers/chat_channel_serializer.rb
index 8bb24577697..6a194047f98 100644
--- a/plugins/chat/app/serializers/chat_channel_serializer.rb
+++ b/plugins/chat/app/serializers/chat_channel_serializer.rb
@@ -9,6 +9,7 @@ class ChatChannelSerializer < ApplicationSerializer
:chatable_url,
:description,
:title,
+ :slug,
:last_message_sent_at,
:status,
:archive_failed,
diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js
index a523b2cea6b..a190e99a4b9 100644
--- a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js
+++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js
@@ -58,7 +58,7 @@ export default {
}
get name() {
- return dasherize(slugifyChannel(this.title));
+ return dasherize(slugifyChannel(this.channel));
}
get classNames() {
@@ -72,7 +72,7 @@ export default {
}
get models() {
- return [this.channel.id, slugifyChannel(this.title)];
+ return [this.channel.id, slugifyChannel(this.channel)];
}
get text() {
@@ -240,7 +240,7 @@ export default {
}
get name() {
- return slugifyChannel(this.title);
+ return slugifyChannel(this.channel);
}
get classNames() {
@@ -254,7 +254,7 @@ export default {
}
get models() {
- return [this.channel.id, slugifyChannel(this.title)];
+ return [this.channel.id, slugifyChannel(this.channel)];
}
get title() {
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 192203a506a..62758b9b0c5 100644
--- a/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js
+++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js
@@ -20,11 +20,15 @@ export default {
(NotificationItemBase) => {
return class extends NotificationItemBase {
get linkHref() {
- const title = this.notification.data.chat_channel_title
- ? slugifyChannel(this.notification.data.chat_channel_title)
- : "-";
-
- return `/chat/channel/${this.notification.data.chat_channel_id}/${title}?messageId=${this.notification.data.chat_message_id}`;
+ const slug = slugifyChannel({
+ title: this.notification.data.chat_channel_title,
+ slug: this.notification.data.chat_channel_slug,
+ });
+ return `/chat/channel/${
+ this.notification.data.chat_channel_id
+ }/${slug || "-"}?messageId=${
+ this.notification.data.chat_message_id
+ }`;
}
get linkTitle() {
@@ -53,11 +57,15 @@ export default {
(NotificationItemBase) => {
return class extends NotificationItemBase {
get linkHref() {
- const title = this.notification.data.chat_channel_title
- ? slugifyChannel(this.notification.data.chat_channel_title)
- : "-";
-
- return `/chat/channel/${this.notification.data.chat_channel_id}/${title}?messageId=${this.notification.data.chat_message_id}`;
+ const slug = slugifyChannel({
+ title: this.notification.data.chat_channel_title,
+ slug: this.notification.data.chat_channel_slug,
+ });
+ return `/chat/channel/${
+ this.notification.data.chat_channel_id
+ }/${slug || "-"}?messageId=${
+ this.notification.data.chat_message_id
+ }`;
}
get linkTitle() {
diff --git a/plugins/chat/assets/javascripts/discourse/lib/slugify-channel.js b/plugins/chat/assets/javascripts/discourse/lib/slugify-channel.js
index 885f9d11dea..f7a2734879e 100644
--- a/plugins/chat/assets/javascripts/discourse/lib/slugify-channel.js
+++ b/plugins/chat/assets/javascripts/discourse/lib/slugify-channel.js
@@ -1,8 +1,19 @@
import { slugify } from "discourse/lib/utilities";
-export default function slugifyChannel(title) {
- const slug = slugify(title);
- return (
- slug.length ? slug : title.trim().toLowerCase().replace(/\s|_+/g, "-")
+export default function slugifyChannel(channel) {
+ if (channel.slug) {
+ return channel.slug;
+ }
+ const slug = slugify(channel.escapedTitle || channel.title);
+ const resolvedSlug = (
+ slug.length
+ ? slug
+ : channel.title.trim().toLowerCase().replace(/\s|_+/g, "-")
).slice(0, 100);
+
+ if (!resolvedSlug) {
+ return "-";
+ }
+
+ return resolvedSlug;
}
diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js
index 97e42858d19..999870ac8a5 100644
--- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js
+++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js
@@ -42,7 +42,7 @@ export default class ChatChannelRoute extends DiscourseRoute {
this.chat.setActiveChannel(model?.chatChannel);
const queryParams = this.paramsFor(this.routeName);
- const slug = slugifyChannel(model.chatChannel.title);
+ const slug = slugifyChannel(model.chatChannel);
if (queryParams?.channelTitle !== slug) {
this.replaceWith("chat.channel.index", model.chatChannel.id, slug);
}
diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js
index af4925489b7..237e8328cf4 100644
--- a/plugins/chat/assets/javascripts/discourse/services/chat.js
+++ b/plugins/chat/assets/javascripts/discourse/services/chat.js
@@ -503,7 +503,7 @@ export default class Chat extends Service {
return this.router.transitionTo(
"chat.channel",
response.id,
- slugifyChannel(response.title),
+ slugifyChannel(response),
{ queryParams }
);
});
@@ -535,7 +535,7 @@ export default class Chat extends Service {
return this.router.transitionTo(
"chat.channel",
channel.id,
- slugifyChannel(channel.title),
+ slugifyChannel(channel),
{ queryParams }
);
} else {
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 01f6cc80854..fc488396a5f 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 @@
@route="chat.channel"
@models={{array
this.model.chatChannel.id
- (slugify-channel this.model.chatChannel.title)
+ (slugify-channel this.model.chatChannel)
}}
class="channel-info__back-btn"
>
@@ -40,7 +40,7 @@
@route={{concat "chat.channel.info." tab}}
@models={{array
this.model.chatChannel.id
- (slugify-channel this.model.chatChannel.title)
+ (slugify-channel this.model.chatChannel)
}}
class="chat-tabs-list__link"
>
diff --git a/plugins/chat/assets/javascripts/discourse/templates/components/chat-channel-card.hbs b/plugins/chat/assets/javascripts/discourse/templates/components/chat-channel-card.hbs
index 04be19f906b..411dc94ea38 100644
--- a/plugins/chat/assets/javascripts/discourse/templates/components/chat-channel-card.hbs
+++ b/plugins/chat/assets/javascripts/discourse/templates/components/chat-channel-card.hbs
@@ -10,7 +10,7 @@
<div class="chat-channel-card__header">
<LinkTo
@route="chat.channel"
- @models={{array this.channel.id (slugify-channel this.channel.title)}}
+ @models={{array this.channel.id (slugify-channel this.channel)}}
class="chat-channel-card__name-container"
>
<span class="chat-channel-card__name">
@@ -27,7 +27,7 @@
@route="chat.channel.info.settings"
@models={{array
this.channel.id
- (slugify-channel this.channel.title)
+ (slugify-channel this.channel)
}}
class="chat-channel-card__tag -muted"
tabindex="-1"
@@ -38,7 +38,7 @@
<LinkTo
@route="chat.channel.info.settings"
- @models={{array this.channel.id (slugify-channel this.channel.title)}}
+ @models={{array this.channel.id (slugify-channel this.channel)}}
class="chat-channel-card__setting"
tabindex="-1"
>
@@ -83,7 +83,7 @@
{{#if (gt this.channel.membershipsCount 0)}}
<LinkTo
@route="chat.channel.info.members"
- @models={{array this.channel.id (slugify-channel this.channel.title)}}
+ @models={{array this.channel.id (slugify-channel this.channel)}}
class="chat-channel-card__members"
tabindex="-1"
>
diff --git a/plugins/chat/assets/javascripts/discourse/templates/components/chat-live-pane.hbs b/plugins/chat/assets/javascripts/discourse/templates/components/chat-live-pane.hbs
index f7c62611509..43ad2f6197a 100644
--- a/plugins/chat/assets/javascripts/discourse/templates/components/chat-live-pane.hbs
+++ b/plugins/chat/assets/javascripts/discourse/templates/components/chat-live-pane.hbs
@@ -10,7 +10,7 @@
</div>
{{/if}}
- <LinkTo @route={{this.infoTabRoute}} @models={{array this.chatChannel.id (slugify-channel this.chatChannel.title)}} class="chat-full-page-header__title">
+ <LinkTo @route={{this.infoTabRoute}} @models={{array this.chatChannel.id (slugify-channel this.chatChannel)}} class="chat-full-page-header__title">
<ChatChannelTitle @channel={{this.chatChannel}} />
</LinkTo>
diff --git a/plugins/chat/assets/javascripts/discourse/templates/components/topic-chat-float.hbs b/plugins/chat/assets/javascripts/discourse/templates/components/topic-chat-float.hbs
index 58ec9d9a0cb..10af503050a 100644
--- a/plugins/chat/assets/javascripts/discourse/templates/components/topic-chat-float.hbs
+++ b/plugins/chat/assets/javascripts/discourse/templates/components/topic-chat-float.hbs
@@ -29,7 +29,7 @@
{{#if this.chat.activeChannel}}
{{#if this.expanded}}
- <LinkTo @route={{this.infoTabRoute}} @models={{array this.chat.activeChannel.id (slugify-channel this.chat.activeChannel.title)}} class="topic-chat-drawer-header__title">
+ <LinkTo @route={{this.infoTabRoute}} @models={{array this.chat.activeChannel.id (slugify-channel this.chat.activeChannel)}} class="topic-chat-drawer-header__title">
<div class="topic-chat-drawer-header__top-line">
<ChatChannelTitle @channel={{this.chat.activeChannel}} />
</div>
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 31d71168fe4..f9367adec4a 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
@@ -34,9 +34,12 @@ createWidgetFrom(DefaultNotificationItem, "chat-invitation-notification-item", {
},
url(data) {
- const title = data.chat_channel_title
- ? slugifyChannel(data.chat_channel_title)
- : "-";
- return `/chat/channel/${data.chat_channel_id}/${title}?messageId=${data.chat_message_id}`;
+ const slug = slugifyChannel({
+ title: data.chat_channel_title,
+ slug: data.chat_channel_slug,
+ });
+ return `/chat/channel/${data.chat_channel_id}/${slug || "-"}?messageId=${
+ 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 ea8c59cfde3..7d194879dc2 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
@@ -44,10 +44,13 @@ const chatNotificationItem = {
},
url(data) {
- const title = data.chat_channel_title
- ? slugifyChannel(data.chat_channel_title)
- : "-";
- return `/chat/channel/${data.chat_channel_id}/${title}?messageId=${data.chat_message_id}`;
+ const slug = slugifyChannel({
+ title: data.chat_channel_title,
+ slug: data.chat_channel_slug,
+ });
+ return `/chat/channel/${data.chat_channel_id}/${slug || "-"}?messageId=${
+ data.chat_message_id
+ }`;
},
};
diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml
index 72d651f2ab2..ea2349f2b7f 100644
--- a/plugins/chat/config/locales/server.en.yml
+++ b/plugins/chat/config/locales/server.en.yml
@@ -107,6 +107,11 @@ en:
multi_user: "%{users}"
multi_user_truncated: "%{users} and %{leftover} others"
+ category_channel:
+ errors:
+ slug_contains_non_ascii_chars: "contains non-ascii characters"
+ is_already_in_use: "is already in use"
+
bookmarkable:
notification_title: "message in %{channel_name}"
diff --git a/plugins/chat/db/post_migrate/20221104054957_backfill_channel_slugs.rb b/plugins/chat/db/post_migrate/20221104054957_backfill_channel_slugs.rb
new file mode 100644
index 00000000000..0efcdec486f
--- /dev/null
+++ b/plugins/chat/db/post_migrate/20221104054957_backfill_channel_slugs.rb
@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+class BackfillChannelSlugs < ActiveRecord::Migration[7.0]
+ def up
+ channels = DB.query(<<~SQL)
+ SELECT chat_channels.id, COALESCE(chat_channels.name, categories.name) AS title, NULL as slug
+ FROM chat_channels
+ INNER JOIN categories ON categories.id = chat_channels.chatable_id
+ WHERE chat_channels.chatable_type = 'Category' AND chat_channels.slug IS NULL
+ SQL
+ return if channels.count.zero?
+
+ DB.exec("CREATE TEMPORARY TABLE tmp_chat_channel_slugs(id int, slug text)")
+
+ taken_slugs = {}
+ channels.each do |channel|
+ # Simplified version of Slug.for generation that doesn't take into
+ # account different encodings to make things a little easier.
+ title = channel.title
+ if title.blank?
+ channel.slug = "channel-#{channel.id}"
+ else
+ channel.slug =
+ title
+ .downcase
+ .chomp
+ .tr("'", "")
+ .parameterize
+ .tr("_", "-")
+ .truncate(255, omission: "")
+ .squeeze("-")
+ .gsub(/\A-+|-+\z/, "")
+ end
+
+ # Deduplicate slugs with the channel IDs, we can always improve
+ # slugs later on.
+ channel.slug = "#{channel.slug}-#{channel.id}" if taken_slugs.key?(channel.slug)
+ taken_slugs[channel.slug] = true
+ end
+
+ values_to_insert =
+ channels.map { |channel| "(#{channel.id}, '#{PG::Connection.escape_string(channel.slug)}')" }
+
+ DB.exec(
+ "INSERT INTO tmp_chat_channel_slugs
+ VALUES #{values_to_insert.join(",\n")}",
+ )
+
+ DB.exec(<<~SQL)
+ UPDATE chat_channels cc
+ SET slug = tmp.slug
+ FROM tmp_chat_channel_slugs tmp
+ WHERE cc.id = tmp.id AND cc.slug IS NULL
+ SQL
+
+ DB.exec("DROP TABLE tmp_chat_channel_slugs(id int, slug text)")
+ end
+
+ def down
+ raise ActiveRecord::IrreversibleMigration
+ end
+end
diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb
index 5a6f7f231dc..ee07f45022a 100644
--- a/plugins/chat/spec/fabricators/chat_fabricator.rb
+++ b/plugins/chat/spec/fabricators/chat_fabricator.rb
@@ -2,9 +2,22 @@
Fabricator(:chat_channel) do
name do
- ["Gaming Lounge", "Music Lodge", "Random", "Politics", "Sports Center", "Kino Buffs"].sample
+ sequence(:name) do |n|
+ random_name = [
+ "Gaming Lounge",
+ "Music Lodge",
+ "Random",
+ "Politics",
+ "Sports Center",
+ "Kino Buffs",
+ ].sample
+ "#{random_name} #{n}"
+ end
end
chatable { Fabricate(:category) }
+ type do |attrs|
+ attrs[:chatable_type] == "Category" || attrs[:chatable]&.is_a?(Category) ? "CategoryChannel" : "DirectMessageChannel"
+ end
status { :open }
end
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 0693f32ca61..32204adc9cd 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/channel/#{public_channel.id}/#{expected_channel_title}?messageId=#{message.id}",
+ "/chat/channel/#{public_channel.id}/#{public_channel.slug}?messageId=#{message.id}",
)
end
@@ -230,7 +230,7 @@ describe Jobs::ChatNotifyMentioned do
tag: Chat::ChatNotifier.push_notification_tag(:mention, public_channel.id),
excerpt: message.push_notification_excerpt,
post_url:
- "/chat/channel/#{public_channel.id}/#{expected_channel_title}?messageId=#{message.id}",
+ "/chat/channel/#{public_channel.id}/#{public_channel.slug}?messageId=#{message.id}",
translated_title: payload_translated_title,
},
)
@@ -259,6 +259,7 @@ describe Jobs::ChatNotifyMentioned do
expect(data_hash[:mentioned_by_username]).to eq(user_1.username)
expect(data_hash[:is_direct_message_channel]).to eq(false)
expect(data_hash[:chat_channel_title]).to eq(expected_channel_title)
+ expect(data_hash[:chat_channel_slug]).to eq(public_channel.slug)
chat_mention =
ChatMention.where(notification: created_notification, user: user_2, chat_message: message)
diff --git a/plugins/chat/spec/jobs/regular/chat_notify_watching_spec.rb b/plugins/chat/spec/jobs/regular/chat_notify_watching_spec.rb
index b62a7a0573c..1690c3b0184 100644
--- a/plugins/chat/spec/jobs/regular/chat_notify_watching_spec.rb
+++ b/plugins/chat/spec/jobs/regular/chat_notify_watching_spec.rb
@@ -50,7 +50,13 @@ RSpec.describe Jobs::ChatNotifyWatching do
{
username: user1.username,
notification_type: Notification.types[:chat_message],
- post_url: "/chat/channel/#{channel.id}/#{channel.title(user2)}",
+ post_url: channel.relative_url,
+ translated_title:
+ I18n.t(
+ "discourse_push_notifications.popup.new_chat_message",
+ { username: user1.username, channel: channel.title(user2) },
+ ),
+ tag: Chat::ChatNotifier.push_notification_tag(:message, channel.id),
excerpt: message.message,
},
)
@@ -81,7 +87,13 @@ RSpec.describe Jobs::ChatNotifyWatching do
{
username: user1.username,
notification_type: Notification.types[:chat_message],
- post_url: "/chat/channel/#{channel.id}/#{channel.title(user2)}",
+ post_url: channel.relative_url,
+ translated_title:
+ I18n.t(
+ "discourse_push_notifications.popup.new_chat_message",
+ { username: user1.username, channel: channel.title(user2) },
+ ),
+ tag: Chat::ChatNotifier.push_notification_tag(:message, channel.id),
excerpt: message.message,
},
),
@@ -176,7 +188,13 @@ RSpec.describe Jobs::ChatNotifyWatching do
{
username: user1.username,
notification_type: Notification.types[:chat_message],
- post_url: "/chat/channel/#{channel.id}/#{channel.title(user2)}",
+ post_url: channel.relative_url,
+ translated_title:
+ I18n.t(
+ "discourse_push_notifications.popup.new_direct_chat_message",
+ { username: user1.username, channel: channel.title(user2) },
+ ),
+ tag: Chat::ChatNotifier.push_notification_tag(:message, channel.id),
excerpt: message.message,
},
)
@@ -207,7 +225,13 @@ RSpec.describe Jobs::ChatNotifyWatching do
{
username: user1.username,
notification_type: Notification.types[:chat_message],
- post_url: "/chat/channel/#{channel.id}/#{channel.title(user2)}",
+ post_url: channel.relative_url,
+ translated_title:
+ I18n.t(
+ "discourse_push_notifications.popup.new_direct_chat_message",
+ { username: user1.username, channel: channel.title(user2) },
+ ),
+ tag: Chat::ChatNotifier.push_notification_tag(:message, channel.id),
excerpt: message.message,
},
),
diff --git a/plugins/chat/spec/models/category_channel_spec.rb b/plugins/chat/spec/models/category_channel_spec.rb
index ad59b3efe91..89c950f5f7a 100644
--- a/plugins/chat/spec/models/category_channel_spec.rb
+++ b/plugins/chat/spec/models/category_channel_spec.rb
@@ -83,4 +83,70 @@ RSpec.describe CategoryChannel do
end
end
end
+
+ describe "slug generation" do
+ subject(:channel) { Fabricate(:category_channel) }
+
+ context "when slug is not provided" do
+ before do
+ channel.slug = nil
+ end
+
+ it "uses channel name when present" do
+ channel.name = "Some Cool Stuff"
+ channel.validate!
+ expect(channel.slug).to eq("some-cool-stuff")
+ end
+
+ it "uses category name when present" do
+ channel.name = nil
+ channel.category.name = "some category stuff"
+ channel.validate!
+ expect(channel.slug).to eq("some-category-stuff")
+ end
+ end
+
+ context "when slug is provided" do
+ context "when using encoded slug generator" do
+ before do
+ SiteSetting.slug_generation_method = "encoded"
+ channel.slug = "测试"
+ end
+ after { SiteSetting.slug_generation_method = "ascii" }
+
+ it "creates a slug with the correct escaping" do
+ channel.validate!
+ expect(channel.slug).to eq("%E6%B5%8B%E8%AF%95")
+ end
+ end
+
+ context "when slug ends up blank" do
+ it "adds a validation error" do
+ channel.slug = "-"
+ channel.validate
+ expect(channel.errors.full_messages).to include("Slug is invalid")
+ end
+ end
+
+ context "when there is a duplicate slug" do
+ before { Fabricate(:category_channel, slug: "awesome-channel") }
+
+ it "adds a validation error" do
+ channel.slug = "awesome-channel"
+ channel.validate
+ expect(channel.errors.full_messages.first).to include(I18n.t("chat.category_channel.errors.is_already_in_use"))
+ end
+ end
+
+ context "if SiteSettings.slug_generation_method = ascii" do
+ before { SiteSetting.slug_generation_method = "ascii" }
+
+ it "fails if slug contains non-ascii characters" do
+ channel.slug = "sem-acentuação"
+ channel.validate
+ expect(channel.errors.full_messages.first).to match(/#{I18n.t("chat.category_channel.errors.slug_contains_non_ascii_chars")}/)
+ end
+ end
+ end
+ end
end
diff --git a/plugins/chat/spec/models/chat_channel_spec.rb b/plugins/chat/spec/models/chat_channel_spec.rb
new file mode 100644
index 00000000000..475390a07a9
--- /dev/null
+++ b/plugins/chat/spec/models/chat_channel_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+RSpec.describe ChatChannel do
+ fab!(:category_channel) { Fabricate(:category_channel) }
+ fab!(:dm_channel) { Fabricate(:direct_message_channel) }
+
+ describe "#relative_url" do
+ context "when the slug is nil" do
+ it "uses a - instead" do
+ category_channel.slug = nil
+ expect(category_channel.relative_url).to eq("/chat/channel/#{category_channel.id}/-")
+ end
+ end
+
+ context "when the slug is not nil" do
+ before do
+ category_channel.update!(slug: "some-cool-channel")
+ end
+
+ it "includes the slug for the channel" do
+ expect(category_channel.relative_url).to eq("/chat/channel/#{category_channel.id}/some-cool-channel")
+ end
+ end
+ end
+end
diff --git a/plugins/chat/spec/models/direct_message_channel_spec.rb b/plugins/chat/spec/models/direct_message_channel_spec.rb
index 85674e858fb..227a143d60d 100644
--- a/plugins/chat/spec/models/direct_message_channel_spec.rb
+++ b/plugins/chat/spec/models/direct_message_channel_spec.rb
@@ -60,4 +60,14 @@ RSpec.describe DirectMessageChannel do
expect(title).to eq("something")
end
end
+
+ describe "slug generation" do
+ subject(:channel) { Fabricate(:direct_message_channel) }
+
+ it "always sets the slug to nil for direct message channels" do
+ channel.name = "Cool Channel"
+ channel.validate!
+ expect(channel.slug).to eq(nil)
+ end
+ end
end
diff --git a/plugins/chat/spec/requests/chat_channel_controller_spec.rb b/plugins/chat/spec/requests/chat_channel_controller_spec.rb
index f40a9ca0149..9e88a47f72f 100644
--- a/plugins/chat/spec/requests/chat_channel_controller_spec.rb
+++ b/plugins/chat/spec/requests/chat_channel_controller_spec.rb
@@ -305,7 +305,7 @@ RSpec.describe Chat::ChatChannelsController do
it "errors when channel for category and same name already exists" do
sign_in(admin)
name = "beep boop hi"
- ChatChannel.create!(chatable: category2, name: name)
+ category2.create_chat_channel!(name: name)
put "/chat/chat_channels.json", params: { id: category2.id, name: name }
expect(response.status).to eq(400)
@@ -313,7 +313,7 @@ RSpec.describe Chat::ChatChannelsController do
it "creates a channel for category and if name is unique" do
sign_in(admin)
- ChatChannel.create!(chatable: category2, name: "this is a name")
+ category2.create_chat_channel!(name: "this is a name")
expect {
put "/chat/chat_channels.json", params: { id: category2.id, name: "Different name!" }
diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb
index 78dc47157f8..ad42097db28 100644
--- a/plugins/chat/spec/requests/chat_controller_spec.rb
+++ b/plugins/chat/spec/requests/chat_controller_spec.rb
@@ -809,6 +809,7 @@ RSpec.describe Chat::ChatController do
chat_message_id: msg.id,
chat_channel_id: msg.chat_channel_id,
chat_channel_title: msg.chat_channel.title(user),
+ chat_channel_slug: msg.chat_channel.slug,
mentioned_by_username: sender.username,
}.to_json,
)
@@ -1029,6 +1030,10 @@ RSpec.describe Chat::ChatController do
}.to change {
user.notifications.where(notification_type: Notification.types[:chat_invitation]).count
}.by(1)
+ notification = user.notifications.where(notification_type: Notification.types[:chat_invitation]).last
+ parsed_data = JSON.parse(notification[:data])
+ expect(parsed_data["chat_channel_title"]).to eq(chat_channel.title(user))
+ expect(parsed_data["chat_channel_slug"]).to eq(chat_channel.slug)
end
it "creates multiple invitations" do
diff --git a/plugins/chat/test/javascripts/chat-fixtures.js b/plugins/chat/test/javascripts/chat-fixtures.js
index 1877116d9f2..c8ab6151605 100644
--- a/plugins/chat/test/javascripts/chat-fixtures.js
+++ b/plugins/chat/test/javascripts/chat-fixtures.js
@@ -78,7 +78,7 @@ const chatables = {
8: {
id: 8,
name: "Public category",
- slug: "public_category",
+ slug: "public-category",
posts_count: 1,
},
12: {
diff --git a/plugins/chat/test/javascripts/unit/lib/slugify-channel-test.js b/plugins/chat/test/javascripts/unit/lib/slugify-channel-test.js
index 52bd96830ef..9ce84b57ae3 100644
--- a/plugins/chat/test/javascripts/unit/lib/slugify-channel-test.js
+++ b/plugins/chat/test/javascripts/unit/lib/slugify-channel-test.js
@@ -2,20 +2,38 @@ import { module, test } from "qunit";
import slugifyChannel from "discourse/plugins/chat/discourse/lib/slugify-channel";
module("Discourse Chat | Unit | slugify-channel", function () {
- test("defaults", function (assert) {
- assert.equal(slugifyChannel("Foo bar"), "foo-bar");
+ test("defaults for title", function (assert) {
+ assert.equal(slugifyChannel({ title: "Foo bar" }), "foo-bar");
});
- test("a very long name", function (assert) {
+ test("a very long name for the title", function (assert) {
const string =
"xAq8l5ca2CtEToeMLe2pEr2VUGQBx3HPlxbkDExKrJHp4f7jCVw9id1EQv1N1lYMRdAIiZNnn94Kr0uU0iiEeVO4XkBVmpW8Mknmd";
- assert.equal(slugifyChannel(string), string.toLowerCase().slice(0, -1));
+ assert.equal(
+ slugifyChannel({ title: string }),
+ string.toLowerCase().slice(0, -1)
+ );
});
- test("a cyrillic name", function (assert) {
+ test("a cyrillic name for the title", function (assert) {
const string = "Русская литература и фольклор";
- assert.equal(slugifyChannel(string), "русская-литература-и-фольклор");
+ assert.equal(
+ slugifyChannel({ title: string }),
+ "русская-литература-и-фольклор"
+ );
+ });
+
+ test("channel has escapedTitle", function (assert) {
+ assert.equal(slugifyChannel({ escapedTitle: "Foo bar" }), "foo-bar");
+ });
+
+ test("channel has slug and title", function (assert) {
+ assert.equal(
+ slugifyChannel({ title: "Foo bar", slug: "some-other-thing" }),
+ "some-other-thing",
+ "slug takes priority"
+ );
});
});
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 a5f561b2a0f..8e850d9f8ff 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
@@ -43,9 +43,9 @@ module(
const data = this.args.data;
assert.strictEqual(
query(".chat-invitation a").getAttribute("href"),
- `/chat/channel/${data.chat_channel_id}/${slugifyChannel(
- data.chat_channel_title
- )}?messageId=${data.chat_message_id}`
+ `/chat/channel/${data.chat_channel_id}/${slugifyChannel({
+ title: data.chat_channel_title,
+ })}?messageId=${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 d341e4ac8ce..65532c33b10 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
@@ -52,9 +52,9 @@ module(
assert.strictEqual(
query(".chat-invitation a").getAttribute("href"),
- `/chat/channel/${data.chat_channel_id}/${slugifyChannel(
- data.chat_channel_title
- )}?messageId=${data.chat_message_id}`
+ `/chat/channel/${data.chat_channel_id}/${slugifyChannel({
+ title: data.chat_channel_title,
+ })}?messageId=${data.chat_message_id}`
);
});
}
@@ -91,9 +91,9 @@ module(
assert.strictEqual(
query(".chat-invitation a").getAttribute("href"),
- `/chat/channel/${data.chat_channel_id}/${slugifyChannel(
- data.chat_channel_title
- )}?messageId=${data.chat_message_id}`
+ `/chat/channel/${data.chat_channel_id}/${slugifyChannel({
+ title: data.chat_channel_title,
+ })}?messageId=${data.chat_message_id}`
);
});
}
@@ -130,9 +130,9 @@ module(
assert.strictEqual(
query(".chat-invitation a").getAttribute("href"),
- `/chat/channel/${data.chat_channel_id}/${slugifyChannel(
- data.chat_channel_title
- )}?messageId=${data.chat_message_id}`
+ `/chat/channel/${data.chat_channel_id}/${slugifyChannel({
+ title: data.chat_channel_title,
+ })}?messageId=${data.chat_message_id}`
);
});
}