From 7b536046a9f3c7d684366055dd8ddc7d013e0820 Mon Sep 17 00:00:00 2001 From: Thomas Chamberlin Date: Fri, 9 Mar 2018 14:04:21 -0500 Subject: [PATCH 1/4] Updated management command's argument handling for Django 1.8\+ It appears that Django switched from optparse to argparse in Django 1.8. In Django 1.10 it appears that the compatibility mode for optparse-style managemnt commands was removed, and thus these were broken https://docs.djangoproject.com/en/1.11/internals/deprecation/#deprecation-removed-in-1-10 https://docs.djangoproject.com/en/1.11/releases/1.8/#extending-management-command-arguments-through-command-option-list https://docs.djangoproject.com/en/1.8/howto/custom-management-commands/ --- .../management/commands/processincomingmessage.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/django_mailbox/management/commands/processincomingmessage.py b/django_mailbox/management/commands/processincomingmessage.py index c48e23b..42276d1 100644 --- a/django_mailbox/management/commands/processincomingmessage.py +++ b/django_mailbox/management/commands/processincomingmessage.py @@ -16,8 +16,13 @@ logging.basicConfig(level=logging.INFO) class Command(BaseCommand): - args = "<[Mailbox Name (optional)]>" - command = "Receive incoming mail via stdin" + help = "Receive incoming mail via stdin" + + def add_arguments(self, parser): + parser.add_argument( + 'mailbox_name', + help="The name of the mailbox that will receive the message" + ) def handle(self, mailbox_name=None, *args, **options): message = email.message_from_string(sys.stdin.read()) From e151006582f417b4c00ec94cd80476ff3492534f Mon Sep 17 00:00:00 2001 From: Thomas Chamberlin Date: Fri, 9 Mar 2018 15:35:38 -0500 Subject: [PATCH 2/4] Fixed bug in mailbox_name argument and added unit tests This is an optional positional argument, and thus needs to have nargs='?' set --- .../commands/processincomingmessage.py | 1 + .../tests/test_processincomingmessage.py | 54 +++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 django_mailbox/tests/test_processincomingmessage.py diff --git a/django_mailbox/management/commands/processincomingmessage.py b/django_mailbox/management/commands/processincomingmessage.py index 42276d1..384c585 100644 --- a/django_mailbox/management/commands/processincomingmessage.py +++ b/django_mailbox/management/commands/processincomingmessage.py @@ -21,6 +21,7 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument( 'mailbox_name', + nargs='?', help="The name of the mailbox that will receive the message" ) diff --git a/django_mailbox/tests/test_processincomingmessage.py b/django_mailbox/tests/test_processincomingmessage.py new file mode 100644 index 0000000..df5362d --- /dev/null +++ b/django_mailbox/tests/test_processincomingmessage.py @@ -0,0 +1,54 @@ +from django.core.management import call_command, CommandError +from django.test import TestCase + +import mock + +class CommandsTestCase(TestCase): + def test_processincomingmessage_no_args(self): + """Check that processincomingmessage works with no args""" + # Mock handle so that the test doesn't hang waiting for input. Note that we are only testing + # the argument parsing here -- functionality should be tested elsewhere + with mock.patch('django_mailbox.management.commands.processincomingmessage.Command.handle') as handle: + # Don't care about the return value + handle.return_value = None + + call_command('processincomingmessage') + + # Define the arguments we expect handle to be called with + required_arguments = { + # This should be None, since it isn't given + 'mailbox_name': None, + # All otheres can be anything; we don't care about them at all + 'no_color': mock.ANY, + 'pythonpath': mock.ANY, + 'settings': mock.ANY, + 'skip_checks': mock.ANY, + 'traceback': mock.ANY, + 'verbosity': mock.ANY + } + # Make sure that we called with the right arguments + handle.assert_called_with(**required_arguments) + + def test_processincomingmessage_with_arg(self): + """Check that processincomingmessage works with mailbox_name given""" + + with mock.patch('django_mailbox.management.commands.processincomingmessage.Command.handle') as handle: + handle.return_value = None + + call_command('processincomingmessage', 'foo_mailbox') + + required_arguments = { + 'mailbox_name': 'foo_mailbox', + 'no_color': mock.ANY, + 'pythonpath': mock.ANY, + 'settings': mock.ANY, + 'skip_checks': mock.ANY, + 'traceback': mock.ANY, + 'verbosity': mock.ANY + } + handle.assert_called_with(**required_arguments) + + def test_processincomingmessage_too_many_args(self): + """Check that processincomingmessage raises an error if too many args""" + with self.assertRaises(CommandError): + call_command('processincomingmessage', 'foo_mailbox', 'invalid_arg') From b5d5207cd8c986051fc2786e115f2b5772e0096a Mon Sep 17 00:00:00 2001 From: Thomas Chamberlin Date: Fri, 9 Mar 2018 16:36:39 -0500 Subject: [PATCH 3/4] Bug fixes to support Django 1.7 --- .../commands/processincomingmessage.py | 1 + .../tests/test_processincomingmessage.py | 64 ++++++++++--------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/django_mailbox/management/commands/processincomingmessage.py b/django_mailbox/management/commands/processincomingmessage.py index 384c585..ec34411 100644 --- a/django_mailbox/management/commands/processincomingmessage.py +++ b/django_mailbox/management/commands/processincomingmessage.py @@ -16,6 +16,7 @@ logging.basicConfig(level=logging.INFO) class Command(BaseCommand): + args = "<[Mailbox Name (optional)]>" help = "Receive incoming mail via stdin" def add_arguments(self, parser): diff --git a/django_mailbox/tests/test_processincomingmessage.py b/django_mailbox/tests/test_processincomingmessage.py index df5362d..8110913 100644 --- a/django_mailbox/tests/test_processincomingmessage.py +++ b/django_mailbox/tests/test_processincomingmessage.py @@ -1,11 +1,15 @@ +from distutils.version import LooseVersion +import mock + from django.core.management import call_command, CommandError from django.test import TestCase - -import mock +import django class CommandsTestCase(TestCase): def test_processincomingmessage_no_args(self): """Check that processincomingmessage works with no args""" + + mailbox_name = None # Mock handle so that the test doesn't hang waiting for input. Note that we are only testing # the argument parsing here -- functionality should be tested elsewhere with mock.patch('django_mailbox.management.commands.processincomingmessage.Command.handle') as handle: @@ -13,42 +17,42 @@ class CommandsTestCase(TestCase): handle.return_value = None call_command('processincomingmessage') - - # Define the arguments we expect handle to be called with - required_arguments = { - # This should be None, since it isn't given - 'mailbox_name': None, - # All otheres can be anything; we don't care about them at all - 'no_color': mock.ANY, - 'pythonpath': mock.ANY, - 'settings': mock.ANY, - 'skip_checks': mock.ANY, - 'traceback': mock.ANY, - 'verbosity': mock.ANY - } + args, kwargs = handle.call_args + # Make sure that we called with the right arguments - handle.assert_called_with(**required_arguments) + try: + self.assertEqual(kwargs['mailbox_name'], mailbox_name) + except KeyError: + # Handle Django 1.7 + # It uses optparse instead of argparse, so instead of being + # set to None, mailbox_name is simply left out altogether + # Thus we expect an empty tuple here + self.assertEqual(args, tuple()) + def test_processincomingmessage_with_arg(self): """Check that processincomingmessage works with mailbox_name given""" + mailbox_name = 'foo_mailbox' + with mock.patch('django_mailbox.management.commands.processincomingmessage.Command.handle') as handle: handle.return_value = None - call_command('processincomingmessage', 'foo_mailbox') - - required_arguments = { - 'mailbox_name': 'foo_mailbox', - 'no_color': mock.ANY, - 'pythonpath': mock.ANY, - 'settings': mock.ANY, - 'skip_checks': mock.ANY, - 'traceback': mock.ANY, - 'verbosity': mock.ANY - } - handle.assert_called_with(**required_arguments) + call_command('processincomingmessage', mailbox_name) + args, kwargs = handle.call_args + try: + self.assertEqual(kwargs['mailbox_name'], mailbox_name) + except KeyError: + # Handle Django 1.7 + # It uses optparse instead of argparse, so instead of being + # in kwargs, mailbox_name is in args + self.assertEqual(args[0], mailbox_name) def test_processincomingmessage_too_many_args(self): """Check that processincomingmessage raises an error if too many args""" - with self.assertRaises(CommandError): - call_command('processincomingmessage', 'foo_mailbox', 'invalid_arg') + # Only perform this test for Django versions greater than 1.7.*. This + # is because, with optparse, too many arguments doesn't result in an + # error, which means this test is worthless anyway + if LooseVersion(django.get_version()) >= LooseVersion('1.8'): + with self.assertRaises(CommandError): + call_command('processincomingmessage', 'foo_mailbox', 'invalid_arg') From 33b48b2f7307ab28ebcad20b5134b5189c05c54f Mon Sep 17 00:00:00 2001 From: Thomas Chamberlin Date: Fri, 9 Mar 2018 17:02:50 -0500 Subject: [PATCH 4/4] Bug fixes for Django versions 1.8 and 1.9 These are versions in which a "compatibility mode" is in place for argument handling, and things didn't work quite the way I expected --- django_mailbox/tests/test_processincomingmessage.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/django_mailbox/tests/test_processincomingmessage.py b/django_mailbox/tests/test_processincomingmessage.py index 8110913..5ec2432 100644 --- a/django_mailbox/tests/test_processincomingmessage.py +++ b/django_mailbox/tests/test_processincomingmessage.py @@ -42,7 +42,7 @@ class CommandsTestCase(TestCase): args, kwargs = handle.call_args try: self.assertEqual(kwargs['mailbox_name'], mailbox_name) - except KeyError: + except (AssertionError, KeyError): # Handle Django 1.7 # It uses optparse instead of argparse, so instead of being # in kwargs, mailbox_name is in args @@ -53,6 +53,13 @@ class CommandsTestCase(TestCase): # Only perform this test for Django versions greater than 1.7.*. This # is because, with optparse, too many arguments doesn't result in an # error, which means this test is worthless anyway - if LooseVersion(django.get_version()) >= LooseVersion('1.8'): + # For the "compatibility" versions, unexpected arguments aren't handled + # very well, and result in a TypeError + if (LooseVersion(django.get_version()) >= LooseVersion('1.8') and + LooseVersion(django.get_version()) < LooseVersion('1.10')): + with self.assertRaises(TypeError): + call_command('processincomingmessage', 'foo_mailbox', 'invalid_arg') + # In 1.10 and later a proper CommandError should be raised + elif LooseVersion(django.get_version()) >= LooseVersion('1.10'): with self.assertRaises(CommandError): call_command('processincomingmessage', 'foo_mailbox', 'invalid_arg')