9 files changed, 125 insertions, 106 deletions

diff --git a/src/mailman/model/docs/domains.rst b/src/mailman/model/docs/domains.rst
index 153f6c19d..abb594a62 100644
--- a/src/mailman/model/docs/domains.rst
+++ b/src/mailman/model/docs/domains.rst
@@ -108,12 +108,7 @@ In the global domain manager, domains are indexed by their email host name.
             base_url: http://lists.example.net,
             contact_address: postmaster@example.com>
 
-    >>> print(manager['doesnotexist.com'])
-    Traceback (most recent call last):
-    ...
-    KeyError: u'doesnotexist.com'
-
-As with a dictionary, you can also get the domain.  If the domain does not
+As with dictionaries, you can also get the domain.  If the domain does not
 exist, ``None`` or a default is returned.
 ::
 
@@ -128,13 +123,6 @@ exist, ``None`` or a default is returned.
     >>> print(manager.get('doesnotexist.com', 'blahdeblah'))
     blahdeblah
 
-Non-existent domains cannot be removed.
-
-    >>> manager.remove('doesnotexist.com')
-    Traceback (most recent call last):
-    ...
-    KeyError: u'doesnotexist.com'
-
 
 Confirmation tokens
 ===================
diff --git a/src/mailman/model/docs/mailinglist.rst b/src/mailman/model/docs/mailinglist.rst
index 3d01710c5..00a01662b 100644
--- a/src/mailman/model/docs/mailinglist.rst
+++ b/src/mailman/model/docs/mailinglist.rst
@@ -114,7 +114,8 @@ Subscribing users
 An alternative way of subscribing to a mailing list is as a user with a
 preferred address.  This way the user can change their subscription address
 just by changing their preferred address.
-::
+
+The user must have a preferred address.
 
     >>> from mailman.utilities.datetime import now
     >>> user = user_manager.create_user('dperson@example.com', 'Dave Person')
@@ -122,6 +123,8 @@ just by changing their preferred address.
     >>> address.verified_on = now()
     >>> user.preferred_address = address
 
+The preferred address is used in the subscription.
+
     >>> mlist.subscribe(user)
     <Member: Dave Person <dperson@example.com> on aardvark@example.com
              as MemberRole.member>
@@ -132,6 +135,10 @@ just by changing their preferred address.
     <Member: Dave Person <dperson@example.com> on aardvark@example.com
              as MemberRole.member>
 
+If the user's preferred address changes, their subscribed email address also
+changes automatically.
+::
+
     >>> new_address = user.register('dave.person@example.com')
     >>> new_address.verified_on = now()
     >>> user.preferred_address = new_address
@@ -143,31 +150,12 @@ just by changing their preferred address.
     <Member: dave.person@example.com on aardvark@example.com
              as MemberRole.member>
 
-A user is not allowed to subscribe more than once to the mailing list.
-
-    >>> mlist.subscribe(user)
-    Traceback (most recent call last):
-    ...
-    AlreadySubscribedError: <User "Dave Person" (1) at ...>
-    is already a MemberRole.member of mailing list aardvark@example.com
-
-However, they are allowed to subscribe again with a specific address, even if
-this address is their preferred address.
+A user is allowed to explicitly subscribe again with a specific address, even
+if this address is their preferred address.
 
     >>> mlist.subscribe(user.preferred_address)
     <Member: dave.person@example.com
              on aardvark@example.com as MemberRole.member>
 
-A user cannot subscribe to a mailing list without a preferred address.
-
-    >>> user = user_manager.create_user('eperson@example.com', 'Elly Person')
-    >>> address = list(user.addresses)[0]
-    >>> address.verified_on = now()
-    >>> mlist.subscribe(user)
-    Traceback (most recent call last):
-    ...
-    MissingPreferredAddressError: User must have a preferred address:
-    <User "Elly Person" (2) at ...>
-
 
 .. _`RFC 2369`: http://www.faqs.org/rfcs/rfc2369.html
diff --git a/src/mailman/model/docs/usermanager.rst b/src/mailman/model/docs/usermanager.rst
index 9a8c35c00..1e46aafca 100644
--- a/src/mailman/model/docs/usermanager.rst
+++ b/src/mailman/model/docs/usermanager.rst
@@ -46,7 +46,7 @@ A user can be assigned a password.
 
     >>> user.password = b'secret'
     >>> dump_list(user.password for user in user_manager.users)
-    secret
+    b'secret'
 
 You can also create a user with an address to start out with.
 
