From 641e94fc3c15c12217e2463a2dc393638ddeb534 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 23 Jan 2023 14:48:33 +1000 Subject: [PATCH] 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. --- app/controllers/slugs_controller.rb | 22 +++ config/routes.rb | 2 + .../api/chat_channels_controller.rb | 3 +- .../discourse/controllers/create-channel.js | 47 +++++- .../templates/modal/create-channel.hbs | 14 ++ .../common/create-channel-modal.scss | 1 + plugins/chat/config/locales/client.en.yml | 1 + .../api/chat_channels_controller_spec.rb | 11 ++ .../chat/spec/system/create_channel_spec.rb | 144 ++++++++++++------ .../spec/system/page_objects/chat/chat.rb | 8 + .../modals/chat_channel_create.rb | 40 +++++ spec/requests/slugs_controller_spec.rb | 61 ++++++++ spec/support/system_helpers.rb | 12 ++ spec/system/page_objects/modals/base.rb | 4 + 14 files changed, 322 insertions(+), 48 deletions(-) create mode 100644 app/controllers/slugs_controller.rb create mode 100644 plugins/chat/spec/system/page_objects/modals/chat_channel_create.rb create mode 100644 spec/requests/slugs_controller_spec.rb diff --git a/app/controllers/slugs_controller.rb b/app/controllers/slugs_controller.rb new file mode 100644 index 00000000000..531f8ff4fd8 --- /dev/null +++ b/app/controllers/slugs_controller.rb @@ -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 diff --git a/config/routes.rb b/config/routes.rb index 606fffd6d50..860ab8bd18f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1064,6 +1064,8 @@ Discourse::Application.routes.draw do resources :associated_groups, only: %i[index], constraints: AdminConstraint.new + get "slugs/generate", to: "slugs#generate" + # aliases so old API code works delete "admin/groups/:id/members" => "groups#remove_member", :constraints => AdminConstraint.new put "admin/groups/:id/members" => "groups#add_members", :constraints => AdminConstraint.new diff --git a/plugins/chat/app/controllers/api/chat_channels_controller.rb b/plugins/chat/app/controllers/api/chat_channels_controller.rb index f61107037ab..5d007000217 100644 --- a/plugins/chat/app/controllers/api/chat_channels_controller.rb +++ b/plugins/chat/app/controllers/api/chat_channels_controller.rb @@ -57,7 +57,7 @@ class Chat::Api::ChatChannelsController < Chat::Api def create 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! if channel_params[:name].length > SiteSetting.max_topic_title_length @@ -81,6 +81,7 @@ class Chat::Api::ChatChannelsController < Chat::Api channel = chatable.create_chat_channel!( name: channel_params[:name], + slug: channel_params[:slug], description: channel_params[:description], user_count: 1, auto_join_users: auto_join_users, diff --git a/plugins/chat/assets/javascripts/discourse/controllers/create-channel.js b/plugins/chat/assets/javascripts/discourse/controllers/create-channel.js index 9d024369470..062a3c76648 100644 --- a/plugins/chat/assets/javascripts/discourse/controllers/create-channel.js +++ b/plugins/chat/assets/javascripts/discourse/controllers/create-channel.js @@ -1,4 +1,7 @@ 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 I18n from "I18n"; import ModalFunctionality from "discourse/mixins/modal-functionality"; @@ -26,6 +29,8 @@ export default class CreateChannelController extends Controller.extend( category = null; categoryId = null; name = ""; + slug = ""; + autoGeneratedSlug = ""; description = ""; categoryPermissionsHint = null; autoJoinUsers = null; @@ -51,11 +56,14 @@ export default class CreateChannelController extends Controller.extend( } onClose() { + cancel(this.generateSlugHandler); this.setProperties({ categoryId: null, category: null, name: "", description: "", + slug: "", + autoGeneratedSlug: "", categoryPermissionsHint: DEFAULT_HINT, autoJoinWarning: "", }); @@ -65,6 +73,7 @@ export default class CreateChannelController extends Controller.extend( const data = { chatable_id: this.categoryId, name: this.name, + slug: this.slug || this.autoGeneratedSlug, description: this.description, 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 onCategoryChange(categoryId) { let category = categoryId ? this.site.categories.findBy("id", categoryId) : null; this._updatePermissionsHint(category); + + const name = this.name || category?.name || ""; this.setProperties({ categoryId, category, - name: this.name || category?.name || "", + name, }); + this._debouncedGenerateSlug(name); + } + + @action + onNameChange(name) { + this._debouncedGenerateSlug(name); } @action diff --git a/plugins/chat/assets/javascripts/discourse/templates/modal/create-channel.hbs b/plugins/chat/assets/javascripts/discourse/templates/modal/create-channel.hbs index 2ed1417f4d7..c25dc4e63b6 100644 --- a/plugins/chat/assets/javascripts/discourse/templates/modal/create-channel.hbs +++ b/plugins/chat/assets/javascripts/discourse/templates/modal/create-channel.hbs @@ -8,6 +8,20 @@ class="create-channel-name-input" @type="text" @value={{this.name}} + {{on "input" (action "onNameChange" value="target.value")}} + /> + + +
+ +
diff --git a/plugins/chat/assets/stylesheets/common/create-channel-modal.scss b/plugins/chat/assets/stylesheets/common/create-channel-modal.scss index e2706683986..bfd8a7735ec 100644 --- a/plugins/chat/assets/stylesheets/common/create-channel-modal.scss +++ b/plugins/chat/assets/stylesheets/common/create-channel-modal.scss @@ -10,6 +10,7 @@ .select-kit.combo-box, .create-channel-name-input, + .create-channel-slug-input, .create-channel-description-input, #choose-topic-title { width: 100%; diff --git a/plugins/chat/config/locales/client.en.yml b/plugins/chat/config/locales/client.en.yml index 6c27e8346cd..bda7d953a38 100644 --- a/plugins/chat/config/locales/client.en.yml +++ b/plugins/chat/config/locales/client.en.yml @@ -289,6 +289,7 @@ en: create: "Create channel" description: "Description (optional)" name: "Channel name" + slug: "Channel slug (optional)" title: "New channel" type: "Type" types: diff --git a/plugins/chat/spec/requests/api/chat_channels_controller_spec.rb b/plugins/chat/spec/requests/api/chat_channels_controller_spec.rb index fd1a3375ea0..eda1712df27 100644 --- a/plugins/chat/spec/requests/api/chat_channels_controller_spec.rb +++ b/plugins/chat/spec/requests/api/chat_channels_controller_spec.rb @@ -263,11 +263,22 @@ RSpec.describe Chat::Api::ChatChannelsController do new_channel = ChatChannel.last 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.chatable_type).to eq(category.class.name) expect(new_channel.chatable_id).to eq(category.id) 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 post "/chat/api/channels", params: params diff --git a/plugins/chat/spec/system/create_channel_spec.rb b/plugins/chat/spec/system/create_channel_spec.rb index a525f09371c..8cf0d19d999 100644 --- a/plugins/chat/spec/system/create_channel_spec.rb +++ b/plugins/chat/spec/system/create_channel_spec.rb @@ -1,35 +1,43 @@ # frozen_string_literal: true RSpec.describe "Create channel", type: :system, js: true do - fab!(:current_admin_user) { Fabricate(:admin) } fab!(:category_1) { Fabricate(:category) } let(:chat_page) { PageObjects::Pages::Chat.new } + let(:channel_modal) { PageObjects::Modals::ChatChannelCreate.new } - before do - chat_system_bootstrap - sign_in(current_admin_user) + before { chat_system_bootstrap } + + 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 context "when can create channel" do + fab!(:current_admin_user) { Fabricate(:admin) } + before { sign_in(current_admin_user) } + context "when selecting a category" do it "shows access hint" do - visit("/chat") - find(".new-channel-btn").click - find(".category-chooser").click - find(".category-row[data-value=\"#{category_1.id}\"]").click + chat_page.visit_browse + chat_page.new_channel_button.click + channel_modal.select_category(category_1) - expect(find(".create-channel-hint")).to have_content(Group[:everyone].name) + expect(channel_modal).to have_create_hint(Group[:everyone].name) end it "does not override channel name if that was already specified" do - visit("/chat") - find(".new-channel-btn").click - fill_in("channel-name", with: "My Cool Channel") - find(".category-chooser").click - find(".category-row[data-value=\"#{category_1.id}\"]").click + chat_page.visit_browse + chat_page.new_channel_button.click + channel_modal.fill_name("My Cool Channel") + channel_modal.select_category(category_1) - expect(page).to have_field("channel-name", with: "My Cool Channel") + expect(channel_modal).to have_name_prefilled("My Cool Channel") end 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) } it "shows access hint when selecting the category" do - visit("/chat") - find(".new-channel-btn").click - find(".category-chooser").click - find(".category-row[data-value=\"#{private_category_1.id}\"]").click + chat_page.visit_browse + chat_page.new_channel_button.click + channel_modal.select_category(private_category_1) - expect(find(".create-channel-hint")).to have_content(group_1.name) + expect(channel_modal).to have_create_hint(group_1.name) end context "when category is a child" do @@ -52,12 +59,11 @@ RSpec.describe "Create channel", type: :system, js: true do end it "shows access hint when selecting the category" do - visit("/chat") - find(".new-channel-btn").click - find(".category-chooser").click - find(".category-row[data-value=\"#{child_category.id}\"]").click + chat_page.visit_browse + chat_page.new_channel_button.click + channel_modal.select_category(child_category) - expect(find(".create-channel-hint")).to have_content(group_2.name) + expect(channel_modal).to have_create_hint(group_2.name) 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) } it "escapes the group name" do - visit("/chat") - find(".new-channel-btn").click - find(".category-chooser").click - find(".category-row[data-value=\"#{private_category_1.id}\"]").click + chat_page.visit_browse + chat_page.new_channel_button.click + channel_modal.select_category(private_category_1) - expect(find(".create-channel-hint")["innerHTML"].strip).to include( + expect(channel_modal.create_channel_hint["innerHTML"].strip).to include( "<script>e</script>", ) 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 error" do it "displays the error" do existing_channel = Fabricate(:chat_channel) - visit("/chat") - find(".new-channel-btn").click - find(".category-chooser").click - find(".category-row[data-value=\"#{existing_channel.chatable_id}\"]").click - fill_in("channel-name", with: existing_channel.name) - find(".create-channel-modal .create").click + chat_page.visit_browse + chat_page.new_channel_button.click + channel_modal.select_category(existing_channel.chatable) + channel_modal.fill_name(existing_channel.name) + channel_modal.click_primary_button expect(page).to have_content(I18n.t("chat.errors.channel_exists_for_category")) 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 it "redirects to created channel" do - visit("/chat") - find(".new-channel-btn").click - name = "Cats" - find(".category-chooser").click - 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 + chat_page.visit_browse + chat_page.new_channel_button.click + channel_modal.select_category(category_1) + expect(channel_modal).to have_name_prefilled(category_1.name) - 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) expect(page).to have_current_path( chat.channel_path(created_channel.id, created_channel.slug), diff --git a/plugins/chat/spec/system/page_objects/chat/chat.rb b/plugins/chat/spec/system/page_objects/chat/chat.rb index 38e341e46d2..0ceceb7373f 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat.rb @@ -51,6 +51,14 @@ module PageObjects container.has_content?(message.message) container.has_content?(message.user.username) end + + def new_channel_button + find(".new-channel-btn") + end + + def has_new_channel_button? + has_css?(".new-channel-btn") + end end end end diff --git a/plugins/chat/spec/system/page_objects/modals/chat_channel_create.rb b/plugins/chat/spec/system/page_objects/modals/chat_channel_create.rb new file mode 100644 index 00000000000..41788b203d7 --- /dev/null +++ b/plugins/chat/spec/system/page_objects/modals/chat_channel_create.rb @@ -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 diff --git a/spec/requests/slugs_controller_spec.rb b/spec/requests/slugs_controller_spec.rb new file mode 100644 index 00000000000..ec56d73a479 --- /dev/null +++ b/spec/requests/slugs_controller_spec.rb @@ -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 diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb index 7813ec137e6..028483bf886 100644 --- a/spec/support/system_helpers.rb +++ b/spec/support/system_helpers.rb @@ -39,6 +39,18 @@ module SystemHelpers retry 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) original_size = page.driver.browser.manage.window.size page.driver.browser.manage.window.resize_to( diff --git a/spec/system/page_objects/modals/base.rb b/spec/system/page_objects/modals/base.rb index 7bad184882c..5d8e68996fa 100644 --- a/spec/system/page_objects/modals/base.rb +++ b/spec/system/page_objects/modals/base.rb @@ -17,6 +17,10 @@ module PageObjects def click_outside find(".modal-outer-container").click(x: 0, y: 0) end + + def click_primary_button + find(".modal-footer .btn-primary").click + end end end end