4 files changed, 163 insertions, 98 deletions

diff --git a/src/mailman/interfaces/preferences.py b/src/mailman/interfaces/preferences.py
index 7191ce93b..024fdea3c 100644
--- a/src/mailman/interfaces/preferences.py
+++ b/src/mailman/interfaces/preferences.py
@@ -80,4 +80,5 @@ class IPreferences(Interface):
 
         :param preferences: The preferences to merge into ourself.
         :type preferences: IPreferences
+        :raises TypeError: if `preferences` isn't a preference.
         """
diff --git a/src/mailman/interfaces/user.py b/src/mailman/interfaces/user.py
index 27a8cbaba..a6c470b6e 100644
--- a/src/mailman/interfaces/user.py
+++ b/src/mailman/interfaces/user.py
@@ -111,7 +111,22 @@ class IUser(Interface):
         """This user's preferences.""")
 
     def absorb(user):
-        """Merge this user's attributes and memberships, and then delete it.
+        """Merge the given user to ourself.
 
-        In case of conflict, the current user's properties are preserved.
+        All IAddresses linked to `user` are relinked to ourself.  A
+        memberships associated with `user` are changed to be memberships
+        with ourself.  See `IPreferences.absorb()`.
+
+        The user's `display_name`, `password`, and `is_server_owner` settings
+        are absorbed into ours, but only if ours is unset and the given user's
+        values are set.
+
+        After being absorbed, the given user and its preferences are
+        deleted.
+
+        It is not an error if `user` is ourself, but it is a no-op.
+
+        :param user: The user to merge into ourself.
+        :type user: IUser
+        :raises TypeError: if `user` is not a user.
         """
diff --git a/src/mailman/model/tests/test_user.py b/src/mailman/model/tests/test_user.py
index 5bb684331..e55267f8f 100644
--- a/src/mailman/model/tests/test_user.py
+++ b/src/mailman/model/tests/test_user.py
@@ -30,7 +30,6 @@ from mailman.interfaces.usermanager import IUserManager
 from mailman.model.preferences import Preferences
 from mailman.testing.helpers import set_preferred
 from mailman.testing.layers import ConfigLayer
-from mailman.utilities.datetime import now
 from sqlalchemy import inspect
 from zope.component import getUtility
 
@@ -124,135 +123,181 @@ class TestUser(unittest.TestCase):
             id=user.preferences.id)
         self.assertEqual(preferences.count(), 0)
 
+    def test_absorb_not_a_user(self):
+        bart = self._manager.create_address('bart@example.com')
+        self.assertRaises(TypeError, self._anne.absorb, bart)
+
     def test_absorb_addresses(self):
-        anne_addr = self._anne.preferred_address
+        # Absorbing the user absorbs all of the users addresses.  I.e. they
+        # are relinked to the absorbing user.
+        anne_preferred = self._anne.preferred_address
         with transaction():
             # This has to happen in a transaction so that both the user and
             # the preferences objects get valid ids.
-            bill = self._manager.create_user(
-                'bill@example.com', 'Bill Person')
-            bill_addr_2 = self._manager.create_address('bill2@example.com')
-            bill.link(bill_addr_2)
-        self._anne.absorb(bill)
-        self.assertIn(
-            'bill@example.com',
-            list(a.email for a in self._anne.addresses))
-        self.assertIn(
-            'bill2@example.com',
-            list(a.email for a in self._anne.addresses))
-        # The preferred address shouldn't change.
-        self.assertEqual(self._anne.preferred_address, anne_addr)
+            bart = self._manager.create_user('bart@example.com', 'Bart Person')
+            bart_secondary = self._manager.create_address(
+                'bart.person@example.com')
+            bart.link(bart_secondary)
+        # Absorb the Bart user into Anne.
+        self._anne.absorb(bart)
+        # Bart's primary and secondary addresses are now linked to Anne.
+        anne_addresses = list(
+            address.email for address in self._anne.addresses)
+        self.assertIn('bart@example.com', anne_addresses)
+        self.assertIn('bart.person@example.com', anne_addresses)
+        # Anne's preferred address shouldn't change.
+        self.assertEqual(self._anne.preferred_address, anne_preferred)
+        # Check the reverse linkages by getting Bart's addresses from the user
+        # manager.  They should both point back to the Anne user.
         self.assertEqual(
-            self._manager.get_user('bill@example.com'), self._anne)
+            self._manager.get_user('bart@example.com'), self._anne)
         self.assertEqual(
-            self._manager.get_user('bill2@example.com'), self._anne)
-        self.assertIsNone(self._manager.get_user_by_id(bill.user_id))
+            self._manager.get_user('bart.person@example.com'), self._anne)
+        # The Bart user has been deleted.
+        self.assertIsNone(self._manager.get_user_by_id(bart.user_id))
 
     def test_absorb_memberships(self):
+        # When a user is absorbed, all of their user-subscribed memberships
+        # are relinked to the absorbing user.
         mlist2 = create_list('test2@example.com')
         mlist3 = create_list('test3@example.com')
         with transaction():
             # This has to happen in a transaction so that both the user and
             # the preferences objects get valid ids.
-            bill = self._manager.create_user(
-                'bill@example.com', 'Bill Person')
-            bill_address = list(bill.addresses)[0]
-            bill_address.verified_on = now()
-            bill.preferred_address = bill_address
+            bart = self._manager.create_user('bart@example.com', 'Bart Person')
+            set_preferred(bart)
         # Subscribe both users to self._mlist.
         self._mlist.subscribe(self._anne, MemberRole.member)
-        self._mlist.subscribe(bill, MemberRole.moderator)
-        # Subscribe only bill to mlist2.
-        mlist2.subscribe(bill, MemberRole.owner)
-        # Subscribe only bill's address to mlist3.
-        mlist3.subscribe(bill.preferred_address, MemberRole.moderator)
+        self._mlist.subscribe(bart, MemberRole.moderator)
+        # Subscribe only Bart to mlist2.
+        mlist2.subscribe(bart, MemberRole.owner)
+        # Subscribe only Bart's address to mlist3.
+        mlist3.subscribe(bart.preferred_address, MemberRole.moderator)
+        # There are now 4 memberships, one with Anne two with Bart's user and
+        # one with Bart's address.
+        all_members = list(self._manager.members)
+        self.assertEqual(len(all_members), 4, all_members)
         # Do the absorption.
-        self._anne.absorb(bill)
-        # Check that bill has been deleted.
-        self.assertEqual(len(list(self._manager.users)), 1)
-        self.assertEqual(list(self._manager.users)[0], self._anne)
-        # Check that there is no leftover membership from user bill.
-        self.assertEqual(len(list(self._manager.members)), 3)
-        # Check that anne is subscribed to all lists.
-        self.assertEqual(self._anne.memberships.member_count, 3)
-        memberships = {}
-        for member in self._anne.memberships.members:
-            memberships[member.list_id] = member
-        self.assertEqual(
-            set(memberships.keys()),
-            set([
-                'test.example.com',
-                'test2.example.com',
-                'test3.example.com',
-                ]))
-        # The subscription to test@example.com already existed, it must not be
-        # overwritten.
-        self.assertEqual(
-            memberships['test.example.com'].role, MemberRole.member)
-        # Check that the subscription roles were imported
+        self._anne.absorb(bart)
+        # The Bart user has been deleted, leaving only the Anne user in the
+        # user manager.
+        all_users = list(self._manager.users)
+        self.assertEqual(len(all_users), 1)
+        self.assertEqual(all_users[0], self._anne)
+        # There are no leftover memberships for user Bart.  Anne owns all the
+        # memberships.
+        all_members = list(self._manager.members)
+        self.assertEqual(len(all_members), 4, all_members)
+        self.assertEqual(self._anne.memberships.member_count, 4)
+        memberships = {(member.list_id, member.role): member
+                       for member in self._anne.memberships.members}
+        # Note that Anne is now both a member and moderator of the test list.
+        self.assertEqual(set(memberships), set([
+            ('test.example.com', MemberRole.member),
+            ('test.example.com', MemberRole.moderator),
+            ('test2.example.com', MemberRole.owner),
+            ('test3.example.com', MemberRole.moderator),
+            ]))
+        # Both of Bart's previous user subscriptions are now transferred to
+        # the Anne user.
         self.assertEqual(
-            memberships['test2.example.com'].role, MemberRole.owner)
-        self.assertEqual(
-            memberships['test3.example.com'].role, MemberRole.moderator)
-        # The user bill was subscribed, the subscription must thus be
-        # transferred to anne's primary address.
-        self.assertEqual(
-            memberships['test2.example.com'].address,
+            memberships[('test.example.com', MemberRole.moderator)].address,
             self._anne.preferred_address)
-        # The address was subscribed, it must not be changed
         self.assertEqual(
-            memberships['test3.example.com'].address.email,
-            'bill@example.com')
+            memberships[('test2.example.com', MemberRole.owner)].address,
+            self._anne.preferred_address)
+        # Bart's address was subscribed; it must not have been changed.  Of
+        # course, Anne now controls bart@example.com.
+        key = ('test3.example.com', MemberRole.moderator)
+        self.assertEqual(memberships[key].address.email, 'bart@example.com')
+        self.assertEqual(self._manager.get_user('bart@example.com'),
+                         self._anne)
+
+    def test_absorb_duplicates(self):
+        # Duplicate memberships, where the list-id and role match, are
+        # ignored.  Here we subscribe Anne to the test list as a member, and
+        # Bart as both a member and an owner.  Anne's member membership
+        # remains unchanged, but she gains an owner membership.
+        with transaction():
+            bart = self._manager.create_user('bart@example.com')
+            set_preferred(bart)
+        self._mlist.subscribe(self._anne, MemberRole.member)
+        self._mlist.subscribe(bart, MemberRole.member)
+        self._mlist.subscribe(bart, MemberRole.owner)
+        # There are now three memberships.
+        all_members = list(self._manager.members)
+        self.assertEqual(len(all_members), 3, all_members)
+        # Do the absorption.
+        self._anne.absorb(bart)
+        # There are now only 2 memberships, both owned by Anne.
+        all_members = list(self._manager.members)
+        self.assertEqual(len(all_members), 2, all_members)
+        memberships = set([
+            (member.list_id, member.role, member.address.email)
+            for member in all_members
+            ])
+        self.assertEqual(memberships, set([
+            ('test.example.com', MemberRole.member, 'anne@example.com'),
+            ('test.example.com', MemberRole.owner, 'anne@example.com'),
+            ]))
 
     def test_absorb_preferences(self):
         with transaction():
             # This has to happen in a transaction so that both the user and
             # the preferences objects get valid ids.
-            bill = self._manager.create_user(
-                'bill@example.com', 'Bill Person')
-        bill.preferences.acknowledge_posts = True
+            bart = self._manager.create_user('bart@example.com', 'Bart Person')
+        bart.preferences.acknowledge_posts = True
         self.assertIsNone(self._anne.preferences.acknowledge_posts)
-        self._anne.absorb(bill)
+        self._anne.absorb(bart)
         self.assertEqual(self._anne.preferences.acknowledge_posts, True)
-        # Check that Bill's preferences were deleted (requires a DB flush).
+        # Check that Bart's preferences were deleted (requires a DB flush).
         config.db.store.flush()
-        self.assertTrue(inspect(bill.preferences).deleted)
+        self.assertTrue(inspect(bart.preferences).deleted)
 
     def test_absorb_properties(self):
-        props = {
+        properties = {
             'password': 'dummy',
             'is_server_owner': True
-        }
+            }
         with transaction():
             # This has to happen in a transaction so that both the user and
             # the preferences objects get valid ids.
-            bill = self._manager.create_user(
-                'bill@example.com', 'Bill Person')
-        for prop, value in props.items():
-            setattr(bill, prop, value)
-        self._anne.absorb(bill)
-        for prop, value in props.items():
-            self.assertEqual(getattr(self._anne, prop), value)
-        # This was not empty so it must not be overwritten
+            bart = self._manager.create_user('bart@example.com', 'Bart Person')
+        for name, value in properties.items():
+            setattr(bart, name, value)
+        self._anne.absorb(bart)
+        for name, value in properties.items():
+            self.assertEqual(getattr(self._anne, name), value)
+        # This was not empty so it must not have been overwritten.
         self.assertEqual(self._anne.display_name, 'Anne Person')
 
+    def test_absorb_display_name(self):
+        # Bart has no display name, but once he absorbs Cate, he gains her
+        # display_name.
+        with transaction():
+            bart = self._manager.create_user('bart@example.com')
+            cate = self._manager.create_user('cate@example.com', 'Cate Person')
+        self.assertEqual(bart.display_name, '')
+        bart.absorb(cate)
+        self.assertEqual(bart.display_name, 'Cate Person')
+
     def test_absorb_delete_user(self):
-        # Make sure the user was deleted
+        # Make sure the user was deleted after being absorbed.
         with transaction():
             # This has to happen in a transaction so that both the user and
             # the preferences objects get valid ids.
-            bill = self._manager.create_user(
-                'bill@example.com', 'Bill Person')
-        bill_user_id = bill.user_id
-        self._anne.absorb(bill)
-        self.assertIsNone(self._manager.get_user_by_id(bill_user_id))
+            bart = self._manager.create_user('bart@example.com', 'Bart Person')
+        bart_user_id = bart.user_id
+        self._anne.absorb(bart)
+        self.assertIsNone(self._manager.get_user_by_id(bart_user_id))
 
     def test_absorb_self(self):
-        # Absorbing oneself should be a no-op (it must not delete the user)
+        # Absorbing oneself should be a no-op (it must not delete the user).
         self._mlist.subscribe(self._anne)
         self._anne.absorb(self._anne)
         new_anne = self._manager.get_user_by_id(self._anne.user_id)
         self.assertIsNotNone(new_anne)
         self.assertEqual(
-            [a.email for a in new_anne.addresses], ['anne@example.com'])
+            [address.email for address in new_anne.addresses],
+            ['anne@example.com'])
         self.assertEqual(new_anne.memberships.member_count, 1)
diff --git a/src/mailman/model/user.py b/src/mailman/model/user.py
index a7d3f2353..a534ef2a5 100644
--- a/src/mailman/model/user.py
+++ b/src/mailman/model/user.py
@@ -179,31 +179,35 @@ class User(Model):
     @dbconnection
     def absorb(self, store, user):
         """See `IUser`."""
-        assert user is not None
+        if not isinstance(user, User):
+            raise TypeError('Not a user {!r}'.format(user))
         if user.id == self.id:
-            return  # Protect against absorbing oneself.
+            # It's a no-op to absorb ourself.
+            return
         # Relink addresses.
         for address in list(user.addresses):
-            # convert to list because we'll mutate the result
+            # Convert these to a list because we'll mutate the result.
             address.user = self
         # Merge memberships.
         other_members = store.query(Member).filter(
             Member.user_id == user.id)
-        subscribed_lists = [m.list_id for m in self.memberships.members]
+        my_subscriptions = set(
+            (member.list_id, member.role)
+            for member in self.memberships.members)
         for member in other_members:
-            # Only import memberships of lists I'm not subscribed to yet,
-            # delete the rest.
-            if member.list_id not in subscribed_lists:
+            # Only import memberships for list/roles I'm not already a member
+            # with.  This prevents duplicate memberships.
+            if (member.list_id, member.role) not in my_subscriptions:
                 member.user_id = self.id
             else:
                 store.delete(member)
-        # Merge the user preferences
+        # Merge the user preferences.
         self.preferences.absorb(user.preferences)
         store.delete(user.preferences)
         # Merge display_name, password and is_server_owner attributes.
-        for prop in ('display_name', 'password', 'is_server_owner'):
-            if getattr(user, prop) and not getattr(self, prop):
-                setattr(self, prop, getattr(user, prop))
+        for name in ('display_name', 'password', 'is_server_owner'):
+            if getattr(user, name) and not getattr(self, name):
+                setattr(self, name, getattr(user, name))
         # Delete the other user.
         store.delete(user)