diff --git a/src/mailman/model/docs/users.rst b/src/mailman/model/docs/users.rst
index 2e7333944..d0725598c 100644
--- a/src/mailman/model/docs/users.rst
+++ b/src/mailman/model/docs/users.rst
@@ -25,7 +25,7 @@ Users may have a real name and a password.
     >>> dump_list(user.display_name for user in user_manager.users)
     Zoe Person
     >>> dump_list(user.password for user in user_manager.users)
-    my password
+    b'my password'
 
 The password and real name can be changed at any time.
 
@@ -34,7 +34,7 @@ The password and real name can be changed at any time.
     >>> dump_list(user.display_name for user in user_manager.users)
     Zoe X. Person
     >>> dump_list(user.password for user in user_manager.users)
-    another password
+    b'another password'
 
 When the user's password is changed, an event is triggered.
 
@@ -53,26 +53,19 @@ The event holds a reference to the `IUser` that changed their password.
     >>> print(saved_event.user.display_name)
     Zoe X. Person
     >>> print(saved_event.user.password)
-    changed again
+    b'changed again'
 
 
 Basic user identification
 =========================
 
-Although rarely visible to users, every user has a unique ID in Mailman, which
-never changes.  This ID is generated randomly at the time the user is created,
-and is represented by a UUID.
+Although rarely visible to users, every user has a unique immutable ID.  This
+ID is generated randomly at the time the user is created, and is represented
+by a UUID.
 
     >>> print(user_1.user_id)
     00000000-0000-0000-0000-000000000001
 
-The user id cannot change.
-
-    >>> user_1.user_id = 'foo'
-    Traceback (most recent call last):
-    ...
-    AttributeError: can't set attribute
-
 User records also have a date on which they where created.
 
     # The test suite uses a predictable timestamp.
@@ -84,8 +77,8 @@ Users addresses
 ===============
 
 One of the pieces of information that a user links to is a set of email
-addresses they control, in the form of IAddress objects.  A user can control
-many addresses, but addresses may be controlled by only one user.
+addresses they control, in the form of ``IAddress`` objects.  A user can
+control many addresses, but addresses may be linked to only one user.
 
 The easiest way to link a user to an address is to just register the new
 address on a user object.
@@ -114,14 +107,6 @@ You can also create the address separately and then link it to the user.
     <BLANKLINE>
     Zoe Person
 
-But don't try to link an address to more than one user.
-
-    >>> another_user = user_manager.create_user()
-    >>> another_user.link(address_1)
-    Traceback (most recent call last):
-    ...
-    AddressAlreadyLinkedError: zperson@example.net
-
 You can also ask whether a given user controls a given address.
 
     >>> user_1.controls(address_1.email)
@@ -149,17 +134,6 @@ Addresses can also be unlinked from a user.
     >>> print(user_manager.get_user('aperson@example.net'))
     None
 
-But don't try to unlink the address from a user it's not linked to.
-
-    >>> user_1.unlink(address_1)
-    Traceback (most recent call last):
-    ...
-    AddressNotLinkedError: zperson@example.net
-    >>> another_user.unlink(address_1)
-    Traceback (most recent call last):
-    ...
-    AddressNotLinkedError: zperson@example.net
-
 
 Preferred address
 =================
@@ -183,20 +157,10 @@ preferred address.
     >>> print(user_2.preferred_address)
     None
 
-The preferred address must be explicitly registered, however only verified
-address may be registered as preferred.
-
-    >>> anne
-    <Address: Anne Person <anne@example.com> [not verified] at ...>
-    >>> user_2.preferred_address = anne
-    Traceback (most recent call last):
-    ...
-    UnverifiedAddressError: Anne Person <anne@example.com>
-
-Once the address has been verified though, it can be set as the preferred
-address, but only if the address is either controlled by the user or
-uncontrolled.  In the latter case, setting it as the preferred address makes
-it controlled by the user.
+Once the address has been verified, it can be set as the preferred address,
+but only if the address is either controlled by the user or uncontrolled.  In
+the latter case, setting it as the preferred address makes it controlled by
+the user.
 ::
 
     >>> from mailman.utilities.datetime import now
@@ -217,17 +181,6 @@ it controlled by the user.
     >>> user_2.controls(aperson.email)
     True
 
-    >>> zperson = user_manager.get_address('zperson@example.com')
-    >>> zperson.verified_on = now()
-    >>> user_2.controls(zperson.email)
-    False
-    >>> user_1.controls(zperson.email)
-    True
-    >>> user_2.preferred_address = zperson
-    Traceback (most recent call last):
-    ...
-    AddressAlreadyLinkedError: Zoe Person <zperson@example.com>
-
 A user can disavow their preferred address.
 
     >>> user_2.preferred_address
@@ -328,11 +281,11 @@ membership role.
     >>> from zope.interface.verify import verifyObject
     >>> verifyObject(IRoster, memberships)
     True
