Merge lp:~postfuturist/mailman/rest-api-mods_owners into lp:mailman

[Stephen A. Goss]

Added ability to add moderators and owners through existing REST API endpoints. Tests were updated as well.

[Stephen A. Goss (postfuturist)]

The AlreadySubscribedError I removed is superfluous, as that error will get raised regardless in the call to `subscribe` if that is the case.

[Barry Warsaw (barry)]

Thanks for the code! It looks pretty good in general. I'm going to make a few stylistic fixes (indentation, parameter order, multiline import formatting), and I'll add a few more "bad path" tests to ensure the exception still gets raised when expected. Then I'll merge and commit.

Cheers!

review: Approve

=== modified file 'src/mailman/app/membership.py'
--- src/mailman/app/membership.py	2011-05-26 01:30:56 +0000
+++ src/mailman/app/membership.py	2011-08-25 23:41:30 +0000
@@ -43,7 +43,8 @@
 
 
 
 
-def add_member(mlist, email, realname, password, delivery_mode, language):
+def add_member(mlist, email, realname, password, delivery_mode, language, 
+        role=MemberRole.member):
     """Add a member right now.
 
     The member's subscription must be approved by whatever policy the list
@@ -70,9 +71,6 @@
     """
     # Let's be extra cautious.
     getUtility(IEmailValidator).validate(email)
-    if mlist.members.get_member(email) is not None:
-        raise AlreadySubscribedError(
-            mlist.fqdn_listname, email, MemberRole.member)
     # Check to see if the email address is banned.
     if getUtility(IBanManager).is_banned(email, mlist.fqdn_listname):
         raise MembershipIsBannedError(mlist, email)
@@ -99,7 +97,7 @@
         # scheme is recorded in the hashed password string.
         user.password = encrypt_password(password)
         user.preferences.preferred_language = language
-        member = mlist.subscribe(address, MemberRole.member)
+        member = mlist.subscribe(address, role)
         member.preferences.delivery_mode = delivery_mode
     else:
         # The user exists and is linked to the address.
@@ -110,7 +108,7 @@
             raise AssertionError(
                 'User should have had linked address: {0}'.format(address))
         # Create the member and set the appropriate preferences.
-        member = mlist.subscribe(address, MemberRole.member)
+        member = mlist.subscribe(address, role)
         member.preferences.preferred_language = language
         member.preferences.delivery_mode = delivery_mode
     return member
 
=== modified file 'src/mailman/app/subscriptions.py'
--- src/mailman/app/subscriptions.py	2011-08-17 23:22:45 +0000
+++ src/mailman/app/subscriptions.py	2011-08-25 23:41:30 +0000
@@ -37,7 +37,7 @@
 from mailman.interfaces.address import InvalidEmailAddressError
 from mailman.interfaces.listmanager import (
     IListManager, ListDeletedEvent, NoSuchListError)
-from mailman.interfaces.member import DeliveryMode
+from mailman.interfaces.member import DeliveryMode, MemberRole
 from mailman.interfaces.subscriptions import (
     ISubscriptionService, MissingUserError)
 from mailman.interfaces.usermanager import IUserManager
@@ -138,16 +138,12 @@
         for member in self.get_members():
             yield member
 
-    def join(self, fqdn_listname, subscriber,
-             real_name= None, delivery_mode=None):
+    def join(self, fqdn_listname, subscriber, real_name= None, 
+            delivery_mode=DeliveryMode.regular, role=MemberRole.member):
         """See `ISubscriptionService`."""
         mlist = getUtility(IListManager).get(fqdn_listname)
         if mlist is None:
             raise NoSuchListError(fqdn_listname)
-        # Convert from string to enum.
-        mode = (DeliveryMode.regular
-                if delivery_mode is None
-                else delivery_mode)
         # Is the subscriber a user or email address?
         if '@' in subscriber:
             # It's an email address, so we'll want a real name.
@@ -163,14 +159,15 @@
             # it can't be retrieved.  Note that none of these are used unless
             # the address is completely new to us.
             password = make_user_friendly_password()
-            return add_member(mlist, subscriber, real_name, password, mode,
-                              system_preferences.preferred_language)
+            return add_member(mlist, subscriber, real_name, password, 
+                              delivery_mode, 
+                              system_preferences.preferred_language, role)
         else:
             # We have to assume it's a user id.
             user = getUtility(IUserManager).get_user_by_id(subscriber)
             if user is None:
                 raise MissingUserError(subscriber)
-            return mlist.subscribe(user)
+            return mlist.subscribe(user, role)
 
     def leave(self, fqdn_listname, address):
         """See `ISubscriptionService`."""
 
