diff --git a/app/assets/javascripts/admin/controllers/admin-group.js.es6 b/app/assets/javascripts/admin/controllers/admin-group.js.es6
index a0f4961b62f..52dbc07791b 100644
--- a/app/assets/javascripts/admin/controllers/admin-group.js.es6
+++ b/app/assets/javascripts/admin/controllers/admin-group.js.es6
@@ -15,6 +15,15 @@ export default Ember.Controller.extend({
];
}.property(),
+ visibilityLevelOptions: function() {
+ return [
+ { name: I18n.t("groups.visibility_levels.public"), value: 0 },
+ { name: I18n.t("groups.visibility_levels.members"), value: 1 },
+ { name: I18n.t("groups.visibility_levels.staff"), value: 2 },
+ { name: I18n.t("groups.visibility_levels.owners"), value: 3 }
+ ];
+ }.property(),
+
trustLevelOptions: function() {
return [
{ name: I18n.t("groups.trust_levels.none"), value: 0 },
@@ -22,14 +31,16 @@ export default Ember.Controller.extend({
];
}.property(),
- @computed('model.visible', 'model.public')
- disableMembershipRequestSetting(visible, publicGroup) {
- return !visible || publicGroup;
+ @computed('model.visibility_level', 'model.public')
+ disableMembershipRequestSetting(visibility_level, publicGroup) {
+ visibility_level = parseInt(visibility_level);
+ return (visibility_level !== 0) || publicGroup;
},
- @computed('model.visible', 'model.allow_membership_requests')
- disablePublicSetting(visible, allowMembershipRequests) {
- return !visible || allowMembershipRequests;
+ @computed('model.visibility_level', 'model.allow_membership_requests')
+ disablePublicSetting(visibility_level, allowMembershipRequests) {
+ visibility_level = parseInt(visibility_level);
+ return (visibility_level !== 0) || allowMembershipRequests;
},
actions: {
diff --git a/app/assets/javascripts/admin/routes/admin-group.js.es6 b/app/assets/javascripts/admin/routes/admin-group.js.es6
index 3575496edd4..0009f834e13 100644
--- a/app/assets/javascripts/admin/routes/admin-group.js.es6
+++ b/app/assets/javascripts/admin/routes/admin-group.js.es6
@@ -4,7 +4,7 @@ export default Discourse.Route.extend({
model(params) {
if (params.name === 'new') {
- return Group.create({ automatic: false, visible: true });
+ return Group.create({ automatic: false, visibility_level: 0 });
}
const group = this.modelFor('adminGroupsType').findBy('name', params.name);
diff --git a/app/assets/javascripts/admin/templates/group.hbs b/app/assets/javascripts/admin/templates/group.hbs
index 79dbcfc3b64..d1e8f3df185 100644
--- a/app/assets/javascripts/admin/templates/group.hbs
+++ b/app/assets/javascripts/admin/templates/group.hbs
@@ -43,10 +43,8 @@
{{/if}}
-
+
+ {{combo-box name="alias" valueAttribute="value" value=model.visibility_level content=visibilityLevelOptions}}
{{#unless model.automatic}}
diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6
index f14507c6abe..67c70e510b5 100644
--- a/app/assets/javascripts/discourse/models/group.js.es6
+++ b/app/assets/javascripts/discourse/models/group.js.es6
@@ -113,23 +113,27 @@ const Group = RestModel.extend({
return aliasLevel === '99';
},
- @observes("visible", "canEveryoneMention")
+ @observes("visibility_level", "canEveryoneMention")
_updateAllowMembershipRequests() {
- if (!this.get('visible') || !this.get('canEveryoneMention')) {
+ if (this.get('visibility_level') !== 0 || !this.get('canEveryoneMention')) {
this.set ('allow_membership_requests', false);
}
},
- @observes("visible")
+ @observes("visibility_level")
_updatePublic() {
- if (!this.get('visible')) this.set('public', false);
+ let visibility_level = parseInt(this.get('visibility_level'));
+ if (visibility_level !== 0) {
+ this.set('public', false);
+ this.set('allow_membership_requests', false);
+ }
},
asJSON() {
return {
name: this.get('name'),
alias_level: this.get('alias_level'),
- visible: !!this.get('visible'),
+ visibility_level: this.get('visibility_level'),
automatic_membership_email_domains: this.get('emailDomains'),
automatic_membership_retroactive: !!this.get('automatic_membership_retroactive'),
title: this.get('title'),
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index c93e287cee7..3613ffc04e2 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -59,7 +59,7 @@ class Admin::GroupsController < Admin::AdminController
def save_group(group)
group.alias_level = group_params[:alias_level].to_i if group_params[:alias_level].present?
- group.visible = group_params[:visible] == "true"
+ group.visibility_level = group_params[:visibility_level]
grant_trust_level = group_params[:grant_trust_level].to_i
group.grant_trust_level = (grant_trust_level > 0 && grant_trust_level <= 4) ? grant_trust_level : nil
@@ -160,7 +160,7 @@ class Admin::GroupsController < Admin::AdminController
def group_params
params.require(:group).permit(
- :name, :alias_level, :visible, :automatic_membership_email_domains,
+ :name, :alias_level, :visibility_level, :automatic_membership_email_domains,
:automatic_membership_retroactive, :title, :primary_group,
:grant_trust_level, :incoming_email, :flair_url, :flair_bg_color,
:flair_color, :bio_raw, :public, :allow_membership_requests, :full_name,
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 292bfd104d0..74b7ad0f66d 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -20,6 +20,12 @@ class GroupsController < ApplicationController
page = params[:page]&.to_i || 0
groups = Group.visible_groups(current_user)
+
+ unless guardian.is_staff?
+ # hide automatic groups from all non stuff to de-clutter page
+ groups = groups.where(automatic: false)
+ end
+
count = groups.count
groups = groups.offset(page * page_size).limit(page_size)
diff --git a/app/models/group.rb b/app/models/group.rb
index 90d6f063542..423b535b6a5 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -1,3 +1,7 @@
+# frozen_string_literal: true
+
+require_dependency 'enum'
+
class Group < ActiveRecord::Base
include HasCustomFields
include AnonCacheInvalidator
@@ -62,17 +66,56 @@ class Group < ActiveRecord::Base
:everyone => 99
}
+ def self.visibility_levels
+ @visibility_levels = Enum.new(
+ public: 0,
+ members: 1,
+ staff: 2,
+ owners: 3
+ )
+ end
+
validates :alias_level, inclusion: { in: ALIAS_LEVELS.values}
scope :visible_groups, ->(user) {
groups = Group.order(name: :asc).where("groups.id > 0")
- if !user || !user.admin
- owner_group_ids = GroupUser.where(user: user, owner: true).pluck(:group_id)
+ unless user&.admin
+ sql = <<~SQL
+ groups.id IN (
+ SELECT g.id FROM groups g WHERE g.visibility_level = :public
+
+ UNION ALL
+
+ SELECT g.id FROM groups g
+ JOIN group_users gu ON gu.group_id = g.id AND
+ gu.user_id = :user_id
+ WHERE g.visibility_level = :members
+
+ UNION ALL
+
+ SELECT g.id FROM groups g
+ LEFT JOIN group_users gu ON gu.group_id = g.id AND
+ gu.user_id = :user_id AND
+ gu.owner
+ WHERE g.visibility_level = :staff AND (gu.id IS NOT NULL OR :is_staff)
+
+ UNION ALL
+
+ SELECT g.id FROM groups g
+ JOIN group_users gu ON gu.group_id = g.id AND
+ gu.user_id = :user_id AND
+ gu.owner
+ WHERE g.visibility_level = :owners
+
+ )
+ SQL
+
+ groups = groups.where(
+ sql,
+ Group.visibility_levels.to_h.merge(user_id: user&.id, is_staff: !!user&.staff?)
+ )
- groups = groups.where("
- (groups.automatic = false AND groups.visible = true) OR groups.id IN (?)
- ", owner_group_ids)
end
groups
@@ -191,7 +234,7 @@ class Group < ActiveRecord::Base
# the everyone group is special, it can include non-users so there is no
# way to have the membership in a table
if name == :everyone
- group.visible = false
+ group.visibility_level = Group.visibility_levels[:owners]
group.save!
return group
end
@@ -282,7 +325,7 @@ class Group < ActiveRecord::Base
end
def self.search_group(name)
- Group.where(visible: true).where(
+ Group.where(visibility_level: visibility_levels[:public]).where(
"name ILIKE :term_like OR full_name ILIKE :term_like", term_like: "#{name}%"
)
end
@@ -487,11 +530,11 @@ class Group < ActiveRecord::Base
return if new_record? && !self.primary_group?
if self.primary_group_changed?
- sql = <1,
"automatic"=>false,
"alias_level"=>0,
- "visible"=>true,
+ "visibility_level"=>0,
"automatic_membership_email_domains"=>nil,
"automatic_membership_retroactive"=>false,
"title"=>nil,
@@ -100,15 +100,17 @@ describe Admin::GroupsController do
expect do
xhr :put, :update, { id: group.id, group: {
- visible: "false",
+ visibility_level: Group.visibility_levels[:owners],
allow_membership_requests: "true"
} }
+
end.to change { GroupHistory.count }.by(2)
expect(response).to be_success
group.reload
- expect(group.visible).to eq(false)
+
+ expect(group.visibility_level).to eq( Group.visibility_levels[:owners])
expect(group.allow_membership_requests).to eq(true)
end
diff --git a/spec/integration/groups_spec.rb b/spec/integration/groups_spec.rb
index 055129425e0..ed5b5bb7c6b 100644
--- a/spec/integration/groups_spec.rb
+++ b/spec/integration/groups_spec.rb
@@ -2,16 +2,11 @@ require 'rails_helper'
describe "Groups" do
let(:user) { Fabricate(:user) }
- let(:group) { Fabricate(:group, users: [user]) }
+ let!(:group) { Fabricate(:group, users: [user]) }
describe 'viewing groups' do
- let(:other_group) do
- Fabricate(:group, name: '0000', visible: true, automatic: false)
- end
-
- before do
- other_group
- group.update_attributes!(automatic: true, visible: true)
+ let!(:staff_group) do
+ Fabricate(:group, name: '0000', visibility_level: Group.visibility_levels[:staff])
end
context 'when group directory is disabled' do
@@ -33,8 +28,8 @@ describe "Groups" do
group_ids = response_body["groups"].map { |g| g["id"] }
expect(response_body["extras"]["group_user_ids"]).to eq([])
- expect(group_ids).to include(other_group.id)
- expect(group_ids).to_not include(group.id)
+ 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(1)
end
@@ -54,7 +49,7 @@ describe "Groups" do
group_ids = response_body["groups"].map { |g| g["id"] }
expect(response_body["extras"]["group_user_ids"]).to eq([group.id])
- expect(group_ids).to include(group.id, other_group.id)
+ expect(group_ids).to include(group.id, staff_group.id)
expect(response_body["load_more_groups"]).to eq("/groups?page=1")
expect(response_body["total_rows_groups"]).to eq(10)
end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 8ec3348e459..d2ba94714b9 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -180,7 +180,7 @@ describe Group do
describe '.refresh_automatic_group!' do
it "makes sure the everyone group is not visible" do
g = Group.refresh_automatic_group!(:everyone)
- expect(g.visible).to eq(false)
+ expect(g.visibility_level).to eq(Group.visibility_levels[:owners])
end
it "uses the localized name if name has not been taken" do
@@ -432,44 +432,54 @@ describe Group do
end
describe ".visible_groups" do
- let(:group) { Fabricate(:group, visible: false) }
- let(:group_2) { Fabricate(:group, visible: true) }
- let(:admin) { Fabricate(:admin) }
- let(:user) { Fabricate(:user) }
- before do
- group
- group_2
+ def can_view?(user, group)
+ Group.visible_groups(user).where(id: group.id).exists?
end
- describe 'when user is an admin' do
- it 'should return the right groups' do
- expect(Group.visible_groups(admin).pluck(:id).sort)
- .to eq([group.id, group_2.id].concat(Group::AUTO_GROUP_IDS.keys - [0]).sort)
- end
+ it 'correctly restricts group visibility' do
+ group = Fabricate.build(:group, visibility_level: Group.visibility_levels[:owners])
+ member = Fabricate(:user)
+ group.add(member)
+ group.save!
+
+ owner = Fabricate(:user)
+ group.add_owner(owner)
+
+ moderator = Fabricate(:user, moderator: true)
+ admin = Fabricate(:user, admin: true)
+
+ expect(can_view?(admin, group)).to eq(true)
+ 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?(nil, group)).to eq(false)
+
+ group.update_columns(visibility_level: Group.visibility_levels[:staff])
+
+ 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(false)
+ expect(can_view?(nil, group)).to eq(false)
+
+ group.update_columns(visibility_level: Group.visibility_levels[:members])
+
+ expect(can_view?(admin, group)).to eq(true)
+ 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?(nil, group)).to eq(false)
+
+ group.update_columns(visibility_level: Group.visibility_levels[:public])
+
+ 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?(nil, group)).to eq(true)
end
- describe 'when user is owner of a group' do
- it 'should return the right groups' do
- group.add_owner(user)
-
- expect(Group.visible_groups(user).pluck(:id).sort)
- .to eq([group.id, group_2.id])
- end
- end
-
- describe 'when user is not the owner of any group' do
- it 'should return the right groups' do
- expect(Group.visible_groups(user).pluck(:id).sort)
- .to eq([group_2.id])
- end
- end
-
- describe 'user is nil' do
- it 'should return the right groups' do
- expect(Group.visible_groups(nil).pluck(:id).sort).to eq([group_2.id])
- end
- end
end
describe '#add' do