FEATURE: Add new group visibility option for "logged on users" (#7814)

Groups can now be marked as visible to "logged on users". All automatic groups (except `everyone`) are now visible to "logged on users", previously they were marked as public but suppressed in the group page for non-staff.

app/assets/javascripts/discourse/components/groups-form-interaction-fields.js.es6

@@ -13,19 +13,25 @@ export default Ember.Component.extend({
       },
       {
         name: I18n.t(
-          "admin.groups.manage.interaction.visibility_levels.members"
+          "admin.groups.manage.interaction.visibility_levels.logged_on_users"
         ),
         value: 1
       },
       {
-        name: I18n.t("admin.groups.manage.interaction.visibility_levels.staff"),
+        name: I18n.t(
+          "admin.groups.manage.interaction.visibility_levels.members"
+        ),
         value: 2
       },
+      {
+        name: I18n.t("admin.groups.manage.interaction.visibility_levels.staff"),
+        value: 3
+      },
       {
         name: I18n.t(
           "admin.groups.manage.interaction.visibility_levels.owners"
         ),
-        value: 3
+        value: 4
       }
     ];
 

app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs

@@ -9,6 +9,10 @@
         content=visibilityLevelOptions
         castInteger=true
         class="groups-form-visibility-level"}}
+
+    <div class="control-instructions">
+      {{i18n 'admin.groups.manage.interaction.visibility_levels.description'}}
+    </div>
   </div>
 {{/if}}
 

app/models/group.rb

@@ -89,9 +89,10 @@ class Group < ActiveRecord::Base
   def self.visibility_levels
     @visibility_levels = Enum.new(
       public: 0,
-      members: 1,
-      staff: 2,
-      owners: 3
+      logged_on_users: 1,
+      members: 2,
+      staff: 3,
+      owners: 4
     )
   end
 
@@ -112,6 +113,11 @@ class Group < ActiveRecord::Base
 
           UNION ALL
 
+          SELECT g.id FROM groups g
+          WHERE g.visibility_level = :logged_on_users AND :user_id IS NOT NULL
+
+          UNION ALL
+
           SELECT g.id FROM groups g
           JOIN group_users gu ON gu.group_id = g.id AND
                                  gu.user_id = :user_id
@@ -324,6 +330,8 @@ class Group < ActiveRecord::Base
       group.update!(messageable_level: ALIAS_LEVELS[:everyone])
     end
 
+    group.update!(visibility_level: Group.visibility_levels[:logged_on_users]) if group.visibility_level == Group.visibility_levels[:public]
+
     # Remove people from groups they don't belong in.
     remove_subquery =
       case name

config/locales/client.en.yml

@@ -3185,9 +3185,11 @@ en:
             visibility_levels:
               title: "Who can see this group?"
               public: "Everyone"
-              members: "Group owners, members and admins"
+              logged_on_users: "Logged on users"
+              members: "Group owners and members"
               staff: "Group owners and staff"
-              owners: "Group owners and admins"
+              owners: "Group owners"
+              description: "Admins can see all groups."
           membership:
             automatic: Automatic
             trust_level: Trust Level

db/fixtures/002_groups.rb

@@ -5,4 +5,4 @@ if g = Group.find_by(name: 'trust_level_5', id: 15)
   g.destroy!
 end
 
-Group.where(name: 'everyone').update_all(visibility_level: Group.visibility_levels[:owners])
+Group.where(name: 'everyone').update_all(visibility_level: Group.visibility_levels[:staff])

db/migrate/20190705173948_renumber_group_visibility_levels.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class RenumberGroupVisibilityLevels < ActiveRecord::Migration[5.2]
+  def up
+    execute "UPDATE groups SET visibility_level = 4 WHERE visibility_level = 3"
+    execute "UPDATE groups SET visibility_level = 3 WHERE visibility_level = 2"
+    execute "UPDATE groups SET visibility_level = 2 WHERE visibility_level = 1"
+  end
+
+  def down
+    execute "UPDATE groups SET visibility_level = 0 WHERE visibility_level = 1"
+    execute "UPDATE groups SET visibility_level = 1 WHERE visibility_level = 2"
+    execute "UPDATE groups SET visibility_level = 2 WHERE visibility_level = 3"
+    execute "UPDATE groups SET visibility_level = 3 WHERE visibility_level = 4"
+  end
+end

lib/guardian.rb

