5 files changed, 94 insertions, 15 deletions

diff --git a/src/mailman/model/member.py b/src/mailman/model/member.py
index 77d7935a4..1fb11218d 100644
--- a/src/mailman/model/member.py
+++ b/src/mailman/model/member.py
@@ -164,11 +164,9 @@ class Member(Model):
         """See `IMember`."""
         missing = object()
         language = self._lookup('preferred_language', missing)
-        if language is missing:
-            language = ((self.mailing_list and
-                         self.mailing_list.preferred_language) or
-                        system_preferences.preferred_language)
-        return language
+        return (self.mailing_list.preferred_language
+                if language is missing
+                else language)
 
     @property
     def receive_list_copy(self):
diff --git a/src/mailman/rest/members.py b/src/mailman/rest/members.py
index 12a413f42..bdaf031ab 100644
--- a/src/mailman/rest/members.py
+++ b/src/mailman/rest/members.py
@@ -332,9 +332,6 @@ class AllMembers(_MemberBase):
         record = RequestRecord(email, display_name, delivery_mode)
         try:
             member = add_member(mlist, record, role)
-        except InvalidEmailAddressError:
-            bad_request(response, b'Invalid email address')
-            return
         except MembershipIsBannedError:
             bad_request(response, b'Membership is banned')
             return
diff --git a/src/mailman/rest/post_moderation.py b/src/mailman/rest/post_moderation.py
index ca3e228ba..673ff68e8 100644
--- a/src/mailman/rest/post_moderation.py
+++ b/src/mailman/rest/post_moderation.py
@@ -46,10 +46,14 @@ class _ModerationBase:
         key, data = results
         resource = dict(key=key, request_id=request_id)
         # Flatten the IRequest payload into the JSON representation.
-        resource.update(data)
+        if data is not None:
+            resource.update(data)
         # Check for a matching request type, and insert the type name into the
         # resource.
-        request_type = RequestType[resource.pop('_request_type')]
+        try:
+            request_type = RequestType[resource.pop('_request_type', None)]
+        except KeyError:
+            request_type = None
         if request_type is not RequestType.held_message:
             return None
         resource['type'] = RequestType.held_message.name
@@ -104,7 +108,7 @@ class HeldMessage(_HeldMessageBase):
         try:
             request_id = int(self._request_id)
         except ValueError:
-            bad_request(response)
+            not_found(response)
             return
         resource = self._make_resource(request_id)
         if resource is None:
@@ -123,7 +127,7 @@ class HeldMessage(_HeldMessageBase):
         try:
             request_id = int(self._request_id)
         except ValueError:
-            bad_request(response)
+            not_found(response)
             return
         results = requests.get_request(request_id, RequestType.held_message)
         if results is None:
diff --git a/src/mailman/rest/tests/test_moderation.py b/src/mailman/rest/tests/test_moderation.py
index 68b92494b..343f80ca1 100644
--- a/src/mailman/rest/tests/test_moderation.py
+++ b/src/mailman/rest/tests/test_moderation.py
@@ -28,8 +28,10 @@ import unittest
 from mailman.app.lifecycle import create_list
 from mailman.app.moderator import hold_message
 from mailman.database.transaction import transaction
+from mailman.interfaces.bans import IBanManager
 from mailman.interfaces.mailinglist import SubscriptionPolicy
 from mailman.interfaces.registrar import IRegistrar
+from mailman.interfaces.requests import IListRequests, RequestType
 from mailman.interfaces.usermanager import IUserManager
 from mailman.testing.helpers import (
     call_api, get_queue_messages, specialized_message_from_string as mfs)
@@ -55,7 +57,7 @@ Message-ID: <alpha>
 Something else.
 """)
 
-    def test_not_found(self):
+    def test_list_not_found(self):
         # When a bogus mailing list is given, 404 should result.
         with self.assertRaises(HTTPError) as cm:
             call_api('http://localhost:9001/3.0/lists/bee@example.com/held')
@@ -66,7 +68,15 @@ Something else.
         with self.assertRaises(HTTPError) as cm:
             call_api(
                 'http://localhost:9001/3.0/lists/ant@example.com/held/bogus')
-        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_bad_held_message_request_id_post(self):
+        # Bad request when request_id is not an integer.
+        with self.assertRaises(HTTPError) as cm:
+            call_api(
+                'http://localhost:9001/3.0/lists/ant@example.com/held/bogus',
+                dict(action='defer'))
+        self.assertEqual(cm.exception.code, 404)
 
     def test_missing_held_message_request_id(self):
         # Not found when the request_id is not in the database.
@@ -74,10 +84,19 @@ Something else.
             call_api('http://localhost:9001/3.0/lists/ant@example.com/held/99')
         self.assertEqual(cm.exception.code, 404)
 
+    def test_request_is_not_held_message(self):
+        requests = IListRequests(self._mlist)
+        with transaction():
+            request_id = requests.hold_request(RequestType.subscription, 'foo')
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/ant.example.com'
+                     '/held/{}'.format(request_id))
+        self.assertEqual(cm.exception.code, 404)
+
     def test_bad_held_message_action(self):
         # POSTing to a held message with a bad action.
         held_id = hold_message(self._mlist, self._msg)
-        url = 'http://localhost:9001/3.0/lists/ant@example.com/held/{0}'
+        url = 'http://localhost:9001/3.0/lists/ant@example.com/held/{}'
         with self.assertRaises(HTTPError) as cm:
             call_api(url.format(held_id), {'action': 'bogus'})
         self.assertEqual(cm.exception.code, 400)
@@ -373,3 +392,29 @@ class TestSubscriptionModeration(unittest.TestCase):
                 action='defer',
                 ))
         self.assertEqual(response.status, 204)
+
+    def test_subscribe_other_role_with_no_preferred_address(self):
+        with transaction():
+            cate = getUtility(IUserManager).create_user('cate@example.com')
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/members', {
+                'list_id': 'ant.example.com',
+                'subscriber': cate.id,
+                'role': 'moderator',
+                })
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason,
+                         b'User without preferred address')
+
+    def test_subscribe_other_role_banned_email_address(self):
+        bans = IBanManager(self._mlist)
+        with transaction():
+            bans.ban('anne@example.com')
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/members', {
+                'list_id': 'ant.example.com',
+                'subscriber': 'anne@example.com',
+                'role': 'moderator',
+                })
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason, b'Membership is banned')
diff --git a/src/mailman/utilities/tests/test_queries.py b/src/mailman/utilities/tests/test_queries.py
new file mode 100644
index 000000000..6281a9f9b
--- /dev/null
+++ b/src/mailman/utilities/tests/test_queries.py
@@ -0,0 +1,35 @@
+# Copyright (C) 2016 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test queries."""
+
+__all__ = [
+    'TestQueries',
+    ]
+
+
+import unittest
+
+from mailman.utilities.queries import QuerySequence
+from operator import getitem
+
+
+class TestQueries(unittest.TestCase):
+
+    def test_index_error(self):
+        query = QuerySequence(None)
+        self.assertRaises(IndexError, getitem, query, 1)