UX: Include public groups in mentionable groups set (#8516)

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)

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

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|<p>test <span class="mention">@#{group.name}</span> test</p>|

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("<a class=\"mention-group\" href=\"/groups/#{group.name}\">@#{group.name}</a>")
+  end
+end

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

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
 

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