@@ -194,6 +194,7 @@ class Guardian
     return true if group.visibility_level == Group.visibility_levels[:public]
     return true if is_admin?
     return true if is_staff? && group.visibility_level == Group.visibility_levels[:staff]
+    return true if authenticated? && group.visibility_level == Group.visibility_levels[:logged_on_users]
     return false if user.blank?
 
     membership = GroupUser.find_by(group_id: group.id, user_id: user.id)
@@ -213,6 +214,7 @@ class Guardian
     return true if groups.all? { |g| g.visibility_level == Group.visibility_levels[:public] }
     return true if is_admin?
     return true if is_staff? && groups.all? { |g| g.visibility_level == Group.visibility_levels[:staff] }
+    return true if authenticated? && groups.all? { |g| g.visibility_level == Group.visibility_levels[:logged_on_users] }
     return false if user.blank?
 
     memberships = GroupUser.where(group: groups, user_id: user.id).pluck(:owner)

spec/components/guardian_spec.rb

@@ -3027,7 +3027,7 @@ describe Guardian do
   end
 
   describe(:can_see_group) do
-    it 'Correctly handles owner visibile groups' do
+    it 'Correctly handles owner visible groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:owners])
 
       member = Fabricate(:user)
@@ -3039,13 +3039,14 @@ describe Guardian do
       group.reload
 
       expect(Guardian.new(admin).can_see_group?(group)).to eq(true)
+      expect(Guardian.new(another_user).can_see_group?(group)).to eq(false)
       expect(Guardian.new(moderator).can_see_group?(group)).to eq(false)
       expect(Guardian.new(member).can_see_group?(group)).to eq(false)
       expect(Guardian.new.can_see_group?(group)).to eq(false)
       expect(Guardian.new(owner).can_see_group?(group)).to eq(true)
     end
 
-    it 'Correctly handles staff visibile groups' do
+    it 'Correctly handles staff visible groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:staff])
 
       member = Fabricate(:user)
@@ -3056,6 +3057,7 @@ describe Guardian do
       group.add_owner(owner)
       group.reload
 
+      expect(Guardian.new(another_user).can_see_group?(group)).to eq(false)
       expect(Guardian.new(member).can_see_group?(group)).to eq(false)
       expect(Guardian.new(admin).can_see_group?(group)).to eq(true)
       expect(Guardian.new(moderator).can_see_group?(group)).to eq(true)
@@ -3063,7 +3065,7 @@ describe Guardian do
       expect(Guardian.new.can_see_group?(group)).to eq(false)
     end
 
-    it 'Correctly handles member visibile groups' do
+    it 'Correctly handles member visible groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:members])
 
       member = Fabricate(:user)
@@ -3076,11 +3078,30 @@ describe Guardian do
 
       expect(Guardian.new(moderator).can_see_group?(group)).to eq(false)
       expect(Guardian.new.can_see_group?(group)).to eq(false)
+      expect(Guardian.new(another_user).can_see_group?(group)).to eq(false)
       expect(Guardian.new(admin).can_see_group?(group)).to eq(true)
       expect(Guardian.new(member).can_see_group?(group)).to eq(true)
       expect(Guardian.new(owner).can_see_group?(group)).to eq(true)
     end
 
+    it 'Correctly handles logged-on-user visible groups' do
+      group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:logged_on_users])
+      member = Fabricate(:user)
+      group.add(member)
+      group.save!
+
+      owner = Fabricate(:user)
+      group.add_owner(owner)
+      group.reload
+
+      expect(Guardian.new.can_see_group?(group)).to eq(false)
+      expect(Guardian.new(moderator).can_see_group?(group)).to eq(true)
+      expect(Guardian.new(admin).can_see_group?(group)).to eq(true)
+      expect(Guardian.new(member).can_see_group?(group)).to eq(true)
+      expect(Guardian.new(owner).can_see_group?(group)).to eq(true)
+      expect(Guardian.new(another_user).can_see_group?(group)).to eq(true)
+    end
+
     it 'Correctly handles public groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:public])
 
@@ -3090,7 +3111,7 @@ describe Guardian do
   end
 
   describe '#can_see_groups?' do
-    it 'correctly handles owner visibile groups' do
+    it 'correctly handles owner visible groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:owners])
 
       member = Fabricate(:user)
@@ -3102,6 +3123,7 @@ describe Guardian do
       group.reload
 
       expect(Guardian.new(admin).can_see_groups?([group])).to eq(true)
