From 9e4ed03b8fad4ef38f8b4dd55e604151cb9558d4 Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Fri, 11 Sep 2020 08:20:13 +0530 Subject: [PATCH] FEATURE: moderators allowed to view groups which members can see. Currently, if a group's visibility is set to "Group owners, members" then the mods can't view those group pages. The same rule is applied for members visibility setting too. This reverts commit 7fc7090. And fixed the spec test fails. --- app/models/group.rb | 114 +++++++++++------------- config/locales/client.en.yml | 4 +- spec/models/group_spec.rb | 4 +- spec/requests/groups_controller_spec.rb | 2 +- 4 files changed, 55 insertions(+), 69 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index e70eab613c7..610e570194c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -121,44 +121,37 @@ class Group < ActiveRecord::Base end if !user&.admin - sql = <<~SQL - groups.id IN ( - SELECT id - FROM groups - WHERE visibility_level = :public + is_staff = !!user&.staff? - UNION ALL + if user.blank? + sql = "groups.visibility_level = :public" + elsif is_staff + sql = "groups.visibility_level IN (:public, :logged_on_users, :members, :staff)" + else + sql = <<~SQL + groups.id IN ( + SELECT id + FROM groups + WHERE visibility_level IN (:public, :logged_on_users) - SELECT id - FROM groups - WHERE visibility_level = :logged_on_users - AND :user_id IS NOT NULL + UNION ALL - 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 - 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 - 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 IN (:staff, :owners) + ) + SQL + end - 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 - - params = Group.visibility_levels.to_h.merge(user_id: user&.id, is_staff: !!user&.staff?) + params = Group.visibility_levels.to_h.merge(user_id: user&.id, is_staff: is_staff) groups = groups.where(sql, params) end @@ -173,44 +166,37 @@ class Group < ActiveRecord::Base end if !user&.admin - sql = <<~SQL - groups.id IN ( - SELECT id - FROM groups - WHERE members_visibility_level = :public + is_staff = !!user&.staff? - UNION ALL + if user.blank? + sql = "groups.members_visibility_level = :public" + elsif is_staff + sql = "groups.members_visibility_level IN (:public, :logged_on_users, :members, :staff)" + else + sql = <<~SQL + groups.id IN ( + SELECT id + FROM groups + WHERE members_visibility_level IN (:public, :logged_on_users) - SELECT id - FROM groups - WHERE members_visibility_level = :logged_on_users - AND :user_id IS NOT NULL + UNION ALL - 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 - 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 - 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 IN (:staff, :owners) + ) + SQL + end - 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 - - params = Group.visibility_levels.to_h.merge(user_id: user&.id, is_staff: !!user&.staff?) + params = Group.visibility_levels.to_h.merge(user_id: user&.id, is_staff: is_staff) groups = groups.where(sql, params) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a51ccb95e0c..e0c002e456b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3588,8 +3588,8 @@ en: title: "Who can see this group?" public: "Everyone" logged_on_users: "Logged on users" - members: "Group owners, members" - staff: "Group owners and staff" + members: "Group owners, members and moderators" + staff: "Group owners and moderators" owners: "Group owners" description: "Admins can see all groups." members_visibility_levels: diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index dad0ef6dad1..0e501d5360a 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -698,7 +698,7 @@ describe Group do 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?(moderator, group)).to eq(true) 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) @@ -763,7 +763,7 @@ describe Group do 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?(moderator, group)).to eq(true) 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) diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index e8269fc6959..a2d7a09421c 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -248,7 +248,7 @@ describe GroupsController do expect(response.status).to eq(200) group_names = response.parsed_body["groups"].map { |g| g["name"] } - expect(group_names).to contain_exactly("0_0", "0_1", "0_3", "1_0", "1_1", "1_3", "3_0", "3_1", "3_3") + expect(group_names).to contain_exactly("0_0", "0_1", "0_2", "0_3", "1_0", "1_1", "1_2", "1_3", "2_0", "2_1", "2_2", "2_3", "3_0", "3_1", "3_2", "3_3") # admin sign_in(admin)