Querying Memberships Fix: ensure distinct memberships when querying f…

…or 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 <[email protected]> * Update spec/policies/membership_policy_spec.rb Co-authored-by: Zach Wolfenbarger <[email protected]> * update membership policy spec spacing format when expecting a long list of memberships --------- Co-authored-by: Zach Wolfenbarger <[email protected]>
zooniverse · May 20, 2024 · 2bc2583 · 2bc2583
1 parent 7db6beb
commit 2bc2583
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 35 deletions.
diff --git 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
@@ -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