-    >>> members = sorted(memberships.members)
-    >>> len(members)
-    4
     >>> def sortkey(member):
     ...     return member.address.email, member.mailing_list, member.role.value
+    >>> members = sorted(memberships.members, key=sortkey)
+    >>> len(members)
+    4
     >>> for member in sorted(members, key=sortkey):
     ...     print(member.address.email, member.mailing_list.list_id,
     ...           member.role)
diff --git a/src/mailman/model/mailinglist.py b/src/mailman/model/mailinglist.py
index c55786fe8..8e42bb172 100644
--- a/src/mailman/model/mailinglist.py
+++ b/src/mailman/model/mailinglist.py
@@ -481,7 +481,9 @@ class MailingList(Model):
                 Member._user == subscriber).first()
             if member:
                 raise AlreadySubscribedError(
-                    self.fqdn_listname, subscriber, role)
+                    self.fqdn_listname,
+                    subscriber.preferred_address.email,
+                    role)
         else:
             raise ValueError('subscriber must be an address or user')
         member = Member(role=role,
diff --git a/src/mailman/model/tests/test_domain.py b/src/mailman/model/tests/test_domain.py
index f9d1ff202..88e2fe312 100644
--- a/src/mailman/model/tests/test_domain.py
+++ b/src/mailman/model/tests/test_domain.py
@@ -45,6 +45,7 @@ class TestDomainManager(unittest.TestCase):
 
     def setUp(self):
         self._events = []
+        self._manager = getUtility(IDomainManager)
 
     def _record_event(self, event):
         self._events.append(event)
@@ -53,7 +54,7 @@ class TestDomainManager(unittest.TestCase):
         # Test that creating a domain in the domain manager propagates the
         # expected events.
         with event_subscribers(self._record_event):
-            domain = getUtility(IDomainManager).add('example.org')
+            domain = self._manager.add('example.org')
         self.assertEqual(len(self._events), 2)
         self.assertTrue(isinstance(self._events[0], DomainCreatingEvent))
         self.assertEqual(self._events[0].mail_host, 'example.org')
@@ -63,15 +64,24 @@ class TestDomainManager(unittest.TestCase):
     def test_delete_domain_event(self):
         # Test that deleting a domain in the domain manager propagates the
         # expected event.
-        domain = getUtility(IDomainManager).add('example.org')
+        domain = self._manager.add('example.org')
         with event_subscribers(self._record_event):
-            getUtility(IDomainManager).remove('example.org')
+            self._manager.remove('example.org')
         self.assertEqual(len(self._events), 2)
         self.assertTrue(isinstance(self._events[0], DomainDeletingEvent))
         self.assertEqual(self._events[0].domain, domain)
         self.assertTrue(isinstance(self._events[1], DomainDeletedEvent))
         self.assertEqual(self._events[1].mail_host, 'example.org')
 
+    def test_lookup_missing_domain(self):
+        # Like dictionaries, getitem syntax raises KeyError on missing domain.
+        with self.assertRaises(KeyError):
+            self._manager['doesnotexist.com']
+
+    def test_delete_missing_domain(self):
+        # Trying to delete a missing domain gives you a KeyError.
+        self.assertRaises(KeyError, self._manager.remove, 'doesnotexist.com')
+
 
 
 class TestDomainLifecycleEvents(unittest.TestCase):
diff --git a/src/mailman/model/tests/test_mailinglist.py b/src/mailman/model/tests/test_mailinglist.py
index 9d6177b54..2fd000422 100644
--- a/src/mailman/model/tests/test_mailinglist.py
+++ b/src/mailman/model/tests/test_mailinglist.py
@@ -21,8 +21,9 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 __all__ = [
-    'TestListArchiver',
     'TestDisabledListArchiver',
+    'TestListArchiver',
+    'TestMailingList',
     ]
 
 
@@ -31,8 +32,47 @@ import unittest
 from mailman.app.lifecycle import create_list
 from mailman.config import config
 from mailman.interfaces.mailinglist import IListArchiverSet
+from mailman.interfaces.member import (
+    AlreadySubscribedError, MemberRole, MissingPreferredAddressError)
+from mailman.interfaces.usermanager import IUserManager
 from mailman.testing.helpers import configuration
 from mailman.testing.layers import ConfigLayer
+from mailman.utilities.datetime import now
+from zope.component import getUtility
+
+
+
+class TestMailingList(unittest.TestCase):
+    layer = ConfigLayer
+
+    def setUp(self):
+        self._mlist = create_list('ant@example.com')
+
+    def test_no_duplicate_subscriptions(self):
+        # A user is not allowed to subscribe more than once to the mailing
+        # list with the same role.
+        anne = getUtility(IUserManager).create_user('anne@example.com')
+        # Give the user a preferred address.
+        preferred = list(anne.addresses)[0]
+        preferred.verified_on = now()
+        anne.preferred_address = preferred
+        # Subscribe Anne to the mailing list as a regular member.
+        member = self._mlist.subscribe(anne)
+        self.assertEqual(member.address, preferred)
+        self.assertEqual(member.role, MemberRole.member)
+        # A second subscription with the same role will fail.
+        with self.assertRaises(AlreadySubscribedError) as cm:
+            self._mlist.subscribe(anne)
+        self.assertEqual(cm.exception.fqdn_listname, 'ant@example.com')
+        self.assertEqual(cm.exception.email, 'anne@example.com')
+        self.assertEqual(cm.exception.role, MemberRole.member)
+
+    def test_subscribing_user_must_have_preferred_address(self):
+        # A user object cannot be subscribed to a mailing list without a
+        # preferred address.
+        anne = getUtility(IUserManager).create_user('anne@example.com')
+        self.assertRaises(MissingPreferredAddressError,
+                          self._mlist.subscribe, anne)
 
 
 
diff --git a/src/mailman/model/tests/test_user.py b/src/mailman/model/tests/test_user.py
index 17d4d24ff..3ae6203ab 100644
--- a/src/mailman/model/tests/test_user.py
+++ b/src/mailman/model/tests/test_user.py
@@ -30,6 +30,9 @@ import unittest
 from zope.component import getUtility
 
 from mailman.app.lifecycle import create_list
+from mailman.interfaces.address import (
+    AddressAlreadyLinkedError, AddressNotLinkedError)
+from mailman.interfaces.user import UnverifiedAddressError
 from mailman.interfaces.usermanager import IUserManager
 from mailman.testing.layers import ConfigLayer
 from mailman.utilities.datetime import now
@@ -74,3 +77,38 @@ class TestUser(unittest.TestCase):
         self.assertEqual(len(emails), 2)
         self.assertEqual(emails,
                          set(['anne@example.com', 'aperson@example.com']))
+
+    def test_uid_is_immutable(self):
+        with self.assertRaises(AttributeError):
+            self._anne.user_id = 'foo'
+
+    def test_addresses_may_only_be_linked_to_one_user(self):
+        user = getUtility(IUserManager).create_user()
+        # Anne's preferred address is already linked to her.
+        with self.assertRaises(AddressAlreadyLinkedError) as cm:
+            user.link(self._anne.preferred_address)
+        self.assertEqual(cm.exception.address, self._anne.preferred_address)
+
+    def test_unlink_from_address_not_linked_to(self):
+        # You cannot unlink an address from a user if that address is not
+        # already linked to the user.
+        user = getUtility(IUserManager).create_user()
+        with self.assertRaises(AddressNotLinkedError) as cm:
+            user.unlink(self._anne.preferred_address)
+        self.assertEqual(cm.exception.address, self._anne.preferred_address)
+
+    def test_unlink_address_which_is_not_linked(self):
+        # You cannot unlink an address which is not linked to any user.
+        address = getUtility(IUserManager).create_address('bart@example.com')
+        user = getUtility(IUserManager).create_user()
+        with self.assertRaises(AddressNotLinkedError) as cm:
+            user.unlink(address)
+        self.assertEqual(cm.exception.address, address)
+
+    def test_set_unverified_preferred_address(self):
+        # A user's preferred address cannot be set to an unverified address.
+        new_preferred = getUtility(IUserManager).create_address(
+            'anne.person@example.com')
+        with self.assertRaises(UnverifiedAddressError) as cm:
+            self._anne.preferred_address = new_preferred
+        self.assertEqual(cm.exception.address, new_preferred)
diff --git a/src/mailman/model/user.py b/src/mailman/model/user.py
index abba68589..3a268fd6f 100644
--- a/src/mailman/model/user.py
+++ b/src/mailman/model/user.py
@@ -24,8 +24,8 @@ __all__ = [
     'User',
     ]
 
-from sqlalchemy import (
-    Column, DateTime, ForeignKey, Integer, LargeBinary, Unicode)
+
+from sqlalchemy import Column, DateTime, ForeignKey, Integer, Unicode
 from sqlalchemy.orm import relationship, backref
 from zope.event import notify
 from zope.interface import implementer
@@ -122,7 +122,7 @@ class User(Model):
 
     def unlink(self, address):
         """See `IUser`."""
-        if address.user is None:
+        if address.user is None or address.user is not self:
             raise AddressNotLinkedError(address)
         address.user = None