Handle case where user is invited to group but previously was a member (

#10471)

* Handle case where user is invited to group but previously was a member

* remove vestigial memberships when user accepting is not user invited

* only accept pending invitations

* fix for spec testing inviting existing member

* move the deletion of unused membership records further up the method

app/services/membership_service.rb

@@ -10,48 +10,52 @@ def self.redeem(membership:, actor:, notify: true)
     # and we dont want any surprises if they already have some memberships.
     # they may be accepting memberships send to a different email (unverified_user)
 
-    group_ids = membership.group.parent_or_self.id_and_subgroup_ids
+    invited_group_id = membership.group_id
+    existing_group_ids = Membership.where(user_id: actor.id).pluck(:group_id)
+    existing_accepted_group_ids = Membership.active.accepted.where(user_id: actor.id).pluck(:group_id)
+    invited_group_ids = Membership.pending.where(user_id: membership.user_id, group_id: membership.group.parent_or_self.id_and_subgroup_ids).pluck(:group_id)
+
+    # unrevoke any memberships the actor was just invited to
+    Membership.revoked
+    .where(user_id: actor.id, group_id: invited_group_ids)
+    .update(revoked_at: nil, revoker_id: nil, inviter_id: membership.inviter_id, accepted_at: DateTime.now)
 
     # ensure actor has accepted any existing pending memberships to this group
     Membership.pending
-    .where(user_id: actor.id, group_id: group_ids)
-    .update_all(accepted_at: DateTime.now)
-
-    # cant accept pending memberships to groups I already belong to
-    existing_group_ids = Membership.pending.where(user_id: membership.user_id,
-                                                  group_id: actor.memberships.accepted.pluck(:group_id)).pluck(:group_id)
+    .where(user_id: actor.id, group_id: invited_group_ids)
+    .update(accepted_at: DateTime.now)
 
-    Membership.pending.where(
-      user_id: membership.user_id,
-      group_id: (group_ids - existing_group_ids))
-    .update_all(
-      user_id: actor.id,
-      accepted_at: DateTime.now)
+    Membership.pending
+    .where(user_id: membership.user_id, group_id: (invited_group_ids - existing_group_ids))
+    .update(user_id: actor.id, accepted_at: DateTime.now)
 
-    member_group_ids = actor.group_ids & membership.group.parent_or_self.id_and_subgroup_ids
+    if (membership.user_id != actor.id)
+      Membership.where(user_id: membership.user_id, group_id: invited_group_ids).destroy_all
+    end
 
-    group_ids.each do |group_id|
+    invited_group_ids.each do |group_id|
       GenericWorker.perform_async('PollService', 'group_members_added', group_id)
     end
 
     # remove any existing guest access in these groups
     DiscussionReader.joins(:discussion)
-    .where(user_id: actor.id, 'discussions.group_id': member_group_ids, guest: true)
+    .where(user_id: actor.id, 'discussions.group_id': invited_group_ids, guest: true)
     .update_all(guest: false, revoked_at: nil, revoker_id: nil)
 
     Stance.joins(:poll)
-    .where(participant_id: actor.id, 'polls.group_id': member_group_ids)
+    .where(participant_id: actor.id, 'polls.group_id': invited_group_ids)
     .update_all(guest: false)
 
     # unrevoke any votes on active polls
     Stance.joins(:poll)
     .where(participant_id: actor.id)
-    .where('polls.group_id': member_group_ids)
+    .where('polls.group_id': invited_group_ids)
     .where('stances.revoked_at is not null')
     .where('polls.closed_at is null')
     .update_all(revoked_at: nil, revoker_id: nil)
 
-    membership.reload
+    return if existing_accepted_group_ids.include?(invited_group_id)
+    membership = Membership.find_by!(group_id: invited_group_id, user_id: actor.id)
     Events::InvitationAccepted.publish!(membership) if notify && membership.accepted_at
   end
 

spec/controllers/memberships_controller_spec.rb

@@ -44,9 +44,7 @@
       expect(membership.accepted_at).to eq nil
       expect {get :consume}.to_not change { Event.count }
       expect(response.status).to eq 200
-      membership.reload
-      expect(membership.accepted_at).to eq nil
-      expect(membership.user_id).to_not eq user.id
+      expect(Membership.where(id: membership.id).exists?).to be false
     end
 
     it "does not redeem accepted membership" do

spec/services/membership_service_spec.rb

@@ -40,10 +40,49 @@
     let!(:another_subgroup) { create :group, parent: group }
     let!(:discussion) { create :discussion, group: group, private: false }
     let!(:poll) { create :poll, group: group }
+    let(:first_inviter) { create :user }
+    let(:second_inviter) { create :user }
+    let(:yesterday) { 1.day.ago }
+    let(:today) { DateTime.now }
+
+    before do
+      group.add_admin! first_inviter
+      group.add_admin! second_inviter
+    end
 
     it 'sets accepted_at' do
       MembershipService.redeem(membership: membership, actor: user)
-      membership.accepted_at.should be_present
+      membership.reload.accepted_at.should be_present
+    end
+
+    it "handles simple case" do
+      new_membership = Membership.create!(user_id: user.id, group_id: group.id, inviter_id: first_inviter.id)
+      MembershipService.redeem(membership: new_membership, actor: user)
+      expect(new_membership.reload.user_id).to eq user.id
+      expect(new_membership.reload.accepted_at).to be_present
+      expect(new_membership.reload.inviter_id).to eq first_inviter.id
+      expect(new_membership.reload.revoked_at).to_not be_present
+    end
+
+    it "handles existing memberships" do
+      existing_membership = Membership.create!(user_id: user.id, group_id: group.id, accepted_at: yesterday, inviter_id: first_inviter.id)
+      new_membership = Membership.create!(user_id: unverified_user.id, group_id: group.id, inviter_id: second_inviter.id)
+      MembershipService.redeem(membership: new_membership, actor: user)
+      expect(existing_membership.reload.user_id).to eq user.id
+      expect(existing_membership.reload.accepted_at).to_not be yesterday
+      expect(existing_membership.reload.accepted_at).to be_present
+      expect(existing_membership.reload.inviter_id).to eq first_inviter.id
+      expect(existing_membership.reload.revoked_at).to_not be_present
+    end
+
+    it "handles revoked memberships" do
+      existing_membership = Membership.create!(user_id: user.id, group_id: group.id, accepted_at: DateTime.now, revoked_at: DateTime.now, inviter_id: first_inviter.id, revoker_id: first_inviter.id)
+      new_membership = Membership.create!(user_id: unverified_user.id, group_id: group.id, inviter_id: second_inviter.id)
+      MembershipService.redeem(membership: new_membership, actor: user)
+      expect(existing_membership.reload.user_id).to eq user.id
+      expect(existing_membership.reload.inviter_id).to eq second_inviter.id
+      expect(existing_membership.reload.accepted_at).to be_present
+      expect(existing_membership.reload.revoked_at).to_not be_present
     end
 
     it 'unrevokes discussion readers and stances' do