Merge lp:~ivo-kracht/launchpad/bug-425934 into lp:launchpad

[Ivo Kracht]

Fix for bug-425934 - email commands are now case insensitive.

I changed one line in the helper.py so that it converts all uppercase characters to lowercase characters. Incoming commands and args are now case insensitive. Converting everything to lowercase in the parse.commands function may be a little bit rough but should not create any problems. The only potential problem i can see is when project names and/or usernames are passed to the function but as far as I can see they have to be lowercase anyway.

Pre-imp call with adeuring

tests:

./bin/test services -vvt test_helpers

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/mail/helpers.py
  lib/lp/services/mail/tests/test_helpers.py

[Benji York (benji)]

This branch looks good.

It might be slightly better to move the comment about commands having to be indented to just before the "if" because the "lower" line doesn't have anything to do with indentation. You could optionally also add a comment about the fact that we ignore capitalisation in commands just before the "lower" line.

review: Approve (code)

[Ivo Kracht (ivo-kracht)]

An EC2 test failed because some email command have to be uppercase (summary, cve) so I had to except those from being converted to lowercase. I did that by adding a new value called lowercase_args that can be set to False if a command should be case sensitive. Also i changed the names function into parsingParameters and wrote a unit test for its new task.

Pre-imp call with adeuring

tests:
lib/lp/services/mail/tests/test_helpers.py
lib/lp/services/mail/tests/test_commands.py

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/mail/commands.py
  lib/lp/bugs/mail/handler.py
  lib/lp/code/mail/codehandler.py
  lib/lp/services/mail/commands.py
  lib/lp/services/mail/helpers.py
  lib/lp/services/mail/tests/test_commands.py
  lib/lp/services/mail/tests/test_helpers.py

review: Resubmit

[Francesco Banconi (frankban)]

Another great contribution Ivo, thank you.
Suggestions follow.

#8
+ lowercase_args = False
Just an idea, and just a naming doubt: it seems that what we want to express here is the concept of a command with case insensitive args, and that converting them to lowercase is just an implementation detail. Maybe you could use a `case_insensitive_args` class attribute instead?

#97 and #104
if len(words) > 0:
Isn't it more "pythonic" to just check `if words:`?

#106
+ if command_names[first]:
+ words = words.lower()
+ words = words.split(' ')
                     commands.append((first, words))
This is really a personal opinion, and maybe I am wrong, but I think the code could be more readable if written as::
    case_insensitive_args = command_names[first]
    if case_insensitive_args:
        words = words.lower()
    commands.append((first, words.split(' ')))

review: Approve (code*)

[Raphaël Badin (rvb)]

Looks good.

[0]

I agree with Francesco's suggestions. Well, all suggestions but one: I think you should keep "if len(words) > 0:" because it's more specific than using "if words". For the same reason, if you want to test if "something" is None, we would rather write explicitly "if something is not None:" rather than "if something:".

[1]

147 + self.assertEqual(
148 + {'one': True,
149 + 'two': False,
150 + },
151 + SampleCommandCollection.parsingParameters())

This is really a detail but I think you could write this as

    self.assertEqual(
        {'one': True, 'two': False},
        SampleCommandCollection.parsingParameters())

It would (artificially) reduce the SLOC and I think it's more readable. But again, it's really a detail so feel to change it or not.

review: Approve

=== modified file 'lib/lp/bugs/mail/commands.py'
--- lib/lp/bugs/mail/commands.py	2012-05-22 12:05:51 +0000
+++ lib/lp/bugs/mail/commands.py	2012-06-20 12:21:30 +0000
@@ -371,6 +371,7 @@
     implements(IBugEditEmailCommand)
     _numberOfArguments = 1
     RANK = 1
+    case_insensitive_args = False
 
     def execute(self, bug, current_event):
         """See IEmailCommand."""
@@ -451,6 +452,7 @@
 
     _numberOfArguments = 1
     RANK = 5
+    case_insensitive_args = False
 
     def execute(self, bug, current_event):
         """See IEmailCommand."""

