6 files changed, 116 insertions, 5 deletions

diff --git a/src/mailman/app/digests.py b/src/mailman/app/digests.py
index f8b217142..0d47df328 100644
--- a/src/mailman/app/digests.py
+++ b/src/mailman/app/digests.py
@@ -56,8 +56,7 @@ def bump_digest_number_and_volume(mlist):
         bump = (now.toordinal() > mlist.digest_last_sent_at.toordinal())
     else:
         raise AssertionError(
-            'Bad DigestFrequency: {0}'.format(
-                mlist.digest_volume_frequency))
+            'Bad DigestFrequency: {}'.format(mlist.digest_volume_frequency))
     if bump:
         mlist.volume += 1
         mlist.next_digest_number = 1
diff --git a/src/mailman/bin/mailman.py b/src/mailman/bin/mailman.py
index 7a443e9d6..b4d144b7c 100644
--- a/src/mailman/bin/mailman.py
+++ b/src/mailman/bin/mailman.py
@@ -24,6 +24,7 @@ from functools import cmp_to_key
 from mailman import public
 from mailman.core.i18n import _
 from mailman.core.initialize import initialize
+from mailman.database.transaction import transaction
 from mailman.interfaces.command import ICLISubCommand
 from mailman.utilities.modules import find_components
 from mailman.version import MAILMAN_VERSION_FULL
@@ -95,4 +96,5 @@ def main():
                    else os.path.abspath(os.path.expanduser(args.config)))
     initialize(config_path)
     # Perform the subcommand option.
-    args.func(args)
+    with transaction():
+        args.func(args)
diff --git a/src/mailman/bin/tests/test_mailman.py b/src/mailman/bin/tests/test_mailman.py
index 7c9d2f2bc..c678ff560 100644
--- a/src/mailman/bin/tests/test_mailman.py
+++ b/src/mailman/bin/tests/test_mailman.py
@@ -19,12 +19,21 @@
 
 import unittest
 
+from contextlib import ExitStack
+from datetime import timedelta
 from io import StringIO
+from mailman.app.lifecycle import create_list
 from mailman.bin.mailman import main
+from mailman.config import config
+from mailman.database.transaction import transaction
+from mailman.testing.layers import ConfigLayer
+from mailman.utilities.datetime import now
 from unittest.mock import patch
 
 
 class TestMailmanCommand(unittest.TestCase):
+    layer = ConfigLayer
+
     def test_mailman_command_without_subcommand_prints_help(self):
         # Issue #137: Running `mailman` without a subcommand raises an
         # AttributeError.
@@ -34,3 +43,51 @@ class TestMailmanCommand(unittest.TestCase):
             with self.assertRaises(SystemExit):
                 main()
         self.assertIn('usage', output.getvalue())
+
+    def test_transaction_commit_after_successful_subcommand(self):
+        # Issue #223: Subcommands which change the database need to commit or
+        # abort the transaction.
+        with transaction():
+            mlist = create_list('ant@example.com')
+            mlist.volume = 5
+            mlist.next_digest_number = 3
+            mlist.digest_last_sent_at = now() - timedelta(days=60)
+        testargs = ['mailman', 'digests', '-b', '-l', 'ant@example.com']
+        output = StringIO()
+        with ExitStack() as resources:
+            enter = resources.enter_context
+            enter(patch('sys.argv', testargs))
+            enter(patch('sys.stdout', output))
+            # Everything is already initialized.
+            enter(patch('mailman.bin.mailman.initialize'))
+            main()
+        # Clear the current transaction to force a database reload.
+        config.db.abort()
+        self.assertEqual(mlist.volume, 6)
+        self.assertEqual(mlist.next_digest_number, 1)
+
+    def test_transaction_abort_after_failing_subcommand(self):
+        with transaction():
+            mlist = create_list('ant@example.com')
+            mlist.volume = 5
+            mlist.next_digest_number = 3
+            mlist.digest_last_sent_at = now() - timedelta(days=60)
+        testargs = ['mailman', 'digests', '-b', '-l', 'ant@example.com',
+                    '--send']
+        output = StringIO()
+        with ExitStack() as resources:
+            enter = resources.enter_context
+            enter(patch('sys.argv', testargs))
+            enter(patch('sys.stdout', output))
+            # Force an exception in the subcommand.
+            enter(patch('mailman.commands.cli_digests.maybe_send_digest_now',
+                        side_effect=RuntimeError))
+            # Everything is already initialized.
+            enter(patch('mailman.bin.mailman.initialize'))
+            with self.assertRaises(RuntimeError):
+                main()
+        # Clear the current transaction to force a database reload.
+        config.db.abort()
+        # The volume and number haven't changed.
+        self.assertEqual(mlist.volume, 5)
+        self.assertEqual(mlist.next_digest_number, 3)
diff --git a/src/mailman/commands/cli_digests.py b/src/mailman/commands/cli_digests.py
index c4623259e..3a4d6614f 100644
--- a/src/mailman/commands/cli_digests.py
+++ b/src/mailman/commands/cli_digests.py
@@ -57,6 +57,15 @@ class Digests:
             help=_("""Increment the digest volume number and reset the digest
                    number to one.  If given with --send, the volume number is
                    incremented before any current digests are sent."""))
