4 files changed, 197 insertions, 34 deletions

diff --git a/src/mailman/config/schema.cfg b/src/mailman/config/schema.cfg
index 88f378159..44cbf6f4e 100644
--- a/src/mailman/config/schema.cfg
+++ b/src/mailman/config/schema.cfg
@@ -601,13 +601,15 @@ plain_digest_keep_headers:
 
 [nntp]
 # Set these variables if you need to authenticate to your NNTP server for
-# Usenet posting or reading.  If no authentication is necessary, specify None
-# for both variables.
-username:
+# Usenet posting or reading.  Leave these blank if no authentication is
+# necessary.
+user:
 password:
 
-# Set this if you have an NNTP server you prefer gatewayed lists to use.
+# Host and port of the NNTP server to connect to.  Leave these blank to use
+# the default localhost:119.
 host:
+port:
 
 # This controls how headers must be cleansed in order to be accepted by your
 # NNTP server.  Some servers like INN reject messages containing prohibited
diff --git a/src/mailman/docs/NEWS.rst b/src/mailman/docs/NEWS.rst
index 6a10dcade..13bb919c1 100644
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -32,6 +32,9 @@ Configuration
    every `[archiver.<name>]` section.  These are used to determine under what
    circumstances a message destined for a specific archiver should have its
    `Date:` header clobbered.  (LP: #963612)
+ * Configuration schema variable changes:
+   [nntp]username -> [nntp]user
+   [nntp]port (added)
 
 Documentation
 -------------
diff --git a/src/mailman/runners/nntp.py b/src/mailman/runners/nntp.py
index 11c3942f0..8339c735e 100644
--- a/src/mailman/runners/nntp.py
+++ b/src/mailman/runners/nntp.py
@@ -17,6 +17,14 @@
 
 """NNTP runner."""
 
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'NNTPRunner',
+    ]
+
+
 import re
 import email
 import socket
@@ -24,7 +32,6 @@ import logging
 import nntplib
 
 from cStringIO import StringIO
-from lazr.config import as_host_port
 
 from mailman.config import config
 from mailman.core.runner import Runner
