From 2bc258375caaaecc3c61a96395455664e3adef13 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Mon, 20 May 2024 10:12:29 -0500 Subject: [PATCH] Querying Memberships Fix: ensure distinct memberships when querying for ones own public memberships (#4333) * ensure unique memberships when querying for ones own public memberships * update membership policy spec with clearer variable names and spec clean up * hound remove extra spacing * Update spec/policies/membership_policy_spec.rb Co-authored-by: Zach Wolfenbarger * Update spec/policies/membership_policy_spec.rb Co-authored-by: Zach Wolfenbarger * update membership policy spec spacing format when expecting a long list of memberships --------- Co-authored-by: Zach Wolfenbarger --- app/policies/membership_policy.rb | 2 +- spec/policies/membership_policy_spec.rb | 107 ++++++++++++++++-------- 2 files changed, 74 insertions(+), 35 deletions(-) diff --git a/app/policies/membership_policy.rb b/app/policies/membership_policy.rb index e22c5b3a9..6e46d8842 100644 --- a/app/policies/membership_policy.rb +++ b/app/policies/membership_policy.rb @@ -20,7 +20,7 @@ def resolve(action) when :show, :index public_memberships = scope.where(identity: false, state: Membership.states[:active]) .joins(:user_group).merge(UserGroup.where(private: false)) - Membership.union_all(query, public_memberships) + Membership.union_all(query, public_memberships).distinct else query end diff --git a/spec/policies/membership_policy_spec.rb b/spec/policies/membership_policy_spec.rb index 292d1dff6..6ef0ac79b 100644 --- a/spec/policies/membership_policy_spec.rb +++ b/spec/policies/membership_policy_spec.rb @@ -18,6 +18,11 @@ MembershipPolicy::Scope.new(api_user, Membership) end + let(:group_admin_public_membership) { Membership.where(user_id: group_admin.id, user_group_id: public_user_group.id, state: 'active')[0] } + let(:group_admin_private_membership) { + Membership.where(user_id: group_admin.id, user_group_id: private_user_group.id, state: 'active')[0] + } + context 'an anonymous user' do let(:api_user) { ApiUser.new(anonymous_user) } @@ -31,44 +36,58 @@ context 'a normal user', :aggregate_failures do let(:api_user) { ApiUser.new(logged_in_user) } + let(:other_user) { create(:user) } + let(:other_user_public_membership) { create(:membership, user: other_user, user_group: public_user_group) } + let(:other_user_private_membership) { create(:membership, user: other_user, user_group: private_user_group) } + let(:logged_in_user_public_membership) { create(:membership, user: logged_in_user, user_group: public_user_group) } + let(:logged_in_user_private_membership) { create(:membership, user: logged_in_user, user_group: private_user_group) } it 'can see other people in public groups' do - other_user = create :user - membership1 = create :membership, user: other_user, user_group: public_user_group - membership2 = create :membership, user: other_user, user_group: private_user_group - membership3 = create :membership, user_group: public_user_group, state: :inactive - - expect(scope.resolve(:index)).to include(membership1) - expect(scope.resolve(:show)).to include(membership1) - expect(scope.resolve(:index)).not_to include(membership2) - expect(scope.resolve(:show)).not_to include(membership2) - expect(scope.resolve(:index)).not_to include(membership3) - expect(scope.resolve(:show)).not_to include(membership3) + inactive_membership = create :membership, user_group: public_user_group, state: :inactive + + expect(scope.resolve(:index)).to contain_exactly(other_user_public_membership, group_admin_public_membership) + expect(scope.resolve(:show)).to contain_exactly(other_user_public_membership, group_admin_public_membership) + expect(scope.resolve(:index)).not_to include(other_user_private_membership) + expect(scope.resolve(:show)).not_to include(other_user_private_membership) + expect(scope.resolve(:index)).not_to include(inactive_membership) + expect(scope.resolve(:show)).not_to include(inactive_membership) end it 'can see who are members of private groups they are in' do - other_user = create :user - membership1 = create :membership, user: logged_in_user, user_group: public_user_group - membership2 = create :membership, user: logged_in_user, user_group: private_user_group - membership3 = create :membership, user: other_user, user_group: public_user_group - membership4 = create :membership, user: other_user, user_group: private_user_group - - expect(scope.resolve(:index)).to include(membership1, membership2, membership3, membership4) - expect(scope.resolve(:show)).to include(membership1, membership2, membership3, membership4) + expect(scope.resolve(:index)).to contain_exactly( + logged_in_user_public_membership, + logged_in_user_private_membership, + other_user_public_membership, + other_user_private_membership, + group_admin_public_membership, + group_admin_private_membership + ) + expect(scope.resolve(:show)).to contain_exactly( + logged_in_user_public_membership, + logged_in_user_private_membership, + other_user_public_membership, + other_user_private_membership, + group_admin_public_membership, + group_admin_private_membership + ) end it "cannot modify other user's memberships" do - other_user = create :user - membership1 = create :membership, user: logged_in_user, user_group: public_user_group - membership2 = create :membership, user: logged_in_user, user_group: private_user_group - membership3 = create :membership, user: other_user, user_group: public_user_group - membership4 = create :membership, user: other_user, user_group: private_user_group - - expect(scope.resolve(:update)).to include(membership1, membership2) - expect(scope.resolve(:destroy)).to include(membership1, membership2) - - expect(scope.resolve(:update)).not_to include(membership3, membership4) - expect(scope.resolve(:destroy)).not_to include(membership3, membership4) + expect(scope.resolve(:update)).to include(logged_in_user_public_membership, logged_in_user_private_membership) + expect(scope.resolve(:destroy)).to include(logged_in_user_public_membership, logged_in_user_private_membership) + + expect(scope.resolve(:update)).not_to include( + other_user_private_membership, + other_user_public_membership, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:destroy)).not_to include( + other_user_private_membership, + other_user_public_membership, + group_admin_private_membership, + group_admin_public_membership + ) end end @@ -79,10 +98,30 @@ membership1 = create :membership, user: logged_in_user, user_group: public_user_group membership2 = create :membership, user: logged_in_user, user_group: private_user_group - expect(scope.resolve(:index)).to include(membership1, membership2) - expect(scope.resolve(:show)).to include(membership1, membership2) - expect(scope.resolve(:update)).to include(membership1, membership2) - expect(scope.resolve(:destroy)).to include(membership1, membership2) + expect(scope.resolve(:index)).to contain_exactly( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:show)).to contain_exactly( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:update)).to include( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:destroy)).to include( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) end end end