FEATURE: Allow changing slug on create channel (#19928)

This commit allows us to set the channel slug when creating new chat
channels. As well as this, it introduces a new `SlugsController` which can
generate a slug using `Slug.for` and a name string for input. We call this
after the user finishes typing the channel name (debounced) and fill in
the autogenerated slug in the background, and update the slug input
placeholder.

This autogenerated slug is used by default, but if the user writes anything
else in the input it will be used instead.
This commit is contained in:
Martin Brennan 2023-01-23 14:48:33 +10:00 committed by GitHub
parent ae20ce8654
commit 641e94fc3c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 322 additions and 48 deletions

View File

@ -0,0 +1,22 @@
# frozen_string_literal: true
class SlugsController < ApplicationController
requires_login
MAX_SLUG_GENERATIONS_PER_MINUTE = 20
def generate
params.require(:name)
raise Discourse::InvalidAccess if !current_user.has_trust_level_or_staff?(TrustLevel[4])
RateLimiter.new(
current_user,
"max-slug-generations-per-minute",
MAX_SLUG_GENERATIONS_PER_MINUTE,
1.minute,
).performed!
render json: success_json.merge(slug: Slug.for(params[:name], ""))
end
end

View File

@ -1064,6 +1064,8 @@ Discourse::Application.routes.draw do
resources :associated_groups, only: %i[index], constraints: AdminConstraint.new resources :associated_groups, only: %i[index], constraints: AdminConstraint.new
get "slugs/generate", to: "slugs#generate"
# aliases so old API code works # aliases so old API code works
delete "admin/groups/:id/members" => "groups#remove_member", :constraints => AdminConstraint.new delete "admin/groups/:id/members" => "groups#remove_member", :constraints => AdminConstraint.new
put "admin/groups/:id/members" => "groups#add_members", :constraints => AdminConstraint.new put "admin/groups/:id/members" => "groups#add_members", :constraints => AdminConstraint.new

View File

@ -57,7 +57,7 @@ class Chat::Api::ChatChannelsController < Chat::Api
def create def create
channel_params = channel_params =
params.require(:channel).permit(:chatable_id, :name, :description, :auto_join_users) params.require(:channel).permit(:chatable_id, :name, :slug, :description, :auto_join_users)
guardian.ensure_can_create_chat_channel! guardian.ensure_can_create_chat_channel!
if channel_params[:name].length > SiteSetting.max_topic_title_length if channel_params[:name].length > SiteSetting.max_topic_title_length
@ -81,6 +81,7 @@ class Chat::Api::ChatChannelsController < Chat::Api
channel = channel =
chatable.create_chat_channel!( chatable.create_chat_channel!(
name: channel_params[:name], name: channel_params[:name],
slug: channel_params[:slug],
description: channel_params[:description], description: channel_params[:description],
user_count: 1, user_count: 1,
auto_join_users: auto_join_users, auto_join_users: auto_join_users,

View File

@ -1,4 +1,7 @@
import { escapeExpression } from "discourse/lib/utilities"; import { escapeExpression } from "discourse/lib/utilities";
import { ajax } from "discourse/lib/ajax";
import { cancel } from "@ember/runloop";
import discourseDebounce from "discourse-common/lib/debounce";
import Controller from "@ember/controller"; import Controller from "@ember/controller";
import I18n from "I18n"; import I18n from "I18n";
import ModalFunctionality from "discourse/mixins/modal-functionality"; import ModalFunctionality from "discourse/mixins/modal-functionality";
@ -26,6 +29,8 @@ export default class CreateChannelController extends Controller.extend(
category = null; category = null;
categoryId = null; categoryId = null;
name = ""; name = "";
slug = "";
autoGeneratedSlug = "";
description = ""; description = "";
categoryPermissionsHint = null; categoryPermissionsHint = null;
autoJoinUsers = null; autoJoinUsers = null;
@ -51,11 +56,14 @@ export default class CreateChannelController extends Controller.extend(
} }
onClose() { onClose() {
cancel(this.generateSlugHandler);
this.setProperties({ this.setProperties({
categoryId: null, categoryId: null,
category: null, category: null,
name: "", name: "",
description: "", description: "",
slug: "",
autoGeneratedSlug: "",
categoryPermissionsHint: DEFAULT_HINT, categoryPermissionsHint: DEFAULT_HINT,
autoJoinWarning: "", autoJoinWarning: "",
}); });
@ -65,6 +73,7 @@ export default class CreateChannelController extends Controller.extend(
const data = { const data = {
chatable_id: this.categoryId, chatable_id: this.categoryId,
name: this.name, name: this.name,
slug: this.slug || this.autoGeneratedSlug,
description: this.description, description: this.description,
auto_join_users: this.autoJoinUsers, auto_join_users: this.autoJoinUsers,
}; };
@ -147,17 +156,53 @@ export default class CreateChannelController extends Controller.extend(
} }
} }
// intentionally not showing AJAX error for this, we will autogenerate
// the slug server-side if they leave it blank
_generateSlug(name) {
ajax("/slugs/generate.json", { type: "GET", data: { name } }).then(
(response) => {
this.set("autoGeneratedSlug", response.slug);
}
);
}
_debouncedGenerateSlug(name) {
cancel(this.generateSlugHandler);
this._clearAutoGeneratedSlug();
if (!name) {
return;
}
this.generateSlugHandler = discourseDebounce(
this,
this._generateSlug,
name,
300
);
}
_clearAutoGeneratedSlug() {
this.set("autoGeneratedSlug", "");
}
@action @action
onCategoryChange(categoryId) { onCategoryChange(categoryId) {
let category = categoryId let category = categoryId
? this.site.categories.findBy("id", categoryId) ? this.site.categories.findBy("id", categoryId)
: null; : null;
this._updatePermissionsHint(category); this._updatePermissionsHint(category);
const name = this.name || category?.name || "";
this.setProperties({ this.setProperties({
categoryId, categoryId,
category, category,
name: this.name || category?.name || "", name,
}); });
this._debouncedGenerateSlug(name);
}
@action
onNameChange(name) {
this._debouncedGenerateSlug(name);
} }
@action @action

View File

@ -8,6 +8,20 @@
class="create-channel-name-input" class="create-channel-name-input"
@type="text" @type="text"
@value={{this.name}} @value={{this.name}}
{{on "input" (action "onNameChange" value="target.value")}}
/>
</div>
<div class="create-channel-control">
<label for="channel-slug" class="create-channel-label">
{{i18n "chat.create_channel.slug"}}
</label>
<Input
name="channel-slug"
class="create-channel-slug-input"
@type="text"
@value={{this.slug}}
placeholder={{this.autoGeneratedSlug}}
/> />
</div> </div>

View File

@ -10,6 +10,7 @@
.select-kit.combo-box, .select-kit.combo-box,
.create-channel-name-input, .create-channel-name-input,
.create-channel-slug-input,
.create-channel-description-input, .create-channel-description-input,
#choose-topic-title { #choose-topic-title {
width: 100%; width: 100%;

View File

@ -289,6 +289,7 @@ en:
create: "Create channel" create: "Create channel"
description: "Description (optional)" description: "Description (optional)"
name: "Channel name" name: "Channel name"
slug: "Channel slug (optional)"
title: "New channel" title: "New channel"
type: "Type" type: "Type"
types: types:

View File

@ -263,11 +263,22 @@ RSpec.describe Chat::Api::ChatChannelsController do
new_channel = ChatChannel.last new_channel = ChatChannel.last
expect(new_channel.name).to eq(params[:channel][:name]) expect(new_channel.name).to eq(params[:channel][:name])
expect(new_channel.slug).to eq("channel-name")
expect(new_channel.description).to eq(params[:channel][:description]) expect(new_channel.description).to eq(params[:channel][:description])
expect(new_channel.chatable_type).to eq(category.class.name) expect(new_channel.chatable_type).to eq(category.class.name)
expect(new_channel.chatable_id).to eq(category.id) expect(new_channel.chatable_id).to eq(category.id)
end end
it "creates a channel using the user-provided slug" do
new_params = params.dup
new_params[:channel][:slug] = "wow-so-cool"
post "/chat/api/channels", params: new_params
new_channel = ChatChannel.last
expect(new_channel.slug).to eq("wow-so-cool")
end
it "creates a channel sets auto_join_users to false by default" do it "creates a channel sets auto_join_users to false by default" do
post "/chat/api/channels", params: params post "/chat/api/channels", params: params

View File

@ -1,35 +1,43 @@
# frozen_string_literal: true # frozen_string_literal: true
RSpec.describe "Create channel", type: :system, js: true do RSpec.describe "Create channel", type: :system, js: true do
fab!(:current_admin_user) { Fabricate(:admin) }
fab!(:category_1) { Fabricate(:category) } fab!(:category_1) { Fabricate(:category) }
let(:chat_page) { PageObjects::Pages::Chat.new } let(:chat_page) { PageObjects::Pages::Chat.new }
let(:channel_modal) { PageObjects::Modals::ChatChannelCreate.new }
before do before { chat_system_bootstrap }
chat_system_bootstrap
sign_in(current_admin_user) context "when user cannot create channel" do
fab!(:current_user) { Fabricate(:user) }
before { sign_in(current_user) }
it "does not show the create channel button" do
chat_page.visit_browse
expect(chat_page).not_to have_new_channel_button
end
end end
context "when can create channel" do context "when can create channel" do
fab!(:current_admin_user) { Fabricate(:admin) }
before { sign_in(current_admin_user) }
context "when selecting a category" do context "when selecting a category" do
it "shows access hint" do it "shows access hint" do
visit("/chat") chat_page.visit_browse
find(".new-channel-btn").click chat_page.new_channel_button.click
find(".category-chooser").click channel_modal.select_category(category_1)
find(".category-row[data-value=\"#{category_1.id}\"]").click
expect(find(".create-channel-hint")).to have_content(Group[:everyone].name) expect(channel_modal).to have_create_hint(Group[:everyone].name)
end end
it "does not override channel name if that was already specified" do it "does not override channel name if that was already specified" do
visit("/chat") chat_page.visit_browse
find(".new-channel-btn").click chat_page.new_channel_button.click
fill_in("channel-name", with: "My Cool Channel") channel_modal.fill_name("My Cool Channel")
find(".category-chooser").click channel_modal.select_category(category_1)
find(".category-row[data-value=\"#{category_1.id}\"]").click
expect(page).to have_field("channel-name", with: "My Cool Channel") expect(channel_modal).to have_name_prefilled("My Cool Channel")
end end
context "when category is private" do context "when category is private" do
@ -37,12 +45,11 @@ RSpec.describe "Create channel", type: :system, js: true do
fab!(:private_category_1) { Fabricate(:private_category, group: group_1) } fab!(:private_category_1) { Fabricate(:private_category, group: group_1) }
it "shows access hint when selecting the category" do it "shows access hint when selecting the category" do
visit("/chat") chat_page.visit_browse
find(".new-channel-btn").click chat_page.new_channel_button.click
find(".category-chooser").click channel_modal.select_category(private_category_1)
find(".category-row[data-value=\"#{private_category_1.id}\"]").click
expect(find(".create-channel-hint")).to have_content(group_1.name) expect(channel_modal).to have_create_hint(group_1.name)
end end
context "when category is a child" do context "when category is a child" do
@ -52,12 +59,11 @@ RSpec.describe "Create channel", type: :system, js: true do
end end
it "shows access hint when selecting the category" do it "shows access hint when selecting the category" do
visit("/chat") chat_page.visit_browse
find(".new-channel-btn").click chat_page.new_channel_button.click
find(".category-chooser").click channel_modal.select_category(child_category)
find(".category-row[data-value=\"#{child_category.id}\"]").click
expect(find(".create-channel-hint")).to have_content(group_2.name) expect(channel_modal).to have_create_hint(group_2.name)
end end
end end
end end
@ -72,45 +78,91 @@ RSpec.describe "Create channel", type: :system, js: true do
fab!(:private_category_1) { Fabricate(:private_category, group: group_1) } fab!(:private_category_1) { Fabricate(:private_category, group: group_1) }
it "escapes the group name" do it "escapes the group name" do
visit("/chat") chat_page.visit_browse
find(".new-channel-btn").click chat_page.new_channel_button.click
find(".category-chooser").click channel_modal.select_category(private_category_1)
find(".category-row[data-value=\"#{private_category_1.id}\"]").click
expect(find(".create-channel-hint")["innerHTML"].strip).to include( expect(channel_modal.create_channel_hint["innerHTML"].strip).to include(
"&lt;script&gt;e&lt;/script&gt;", "&lt;script&gt;e&lt;/script&gt;",
) )
end end
end end
it "autogenerates slug from name and changes slug placeholder" do
chat_page.visit_browse
chat_page.new_channel_button.click
name = "Cats & Dogs"
channel_modal.select_category(category_1)
channel_modal.fill_name(name)
channel_modal.fill_description("All kind of cute cats")
wait_for_attribute(channel_modal.slug_input, :placeholder, "cats-dogs")
channel_modal.click_primary_button
expect(page).to have_content(name)
created_channel = ChatChannel.find_by(chatable_id: category_1.id)
expect(created_channel.slug).to eq("cats-dogs")
expect(page).to have_current_path(chat.channel_path(created_channel.id, created_channel.slug))
end
it "allows the user to set a slug independently of name" do
chat_page.visit_browse
chat_page.new_channel_button.click
name = "Cats & Dogs"
channel_modal.select_category(category_1)
channel_modal.fill_name(name)
channel_modal.fill_description("All kind of cute cats")
channel_modal.fill_slug("pets-everywhere")
channel_modal.click_primary_button
expect(page).to have_content(name)
created_channel = ChatChannel.find_by(chatable_id: category_1.id)
expect(created_channel.slug).to eq("pets-everywhere")
expect(page).to have_current_path(chat.channel_path(created_channel.id, created_channel.slug))
end
context "when saving" do context "when saving" do
context "when error" do context "when error" do
it "displays the error" do it "displays the error" do
existing_channel = Fabricate(:chat_channel) existing_channel = Fabricate(:chat_channel)
visit("/chat") chat_page.visit_browse
find(".new-channel-btn").click chat_page.new_channel_button.click
find(".category-chooser").click channel_modal.select_category(existing_channel.chatable)
find(".category-row[data-value=\"#{existing_channel.chatable_id}\"]").click channel_modal.fill_name(existing_channel.name)
fill_in("channel-name", with: existing_channel.name) channel_modal.click_primary_button
find(".create-channel-modal .create").click
expect(page).to have_content(I18n.t("chat.errors.channel_exists_for_category")) expect(page).to have_content(I18n.t("chat.errors.channel_exists_for_category"))
end end
end end
context "when slug is already being used" do
it "displays the error" do
Fabricate(:chat_channel, slug: "pets-everywhere")
chat_page.visit_browse
chat_page.new_channel_button.click
channel_modal.select_category(category_1)
channel_modal.fill_name("Testing")
channel_modal.fill_slug("pets-everywhere")
channel_modal.click_primary_button
expect(page).to have_content(
"Slug " + I18n.t("chat.category_channel.errors.is_already_in_use"),
)
end
end
context "when successful" do context "when successful" do
it "redirects to created channel" do it "redirects to created channel" do
visit("/chat") chat_page.visit_browse
find(".new-channel-btn").click chat_page.new_channel_button.click
name = "Cats" channel_modal.select_category(category_1)
find(".category-chooser").click expect(channel_modal).to have_name_prefilled(category_1.name)
find(".category-row[data-value=\"#{category_1.id}\"]").click
expect(page).to have_field("channel-name", with: category_1.name)
fill_in("channel-name", with: name)
fill_in("channel-description", with: "All kind of cute cats")
find(".create-channel-modal .create").click
expect(page).to have_content(name) channel_modal.fill_description("All kind of cute cats")
channel_modal.click_primary_button
expect(page).to have_content(category_1.name)
created_channel = ChatChannel.find_by(chatable_id: category_1.id) created_channel = ChatChannel.find_by(chatable_id: category_1.id)
expect(page).to have_current_path( expect(page).to have_current_path(
chat.channel_path(created_channel.id, created_channel.slug), chat.channel_path(created_channel.id, created_channel.slug),

View File

@ -51,6 +51,14 @@ module PageObjects
container.has_content?(message.message) container.has_content?(message.message)
container.has_content?(message.user.username) container.has_content?(message.user.username)
end end
def new_channel_button
find(".new-channel-btn")
end
def has_new_channel_button?
has_css?(".new-channel-btn")
end
end end
end end
end end

View File

@ -0,0 +1,40 @@
# frozen_string_literal: true
module PageObjects
module Modals
class ChatChannelCreate < PageObjects::Modals::Base
def select_category(category)
find(".category-chooser").click
find(".category-row[data-value=\"#{category.id}\"]").click
end
def create_channel_hint
find(".create-channel-hint")
end
def slug_input
find(".create-channel-slug-input")
end
def has_create_hint?(content)
create_channel_hint.has_content?(content)
end
def fill_name(name)
fill_in("channel-name", with: name)
end
def fill_slug(slug)
fill_in("channel-slug", with: slug)
end
def fill_description(description)
fill_in("channel-description", with: description)
end
def has_name_prefilled?(name)
has_field?("channel-name", with: name)
end
end
end
end

View File

@ -0,0 +1,61 @@
# frozen_string_literal: true
RSpec.describe SlugsController do
fab!(:current_user) { Fabricate(:user, trust_level: TrustLevel[4]) }
describe "#generate" do
let(:name) { "Arts & Media" }
context "when user not logged in" do
it "returns a 403 error" do
get "/slugs/generate.json?name=#{name}"
expect(response.status).to eq(403)
end
end
context "when user is logged in" do
before { sign_in(current_user) }
it "generates a slug from the name" do
get "/slugs/generate.json", params: { name: name }
expect(response.status).to eq(200)
expect(response.parsed_body["slug"]).to eq(Slug.for(name, ""))
end
it "rate limits" do
RateLimiter.enable
stub_const(SlugsController, "MAX_SLUG_GENERATIONS_PER_MINUTE", 1) do
get "/slugs/generate.json?name=#{name}"
get "/slugs/generate.json?name=#{name}"
end
expect(response.status).to eq(429)
end
it "requires name" do
get "/slugs/generate.json"
expect(response.status).to eq(400)
end
context "when user is not TL4 or higher" do
before { current_user.change_trust_level!(1) }
it "returns a 403 error" do
get "/slugs/generate.json?name=#{name}"
expect(response.status).to eq(403)
end
end
context "when user is admin" do
fab!(:current_user) { Fabricate(:admin) }
it "generates a slug from the name" do
get "/slugs/generate.json", params: { name: name }
expect(response.status).to eq(200)
expect(response.parsed_body["slug"]).to eq(Slug.for(name, ""))
end
end
end
end
end

View File

@ -39,6 +39,18 @@ module SystemHelpers
retry retry
end end
def wait_for_attribute(
element,
attribute,
value,
timeout: Capybara.default_max_wait_time,
frequency: 0.01
)
try_until_success(timeout: timeout, frequency: frequency) do
expect(element[attribute.to_sym]).to eq(value)
end
end
def resize_window(width: nil, height: nil) def resize_window(width: nil, height: nil)
original_size = page.driver.browser.manage.window.size original_size = page.driver.browser.manage.window.size
page.driver.browser.manage.window.resize_to( page.driver.browser.manage.window.resize_to(

View File

@ -17,6 +17,10 @@ module PageObjects
def click_outside def click_outside
find(".modal-outer-container").click(x: 0, y: 0) find(".modal-outer-container").click(x: 0, y: 0)
end end
def click_primary_button
find(".modal-footer .btn-primary").click
end
end end
end end
end end