From 3ec2081059788e2c248f98e880bee8a1a13830b2 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Thu, 12 Dec 2019 13:13:40 +0200 Subject: [PATCH] UX: Include public groups in mentionable groups set (#8516) --- app/models/group.rb | 36 ++++++++++++++----------- app/services/post_alerter.rb | 2 +- spec/components/pretty_text_spec.rb | 5 +++- spec/integration/group_spec.rb | 25 +++++++++++++++++ spec/models/post_spec.rb | 1 + spec/requests/groups_controller_spec.rb | 16 +++++++++++ spec/requests/users_controller_spec.rb | 17 ++++++------ 7 files changed, 77 insertions(+), 25 deletions(-) create mode 100644 spec/integration/group_spec.rb diff --git a/app/models/group.rb b/app/models/group.rb index a773593107b..a0d259c8923 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -204,8 +204,8 @@ class Group < ActiveRecord::Base groups } - scope :mentionable, lambda { |user| - where(self.mentionable_sql_clause, + scope :mentionable, lambda { |user, include_public: true| + where(self.mentionable_sql_clause(include_public: include_public), levels: alias_levels(user), user_id: user&.id ) @@ -222,19 +222,25 @@ class Group < ActiveRecord::Base )", levels: alias_levels(user), user_id: user && user.id) } - def self.mentionable_sql_clause - <<~SQL - mentionable_level in (:levels) - OR ( - mentionable_level = #{ALIAS_LEVELS[:members_mods_and_admins]} - AND id in ( - SELECT group_id FROM group_users WHERE user_id = :user_id) - ) OR ( - mentionable_level = #{ALIAS_LEVELS[:owners_mods_and_admins]} - AND id in ( - SELECT group_id FROM group_users WHERE user_id = :user_id AND owner IS TRUE) - ) - SQL + def self.mentionable_sql_clause(include_public: true) + clause = +<<~SQL + mentionable_level in (:levels) + OR ( + mentionable_level = #{ALIAS_LEVELS[:members_mods_and_admins]} + AND id in ( + SELECT group_id FROM group_users WHERE user_id = :user_id) + ) OR ( + mentionable_level = #{ALIAS_LEVELS[:owners_mods_and_admins]} + AND id in ( + SELECT group_id FROM group_users WHERE user_id = :user_id AND owner IS TRUE) + ) + SQL + + if include_public + clause << "OR visibility_level = #{Group.visibility_levels[:public]}" + end + + clause end def self.alias_levels(user) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index f769649e53a..bc6c80ad855 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -458,7 +458,7 @@ class PostAlerter def expand_group_mentions(groups, post) return unless post.user && groups - Group.mentionable(post.user).where(id: groups.map(&:id)).each do |group| + Group.mentionable(post.user, include_public: false).where(id: groups.map(&:id)).each do |group| next if group.user_count >= SiteSetting.max_users_notified_per_group_mention yield group, group.users end diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb index d87cd1c0ce3..c87a449964e 100644 --- a/spec/components/pretty_text_spec.rb +++ b/spec/components/pretty_text_spec.rb @@ -341,7 +341,10 @@ describe PrettyText do end it "does not create mention for a non mentionable group" do - group = Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:nobody]) + group = Fabricate(:group, + visibility_level: Group.visibility_levels[:members], + mentionable_level: Group::ALIAS_LEVELS[:nobody] + ) expect(PrettyText.cook("test @#{group.name} test")).to eq( %Q|

test @#{group.name} test

| diff --git a/spec/integration/group_spec.rb b/spec/integration/group_spec.rb new file mode 100644 index 00000000000..639ff7a2ffb --- /dev/null +++ b/spec/integration/group_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Group do + let(:group) do + Fabricate( + :group, + visibility_level: Group.visibility_levels[:public], + mentionable_level: Group::ALIAS_LEVELS[:nobody], + users: [ Fabricate(:user) ] + ) + end + + let(:post) { Fabricate(:post, raw: "mention @#{group.name}") } + + before do + Jobs.run_immediately! + end + + it 'users can mention public groups, but does not create a notification' do + expect { post }.not_to change { Notification.where(notification_type: Notification.types[:group_mentioned]).count } + expect(post.cooked).to include("@#{group.name}") + end +end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 9b45cd9bec5..e82c21d87a9 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -985,6 +985,7 @@ describe Post do describe 'mentions' do fab!(:group) do Fabricate(:group, + visibility_level: Group.visibility_levels[:members], mentionable_level: Group::ALIAS_LEVELS[:members_mods_and_admins] ) end diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 42d76b4d50b..071d986b5cd 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -451,6 +451,11 @@ describe GroupsController do it "should return the right response" do sign_in(user) + group.update!( + mentionable_level: Group::ALIAS_LEVELS[:owners_mods_and_admins], + visibility_level: Group.visibility_levels[:logged_on_users] + ) + get "/groups/#{group.name}/mentionable.json" expect(response.status).to eq(200) @@ -467,6 +472,17 @@ describe GroupsController do response_body = JSON.parse(response.body) expect(response_body["mentionable"]).to eq(true) + + group.update!( + mentionable_level: Group::ALIAS_LEVELS[:nobody], + visibility_level: Group.visibility_levels[:public] + ) + + get "/groups/#{group.name}/mentionable.json" + expect(response.status).to eq(200) + + response_body = JSON.parse(response.body) + expect(response_body["mentionable"]).to eq(true) end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index e33e526cae2..179bedc690f 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -3163,26 +3163,27 @@ describe UsersController do context 'groups' do let!(:mentionable_group) do Fabricate(:group, - mentionable_level: 99, - messageable_level: 0, - visibility_level: 0, + mentionable_level: Group::ALIAS_LEVELS[:everyone], + messageable_level: Group::ALIAS_LEVELS[:nobody], + visibility_level: Group.visibility_levels[:public], name: 'aaa1' ) end let!(:mentionable_group_2) do Fabricate(:group, - mentionable_level: 99, - messageable_level: 0, - visibility_level: 1, + mentionable_level: Group::ALIAS_LEVELS[:everyone], + messageable_level: Group::ALIAS_LEVELS[:nobody], + visibility_level: Group.visibility_levels[:logged_on_users], name: 'aaa2' ) end let!(:messageable_group) do Fabricate(:group, - mentionable_level: 0, - messageable_level: 99, + mentionable_level: Group::ALIAS_LEVELS[:nobody], + messageable_level: Group::ALIAS_LEVELS[:everyone], + visibility_level: Group.visibility_levels[:logged_on_users], name: 'aaa3' ) end