Skip to content
Permalink
Browse files

FEATURE: add support for group members visibility level (#8004)

There are 5 visibility levels (similar to group visibility)

public (default)
logged-in users
members only
staff
owners

Admins & group owners always have visibility to group members.
  • Loading branch information...
vinothkannans committed Aug 14, 2019
1 parent f4aa609 commit 88359b0f167a16b9eb59a9315ceefa84e4c7ec7c
@@ -29,7 +29,7 @@ export default Ember.Controller.extend({
this.set("loading", true);
const model = this.model;

if (model) {
if (model && model.can_see_members) {
model.findMembers(this.memberParams).finally(() => {
this.set(
"application.showFooter",
@@ -162,6 +162,7 @@ const Group = RestModel.extend({
mentionable_level: this.mentionable_level,
messageable_level: this.messageable_level,
visibility_level: this.visibility_level,
members_visibility_level: this.members_visibility_level,
automatic_membership_email_domains: this.emailDomains,
automatic_membership_retroactive: !!this.automatic_membership_retroactive,
title: this.title,
@@ -1,5 +1,10 @@
export default Ember.Route.extend({
beforeModel: function() {
this.transitionTo("group.activity.posts");
beforeModel() {
const group = this.modelFor("group");
if (group.can_see_members) {
this.transitionTo("group.activity.posts");
} else {
this.transitionTo("group.activity.mentions");
}
}
});
@@ -14,6 +14,21 @@
{{i18n 'admin.groups.manage.interaction.visibility_levels.description'}}
</div>
</div>

<div class="control-group">
<label for="visiblity">{{i18n 'admin.groups.manage.interaction.members_visibility_levels.title'}}</label>

{{combo-box name="alias"
valueAttribute="value"
value=model.members_visibility_level
content=visibilityLevelOptions
castInteger=true
class="groups-form-members-visibility-level"}}

<div class="control-instructions">
{{i18n 'admin.groups.manage.interaction.members_visibility_levels.description'}}
</div>
</div>
{{/if}}

<div class="control-group">
@@ -1,10 +1,12 @@
<section class="user-content">

<div class="group-members-actions">
{{text-field value=filterInput
placeholderKey=filterPlaceholder
autocomplete="discourse"
class="group-username-filter no-blur"}}
{{#if model.can_see_members}}
{{text-field value=filterInput
placeholderKey=filterPlaceholder
autocomplete="discourse"
class="group-username-filter no-blur"}}
{{/if}}

<div class="group-members-manage">
{{#if canManageGroup}}
@@ -76,9 +78,13 @@
{{/load-more}}

{{conditional-loading-spinner condition=loading}}
{{else}}
{{else if model.can_see_members}}
<br>

<div>{{i18n "groups.empty.members"}}</div>
{{else}}
<br>

<div>{{i18n "groups.members.forbidden"}}</div>
{{/if}}
</section>
</section>
@@ -1,7 +1,9 @@
<section class="user-secondary-navigation">
{{#mobile-nav class='activity-nav' desktopClass='action-list activity-list nav-stacked' currentPath=router._router.currentPath}}
{{group-activity-filter filter="posts" categoryId=category_id}}
{{group-activity-filter filter="topics" categoryId=category_id}}
{{#if model.can_see_members}}
{{group-activity-filter filter="posts" categoryId=category_id}}
{{group-activity-filter filter="topics" categoryId=category_id}}
{{/if}}
{{#if siteSettings.enable_mentions}}
{{group-activity-filter filter="mentions" categoryId=category_id}}
{{/if}}
@@ -135,6 +135,7 @@ def group_params
:mentionable_level,
:messageable_level,
:visibility_level,
:members_visibility_level,
:automatic_membership_email_domains,
:automatic_membership_retroactive,
:title,
@@ -159,6 +159,8 @@ def update

def posts
group = find_group(:group_id)
guardian.ensure_can_see_group_members!(group)

posts = group.posts_for(
guardian,
params.permit(:before_post_id, :category_id)
@@ -168,6 +170,8 @@ def posts

def posts_feed
group = find_group(:group_id)
guardian.ensure_can_see_group_members!(group)

@posts = group.posts_for(
guardian,
params.permit(:before_post_id, :category_id)
@@ -204,6 +208,8 @@ def mentions_feed
def members
group = find_group(:group_id)

guardian.ensure_can_see_group_members!(group)

limit = (params[:limit] || 20).to_i
offset = params[:offset].to_i

@@ -542,6 +548,7 @@ def group_params(automatic: false)
:incoming_email,
:primary_group,
:visibility_level,
:members_visibility_level,
:name,
:grant_trust_level,
:automatic_membership_email_domains,
@@ -161,6 +161,7 @@ def group_topics
group = Group.find_by(name: params[:group_name])
raise Discourse::NotFound unless group
guardian.ensure_can_see_group!(group)
guardian.ensure_can_see_group_members!(group)

list_opts = build_topic_list_options
list = generate_list_for("group_topics", group, list_opts)
@@ -153,6 +153,59 @@ def self.visibility_levels
groups
}

scope :members_visible_groups, Proc.new { |user, order, opts|
groups = self.order(order || "name ASC")

if !opts || !opts[:include_everyone]
groups = groups.where("groups.id > 0")
end

unless user&.admin
sql = <<~SQL

This comment has been minimized.

Copy link
@eviltrout

eviltrout Aug 14, 2019

Member

This is quite the SQL statement to add to the UserSerializer - can you confirm that this does not have any performance issues? I suspect things could be much slower with these 5+ queries each time a user is serialized.

This comment has been minimized.

Copy link
@discoursereviewbot

discoursereviewbot Aug 20, 2019

SamSaffron posted:

This adds this to current user serialization

image|653x500

Which mean we run this thing on first load.

It certainly is not free and every paper cut counts. It does not fire on subsequent topic visits or topic list visits. It is not a major perf regression.

I am a bit uneasy in the change of semantics here. A group may be visible to current user, only members are not visible.

I would like this particular change reverted and a different approach taken.

  def groups
    object.groups.order(:id)
+     .visible_groups(scope.user)
-     .visible_groups(scope.user).members_visible_groups(scope.user)
  end

This comment has been minimized.

Copy link
@discoursereviewbot

discoursereviewbot Aug 20, 2019

Vinoth Kannan posted:

[quote="SamSaffron, post:5, topic:5215"]
I am a bit uneasy in the change of semantics here. A group may be visible to current user, only members are not visible.
[/quote]

In every user profile pages it is loading the list of groups where that user have membership. I'm hiding it to not reveal the membership.

This comment has been minimized.

Copy link
@discoursereviewbot

discoursereviewbot Aug 20, 2019

SamSaffron posted:

Oh I see, site serializer is issuing this:

def groups
Group.visible_groups(@guardian.user, "name ASC", include_everyone: true)
end

I am still confused, why does current user serializer need the same list again?

def groups
object.visible_groups.pluck(:id, :name).map { |id, name| { id: id, name: name.downcase } }
end

Seems to me like we are doubling up on info here on first load?

groups.id IN (
SELECT g.id FROM groups g WHERE g.members_visibility_level = :public
UNION ALL
SELECT g.id FROM groups g
WHERE g.members_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
WHERE g.members_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.members_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.members_visibility_level = :owners
)
SQL

groups = groups.where(
sql,
Group.visibility_levels.to_h.merge(user_id: user&.id, is_staff: !!user&.staff?)
)

end

groups
}

scope :mentionable, lambda { |user|
where(self.mentionable_sql_clause,
levels: alias_levels(user),
@@ -828,6 +881,7 @@ def enqueue_update_mentions_job
# membership_request_template :text
# messageable_level :integer default(0)
# mentionable_level :integer default(0)
# members_visibility_level :integer default(0), not null
#
# Indexes
#
@@ -29,7 +29,9 @@ class BasicGroupSerializer < ApplicationSerializer
:default_notification_level,
:membership_request_template,
:is_group_user,
:is_group_owner
:is_group_owner,
:members_visibility_level,
:can_see_members

def include_display_name?
object.automatic
@@ -81,6 +83,10 @@ def is_group_owner
owner_group_ids.include?(object.id)
end

def can_see_members
scope.can_see_group_members?(object)
end

private

def staff?
@@ -143,7 +143,7 @@ def mailing_list_posts_per_day

def groups
object.groups.order(:id)
.visible_groups(scope.user)
.visible_groups(scope.user).members_visible_groups(scope.user)
end

def group_users
@@ -651,6 +651,7 @@ en:
remove_owner: "Remove as Owner"
remove_owner_description: "Remove <b>%{username}</b> as an owner of this group"
owner: "Owner"
forbidden: "You're not allowed to view the members."
topics: "Topics"
posts: "Posts"
mentions: "Mentions"
@@ -3211,6 +3212,9 @@ en:
staff: "Group owners and staff"
owners: "Group owners"
description: "Admins can see all groups."
members_visibility_levels:
title: "Who can see this group members?"
description: "Admins can see members of all groups."

membership:
automatic: Automatic
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class AddMembersVisibilityLevelToGroups < ActiveRecord::Migration[5.2]
def change
add_column :groups, :members_visibility_level, :integer, default: 0, null: false
end
end
@@ -209,6 +209,26 @@ def can_see_group?(group)
true
end

def can_see_group_members?(group)
return false if group.blank?
return true if group.members_visibility_level == Group.visibility_levels[:public]
return true if is_admin?
return true if is_staff? && group.members_visibility_level == Group.visibility_levels[:staff]
return true if authenticated? && group.members_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)

return false unless membership

if !membership.owner
return false if group.members_visibility_level == Group.visibility_levels[:owners]
return false if group.members_visibility_level == Group.visibility_levels[:staff]
end

true
end

def can_see_groups?(groups)
return false if groups.blank?
return true if groups.all? { |g| g.visibility_level == Group.visibility_levels[:public] }
@@ -3048,6 +3048,90 @@

end

describe(:can_see_group_members) do
it 'Correctly handles group members visibility for owner' do
group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:owners])

member = Fabricate(:user)
group.add(member)
group.save!

owner = Fabricate(:user)
group.add_owner(owner)
group.reload

expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(false)
expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(false)
expect(Guardian.new(member).can_see_group_members?(group)).to eq(false)
expect(Guardian.new.can_see_group_members?(group)).to eq(false)
expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
end

it 'Correctly handles group members visibility for staff' do
group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:staff])

member = Fabricate(:user)
group.add(member)
group.save!

owner = Fabricate(:user)
group.add_owner(owner)
group.reload

expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(false)
expect(Guardian.new(member).can_see_group_members?(group)).to eq(false)
expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
expect(Guardian.new.can_see_group_members?(group)).to eq(false)
end

it 'Correctly handles group members visibility for member' do
group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:members])

member = Fabricate(:user)
group.add(member)
group.save!

owner = Fabricate(:user)
group.add_owner(owner)
group.reload

expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(false)
expect(Guardian.new.can_see_group_members?(group)).to eq(false)
expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(false)
expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(member).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
end

it 'Correctly handles group members visibility for logged-on-user' do
group = Group.new(name: 'group', members_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_members?(group)).to eq(false)
expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(member).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(true)
end

it 'Correctly handles group members visibility for public' do
group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:public])

expect(Guardian.new.can_see_group_members?(group)).to eq(true)
end

end

describe '#can_see_groups?' do
it 'correctly handles owner visible groups' do
group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:owners])

4 comments on commit 88359b0

@discoursebot

This comment has been minimized.

Copy link

replied Aug 14, 2019

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/semi-private-groups/116594/15

@discoursereviewbot

This comment has been minimized.

Copy link

replied Aug 20, 2019

Vinoth Kannan posted:

[quote="SamSaffron, post:5, topic:5215"]
This adds this to current user serialization
[/quote]

No, current user serialization have different visibility_level scope only (which is similar to this one).

@discoursereviewbot

This comment has been minimized.

Copy link

replied Aug 20, 2019

Vinoth Kannan posted:

98d09c9#diff-4676c008b11a5480d73d4a6de01e45b9R356-R358

As per the above commit groups in current_user_serializer is list of visible groups where the user have membership. But the groups in site_serializer is list of all visible groups in the site.

@discoursereviewbot

This comment has been minimized.

Copy link

replied Aug 20, 2019

SamSaffron posted:

Ahhh I see why we have the 2 queries.

I don't think we need to do anymore here.

Please sign in to comment.
You can’t perform that action at this time.