+        command_parser.add_argument(
+            '-n', '--dry-run',
+            default=False, action='store_true',
+            help=_("""Don't actually do anything, but in conjunction with
+                   --verbose, show what would happen."""))
+        command_parser.add_argument(
+            '-v', '--verbose',
+            default=False, action='store_true',
+            help=_("""Print some additional status."""))
 
     def process(self, args):
         """See `ICLISubCommand`."""
@@ -77,7 +86,20 @@ class Digests:
             lists = list(list_manager.mailing_lists)
         if args.bump:
             for mlist in lists:
-                bump_digest_number_and_volume(mlist)
+                if args.verbose:
+                    print(_('\
+$mlist.list_id is at volume $mlist.volume, number \
+${mlist.next_digest_number}'))
+                if not args.dry_run:
+                    bump_digest_number_and_volume(mlist)
+                    if args.verbose:
+                        print(_('\
+$mlist.list_id bumped to volume $mlist.volume, number \
+${mlist.next_digest_number}'))
         if args.send:
             for mlist in lists:
-                maybe_send_digest_now(mlist, force=True)
+                if args.verbose:
+                    print(_('\
+$mlist.list_id sent volume $mlist.volume, number ${mlist.next_digest_number}'))
+                if not args.dry_run:
+                    maybe_send_digest_now(mlist, force=True)
diff --git a/src/mailman/commands/tests/test_digests.py b/src/mailman/commands/tests/test_digests.py
index b14820cbd..4c62cea38 100644
--- a/src/mailman/commands/tests/test_digests.py
+++ b/src/mailman/commands/tests/test_digests.py
@@ -41,6 +41,8 @@ class FakeArgs:
         self.lists = []
         self.send = False
         self.bump = False
+        self.dry_run = False
+        self.verbose = False
 
 
 class TestSendDigests(unittest.TestCase):
@@ -421,3 +423,29 @@ class TestBumpVolume(unittest.TestCase):
         self.assertEqual(self._mlist.volume, 8)
         self.assertEqual(self._mlist.next_digest_number, 1)
         self.assertEqual(self._mlist.digest_last_sent_at, self.right_now)
+
+    def test_bump_verbose(self):
+        args = FakeArgs()
+        args.bump = True
+        args.verbose = True
+        args.lists.append('ant.example.com')
+        output = StringIO()
+        with patch('sys.stdout', output):
+            self._command.process(args)
+        self.assertMultiLineEqual(output.getvalue(), """\
+ant.example.com is at volume 7, number 4
+ant.example.com bumped to volume 7, number 5
+""")
+
+    def test_send_verbose(self):
+        args = FakeArgs()
+        args.send = True
+        args.verbose = True
+        args.dry_run = True
+        args.lists.append('ant.example.com')
+        output = StringIO()
+        with patch('sys.stdout', output):
+            self._command.process(args)
+        self.assertMultiLineEqual(output.getvalue(), """\
+ant.example.com sent volume 7, number 4
+""")
diff --git a/src/mailman/docs/NEWS.rst b/src/mailman/docs/NEWS.rst
index b14738330..10473e8ce 100644
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -94,6 +94,9 @@ Command line
    compatibility, but now there is a ``-D``/``--no-domain`` option to prevent
    missing domains from being create, forcing an error in those cases.
    Given by Gurkirpal Singh.  (Closes #39)
+ * ``mailman`` subcommands now properly commit any outstanding transactions.
+   (Closes #223)
+ * ``mailman digests`` has grown ``--verbose`` and ``-dry-run`` options.
 
 Interfaces
 ----------