From a3d61ba1c43931eb688f9b2b85c207b5bab02b8c Mon Sep 17 00:00:00 2001 From: Gabriel Grubba <70247653+Grubba27@users.noreply.github.com> Date: Tue, 30 Jul 2024 10:25:22 -0300 Subject: [PATCH] DEV: rename chat preferred mobile index to chat preferred index (#27953) * DEV: rename chat preferred mobile index to chat preferred index * UX: change routing to be consistent with mobile * DEV: change migration file to use script * UX: show footer only if more than one option is available * UX: Remove desktopView only checks for chat * DEV: Remove unused imports * UX: Update chat footer checks and Add rerouting to chat drawer * UX: Add margin to chat row in desktop and update chat drawer logic * UX: Change chat in desktop to use flexbox * UX: Add drawer actions to chat navbar * DEV: Update page object with new chat css classes removed `.open-browse-page-btn` usage in 7bd65006d760886b261f0587fafe28443bf2d3ec * DEV: rename `browse/open` in chat url to `channels` * UX: Adjust css for when in threads mode * DEV: change css class name in no_sidebar_spec.rb * DEV: rename tests to be more descriptive with the action they are testing update chat template to not rely on `:has` * DEV: update test and add method to chat page object * DEV: update no_sidebar_spec for chat changes * DEV: remove tests from navigation_spec that no longer apply * DEV: revert typo in test * DEV: change url path for mobile chat in test specs * DEV: Add check for when is desktop in rerouting * UX: Removed footer from desktop. Made `hasThreads` and `hasDirectMessages` methods in chat-drawer public * UX: remove sidebar on desktop full page if dm list is empty * DEV: Address review comments * DEV: Adjust reroute logic for chat browse remove unused code * UX: Adjust rerouting to go to browse.open * UX: Change rerouting to be more consistent Add chat_default_channel_id routing * UX: Update rerouting configuration for chat routes * DEV: Update tests with the new chat behavior * DEV: revert changes made in tests and bring back toggle for drawer * DEV: revert classes in page objects * DEV: Add tests to new chat navigation behavior remove unused stylesheets revert deleted lines in tests update concat class logic in chat dm template * DEV: update css on test --- ...ame_chat_preferred_mobile_index_setting.rb | 11 +++ .../components/channels-list-direct.gjs | 24 ++++--- .../discourse/components/chat-footer.gjs | 11 ++- .../components/chat/routes/channels.gjs | 1 + .../chat/routes/direct-messages.gjs | 1 + .../javascripts/discourse/controllers/chat.js | 28 +++++++- .../discourse/routes/chat-channels.js | 27 +++++++- .../discourse/routes/chat-direct-messages.js | 12 +++- .../discourse/routes/chat-index.js | 46 ++++++------- .../discourse/services/chat-drawer-router.js | 68 +++++++++++++++++-- .../desktop/chat-index-full-page.scss | 3 + plugins/chat/config/locales/server.en.yml | 2 +- plugins/chat/config/settings.yml | 2 +- plugins/chat/spec/system/drawer/index_spec.rb | 7 ++ .../spec/system/list_channels/mobile_spec.rb | 18 ++--- plugins/chat/spec/system/navigation_spec.rb | 29 +++++--- .../spec/system/page_objects/chat/chat.rb | 4 ++ 17 files changed, 232 insertions(+), 62 deletions(-) create mode 100644 db/migrate/20240717171840_rename_chat_preferred_mobile_index_setting.rb diff --git a/db/migrate/20240717171840_rename_chat_preferred_mobile_index_setting.rb b/db/migrate/20240717171840_rename_chat_preferred_mobile_index_setting.rb new file mode 100644 index 00000000000..bc628be6976 --- /dev/null +++ b/db/migrate/20240717171840_rename_chat_preferred_mobile_index_setting.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class RenameChatPreferredMobileIndexSetting < ActiveRecord::Migration[7.0] + def up + execute "UPDATE site_settings SET name = 'chat_preferred_index' WHERE name = 'chat_preferred_mobile_index'" + end + + def down + execute "UPDATE site_settings SET name = 'chat_preferred_mobile_index' WHERE name = 'chat_preferred_index'" + end +end diff --git a/plugins/chat/assets/javascripts/discourse/components/channels-list-direct.gjs b/plugins/chat/assets/javascripts/discourse/components/channels-list-direct.gjs index c58745a6288..8a0a5bcb93a 100644 --- a/plugins/chat/assets/javascripts/discourse/components/channels-list-direct.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/channels-list-direct.gjs @@ -6,6 +6,7 @@ import { service } from "@ember/service"; import { and } from "truth-helpers"; import DButton from "discourse/components/d-button"; import PluginOutlet from "discourse/components/plugin-outlet"; +import concatClass from "discourse/helpers/concat-class"; import dIcon from "discourse-common/helpers/d-icon"; import i18n from "discourse-common/helpers/i18n"; import ChatModalNewMessage from "discourse/plugins/chat/discourse/components/chat/modal/new-message"; @@ -15,6 +16,7 @@ import ChatChannelRow from "./chat-channel-row"; export default class ChannelsListDirect extends Component { @service chat; @service chatChannelsManager; + @service chatStateManager; @service site; @service modal; @@ -40,12 +42,6 @@ export default class ChannelsListDirect extends Component { return this.chat.userCanDirectMessage; } - get directMessageChannelClasses() { - return `channels-list-container direct-message-channels ${ - this.inSidebar ? "collapsible-sidebar-section" : "" - }`; - } - get directMessageChannelsEmpty() { return this.chatChannelsManager.directMessageChannels?.length === 0; } @@ -66,8 +62,13 @@ export default class ChannelsListDirect extends Component { @tagName="" @outletArgs={{hash inSidebar=this.inSidebar}} /> - - {{#if (and this.showDirectMessageChannels this.site.desktopView)}} + {{#if + (and + this.showDirectMessageChannels + this.site.desktopView + this.chatStateManager.isDrawerActive + ) + }}
{{#if this.inSidebar}} {{#if this.directMessageChannelsEmpty}} 1 ); } diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/routes/channels.gjs b/plugins/chat/assets/javascripts/discourse/components/chat/routes/channels.gjs index f2aaab363e9..8eef35e1a49 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/routes/channels.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/routes/channels.gjs @@ -12,6 +12,7 @@ export default class ChatRoutesChannels extends Component { + diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/routes/direct-messages.gjs b/plugins/chat/assets/javascripts/discourse/components/chat/routes/direct-messages.gjs index e75aba6eca6..22253be28ee 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/routes/direct-messages.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/routes/direct-messages.gjs @@ -12,6 +12,7 @@ export default class ChatRoutesDirectMessages extends Component { + diff --git a/plugins/chat/assets/javascripts/discourse/controllers/chat.js b/plugins/chat/assets/javascripts/discourse/controllers/chat.js index 5e2394500b2..59b9bf7a444 100644 --- a/plugins/chat/assets/javascripts/discourse/controllers/chat.js +++ b/plugins/chat/assets/javascripts/discourse/controllers/chat.js @@ -5,6 +5,8 @@ import { FOOTER_NAV_ROUTES } from "discourse/plugins/chat/discourse/lib/chat-con export default class ChatController extends Controller { @service chat; @service chatStateManager; + @service chatChannelsManager; + @service siteSettings; @service router; get shouldUseChatSidebar() { @@ -15,18 +17,38 @@ export default class ChatController extends Controller { if (this.shouldUseCoreSidebar) { return false; } - + if ( + this.publicMessageChannelsEmpty && + this.enabledRouteCount === 1 && + this.chat.userCanAccessDirectMessages + ) { + return false; + } return true; } + get publicMessageChannelsEmpty() { + return ( + this.chatChannelsManager.publicMessageChannels?.length === 0 && + this.chatStateManager.hasPreloadedChannels + ); + } get shouldUseCoreSidebar() { return this.siteSettings.navigation_menu === "sidebar"; } + get enabledRouteCount() { + return [ + this.siteSettings.chat_threads_enabled, + this.chat.userCanAccessDirectMessages, + this.siteSettings.enable_public_channels, + ].filter(Boolean).length; + } + get shouldUseChatFooter() { return ( - this.site.mobileView && - FOOTER_NAV_ROUTES.includes(this.router.currentRouteName) + FOOTER_NAV_ROUTES.includes(this.router.currentRouteName) && + this.enabledRouteCount > 1 ); } diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channels.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channels.js index 817fc160328..555c50ce7aa 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channels.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channels.js @@ -4,14 +4,39 @@ import DiscourseRoute from "discourse/routes/discourse"; export default class ChatChannelsRoute extends DiscourseRoute { @service chat; @service chatChannelsManager; + @service siteSettings; activate() { this.chat.activeChannel = null; } beforeModel() { + const id = this.currentUser.custom_fields.last_chat_channel_id; + const defaultChannelId = this.siteSettings.chat_default_channel_id; if (this.site.desktopView) { - this.router.transitionTo("chat"); + if (id) { + this.chatChannelsManager.find(id).then((c) => { + return this.router.replaceWith("chat.channel", ...c.routeModels); + }); + } else { + // first time browsing chat in desktop and the preferred index is channels + if (defaultChannelId) { + this.chatChannelsManager.find(defaultChannelId).then((c) => { + return this.router.replaceWith("chat.channel", ...c.routeModels); + }); + } else { + this.router.replaceWith("chat.browse.open"); + } + } + } else { + if ( + defaultChannelId && + this.router.currentRoute?.parent?.params?.channelId !== defaultChannelId + ) { + this.chatChannelsManager.find(defaultChannelId).then((c) => { + return this.router.replaceWith("chat.channel", ...c.routeModels); + }); + } } } diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-direct-messages.js b/plugins/chat/assets/javascripts/discourse/routes/chat-direct-messages.js index a9f817b64dd..ae303f95c8e 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-direct-messages.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-direct-messages.js @@ -11,7 +11,17 @@ export default class ChatDirectMessagesRoute extends DiscourseRoute { beforeModel() { if (this.site.desktopView) { - this.router.transitionTo("chat"); + if (this.chatChannelsManager.directMessageChannels.length === 0) { + // first time browsing chat and the preferred index is dms + this.router.replaceWith("chat.direct-messages"); + } else { + // there should be at least one dm channel + // we can reroute using the last channel id + const id = this.currentUser.custom_fields.last_chat_channel_id; + this.chatChannelsManager.find(id).then((c) => { + return this.router.replaceWith("chat.channel", ...c.routeModels); + }); + } } } diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-index.js b/plugins/chat/assets/javascripts/discourse/routes/chat-index.js index c80c51e6770..6143b9d870f 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-index.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-index.js @@ -20,6 +20,10 @@ export default class ChatIndexRoute extends DiscourseRoute { return this.chat.userCanAccessDirectMessages; } + get isPublicChannelsEnabled() { + return this.siteSettings.enable_public_channels; + } + activate() { this.chat.activeChannel = null; } @@ -29,31 +33,25 @@ export default class ChatIndexRoute extends DiscourseRoute { } async redirect() { - // on mobile redirect user to the first footer tab route - if (this.site.mobileView) { - if ( - this.siteSettings.chat_preferred_mobile_index === "my_threads" && - this.hasThreads - ) { - return this.router.replaceWith("chat.threads"); - } else if ( - this.siteSettings.chat_preferred_mobile_index === "direct_messages" && - this.hasDirectMessages - ) { - return this.router.replaceWith("chat.direct-messages"); - } else { - return this.router.replaceWith("chat.channels"); - } + if ( + this.siteSettings.chat_preferred_index === "my_threads" && + this.hasThreads + ) { + return this.router.replaceWith("chat.threads"); + } else if ( + this.siteSettings.chat_preferred_index === "direct_messages" && + this.hasDirectMessages + ) { + return this.router.replaceWith("chat.direct-messages"); + } else if ( + this.siteSettings.chat_preferred_index === "channels" && + this.isPublicChannelsEnabled + ) { + return this.router.replaceWith("chat.channels"); } - - // We are on desktop. Check for last visited channel and transition if so - const id = this.currentUser.custom_fields.last_chat_channel_id; - if (id) { - return this.chatChannelsManager.find(id).then((c) => { - return this.router.replaceWith("chat.channel", ...c.routeModels); - }); - } else { - return this.router.replaceWith("chat.browse"); + if (!this.isPublicChannelsEnabled && this.hasDirectMessages) { + return this.router.replaceWith("chat.direct-messages"); } + return this.router.replaceWith("chat.browse.open"); } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js b/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js index 8fc5f802f2e..c1fa0dacb48 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-drawer-router.js @@ -16,11 +16,11 @@ const ROUTES = { // order matters, non index before index "chat.browse": { name: ChatDrawerRoutesBrowse, - extractParams: () => ({ currentTab: "all" }), + extractParams: () => ({ currentTab: "open" }), }, "chat.browse.index": { name: ChatDrawerRoutesBrowse, - extractParams: () => ({ currentTab: "all" }), + extractParams: () => ({ currentTab: "open" }), }, "chat.browse.open": { name: ChatDrawerRoutesBrowse, @@ -131,6 +131,10 @@ const ROUTES = { export default class ChatDrawerRouter extends Service { @service router; @service chatHistory; + @service chat; + @service siteSettings; + @service chatStateManager; + @service chatChannelsManager; @tracked component = null; @tracked drawerRoute = null; @@ -138,16 +142,72 @@ export default class ChatDrawerRouter extends Service { routeNames = Object.keys(ROUTES); + get hasThreads() { + if (!this.siteSettings.chat_threads_enabled) { + return false; + } + + return this.chatChannelsManager.hasThreadedChannels; + } + + get hasDirectMessages() { + return this.chat.userCanAccessDirectMessages; + } + + #routeFromURL(url) { + let route = this.router.recognize(url); + + // ember might recognize the index subroute + if (route.localName === "index") { + route = route.parent; + } + + return route; + } + + #redirect() { + if ( + this.siteSettings.chat_preferred_index === "my_threads" && + this.hasThreads + ) { + return this.stateFor(this.#routeFromURL("/chat/threads")); + } + if ( + this.siteSettings.chat_preferred_index === "direct_messages" && + this.hasDirectMessages + ) { + return this.stateFor(this.#routeFromURL("/chat/direct-messages")); + } + + if (this.siteSettings.chat_default_channel_id) { + return this.chatChannelsManager + .find(this.siteSettings.chat_default_channel_id) + .then((c) => { + return this.router.replaceWith("chat.channel", ...c.routeModels); + }); + } + + if (!this.siteSettings.enable_public_channels) { + return this.stateFor(this.#routeFromURL("/chat/direct-messages")); + } + } + stateFor(route) { this.drawerRoute?.deactivate?.(this.chatHistory.currentRoute); this.chatHistory.visit(route); - this.drawerRoute = ROUTES[route.name]; this.params = this.drawerRoute?.extractParams?.(route) || route.params; this.component = this.drawerRoute?.name || ChatDrawerRoutesChannels; - this.currentRouteName = route.name; + if ( + !this.chatStateManager.isDrawerActive && // only when opening the drawer + this.component.name === "ChatDrawerRoutesChannels" // we should check if redirect to channels + ) { + this.#redirect(); + } + + this.currentRouteName = route.name; this.drawerRoute.activate?.(route); } } diff --git a/plugins/chat/assets/stylesheets/desktop/chat-index-full-page.scss b/plugins/chat/assets/stylesheets/desktop/chat-index-full-page.scss index 311666e45a0..1fb47090f77 100644 --- a/plugins/chat/assets/stylesheets/desktop/chat-index-full-page.scss +++ b/plugins/chat/assets/stylesheets/desktop/chat-index-full-page.scss @@ -54,5 +54,8 @@ .channels-list-container { height: auto; } + .channels-list-container.center-empty-channels-list { + height: 90%; + } } } diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index e3dc78ca341..7b049c2cca0 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -25,7 +25,7 @@ en: chat_editing_grace_period: "For (n) seconds after sending a chat, editing will not display the (edited) tag by the chat message." chat_editing_grace_period_max_diff_low_trust: "Maximum number of character changes allowed in chat editing grace period, if more changed display the (edited) tag by the chat message (trust level 0 and 1)." chat_editing_grace_period_max_diff_high_trust: "Maximum number of character changes allowed in chat editing grace period, if more changed display the (edited) tag by the chat message (trust level 2 and up)." - chat_preferred_mobile_index: "Preferred tab when loading /chat on mobile." + chat_preferred_index: "Preferred tab when loading /chat." errors: chat_default_channel: "The default chat channel must be a public channel." direct_message_enabled_groups_invalid: "You must specify at least one group for this setting. If you do not want anyone except staff to send direct messages, choose the staff group." diff --git a/plugins/chat/config/settings.yml b/plugins/chat/config/settings.yml index ee9f99a5311..1cd4a81f639 100644 --- a/plugins/chat/config/settings.yml +++ b/plugins/chat/config/settings.yml @@ -139,7 +139,7 @@ chat: type: integer default: 40 min: 0 - chat_preferred_mobile_index: + chat_preferred_index: client: true type: enum default: channels diff --git a/plugins/chat/spec/system/drawer/index_spec.rb b/plugins/chat/spec/system/drawer/index_spec.rb index 1b7165100b0..fa061eda00a 100644 --- a/plugins/chat/spec/system/drawer/index_spec.rb +++ b/plugins/chat/spec/system/drawer/index_spec.rb @@ -51,4 +51,11 @@ RSpec.describe "Drawer - index", type: :system do expect(drawer_page.browse).to have_channel(name: channel.name) end + + it "shows empty state when no dms" do + drawer_page.visit_index + drawer_page.click_direct_messages + expect(page).to have_css("#c-footer-direct-messages.--active") + expect(page).to have_selector(".channel-list-empty-message") + end end diff --git a/plugins/chat/spec/system/list_channels/mobile_spec.rb b/plugins/chat/spec/system/list_channels/mobile_spec.rb index 57cb6d2caa7..1427e3e5f45 100644 --- a/plugins/chat/spec/system/list_channels/mobile_spec.rb +++ b/plugins/chat/spec/system/list_channels/mobile_spec.rb @@ -201,8 +201,8 @@ RSpec.describe "List channels | mobile", type: :system, mobile: true do end end - context "when chat_preferred_mobile_index is set to direct_messages" do - before { SiteSetting.chat_preferred_mobile_index = "direct_messages" } + context "when chat_preferred_index is set to direct_messages" do + before { SiteSetting.chat_preferred_index = "direct_messages" } it "changes the default index" do visit("/chat") @@ -213,15 +213,15 @@ RSpec.describe "List channels | mobile", type: :system, mobile: true do context "when user can't use direct messages" do before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:staff] } - it "redirects to channels" do + it "redirects to browse" do visit("/chat") - expect(page).to have_current_path("/chat/channels") + expect(page).to have_current_path("/chat/browse/open") end end end - context "when chat_preferred_mobile_index is not set" do + context "when chat_preferred_index is not set" do it "redirects to channels" do visit("/chat") @@ -229,10 +229,10 @@ RSpec.describe "List channels | mobile", type: :system, mobile: true do end end - context "when chat_preferred_mobile_index is set to my_threads" do + context "when chat_preferred_index is set to my_threads" do before do SiteSetting.chat_threads_enabled = true - SiteSetting.chat_preferred_mobile_index = "my_threads" + SiteSetting.chat_preferred_index = "my_threads" end it "redirects to threads" do @@ -247,10 +247,10 @@ RSpec.describe "List channels | mobile", type: :system, mobile: true do context "when no threads" do before { SiteSetting.chat_threads_enabled = false } - it "redirects to channels" do + it "redirects to browse" do visit("/chat") - expect(page).to have_current_path("/chat/channels") + expect(page).to have_current_path("/chat/browse/open") end end end diff --git a/plugins/chat/spec/system/navigation_spec.rb b/plugins/chat/spec/system/navigation_spec.rb index 46bdb5b1e06..31b5630f260 100644 --- a/plugins/chat/spec/system/navigation_spec.rb +++ b/plugins/chat/spec/system/navigation_spec.rb @@ -95,14 +95,6 @@ RSpec.describe "Navigation", type: :system do chat.channel_path(category_channel.slug, category_channel.id), ) end - - it "redirects /chat/direct-messages to browse" do - visit("/chat/direct-messages") - - expect(page).to have_current_path( - chat.channel_path(category_channel.slug, category_channel.id), - ) - end end context "when opening chat" do @@ -265,6 +257,27 @@ RSpec.describe "Navigation", type: :system do end end + context "when public channels are disabled" do + before { SiteSetting.enable_public_channels = false } + + it "only show dms in drawer" do + visit("/") + chat_page.open_from_header + + expect(page).to have_css(".direct-message-channels.center-empty-channels-list") + expect(chat_page).to have_no_messages + end + + it "only show dms in desktop" do + visit("/") + chat_page.prefers_full_page + chat_page.open_from_header + + expect(chat_page).to have_no_messages + expect(page).to have_css(".c-routes.--direct-messages") + end + end + context "when sidebar is configured as the navigation menu" do before { SiteSetting.navigation_menu = "sidebar" } diff --git a/plugins/chat/spec/system/page_objects/chat/chat.rb b/plugins/chat/spec/system/page_objects/chat/chat.rb index 374cc87a960..a7428f6e907 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat.rb @@ -130,6 +130,10 @@ module PageObjects has_no_css?(NEW_CHANNEL_BUTTON_SELECTOR) end + def has_no_messages? + have_selector(".channel-list-empty-message") + end + private def drawer?(expectation:, channel_id: nil, expanded: true)