From 845170bd6b53c3bea8a6f03d44971d8da1613176 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 3 Jul 2017 15:26:46 -0400 Subject: [PATCH] FEATURE: add support for group visibility level There are 4 visibility levels - public (default) - members only - staff - owners Note, admins and group owners ALWAYS have visibility to groups Migration treated old "non public" as "members only" --- .../admin/controllers/admin-group.js.es6 | 23 +++-- .../admin/routes/admin-group.js.es6 | 2 +- .../javascripts/admin/templates/group.hbs | 6 +- .../javascripts/discourse/models/group.js.es6 | 14 +-- app/controllers/admin/groups_controller.rb | 4 +- app/controllers/groups_controller.rb | 6 ++ app/models/group.rb | 69 ++++++++++++--- app/serializers/basic_group_serializer.rb | 2 +- app/serializers/user_serializer.rb | 9 +- config/locales/client.en.yml | 9 +- db/fixtures/002_groups.rb | 3 +- ...03144855_add_visibility_level_to_groups.rb | 12 +++ lib/guardian.rb | 15 +++- spec/components/guardian_spec.rb | 88 +++++++++++++------ .../admin/groups_controller_spec.rb | 8 +- spec/integration/groups_spec.rb | 17 ++-- spec/models/group_spec.rb | 78 +++++++++------- 17 files changed, 248 insertions(+), 117 deletions(-) create mode 100644 db/migrate/20170703144855_add_visibility_level_to_groups.rb 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