=== modified file 'lib/lp/bugs/mail/handler.py'
--- lib/lp/bugs/mail/handler.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/mail/handler.py	2012-06-20 12:21:30 +0000
@@ -188,9 +188,10 @@
         content = get_main_body(signed_msg)
         if content is None:
             return []
-        return [BugEmailCommands.get(name=name, string_args=args) for
-                name, args in parse_commands(content,
-                                             BugEmailCommands.names())]
+        return [
+            BugEmailCommands.get(name=name, string_args=args) for
+            name, args in parse_commands(
+                content, BugEmailCommands.parsingParameters())]
 
     def extractAndAuthenticateCommands(self, signed_msg, to_addr):
         """Extract commands and handle special destinations.

=== modified file 'lib/lp/code/mail/codehandler.py'
--- lib/lp/code/mail/codehandler.py	2012-06-14 05:18:22 +0000
+++ lib/lp/code/mail/codehandler.py	2012-06-20 12:21:30 +0000
@@ -209,7 +209,7 @@
             return []
         commands = [klass.get(name=name, string_args=args) for
                     name, args in parse_commands(message_body,
-                                                 klass._commands.keys())]
+                                                 klass.parsingParameters())]
         return sorted(commands, key=operator.attrgetter('sort_order'))
 
     @classmethod

=== modified file 'lib/lp/services/mail/commands.py'
--- lib/lp/services/mail/commands.py	2011-08-12 14:39:51 +0000
+++ lib/lp/services/mail/commands.py	2012-06-20 12:21:30 +0000
@@ -63,6 +63,9 @@
     """
     _numberOfArguments = None
 
+    # Should command arguments be converted to lowercase?
+    case_insensitive_args = True
+
     def __init__(self, name, string_args):
         self.name = name
         self.string_args = normalize_arguments(string_args)
@@ -139,9 +142,11 @@
     """A collection of email commands."""
 
     @classmethod
-    def names(klass):
+    def parsingParameters(klass):
         """Returns all the command names."""
-        return klass._commands.keys()
+        return dict(
+            (command_name, command.case_insensitive_args)
+            for command_name, command in klass._commands.items())
 
     @classmethod
     def get(klass, name, string_args):