=== modified file 'src/mailman/app/tests/test_membership.py'
--- src/mailman/app/tests/test_membership.py	2011-04-09 00:09:13 +0000
+++ src/mailman/app/tests/test_membership.py	2011-08-25 23:41:30 +0000
@@ -34,7 +34,8 @@
 from mailman.config import config
 from mailman.core.constants import system_preferences
 from mailman.interfaces.bans import IBanManager
-from mailman.interfaces.member import DeliveryMode, MembershipIsBannedError
+from mailman.interfaces.member import (DeliveryMode, MembershipIsBannedError,
+    MemberRole)
 from mailman.interfaces.usermanager import IUserManager
 from mailman.testing.helpers import reset_the_world
 from mailman.testing.layers import ConfigLayer
@@ -58,6 +59,7 @@
                             system_preferences.preferred_language)
         self.assertEqual(member.address.email, '[e-mail]')
         self.assertEqual(member.mailing_list, '[e-mail]')
+        self.assertEqual(member.role, MemberRole.member)
 
     def test_add_member_existing_user(self):
         # Test subscribing a user to a mailing list when the email address has
@@ -124,6 +126,16 @@
                             system_preferences.preferred_language)
         self.assertEqual(member.address.email, '[e-mail]')
 
+    def test_add_member_moderator(self):
+        # Test adding a moderator to a mailing list
+        member = add_member(self._mlist, '[e-mail]',
+                            'Anne Person', '123', DeliveryMode.regular,
+                            system_preferences.preferred_language,
+                            MemberRole.moderator)
+        self.assertEqual(member.address.email, '[e-mail]')
+        self.assertEqual(member.mailing_list, '[e-mail]')
+        self.assertEqual(member.role, MemberRole.moderator)
+
 
 
 
 class AddMemberPasswordTest(unittest.TestCase):
 
=== modified file 'src/mailman/interfaces/subscriptions.py'
--- src/mailman/interfaces/subscriptions.py	2011-08-17 23:10:39 +0000
+++ src/mailman/interfaces/subscriptions.py	2011-08-25 23:41:30 +0000
@@ -28,6 +28,7 @@
 from zope.interface import Interface
 
 from mailman.interfaces.errors import MailmanError
+from mailman.interfaces.member import DeliveryMode, MemberRole
 
 
 
 
@@ -91,7 +92,8 @@
     def __iter__():
         """See `get_members()`."""
 
-    def join(fqdn_listname, subscriber, real_name=None, delivery_mode=None):
+    def join(fqdn_listname, subscriber, real_name=None, 
+            delivery_mode=DeliveryMode.regular, role=MemberRole.member):
         """Subscribe to a mailing list.
 
         A user for the address is created if it is not yet known to Mailman,
 
=== modified file 'src/mailman/rest/docs/membership.rst'
--- src/mailman/rest/docs/membership.rst	2011-08-17 23:10:39 +0000
+++ src/mailman/rest/docs/membership.rst	2011-08-25 23:41:30 +0000
@@ -197,8 +197,27 @@
 mailing list.
 ::
 
-    >>> subscribe(ant, 'Dave', MemberRole.moderator)
-    >>> subscribe(bee, 'Cris', MemberRole.owner)
+    >>> dump_json('http://localhost:9001/3.0/members', {
+    ...           'fqdn_listname': '[e-mail]',
+    ...           'subscriber': '[e-mail]',
+    ...           'role': 'moderator',
+    ...           })
+    content-length: 0
+    date: ...
+    location: http://localhost:9001/3.0/members/6
+    server: ...
+    status: 201
+
+    >>> dump_json('http://localhost:9001/3.0/members', {
+    ...           'fqdn_listname': '[e-mail]',
+    ...           'subscriber': '[e-mail]',
+    ...           'role': 'owner',
+    ...           })
+    content-length: 0
+    date: ...
+    location: http://localhost:9001/3.0/members/7
+    server: ...
+    status: 201
 
     >>> dump_json('http://localhost:9001/3.0/members')
     entry 0:
 
=== modified file 'src/mailman/rest/members.py'
--- src/mailman/rest/members.py	2011-08-17 23:10:39 +0000
+++ src/mailman/rest/members.py	2011-08-25 23:41:30 +0000
@@ -151,7 +151,8 @@
                                   subscriber=unicode,
                                   real_name=unicode,
                                   delivery_mode=enum_validator(DeliveryMode),
-                                  _optional=('delivery_mode', 'real_name'))
+                                  role=enum_validator(MemberRole),
+                                  _optional=('delivery_mode', 'real_name', 'role'))
             member = service.join(**validator(request))
         except AlreadySubscribedError:
             return http.conflict([], b'Member already subscribed')