@@ -50,39 +57,46 @@ mcre = re.compile(r"""
 
 
 
-class NewsRunner(Runner):
+class NNTPRunner(Runner):
     def _dispose(self, mlist, msg, msgdata):
+        # Get NNTP server connection information.
+        host = config.nntp.host.strip()
+        port = config.nntp.port.strip()
+        if len(port) == 0:
+            port = 119
+        else:
+            try:
+                port = int(port)
+            except (TypeError, ValueError):
+                log.exception('Bad [nntp]port value: {0}'.format(port))
+                port = 119
         # Make sure we have the most up-to-date state
-        mlist.Load()
         if not msgdata.get('prepped'):
             prepare_message(mlist, msg, msgdata)
+        # Flatten the message object, sticking it in a StringIO object
+        fp = StringIO(msg.as_string())
+        conn = None
         try:
-            # Flatten the message object, sticking it in a StringIO object
-            fp = StringIO(msg.as_string())
-            conn = None
-            try:
-                try:
-                    nntp_host, nntp_port = as_host_port(
-                        mlist.nntp_host, default_port=119)
-                    conn = nntplib.NNTP(nntp_host, nntp_port,
-                                        readermode=True,
-                                        user=config.nntp.username,
-                                        password=config.nntp.password)
-                    conn.post(fp)
-                except nntplib.error_temp, e:
-                    log.error('(NNTPDirect) NNTP error for list "%s": %s',
-                              mlist.internal_name(), e)
-                except socket.error, e:
-                    log.error('(NNTPDirect) socket error for list "%s": %s',
-                              mlist.internal_name(), e)
-            finally:
-                if conn:
-                    conn.quit()
-        except Exception, e:
+            conn = nntplib.NNTP(host, port,
+                                readermode=True,
+                                user=config.nntp.user,
+                                password=config.nntp.password)
+            conn.post(fp)
+        except nntplib.error_temp:
+            log.exception('{0} NNTP error for {1}'.format(
+                msg.get('message-id', 'n/a'), mlist.fqdn_listname))
+        except socket.error:
+            log.exception('{0} NNTP socket error for {1}'.format(
+                msg.get('message-id', 'n/a'), mlist.fqdn_listname))
+        except Exception:
             # Some other exception occurred, which we definitely did not
             # expect, so set this message up for requeuing.
-            self._log(e)
+            log.exception('{0} NNTP unexpected exception for {1}'.format(
+                msg.get('message-id', 'n/a'), mlist.fqdn_listname))
             return True
+        finally:
+            if conn:
+                conn.quit()
         return False
 
 
diff --git a/src/mailman/runners/tests/test_nntp.py b/src/mailman/runners/tests/test_nntp.py
index dcd3cfac4..426e829d8 100644
--- a/src/mailman/runners/tests/test_nntp.py
+++ b/src/mailman/runners/tests/test_nntp.py
@@ -21,24 +21,32 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 __all__ = [
-    'TestNNTP',
+    'TestPrepareMessage',
+    'TestNNTPRunner',
     ]
 
 
+import mock
+import socket
+import nntplib
 import unittest
 
 from mailman.app.lifecycle import create_list
+from mailman.config import config
 from mailman.interfaces.nntp import NewsModeration
 from mailman.runners import nntp
 from mailman.testing.helpers import (
+    LogFileMark,
     configuration,
+    get_queue_messages,
+    make_testable_runner,
     specialized_message_from_string as mfs)
 from mailman.testing.layers import ConfigLayer
 
 
 
-class TestNNTP(unittest.TestCase):
-    """Test the NNTP runner and related utilities."""
+class TestPrepareMessage(unittest.TestCase):
+    """Test message preparation."""
 
     layer = ConfigLayer
 
@@ -202,7 +210,7 @@ Testing
         self.assertEqual(tos[0], 'test@example.com')
         original_tos = self._msg.get_all('x-original-to')
         self.assertEqual(len(original_tos), 2)
-        self.assertEqual(original_tos, 
+        self.assertEqual(original_tos,
                          ['test@example.org', 'test@example.net'])
         fakes = self._msg.get_all('x-fake')
         self.assertEqual(len(fakes), 1)
@@ -224,3 +232,139 @@ Testing
         fakes = self._msg.get_all('x-fake')
         self.assertEqual(len(fakes), 3)
         self.assertEqual(fakes, ['one', 'two', 'three'])
+
+
+
+class TestNNTPRunner(unittest.TestCase):
+    """The NNTP runner hands messages off to the NNTP server."""
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        self._mlist = create_list('test@example.com')
+        self._mlist.linked_newsgroup = 'example.test'
+        self._msg = mfs("""\
+From: anne@example.com
+To: test@example.com
+Subject: A newsgroup posting
+Message-ID: <ant>
+
+Testing
+""")
+        self._runner = make_testable_runner(nntp.NNTPRunner, 'nntp')
+        self._nntpq = config.switchboards['nntp']
+
+    @mock.patch('nntplib.NNTP')
+    def test_connect(self, class_mock):
+        # Test connection to the NNTP server with default values.
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        self._runner.run()
+        class_mock.assert_called_once_with(
+            '', 119, user='', password='', readermode=True)
+
+    @configuration('nntp', user='alpha', password='beta',
+                   host='nntp.example.com', port='2112')
+    @mock.patch('nntplib.NNTP')
+    def test_connect_with_configuration(self, class_mock):
+        # Test connection to the NNTP server with specific values.
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        self._runner.run()
+        class_mock.assert_called_once_with(
+            'nntp.example.com', 2112,
+            user='alpha', password='beta', readermode=True)
+
+    @mock.patch('nntplib.NNTP')
+    def test_post(self, class_mock):
+        # Test that the message is posted to the NNTP server.
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        self._runner.run()
+        # Get the mocked instance, which was used in the runner.
+        conn_mock = class_mock()
+        # The connection object's post() method was called once with a
+        # file-like object containing the message's bytes.  Read those bytes
+        # and make some simple checks that the message is what we expected.
+        args = conn_mock.post.call_args
+        # One positional argument.
+        self.assertEqual(len(args[0]), 1)
+        # No keyword arguments.
+        self.assertEqual(len(args[1]), 0)
+        msg = mfs(args[0][0].read())
+        self.assertEqual(msg['subject'], 'A newsgroup posting')
+
+    @mock.patch('nntplib.NNTP')
+    def test_connection_got_quit(self, class_mock):
+        # The NNTP connection gets closed after a successful post.
+        # Test that the message is posted to the NNTP server.
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        self._runner.run()
+        # Get the mocked instance, which was used in the runner.
+        conn_mock = class_mock()
+        # The connection object's post() method was called once with a
+        # file-like object containing the message's bytes.  Read those bytes
+        # and make some simple checks that the message is what we expected.
+        conn_mock.quit.assert_called_once_with()
+
+    @mock.patch('nntplib.NNTP', side_effect=nntplib.error_temp)
+    def test_connect_with_nntplib_failure(self, class_mock):
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        mark = LogFileMark('mailman.error')
+        self._runner.run()
+        log_message = mark.readline()[:-1]
+        self.assertTrue(log_message.endswith(
+            'NNTP error for test@example.com'))
+
+    @mock.patch('nntplib.NNTP', side_effect=socket.error)
+    def test_connect_with_socket_failure(self, class_mock):
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        mark = LogFileMark('mailman.error')
+        self._runner.run()
+        log_message = mark.readline()[:-1]
+        self.assertTrue(log_message.endswith(
+            'NNTP socket error for test@example.com'))
+
+    @mock.patch('nntplib.NNTP', side_effect=RuntimeError)
+    def test_connect_with_other_failure(self, class_mock):
+        # In this failure mode, the message stays queued, so we can only run
+        # the nntp runner once.
+        def once(runner):
+            # I.e. stop immediately, since the queue will not be empty.
+            return True
+        runner = make_testable_runner(nntp.NNTPRunner, 'nntp', predicate=once)
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        mark = LogFileMark('mailman.error')
+        runner.run()
+        log_message = mark.readline()[:-1]
+        self.assertTrue(log_message.endswith(
+            'NNTP unexpected exception for test@example.com'))
+        messages = get_queue_messages('nntp')
+        self.assertEqual(len(messages), 1)
+        self.assertEqual(messages[0].msgdata['listname'], 'test@example.com')
+        self.assertEqual(messages[0].msg['subject'], 'A newsgroup posting')
+
+    @mock.patch('nntplib.NNTP', side_effect=nntplib.error_temp)
+    def test_connection_never_gets_quit_after_failures(self, class_mock):
+        # The NNTP connection doesn't get closed after a unsuccessful
+        # connection, since there's nothing to close.
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        self._runner.run()
+        # Get the mocked instance, which was used in the runner.  Turn off the
+        # exception raising side effect first though!
+        class_mock.side_effect = None
+        conn_mock = class_mock()
+        # The connection object's post() method was called once with a
+        # file-like object containing the message's bytes.  Read those bytes
+        # and make some simple checks that the message is what we expected.
+        self.assertEqual(conn_mock.quit.call_count, 0)
+
+    @mock.patch('nntplib.NNTP')
+    def test_connection_got_quit_after_post_failure(self, class_mock):
+        # The NNTP connection does get closed after a unsuccessful post.
+        # Add a side-effect to the instance mock's .post() method.
+        conn_mock = class_mock()
+        conn_mock.post.side_effect = nntplib.error_temp
+        self._nntpq.enqueue(self._msg, {}, listname='test@example.com')
+        self._runner.run()
+        # The connection object's post() method was called once with a
+        # file-like object containing the message's bytes.  Read those bytes
+        # and make some simple checks that the message is what we expected.
+        conn_mock.quit.assert_called_once_with()