+      expect(Guardian.new(another_user).can_see_groups?([group])).to eq(false)
       expect(Guardian.new(moderator).can_see_groups?([group])).to eq(false)
       expect(Guardian.new(member).can_see_groups?([group])).to eq(false)
       expect(Guardian.new.can_see_groups?([group])).to eq(false)
@@ -3126,9 +3148,10 @@ describe Guardian do
       expect(Guardian.new(member).can_see_groups?([group, group2])).to eq(false)
       expect(Guardian.new.can_see_groups?([group, group2])).to eq(false)
       expect(Guardian.new(owner).can_see_groups?([group, group2])).to eq(false)
+      expect(Guardian.new(another_user).can_see_groups?([group, group2])).to eq(false)
     end
 
-    it 'correctly handles staff visibile groups' do
+    it 'correctly handles staff visible groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:staff])
 
       member = Fabricate(:user)
@@ -3144,9 +3167,10 @@ describe Guardian do
       expect(Guardian.new(moderator).can_see_groups?([group])).to eq(true)
       expect(Guardian.new(owner).can_see_groups?([group])).to eq(true)
       expect(Guardian.new.can_see_groups?([group])).to eq(false)
+      expect(Guardian.new(another_user).can_see_groups?([group])).to eq(false)
     end
 
-    it 'correctly handles member visibile groups' do
+    it 'correctly handles member visible groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:members])
 
       member = Fabricate(:user)
@@ -3157,6 +3181,7 @@ describe Guardian do
       group.add_owner(owner)
       group.reload
 
+      expect(Guardian.new(another_user).can_see_groups?([group])).to eq(false)
       expect(Guardian.new(moderator).can_see_groups?([group])).to eq(false)
       expect(Guardian.new.can_see_groups?([group])).to eq(false)
       expect(Guardian.new(admin).can_see_groups?([group])).to eq(true)
@@ -3164,6 +3189,25 @@ describe Guardian do
       expect(Guardian.new(owner).can_see_groups?([group])).to eq(true)
     end
 
+    it 'correctly handles logged-on-user visible groups' do
+      group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:logged_on_users])
+
+      member = Fabricate(:user)
+      group.add(member)
+      group.save!
+
+      owner = Fabricate(:user)
+      group.add_owner(owner)
+      group.reload
+
+      expect(Guardian.new(member).can_see_groups?([group])).to eq(true)
+      expect(Guardian.new(admin).can_see_groups?([group])).to eq(true)
+      expect(Guardian.new(moderator).can_see_groups?([group])).to eq(true)
+      expect(Guardian.new(owner).can_see_groups?([group])).to eq(true)
+      expect(Guardian.new.can_see_groups?([group])).to eq(false)
+      expect(Guardian.new(another_user).can_see_groups?([group])).to eq(true)
+    end
+
     it 'correctly handles the case where the user is not a member of every group' do
       group1 = Group.new(name: 'group', visibility_level: Group.visibility_levels[:members])
       group2 = Group.new(name: 'group2', visibility_level: Group.visibility_levels[:members])
@@ -3190,7 +3234,7 @@ describe Guardian do
       expect(Guardian.new.can_see_groups?([group])).to eq(true)
     end
 
-    it 'correctly handles there case where not every group is public' do
+    it 'correctly handles case where not every group is public' do
       group1 = Group.new(name: 'group', visibility_level: Group.visibility_levels[:public])
       group2 = Group.new(name: 'group', visibility_level: Group.visibility_levels[:private])
 

spec/models/group_spec.rb

@@ -243,6 +243,14 @@ describe Group do
       expect(g.visibility_level).to eq(Group.visibility_levels[:staff])
     end
 
+    it "makes sure automatic groups are visible to logged on users" do
+      g = Group.refresh_automatic_group!(:moderators)
+      expect(g.visibility_level).to eq(Group.visibility_levels[:logged_on_users])
+
+      tl0 = Group.refresh_automatic_group!(:trust_level_0)
+      expect(tl0.visibility_level).to eq(Group.visibility_levels[:logged_on_users])
+    end
+
     it "ensures that the moderators group is messageable by all" do
       group = Group.find(Group::AUTO_GROUPS[:moderators])
       group.update!(messageable_level: Group::ALIAS_LEVELS[:nobody])
@@ -651,6 +659,7 @@ describe Group do
 
     it 'correctly restricts group visibility' do
       group = Fabricate.build(:group, visibility_level: Group.visibility_levels[:owners])