=== modified file 'lib/lp/services/mail/helpers.py'
--- lib/lp/services/mail/helpers.py	2012-03-02 19:45:41 +0000
+++ lib/lp/services/mail/helpers.py	2012-06-20 12:21:30 +0000
@@ -108,6 +108,7 @@
     A list of (command, args) tuples is returned.
     """
     commands = []
+
     for line in content.splitlines():
         # All commands have to be indented.
         if line.startswith(' ') or line.startswith('\t'):
@@ -116,12 +117,18 @@
                 # If the 'done' statement is encountered,
                 # stop reading any more commands.
                 break
-            words = command_string.split(' ')
+            words = command_string.split(' ', 1)
             if len(words) > 0:
-                first = words.pop(0)
+                # Capitalization gets ignored
+                first = words.pop(0).lower()
                 if first.endswith(':'):
                     first = first[:-1]
                 if first in command_names:
+                    if len(words) > 0:
+                        words = words[0]
+                        if command_names[first]:
+                            words = words.lower()
+                        words = words.split(' ')
                     commands.append((first, words))
     return commands
 

=== modified file 'lib/lp/services/mail/tests/test_commands.py'
--- lib/lp/services/mail/tests/test_commands.py	2011-08-11 21:24:58 +0000
+++ lib/lp/services/mail/tests/test_commands.py	2012-06-20 12:21:30 +0000
@@ -2,7 +2,40 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from doctest import DocTestSuite
+import unittest
+
+from lp.testing import TestCase
+
+from lp.services.mail.commands import(
+    EmailCommand,
+    EmailCommandCollection,
+    )
+
+
+class CommandOne(EmailCommand):
+    pass
+
+
+class CommandTwo(EmailCommand):
+    case_insensitive_args = False
+
+
+class SampleCommandCollection(EmailCommandCollection):
+    _commands = {
+        'one': CommandOne,
+        'two': CommandTwo,
+    }
+
+
+class TestEmailCommandCollection(TestCase):
+    def test_parsingParameters(self):
+        self.assertEqual(
+            {'one': True, 'two': False},
+            SampleCommandCollection.parsingParameters())
 
 
 def test_suite():
-    return DocTestSuite('lp.services.mail.commands')
+    suite = unittest.TestSuite()
+    suite.addTest(DocTestSuite('lp.services.mail.commands'))
+    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
+    return suite

=== modified file 'lib/lp/services/mail/tests/test_helpers.py'
--- lib/lp/services/mail/tests/test_helpers.py	2012-03-05 20:13:15 +0000
+++ lib/lp/services/mail/tests/test_helpers.py	2012-06-20 12:21:30 +0000
@@ -39,28 +39,42 @@
 
     def test_parse_commandsEmpty(self):
         """Empty messages have no commands."""
-        self.assertEqual([], parse_commands('', ['command']))
+        self.assertEqual([], parse_commands('', {'command': True}))
 
     def test_parse_commandsNoIndent(self):
         """Commands with no indent are not commands."""
-        self.assertEqual([], parse_commands('command', ['command']))
+        self.assertEqual([], parse_commands('command',  {'command': True}))
 
     def test_parse_commandsSpaceIndent(self):
         """Commands indented with spaces are recognized."""
         self.assertEqual(
-            [('command', [])], parse_commands(' command', ['command']))
+            [('command', [])], parse_commands(' command', {'command': True}))
 
     def test_parse_commands_args(self):
         """Commands indented with spaces are recognized."""
         self.assertEqual(
             [('command', ['arg1', 'arg2'])],
-            parse_commands(' command arg1 arg2', ['command']))
+            parse_commands(' command arg1 arg2', {'command': True}))
+
+    def test_parse_commands_args_uppercase_unchanged(self):
+        """Commands and args containing uppercase letters are not
+        converted to lowercase if the flag is False."""
+        self.assertEqual(
+            [('command', ['Arg1', 'aRg2'])],
+            parse_commands(' comMand Arg1 aRg2', {'command': False}))
+
+    def test_parse_commands_args_uppercase_to_lowercase(self):
+        """Commands and args containing uppercase letters are converted to
+        lowercase."""
+        self.assertEqual(
+            [('command', ['arg1', 'arg2'])],
+            parse_commands(' comMand Arg1 aRg2', {'command': True}))
 
     def test_parse_commands_args_quoted(self):
         """Commands indented with spaces are recognized."""
         self.assertEqual(
             [('command', ['"arg1', 'arg2"'])],
-            parse_commands(' command "arg1 arg2"', ['command']))
+            parse_commands(' command "arg1 arg2"', {'command': True}))
 
     def test_parse_commandsTabIndent(self):
         """Commands indented with tabs are recognized.
@@ -68,29 +82,30 @@
         (Tabs?  What are we, make?)
         """
         self.assertEqual(
-            [('command', [])], parse_commands('\tcommand', ['command']))
+            [('command', [])], parse_commands('\tcommand', {'command': True}))
 
     def test_parse_commandsDone(self):
         """The 'done' pseudo-command halts processing."""
         self.assertEqual(
             [('command', []), ('command', [])],
-            parse_commands(' command\n command', ['command']))
+            parse_commands(' command\n command', {'command': True}))
         self.assertEqual(
             [('command', [])],
-            parse_commands(' command\n done\n command', ['command']))
+            parse_commands(' command\n done\n command', {'command': True}))
         # done takes no arguments.
         self.assertEqual(
             [('command', []), ('command', [])],
-            parse_commands(' command\n done commands\n command', ['command']))
+            parse_commands(
+                ' command\n done commands\n command', {'command': True}))
 
     def test_parse_commands_optional_colons(self):
         """Colons at the end of commands are accepted and stripped."""
         self.assertEqual(
             [('command', ['arg1', 'arg2'])],
-            parse_commands(' command: arg1 arg2', ['command']))
+            parse_commands(' command: arg1 arg2', {'command': True}))
         self.assertEqual(
             [('command', [])],
-            parse_commands(' command:', ['command']))
+            parse_commands(' command:', {'command': True}))
 
 
 class TestEnsureSaneSignatureTimestamp(unittest.TestCase):