From 5720bb7b64d137cb05e5c252bce2dc9ffba6bab3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 16 Feb 2023 12:06:51 +0100 Subject: [PATCH 1/8] Build(deps): Bump @uppy/xhr-upload in /app/assets/javascripts (#20296) Bumps [@uppy/xhr-upload](https://github.com/transloadit/uppy) from 3.0.4 to 3.1.0. - [Release notes](https://github.com/transloadit/uppy/releases) - [Changelog](https://github.com/transloadit/uppy/blob/main/CHANGELOG.md) - [Commits](https://github.com/transloadit/uppy/compare/@uppy/xhr-upload@3.0.4...@uppy/xhr-upload@3.1.0) --- updated-dependencies: - dependency-name: "@uppy/xhr-upload" dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .../javascripts/discourse-common/package.json | 2 +- app/assets/javascripts/discourse/package.json | 2 +- app/assets/javascripts/yarn.lock | 20 +++++++++---------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/discourse-common/package.json b/app/assets/javascripts/discourse-common/package.json index 6dc33fd7cf..8146a41841 100644 --- a/app/assets/javascripts/discourse-common/package.json +++ b/app/assets/javascripts/discourse-common/package.json @@ -20,7 +20,7 @@ "@uppy/core": "^3.0.4", "@uppy/drop-target": "^2.0.1", "@uppy/utils": "^5.1.1", - "@uppy/xhr-upload": "^3.0.4", + "@uppy/xhr-upload": "^3.1.0", "ember-auto-import": "^2.6.0", "ember-cli-babel": "^7.26.10", "ember-cli-htmlbars": "^6.1.1", diff --git a/app/assets/javascripts/discourse/package.json b/app/assets/javascripts/discourse/package.json index 018aca4469..4d12e8a4cc 100644 --- a/app/assets/javascripts/discourse/package.json +++ b/app/assets/javascripts/discourse/package.json @@ -34,7 +34,7 @@ "@uppy/core": "^3.0.4", "@uppy/drop-target": "^2.0.1", "@uppy/utils": "^5.1.1", - "@uppy/xhr-upload": "^3.0.4", + "@uppy/xhr-upload": "^3.1.0", "a11y-dialog": "7.5.2", "admin": "1.0.0", "babel-import-util": "^1.3.0", diff --git a/app/assets/javascripts/yarn.lock b/app/assets/javascripts/yarn.lock index f85db172bf..70539046e4 100644 --- a/app/assets/javascripts/yarn.lock +++ b/app/assets/javascripts/yarn.lock @@ -1613,20 +1613,20 @@ resolved "https://registry.yarnpkg.com/@uppy/store-default/-/store-default-3.0.2.tgz#870724c45a2f671d625123cb4a412e3bfae935d9" integrity sha512-kIQMCjXui6tjF1E9xGo4YHkvt71McXkU0FStrcQuBrRXuOhb+NcuWh3sMh3KryivVNgT6w5Odrlw2FUFkl9cqA== -"@uppy/utils@^5.0.2", "@uppy/utils@^5.1.1": - version "5.1.1" - resolved "https://registry.yarnpkg.com/@uppy/utils/-/utils-5.1.1.tgz#9597e8696e17d71413672bd56eb082c7410514a3" - integrity sha512-uoI+PcIVQboky0ZbN4PQeK1seZnnJocomzeK7blId9HKJ6QNgZLf2ibk2CQuQxrOuNsWhgrhs5uLO5Si0oM0Yw== +"@uppy/utils@^5.0.2", "@uppy/utils@^5.1.1", "@uppy/utils@^5.1.2": + version "5.1.2" + resolved "https://registry.yarnpkg.com/@uppy/utils/-/utils-5.1.2.tgz#136e4a2d3fc7222c6d19e0111fdd56bb2449be60" + integrity sha512-a/QSxcYeK1SSOjyyohi0oSwLDMJ9SSWwDELDii1WOKiJwmtt5O6pGCKHTnvrr6RwbiedngG0dZi3koYOG+MdoA== dependencies: lodash.throttle "^4.1.1" -"@uppy/xhr-upload@^3.0.4": - version "3.0.4" - resolved "https://registry.yarnpkg.com/@uppy/xhr-upload/-/xhr-upload-3.0.4.tgz#219a92c832bee1f089992958d27ec71dbe9d9d7d" - integrity sha512-uJ1oxcwEaSLnrexvi6Lp57hV3z3DsovgVmYIVwg+z/EnrRcL32wNRE7FcIr8Mk9e1jdMiFYlk6cQmiP2dZep8A== +"@uppy/xhr-upload@^3.0.4", "@uppy/xhr-upload@^3.1.0": + version "3.1.0" + resolved "https://registry.yarnpkg.com/@uppy/xhr-upload/-/xhr-upload-3.1.0.tgz#a6a2d11329ac8a8745ff15535c156082ebc41e3a" + integrity sha512-v0OsrB74w9RcFU8VMFElxMuGaSFHBuAlkbK30zHEY2YEKUsCfkWMdUbpaWNc1KWERH66Fv57IsA3AbK2HTjWdw== dependencies: - "@uppy/companion-client" "^3.0.2" - "@uppy/utils" "^5.0.2" + "@uppy/companion-client" "^3.1.1" + "@uppy/utils" "^5.1.2" nanoid "^4.0.0" "@webassemblyjs/ast@1.11.1": From 7cc0c5946947b51c3017b408903b4676707a8b52 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 16 Feb 2023 12:58:47 +0100 Subject: [PATCH 2/8] FIX: removes stored scroll position in drawer (#20328) --- .../javascripts/discourse/components/channels-list.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/plugins/chat/assets/javascripts/discourse/components/channels-list.js b/plugins/chat/assets/javascripts/discourse/components/channels-list.js index 9cac0ff735..bb1b660ebc 100644 --- a/plugins/chat/assets/javascripts/discourse/components/channels-list.js +++ b/plugins/chat/assets/javascripts/discourse/components/channels-list.js @@ -84,12 +84,20 @@ export default class ChannelsList extends Component { @action storeScrollPosition() { + if (this.chatStateManager.isDrawerActive) { + return; + } + const scrollTop = document.querySelector(".channels-list")?.scrollTop || 0; this.session.channelsListPosition = scrollTop; } @bind _applyScrollPosition() { + if (this.chatStateManager.isDrawerActive) { + return; + } + const position = this.chatStateManager.isFullPageActive ? this.session.channelsListPosition || 0 : 0; From c541bd05a866ab2c399cf55a88801292deb27012 Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Thu, 16 Feb 2023 13:58:23 +0100 Subject: [PATCH 3/8] DEV: Add missing `decorateCookedElement` id (#20330) Fixes "`decorateCooked` should be supplied with an `id` option to avoid memory leaks in test mode. The id will be used to ensure the decorator is only applied once." warnings --- .../topic-post-decorate-cooked-test.js | 48 ++++++++++--------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-post-decorate-cooked-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-post-decorate-cooked-test.js index 61ccbe8025..2bc38d5993 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-post-decorate-cooked-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/topic-post-decorate-cooked-test.js @@ -23,30 +23,34 @@ acceptance("Acceptance | decorateCookedElement", function () { DemoComponent ); - withPluginApi(0, (api) => { - api.decorateCookedElement((cooked, helper) => { - if (helper.getModel().post_number !== 1) { - return; - } - cooked.innerHTML = - "
Some existing content
"; + withPluginApi( + 0, + (api) => { + api.decorateCookedElement((cooked, helper) => { + if (helper.getModel().post_number !== 1) { + return; + } + cooked.innerHTML = + "
Some existing content
"; - // Create new wrapper element and append - cooked.appendChild( + // Create new wrapper element and append + cooked.appendChild( + helper.renderGlimmer( + "div.glimmer-wrapper", + hbs`<@data.component />`, + { component: DemoComponent } + ) + ); + + // Append to existing element helper.renderGlimmer( - "div.glimmer-wrapper", - hbs`<@data.component />`, - { component: DemoComponent } - ) - ); - - // Append to existing element - helper.renderGlimmer( - cooked.querySelector(".existing-wrapper"), - hbs` with more content from glimmer` - ); - }); - }); + cooked.querySelector(".existing-wrapper"), + hbs` with more content from glimmer` + ); + }); + }, + { id: "render-glimmer-test" } + ); await visit("/t/internationalization-localization/280"); From 5423e7c5b765ea24ae7267066f4534281b51a69f Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Thu, 16 Feb 2023 16:02:09 +0300 Subject: [PATCH 4/8] DEV: Add backend support for unread and new topics list (#20293) This commit adds backend support for a new topics list that combines both the current unread and new topics lists. We're going to experiment with this new list (name TBD) internally and decide if this feature is something that we want to fully build. Internal topic: t/77234. --- lib/topic_query.rb | 57 ++++++++++---- spec/lib/topic_query_spec.rb | 142 +++++++++++++++++++++++++++++++++++ 2 files changed, 184 insertions(+), 15 deletions(-) diff --git a/lib/topic_query.rb b/lib/topic_query.rb index cbfbab113f..abdb52d42d 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -507,21 +507,7 @@ class TopicQuery whisperer: @user&.whisperer?, ).order("CASE WHEN topics.user_id = tu.user_id THEN 1 ELSE 2 END") - if @user - # micro optimisation so we don't load up all of user stats which we do not need - unread_at = - DB.query_single("select first_unread_at from user_stats where user_id = ?", @user.id).first - - if max_age = options[:max_age] - max_age_date = max_age.days.ago - unread_at ||= max_age_date - unread_at = unread_at > max_age_date ? unread_at : max_age_date - end - - # perf note, in the past we tried doing this in a subquery but performance was - # terrible, also tried with a join and it was bad - result = result.where("topics.updated_at >= ?", unread_at) - end + result = apply_max_age_limit(result, options) self.class.results_filter_callbacks.each do |filter_callback| result = filter_callback.call(:unread, result, @user, options) @@ -548,6 +534,28 @@ class TopicQuery suggested_ordering(result, options) end + def new_and_unread_results(options = {}) + base = default_results(options.reverse_merge(unordered: true)) + + new_results = + TopicQuery.new_filter( + base, + treat_as_new_topic_start_date: @user.user_option.treat_as_new_topic_start_date, + ) + new_results = remove_muted(new_results, @user, options) + new_results = remove_dismissed(new_results, @user) + + unread_results = + apply_max_age_limit(TopicQuery.unread_filter(base, whisperer: @user&.whisperer?), options) + + base.joins_values.concat(new_results.joins_values, unread_results.joins_values) + base.joins_values.uniq! + results = base.merge(new_results.or(unread_results)) + + results = results.order("CASE WHEN topics.user_id = tu.user_id THEN 1 ELSE 2 END") + suggested_ordering(results, options) + end + protected def per_page_setting @@ -1167,4 +1175,23 @@ class TopicQuery col_name = whisperer ? "highest_staff_post_number" : "highest_post_number" list.where("tu.last_read_post_number IS NULL OR tu.last_read_post_number < topics.#{col_name}") end + + def apply_max_age_limit(results, options) + if @user + # micro optimisation so we don't load up all of user stats which we do not need + unread_at = + DB.query_single("select first_unread_at from user_stats where user_id = ?", @user.id).first + + if max_age = options[:max_age] + max_age_date = max_age.days.ago + unread_at ||= max_age_date + unread_at = unread_at > max_age_date ? unread_at : max_age_date + end + + # perf note, in the past we tried doing this in a subquery but performance was + # terrible, also tried with a join and it was bad + results = results.where("topics.updated_at >= ?", unread_at) + end + results + end end diff --git a/spec/lib/topic_query_spec.rb b/spec/lib/topic_query_spec.rb index 261a4cbb7c..520ee27e5a 100644 --- a/spec/lib/topic_query_spec.rb +++ b/spec/lib/topic_query_spec.rb @@ -1731,4 +1731,146 @@ RSpec.describe TopicQuery do end end end + + describe "#new_and_unread_results" do + fab!(:unread_topic) { Fabricate(:post).topic } + fab!(:new_topic) { Fabricate(:post).topic } + fab!(:read_topic) { Fabricate(:post).topic } + + before do + unread_post = Fabricate(:post, topic: unread_topic) + read_post = Fabricate(:post, topic: read_topic) + + TopicUser.change( + user.id, + unread_topic.id, + notification_level: TopicUser.notification_levels[:tracking], + ) + TopicUser.change( + user.id, + read_topic.id, + notification_level: TopicUser.notification_levels[:tracking], + ) + TopicUser.update_last_read(user, unread_topic.id, unread_post.post_number - 1, 1, 1) + TopicUser.update_last_read(user, read_topic.id, read_post.post_number, 1, 1) + end + + it "includes unread and new topics for the user" do + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + unread_topic.id, + new_topic.id, + ) + end + + it "doesn't include deleted topics" do + unread_topic.trash! + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + new_topic.id, + ) + end + + it "doesn't include muted topics with unread posts" do + TopicUser.change( + user.id, + unread_topic.id, + notification_level: TopicUser.notification_levels[:muted], + ) + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + new_topic.id, + ) + end + + it "doesn't include muted new topics" do + TopicUser.change( + user.id, + new_topic.id, + notification_level: TopicUser.notification_levels[:muted], + ) + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + unread_topic.id, + ) + end + + it "doesn't include new topics in muted category" do + CategoryUser.create!( + user_id: user.id, + category_id: new_topic.category.id, + notification_level: CategoryUser.notification_levels[:muted], + ) + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + unread_topic.id, + ) + end + + it "includes unread and trakced topics even if they're in a muted category" do + new_topic.update!(category: Fabricate(:category)) + CategoryUser.create!( + user_id: user.id, + category_id: unread_topic.category.id, + notification_level: CategoryUser.notification_levels[:muted], + ) + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + unread_topic.id, + new_topic.id, + ) + end + + it "doesn't include new topics that have a muted tag(s)" do + SiteSetting.tagging_enabled = true + + tag = Fabricate(:tag) + new_topic.tags << tag + new_topic.save! + + TagUser.create!( + tag_id: tag.id, + user_id: user.id, + notification_level: NotificationLevels.all[:muted], + ) + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + unread_topic.id, + ) + end + + it "includes unread and tracked topics even if they have a muted tag(s)" do + SiteSetting.tagging_enabled = true + + tag = Fabricate(:tag) + unread_topic.tags << tag + unread_topic.save! + + TagUser.create!( + tag_id: tag.id, + user_id: user.id, + notification_level: NotificationLevels.all[:muted], + ) + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + unread_topic.id, + new_topic.id, + ) + end + + it "doesn't include topics in restricted categories that user cannot access" do + category = Fabricate(:category_with_definition) + group = Fabricate(:group) + category.set_permissions(group => :full) + category.save! + + unread_topic.update!(category: category) + new_topic.update!(category: category) + + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to be_blank + end + + it "doesn't include dismissed topics" do + DismissedTopicUser.create!( + user_id: user.id, + topic_id: new_topic.id, + created_at: Time.zone.now, + ) + expect(TopicQuery.new(user).new_and_unread_results.pluck(:id)).to contain_exactly( + unread_topic.id, + ) + end + end end From 75ae70c27cbcb3a33758872c9aad315cebd415fa Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Thu, 16 Feb 2023 18:15:56 +0300 Subject: [PATCH 5/8] DEV: rtlcss_wrapper renamed to rtlcss (#20331) The `rtlcss_wrapper` gem has been renamed to `rtlcss` per https://github.com/discourse/rtlcss/commit/bd89847a39ef61f823afe3fe4dd05403b63bbcf8. --- Gemfile | 2 +- Gemfile.lock | 4 ++-- lib/stylesheet/compiler.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index cbb730e262..96506d54b5 100644 --- a/Gemfile +++ b/Gemfile @@ -105,7 +105,7 @@ gem "pg" gem "mini_sql" gem "pry-rails", require: false gem "pry-byebug", require: false -gem "rtlcss_wrapper", require: false +gem "rtlcss", require: false gem "rake" gem "thor", require: false diff --git a/Gemfile.lock b/Gemfile.lock index 64d256d821..c9c0ba4eff 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -408,7 +408,7 @@ GEM json-schema (>= 2.2, < 4.0) railties (>= 3.1, < 7.1) rspec-core (>= 2.14) - rtlcss_wrapper (0.1.0) + rtlcss (0.2.0) mini_racer (~> 0.6.3) rubocop (1.44.1) json (~> 2.3) @@ -638,7 +638,7 @@ DEPENDENCIES rspec-rails rss rswag-specs - rtlcss_wrapper + rtlcss rubocop-discourse ruby-prof ruby-readability diff --git a/lib/stylesheet/compiler.rb b/lib/stylesheet/compiler.rb index 536249ae6d..43018e2705 100644 --- a/lib/stylesheet/compiler.rb +++ b/lib/stylesheet/compiler.rb @@ -66,8 +66,8 @@ module Stylesheet result = engine.render if options[:rtl] - require "rtlcss_wrapper" - [RtlcssWrapper.flip_css(result), nil] + require "rtlcss" + [Rtlcss.flip_css(result), nil] else source_map = engine.source_map source_map.force_encoding("UTF-8") From 7391f8e077f6cc18057617423e23409df3054701 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 16 Feb 2023 16:49:17 +0100 Subject: [PATCH 6/8] FIX: correctly account for ipad footer nav height (#20334) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This code also removes duplicated code in chat which doesn’t seem necessary anymore. --- app/assets/stylesheets/common/base/discourse.scss | 1 + plugins/chat/assets/stylesheets/common/common.scss | 13 ++----------- plugins/chat/assets/stylesheets/mobile/mobile.scss | 7 ------- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/app/assets/stylesheets/common/base/discourse.scss b/app/assets/stylesheets/common/base/discourse.scss index b8782c233d..e875d3accd 100644 --- a/app/assets/stylesheets/common/base/discourse.scss +++ b/app/assets/stylesheets/common/base/discourse.scss @@ -67,6 +67,7 @@ body { background-attachment: fixed; background-size: cover; min-height: 100%; + box-sizing: border-box; @include clearfix; } diff --git a/plugins/chat/assets/stylesheets/common/common.scss b/plugins/chat/assets/stylesheets/common/common.scss index 7cc3ae8d39..a36e52e46c 100644 --- a/plugins/chat/assets/stylesheets/common/common.scss +++ b/plugins/chat/assets/stylesheets/common/common.scss @@ -606,7 +606,8 @@ html.has-full-page-chat { #main-outlet-wrapper { // restrict the row height, including when virtual keyboard is open grid-template-rows: calc( - var(--chat-vh, 1vh) * 100 - var(--header-offset) + var(--chat-vh, 1vh) * 100 - var(--header-offset, 0px) - + var(--footer-nav-height, 0px) ); .sidebar-wrapper { // prevents sidebar from overflowing behind the virtual keyboard @@ -614,16 +615,6 @@ html.has-full-page-chat { } } - // iPad webview - .footer-nav-ipad { - #main-outlet-wrapper { - // restrict the row height, including when virtual keyboard is open - grid-template-rows: calc( - var(--chat-vh, 1vh) * 100 - calc(var(--header-offset)) - ); - } - } - .full-page-chat, .chat-live-pane, #main-outlet { diff --git a/plugins/chat/assets/stylesheets/mobile/mobile.scss b/plugins/chat/assets/stylesheets/mobile/mobile.scss index 8e54337969..107c8629dc 100644 --- a/plugins/chat/assets/stylesheets/mobile/mobile.scss +++ b/plugins/chat/assets/stylesheets/mobile/mobile.scss @@ -92,13 +92,6 @@ html.has-full-page-chat { } } -html.has-full-page-chat body { - #main-outlet-wrapper { - // restricts the height of the page - grid-template-rows: calc(var(--chat-vh, 1vh) * 100 - var(--header-offset)); - } -} - .chat-message-separator { margin-left: 0; } From 75b81b6854087842b53b4c9559ef5836d9516269 Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Thu, 16 Feb 2023 19:55:18 +0400 Subject: [PATCH 7/8] DEV: extract the logic for extracting and expanding mentions from ChatNotifier (#20290) Initially, the ChatMention model / db table was introduced to better support notifications (see discourse/discourse-chat@0801d10). That means that currently, we create a new chat_mention record only if a user will be notified about the mention. Now we plan to start using the ChatMention model in other scenarios (for example for implementing user status on mentions) so we need to always create a new record in the chat_mention table. This PR does the first step into that direction by decoupling the logic for extracting and expanding mentions from the code related to notifications. This doesn't change any behavior, only extracts code from ChatNotifier. --- plugins/chat/lib/chat_message_mentions.rb | 88 ++++++++++ plugins/chat/lib/chat_notifier.rb | 107 +++--------- plugins/chat/plugin.rb | 1 + .../spec/lib/chat_message_mentions_spec.rb | 154 ++++++++++++++++++ 4 files changed, 270 insertions(+), 80 deletions(-) create mode 100644 plugins/chat/lib/chat_message_mentions.rb create mode 100644 plugins/chat/spec/lib/chat_message_mentions_spec.rb diff --git a/plugins/chat/lib/chat_message_mentions.rb b/plugins/chat/lib/chat_message_mentions.rb new file mode 100644 index 0000000000..84516466b0 --- /dev/null +++ b/plugins/chat/lib/chat_message_mentions.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +class Chat::ChatMessageMentions + def initialize(message) + @message = message + + mentions = parse_mentions(message) + group_mentions = parse_group_mentions(message) + + @has_global_mention = mentions.include?("@all") + @has_here_mention = mentions.include?("@here") + @parsed_direct_mentions = normalize(mentions) + @parsed_group_mentions = normalize(group_mentions) + end + + attr_accessor :has_global_mention, + :has_here_mention, + :parsed_direct_mentions, + :parsed_group_mentions + + def global_mentions + return User.none unless @has_global_mention + channel_members.where.not(username_lower: @parsed_direct_mentions) + end + + def direct_mentions + chat_users.where(username_lower: @parsed_direct_mentions) + end + + def group_mentions + chat_users.includes(:groups).joins(:groups).where(groups: mentionable_groups) + end + + def here_mentions + return User.none unless @has_here_mention + + channel_members + .where("last_seen_at > ?", 5.minutes.ago) + .where.not(username_lower: @parsed_direct_mentions) + end + + def mentionable_groups + @mentionable_groups ||= + Group.mentionable(@message.user, include_public: false).where(id: visible_groups.map(&:id)) + end + + def visible_groups + @visible_groups ||= + Group.where("LOWER(name) IN (?)", @parsed_group_mentions).visible_groups(@message.user) + end + + private + + def channel_members + chat_users.where( + user_chat_channel_memberships: { + following: true, + chat_channel_id: @message.chat_channel.id, + }, + ) + end + + def chat_users + User + .includes(:user_chat_channel_memberships, :group_users) + .distinct + .joins("LEFT OUTER JOIN user_chat_channel_memberships uccm ON uccm.user_id = users.id") + .joins(:user_option) + .real + .not_suspended + .where(user_options: { chat_enabled: true }) + .where.not(username_lower: @message.user.username.downcase) + end + + def parse_mentions(message) + Nokogiri::HTML5.fragment(message.cooked).css(".mention").map(&:text) + end + + def parse_group_mentions(message) + Nokogiri::HTML5.fragment(message.cooked).css(".mention-group").map(&:text) + end + + def normalize(mentions) + mentions.reduce([]) do |memo, mention| + %w[@here @all].include?(mention.downcase) ? memo : (memo << mention[1..-1].downcase) + end + end +end diff --git a/plugins/chat/lib/chat_notifier.rb b/plugins/chat/lib/chat_notifier.rb index 4719b96801..7887cfade9 100644 --- a/plugins/chat/lib/chat_notifier.rb +++ b/plugins/chat/lib/chat_notifier.rb @@ -60,6 +60,7 @@ class Chat::ChatNotifier @timestamp = timestamp @chat_channel = @chat_message.chat_channel @user = @chat_message.user + @mentions = Chat::ChatMessageMentions.new(chat_message) end ### Public API @@ -108,11 +109,12 @@ class Chat::ChatNotifier private def list_users_to_notify - direct_mentions_count = direct_mentions_from_cooked.length - group_mentions_count = group_name_mentions.length + mentions_count = + @mentions.parsed_direct_mentions.length + @mentions.parsed_group_mentions.length + mentions_count += 1 if @mentions.has_global_mention + mentions_count += 1 if @mentions.has_here_mention - skip_notifications = - (direct_mentions_count + group_mentions_count) > SiteSetting.max_mentions_per_chat_message + skip_notifications = mentions_count > SiteSetting.max_mentions_per_chat_message {}.tap do |to_notify| # The order of these methods is the precedence @@ -131,48 +133,13 @@ class Chat::ChatNotifier end end - def chat_users - User - .includes(:user_chat_channel_memberships, :group_users) - .distinct - .joins("LEFT OUTER JOIN user_chat_channel_memberships uccm ON uccm.user_id = users.id") - .joins(:user_option) - .real - .not_suspended - .where(user_options: { chat_enabled: true }) - .where.not(username_lower: @user.username.downcase) - end - - def rest_of_the_channel - chat_users.where( - user_chat_channel_memberships: { - following: true, - chat_channel_id: @chat_channel.id, - }, - ) - end - - def members_accepting_channel_wide_notifications - rest_of_the_channel.where(user_options: { ignore_channel_wide_mention: [false, nil] }) - end - - def direct_mentions_from_cooked - @direct_mentions_from_cooked ||= - Nokogiri::HTML5.fragment(@chat_message.cooked).css(".mention").map(&:text) - end - - def normalized_mentions(mentions) - mentions.reduce([]) do |memo, mention| - %w[@here @all].include?(mention.downcase) ? memo : (memo << mention[1..-1].downcase) - end - end - def expand_global_mention(to_notify, already_covered_ids, skip) - typed_global_mention = direct_mentions_from_cooked.include?("@all") + has_all_mention = @mentions.has_global_mention - if typed_global_mention && @chat_channel.allow_channel_wide_mentions && !skip - to_notify[:global_mentions] = members_accepting_channel_wide_notifications - .where.not(username_lower: normalized_mentions(direct_mentions_from_cooked)) + if has_all_mention && @chat_channel.allow_channel_wide_mentions && !skip + to_notify[:global_mentions] = @mentions + .global_mentions + .where(user_options: { ignore_channel_wide_mention: [false, nil] }) .where.not(id: already_covered_ids) .pluck(:id) @@ -183,12 +150,12 @@ class Chat::ChatNotifier end def expand_here_mention(to_notify, already_covered_ids, skip) - typed_here_mention = direct_mentions_from_cooked.include?("@here") + has_here_mention = @mentions.has_here_mention - if typed_here_mention && @chat_channel.allow_channel_wide_mentions && !skip - to_notify[:here_mentions] = members_accepting_channel_wide_notifications - .where("last_seen_at > ?", 5.minutes.ago) - .where.not(username_lower: normalized_mentions(direct_mentions_from_cooked)) + if has_here_mention && @chat_channel.allow_channel_wide_mentions && !skip + to_notify[:here_mentions] = @mentions + .here_mentions + .where(user_options: { ignore_channel_wide_mention: [false, nil] }) .where.not(id: already_covered_ids) .pluck(:id) @@ -225,10 +192,7 @@ class Chat::ChatNotifier if skip direct_mentions = [] else - direct_mentions = - chat_users - .where(username_lower: normalized_mentions(direct_mentions_from_cooked)) - .where.not(id: already_covered_ids) + direct_mentions = @mentions.direct_mentions.where.not(id: already_covered_ids) end grouped = group_users_to_notify(direct_mentions) @@ -239,48 +203,31 @@ class Chat::ChatNotifier already_covered_ids.concat(to_notify[:direct_mentions]) end - def group_name_mentions - @group_mentions_from_cooked ||= - normalized_mentions( - Nokogiri::HTML5.fragment(@chat_message.cooked).css(".mention-group").map(&:text), - ) - end - - def visible_groups - @visible_groups ||= Group.where("LOWER(name) IN (?)", group_name_mentions).visible_groups(@user) - end - def expand_group_mentions(to_notify, already_covered_ids, skip) - return [] if skip || visible_groups.empty? + return [] if skip || @mentions.visible_groups.empty? - mentionable_groups = - Group.mentionable(@user, include_public: false).where(id: visible_groups.map(&:id)) - - mentions_disabled = visible_groups - mentionable_groups + reached_by_group = + @mentions + .group_mentions + .where("user_count <= ?", SiteSetting.max_users_notified_per_group_mention) + .where.not(id: already_covered_ids) too_many_members, mentionable = - mentionable_groups.partition do |group| + @mentions.mentionable_groups.partition do |group| group.user_count > SiteSetting.max_users_notified_per_group_mention end + mentions_disabled = @mentions.visible_groups - @mentions.mentionable_groups to_notify[:group_mentions_disabled] = mentions_disabled to_notify[:too_many_members] = too_many_members - mentionable.each { |g| to_notify[g.name.downcase] = [] } - reached_by_group = - chat_users - .includes(:groups) - .joins(:groups) - .where(groups: mentionable) - .where.not(id: already_covered_ids) - grouped = group_users_to_notify(reached_by_group) - grouped[:already_participating].each do |user| # When a user is a member of multiple mentioned groups, # the most far to the left should take precedence. - ordered_group_names = group_name_mentions & mentionable.map { |mg| mg.name.downcase } + ordered_group_names = + @mentions.parsed_group_mentions & mentionable.map { |mg| mg.name.downcase } user_group_names = user.groups.map { |ug| ug.name.downcase } group_name = ordered_group_names.detect { |gn| user_group_names.include?(gn) } diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index c5b2f5c625..8a4792cb65 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -178,6 +178,7 @@ after_initialize do load File.expand_path("../lib/chat_message_updater.rb", __FILE__) load File.expand_path("../lib/chat_message_rate_limiter.rb", __FILE__) load File.expand_path("../lib/chat_message_reactor.rb", __FILE__) + load File.expand_path("../lib/chat_message_mentions.rb", __FILE__) load File.expand_path("../lib/chat_notifier.rb", __FILE__) load File.expand_path("../lib/chat_seeder.rb", __FILE__) load File.expand_path("../lib/chat_statistics.rb", __FILE__) diff --git a/plugins/chat/spec/lib/chat_message_mentions_spec.rb b/plugins/chat/spec/lib/chat_message_mentions_spec.rb new file mode 100644 index 0000000000..7ff25c873d --- /dev/null +++ b/plugins/chat/spec/lib/chat_message_mentions_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Chat::ChatMessageMentions do + fab!(:channel_member_1) { Fabricate(:user) } + fab!(:channel_member_2) { Fabricate(:user) } + fab!(:channel_member_3) { Fabricate(:user) } + fab!(:not_a_channel_member) { Fabricate(:user) } + fab!(:chat_channel) { Fabricate(:chat_channel) } + + before do + chat_channel.add(channel_member_1) + chat_channel.add(channel_member_2) + chat_channel.add(channel_member_3) + end + + describe "#global_mentions" do + it "returns all members of the channel" do + message = create_message("mentioning @all") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.global_mentions.pluck(:username) + + expect(result).to contain_exactly( + channel_member_1.username, + channel_member_2.username, + channel_member_3.username, + ) + end + + it "doesn't include users that were also mentioned directly" do + message = create_message("mentioning @all and @#{channel_member_1.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.global_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_2.username, channel_member_3.username) + end + + it "returns an empty list if there are no global mentions" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.global_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + describe "#here_mentions" do + before do + freeze_time + channel_member_1.update(last_seen_at: 2.minutes.ago) + channel_member_2.update(last_seen_at: 2.minutes.ago) + channel_member_3.update(last_seen_at: 5.minutes.ago) + end + + it "returns all members of the channel who were online in the last 5 minutes" do + message = create_message("mentioning @here") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.here_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_1.username, channel_member_2.username) + end + + it "doesn't include users that were also mentioned directly" do + message = create_message("mentioning @here and @#{channel_member_1.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.here_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_2.username) + end + + it "returns an empty list if there are no here mentions" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.here_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + describe "#direct_mentions" do + it "returns users who were mentioned directly" do + message = + create_message("mentioning @#{channel_member_1.username} and @#{channel_member_2.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.direct_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_1.username, channel_member_2.username) + end + + it "returns a mentioned user even if he's not a member of the channel" do + message = create_message("mentioning @#{not_a_channel_member.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.direct_mentions.pluck(:username) + + expect(result).to contain_exactly(not_a_channel_member.username) + end + + it "returns an empty list if no one was mentioned directly" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.direct_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + describe "#group_mentions" do + fab!(:group1) { Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:group_member_1) { Fabricate(:user, group_ids: [group1.id]) } + fab!(:group_member_2) { Fabricate(:user, group_ids: [group1.id]) } + fab!(:group_member_3) { Fabricate(:user, group_ids: [group1.id]) } + + before do + chat_channel.add(group_member_1) + chat_channel.add(group_member_2) + end + + it "returns members of a mentioned group even if some of them is not members of the channel" do + message = create_message("mentioning @#{group1.name}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.group_mentions.pluck(:username) + + expect(result).to contain_exactly( + group_member_1.username, + group_member_2.username, + group_member_3.username, + ) + end + + it "returns an empty list if no group was mentioned" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.group_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + def create_message(text) + Fabricate(:chat_message, chat_channel: chat_channel, message: text) + end +end From 479c0a3051210da82b127c31088926c4f2a1074a Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 16 Feb 2023 17:17:16 +0100 Subject: [PATCH 8/8] DEV: adds is-new-user and primary group class (#20322) This commit also refactors chat-message-info to use glimmer. --- .../components/chat-message-info.hbs | 19 ++- .../discourse/components/chat-message-info.js | 91 +++++++------- .../components/chat-message-info-test.js | 116 +++++++++++++----- 3 files changed, 152 insertions(+), 74 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-info.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-info.hbs index efea5081cd..2520c2186b 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-info.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-info.hbs @@ -1,7 +1,16 @@ -
+
{{#if @message.chat_webhook_event}} {{#if @message.chat_webhook_event.username}} - + {{@message.chat_webhook_event.username}} {{/if}} @@ -12,7 +21,11 @@ {{else}} {{this.name}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-info.js b/plugins/chat/assets/javascripts/discourse/components/chat-message-info.js index 9bb31d6f4c..c33af19bca 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-info.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-info.js @@ -1,57 +1,27 @@ -import { computed } from "@ember/object"; -import Component from "@ember/component"; +import Component from "@glimmer/component"; import { prioritizeNameInUx } from "discourse/lib/settings"; +import { inject as service } from "@ember/service"; +import { bind } from "discourse-common/utils/decorators"; export default class ChatMessageInfo extends Component { - tagName = ""; - message = null; - details = null; + @service siteSettings; - didInsertElement() { - this._super(...arguments); - this.message.user?.trackStatus?.(); + @bind + trackStatus() { + this.#user?.trackStatus?.(); } - willDestroyElement() { - this._super(...arguments); - this.message.user?.stopTrackingStatus?.(); + @bind + stopTrackingStatus() { + this.#user?.stopTrackingStatus?.(); } - @computed("message.user") - get name() { - return this.prioritizeName - ? this.message.user.name - : this.message.user.username; - } - - @computed("message.reviewable_id", "message.user_flag_status") - get isFlagged() { - return this.message?.reviewable_id || this.message?.user_flag_status === 0; - } - - @computed("message.user.name") - get prioritizeName() { - return ( - this.siteSettings.display_name_on_posts && - prioritizeNameInUx(this.message?.user?.name) - ); - } - - @computed("message.user.status") - get showStatus() { - return !!this.message.user?.status; - } - - @computed("message.user") get usernameClasses() { - const user = this.message?.user; - + const user = this.#user; const classes = this.prioritizeName ? ["is-full-name"] : ["is-username"]; - if (!user) { return classes; } - if (user.staff) { classes.push("is-staff"); } @@ -64,7 +34,44 @@ export default class ChatMessageInfo extends Component { if (user.groupModerator) { classes.push("is-category-moderator"); } - + if (user.new_user) { + classes.push("is-new-user"); + } + if (user.primary_group_name) { + classes.push("group--" + user.primary_group_name); + } return classes.join(" "); } + + get name() { + return this.prioritizeName + ? this.#user?.get("name") + : this.#user?.get("username"); + } + + get isFlagged() { + return ( + this.#message?.get("reviewable_id") || + this.#message?.get("user_flag_status") === 0 + ); + } + + get prioritizeName() { + return ( + this.siteSettings.display_name_on_posts && + prioritizeNameInUx(this.#user?.get("name")) + ); + } + + get showStatus() { + return !!this.#user?.get("status"); + } + + get #user() { + return this.#message?.get("user"); + } + + get #message() { + return this.args.message; + } } diff --git a/plugins/chat/test/javascripts/components/chat-message-info-test.js b/plugins/chat/test/javascripts/components/chat-message-info-test.js index 4fea2ebd07..989656e803 100644 --- a/plugins/chat/test/javascripts/components/chat-message-info-test.js +++ b/plugins/chat/test/javascripts/components/chat-message-info-test.js @@ -4,14 +4,17 @@ import hbs from "htmlbars-inline-precompile"; import { exists, query } from "discourse/tests/helpers/qunit-helpers"; import I18n from "I18n"; import { module, test } from "qunit"; -import User from "discourse/models/user"; import { render } from "@ember/test-helpers"; +import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message"; module("Discourse Chat | Component | chat-message-info", function (hooks) { setupRenderingTest(hooks); test("chat_webhook_event", async function (assert) { - this.set("message", { chat_webhook_event: { username: "discobot" } }); + this.set( + "message", + ChatMessage.create({ chat_webhook_event: { username: "discobot" } }) + ); await render(hbs``); @@ -26,7 +29,7 @@ module("Discourse Chat | Component | chat-message-info", function (hooks) { }); test("user", async function (assert) { - this.set("message", { user: { username: "discobot" } }); + this.set("message", ChatMessage.create({ user: { username: "discobot" } })); await render(hbs``); @@ -37,10 +40,13 @@ module("Discourse Chat | Component | chat-message-info", function (hooks) { }); test("date", async function (assert) { - this.set("message", { - user: { username: "discobot" }, - created_at: moment(), - }); + this.set( + "message", + ChatMessage.create({ + user: { username: "discobot" }, + created_at: moment(), + }) + ); await render(hbs``); @@ -48,13 +54,16 @@ module("Discourse Chat | Component | chat-message-info", function (hooks) { }); test("bookmark (with reminder)", async function (assert) { - this.set("message", { - user: { username: "discobot" }, - bookmark: Bookmark.create({ - reminder_at: moment(), - name: "some name", - }), - }); + this.set( + "message", + ChatMessage.create({ + user: { username: "discobot" }, + bookmark: Bookmark.create({ + reminder_at: moment(), + name: "some name", + }), + }) + ); await render(hbs``); @@ -64,12 +73,15 @@ module("Discourse Chat | Component | chat-message-info", function (hooks) { }); test("bookmark (no reminder)", async function (assert) { - this.set("message", { - user: { username: "discobot" }, - bookmark: Bookmark.create({ - name: "some name", - }), - }); + this.set( + "message", + ChatMessage.create({ + user: { username: "discobot" }, + bookmark: Bookmark.create({ + name: "some name", + }), + }) + ); await render(hbs``); @@ -78,7 +90,7 @@ module("Discourse Chat | Component | chat-message-info", function (hooks) { test("user status", async function (assert) { const status = { description: "off to dentist", emoji: "tooth" }; - this.set("message", { user: User.create({ status }) }); + this.set("message", ChatMessage.create({ user: { status } })); await render(hbs``); @@ -86,10 +98,13 @@ module("Discourse Chat | Component | chat-message-info", function (hooks) { }); test("reviewable", async function (assert) { - this.set("message", { - user: { username: "discobot" }, - user_flag_status: 0, - }); + this.set( + "message", + ChatMessage.create({ + user: { username: "discobot" }, + user_flag_status: 0, + }) + ); await render(hbs``); @@ -98,14 +113,57 @@ module("Discourse Chat | Component | chat-message-info", function (hooks) { I18n.t("chat.you_flagged") ); - this.set("message", { - user: { username: "discobot" }, - reviewable_id: 1, - }); + this.set( + "message", + ChatMessage.create({ + user: { username: "discobot" }, + reviewable_id: 1, + }) + ); assert.strictEqual( query(".chat-message-info__flag a .svg-icon-title").title, I18n.t("chat.flagged") ); }); + + test("with username classes", async function (assert) { + this.set( + "message", + ChatMessage.create({ + user: { + username: "discobot", + admin: true, + moderator: true, + groupModerator: true, + new_user: true, + primary_group_name: "foo", + }, + }) + ); + + await render(hbs``); + + assert.dom(".chat-message-info__username.is-staff").exists(); + assert.dom(".chat-message-info__username.is-admin").exists(); + assert.dom(".chat-message-info__username.is-moderator").exists(); + assert.dom(".chat-message-info__username.is-category-moderator ").exists(); + assert.dom(".chat-message-info__username.is-new-user").exists(); + assert.dom(".chat-message-info__username.group--foo").exists(); + }); + + test("without username classes", async function (assert) { + this.set("message", ChatMessage.create({ user: { username: "discobot" } })); + + await render(hbs``); + + assert.dom(".chat-message-info__username.is-staff").doesNotExist(); + assert.dom(".chat-message-info__username.is-admin").doesNotExist(); + assert.dom(".chat-message-info__username.is-moderator").doesNotExist(); + assert + .dom(".chat-message-info__username.is-category-moderator ") + .doesNotExist(); + assert.dom(".chat-message-info__username.is-new-user").doesNotExist(); + assert.dom(".chat-message-info__username.group--foo").doesNotExist(); + }); });