+      logged_on_user = Fabricate(:user)
       member = Fabricate(:user)
       group.add(member)
       group.save!
@@ -665,6 +674,7 @@ describe Group do
       expect(can_view?(owner, group)).to eq(true)
       expect(can_view?(moderator, group)).to eq(false)
       expect(can_view?(member, group)).to eq(false)
+      expect(can_view?(logged_on_user, group)).to eq(false)
       expect(can_view?(nil, group)).to eq(false)
 
       group.update_columns(visibility_level: Group.visibility_levels[:staff])
@@ -673,6 +683,7 @@ describe Group do
       expect(can_view?(owner, group)).to eq(true)
       expect(can_view?(moderator, group)).to eq(true)
       expect(can_view?(member, group)).to eq(false)
+      expect(can_view?(logged_on_user, group)).to eq(false)
       expect(can_view?(nil, group)).to eq(false)
 
       group.update_columns(visibility_level: Group.visibility_levels[:members])
@@ -681,6 +692,7 @@ describe Group do
       expect(can_view?(owner, group)).to eq(true)
       expect(can_view?(moderator, group)).to eq(false)
       expect(can_view?(member, group)).to eq(true)
+      expect(can_view?(logged_on_user, group)).to eq(false)
       expect(can_view?(nil, group)).to eq(false)
 
       group.update_columns(visibility_level: Group.visibility_levels[:public])
@@ -689,7 +701,17 @@ describe Group do
       expect(can_view?(owner, group)).to eq(true)
       expect(can_view?(moderator, group)).to eq(true)
       expect(can_view?(member, group)).to eq(true)
+      expect(can_view?(logged_on_user, group)).to eq(true)
       expect(can_view?(nil, group)).to eq(true)
+
+      group.update_columns(visibility_level: Group.visibility_levels[:logged_on_users])
+
+      expect(can_view?(admin, group)).to eq(true)
+      expect(can_view?(owner, group)).to eq(true)
+      expect(can_view?(moderator, group)).to eq(true)
+      expect(can_view?(member, group)).to eq(true)
+      expect(can_view?(logged_on_user, group)).to eq(true)
+      expect(can_view?(nil, group)).to eq(false)
     end
 
   end

spec/requests/groups_controller_spec.rb

@@ -62,6 +62,10 @@ describe GroupsController do
     end
 
     context 'sortable' do
+      before do
+        sign_in(user)
+      end
+
       let!(:other_group) { Fabricate(:group, name: "zzzzzz", users: [user]) }
 
       %w{
@@ -126,7 +130,7 @@ describe GroupsController do
       expect(group_ids).to include(group.id)
       expect(group_ids).to_not include(staff_group.id)
       expect(response_body["load_more_groups"]).to eq("/groups?page=1")
-      expect(response_body["total_rows_groups"]).to eq(2)
+      expect(response_body["total_rows_groups"]).to eq(1)
 
       expect(response_body["extras"]["type_filters"].map(&:to_sym)).to eq(
         described_class::TYPE_FILTERS.keys - [:my, :owner, :automatic]
@@ -1371,7 +1375,8 @@ describe GroupsController do
     before do
       group.update!(
         name: 'GOT',
-        full_name: 'Daenerys Targaryen'
+        full_name: 'Daenerys Targaryen',
+        visibility_level: Group.visibility_levels[:logged_on_users]
       )
 
       hidden_group

spec/requests/list_controller_spec.rb

@@ -242,6 +242,16 @@ RSpec.describe ListController do
         end
       end
 
+      describe 'group restricted to logged-on-users' do
+        before { group.update!(visibility_level: Group.visibility_levels[:logged_on_users]) }
+
+        it 'should return the right response' do
+          get "/topics/groups/#{group.name}.json"
+
+          expect(response.status).to eq(403)
+        end
+      end
+
       describe 'restricted group' do
         before { group.update!(visibility_level: Group.visibility_levels[:staff]) }
 
@@ -265,6 +275,16 @@ RSpec.describe ListController do
           expect(response.status).to eq(403)
         end
       end
+
+      describe 'group restricted to logged-on-users' do
+        before { group.update!(visibility_level: Group.visibility_levels[:logged_on_users]) }
+
+        it 'should return the right response' do
+          get "/topics/groups/#{group.name}.json"
+
+          expect(response.status).to eq(200)
+        end
+      end
     end
 
     describe 'for a group user' do