Merge lp:~milo/linaro-patchmetrics/parsemail-fixes into lp:linaro-patchmetrics

[Milo Casagrande]

This is a fix for the script that parses the emails searching for patches. With the new changes, if the submitter of a patch nor the author of a patch are using Linaro email address, we do not consider the patch and we move on.

Another change made, is to when the saving action on a new Person (in this case a new author) happens. Previously, if a new author was found, it was saved immediately, now we save it only after making sure it is a valid Linaro engineer. In this way we do the exact same thing as it was done with the Submitter person, and we do not save authors that we do not want to track.

[Paul Sokolovsky (pfalcon)]

Logic looks good. And I usually don't go at such details with stylistic changes, but for few quick looks at linaro-patchmetrics codebase, I felt confused and saw few less-than-ideal practices and hardcodings. So, let's try to do better.

1. Can "new_author" var be named "is_new_auhor"?

2.

8 - return None, None
9 + return None, None, None

From the rest of patch it's possible to figure out what 3rd return value is, and I'm not sure if that func had a docstring, but maybe it might?

3.

57 + # If submitter nor author of a patch are not Linaro, we move on.
58 + # TODO: may be worth to integrate with crowd.
59 + if "@linaro.org" not in submitter.email:
60 + if author is not None and "@linaro.org" not in author.email:
61 + return 0
62 + return 0

There're 2 issues here: one is that hardcoded "@linaro.org". We can do better ;-) (yeah, I know it takes much more patch lines to use config settings, and sometimes with my changes I wonder if I should bother either; friendly answer looking from outside - yes ;-) ). And we should anticipate need to support multiple patterns - either as array, or better just as a regexp.

Another is that there's superfluous/erroneous logic - if first "if" fired, it's "return 0" regardless of second
"if". If code should look like that, there should be a comment (like, "one sweet day, we may want to handle these conditions separately"). Otherwise...

review: Needs Fixing

[Milo Casagrande (milo)]

Thanks Paul for looking into it!

On Wed, Dec 4, 2013 at 2:25 PM, Paul Sokolovsky
<email address hidden> wrote:
> Review: Needs Fixing
>
> Logic looks good. And I usually don't go at such details with stylistic changes, but for few quick looks at linaro-patchmetrics codebase, I felt confused and saw few less-than-ideal practices and hardcodings. So, let's try to do better.
>
> 1. Can "new_author" var be named "is_new_auhor"?
>
> 2.
>
> 8 - return None, None
> 9 + return None, None, None
>
> >From the rest of patch it's possible to figure out what 3rd return value is, and I'm not sure if that func had a docstring, but maybe it might?

I added info in the function doc string.

>
> 57 + # If submitter nor author of a patch are not Linaro, we move on.
> 58 + # TODO: may be worth to integrate with crowd.
> 59 + if "@linaro.org" not in submitter.email:
> 60 + if author is not None and "@linaro.org" not in author.email:
> 61 + return 0
> 62 + return 0
>
> There're 2 issues here: one is that hardcoded "@linaro.org". We can do better ;-) (yeah, I know it takes much more patch lines to use config settings, and sometimes with my changes I wonder if I should bother either; friendly answer looking from outside - yes ;-) ). And we should anticipate need to support multiple patterns - either as array, or better just as a regexp.

I know about the hardcoded @linaro.org values (there are a few more in
Linaro patchwork code), but I'm not going to fix them here. The code
cannot go in production like that, I'm aware of it, that's why I would
like the other MP in first, to be able to address this issue as well.
The other MP, that is bigger, adds some integration with Crowd that we
can use here as well, that's why there is that TODO.

I do not like the regexp approach, at least in this case: we can do
even better than that, and it is taking data out of Crowd.

The problem cannot be solved by simply querying Crowd though:
switching to a subscription model for patches.l.o (we subscribe to the
mailing list, they do not send the patches directly anymore), it means
that we can receive even more than 500 mails per day, that will lead
to 500 Crowd requests. When I tested the old patch, IT were already
looking at who was hitting the server (and it was with far less
requests). We need to locally cache those requests: the other MP tries
to solve that.

> Another is that there's superfluous/erroneous logic - if first "if" fired, it's "return 0" regardless of second
> "if". If code should look like that, there should be a comment (like, "one sweet day, we may want to handle these conditions separately"). Otherwise...

There was one "return 0" too much, fixed.

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[lp:~milo/linaro-patchmetrics/parsemail-fixes]

401. By Milo Casagrande on 2013-12-04

Fixed doc string, refactored variable, fixed if-logic.

402. By Milo Casagrande on 2013-12-05

Fixed if clause.

403. By Milo Casagrande on 2013-12-05

Merged from trunk.

404. By Milo Casagrande on 2013-12-05

Added crowd integration for email checking.

405. By Milo Casagrande on 2013-12-05

Fixed if clause.

406. By Milo Casagrande on 2013-12-05

Refactored code.

  * Moved one function from the parsemail script into the utils.py file.
    Had to import one function from utils.py in parsemail.py, but apparently
    the import was breaking something (probably circular import).

[Milo Casagrande (milo)]

Added the Crowd integration also on the check for valid patch submitter or authors.
Had to refactor some code: I needed to import a function form utils.py, but utils.py was importing a function from parsemail.py, resulting in an import error when the script is executed.

=== modified file 'apps/patchwork/bin/parsemail.py'
--- apps/patchwork/bin/parsemail.py	2013-11-29 16:20:37 +0000
+++ apps/patchwork/bin/parsemail.py	2013-12-05 18:37:48 +0000
@@ -20,6 +20,7 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
 import _pythonpath
+import atexit
 import chardet
 import datetime
 import operator
@@ -37,8 +38,14 @@
     from email.Header import Header, decode_header
     from email.Utils import parsedate_tz, mktime_tz
 
+from patchmetrics.crowd import Crowd
+from patchwork.db import PatchworkDB
 from patchwork.parser import parse_patch
 from patchwork.models import Patch, Project, Person, Comment, State
+from patchwork.utils import (
+    is_linaro_user,
+    parse_name_and_email,
+)
 from django.contrib.auth.models import User
 
 default_patch_state = 'New'
@@ -118,33 +125,6 @@
     return project
 
 
-def parse_name_and_email(text):
-    # tuple of (regex, fn)
-    #  - where fn returns a (name, email) tuple from the match groups resulting
-    #    from re.match().groups()
-    from_res = [
-        # for "Firstname Lastname" <[e-mail]> style addresses
-        (re.compile('"?(.*?)"?\s*<([^>]+)>'), (lambda g: (g[0], g[1]))),
-
-        # for [e-mail] (Firstname Lastname) style addresses
-        (re.compile('"?(.*?)"?\s*\(([^\)]+)\)'), (lambda g: (g[1], g[0]))),
-
-        # everything else
-        (re.compile('(.*)'), (lambda g: (None, g[0]))),
-    ]
-
-    for regex, fn in from_res:
-        match = regex.match(text)
-        if match:
-            name, email = fn(match.groups())
-            if name is not None:
-                name = name.strip()
-            if email is not None:
-                email = email.strip()
-            return name, email
-    return None, None
-
-
 def find_submitter(mail):
 
     from_header = clean_header(mail.get('From'))
@@ -282,18 +262,22 @@
 
 
 def find_patch_name_and_author(comment):
-    """Return the patch name and author as specified in the given comment.
+    """Finds the patch name, and the patch author.
 
     The name and author of a patch are sometimes different from the Subject
     and From header fields and so are specified in the body of the message,
     before the actual patch. This is identified by "From: " and "Subject: "
     lines starting the body, which is the format understood by 'git-am'.
+
+    :return A tuple consisting of: the name of the patch, the author of the
+        patch, and a boolean indicating if the author needs to be saved in the
+        database.
     """
     if not comment:
-        return None, None
+        return None, None, None
     lines = comment.content.split('\n')
     if len(lines) == 0:
-        return None, None
+        return None, None, None
     # Append an extra line at the end so that our code can assume there will
     # always be at least two lines, simplifying things significantly.
     lines.append('')
@@ -308,25 +292,26 @@
         if second_line.startswith('From:'):
             author_line = second_line
     else:
-        return None, None
+        return None, None, None
 
     subject = None
     if subject_line is not None:
         subject = subject_line.replace('Subject:', '').strip()
 
     if author_line is None:
-        return subject, None
+        return subject, None, None
 
     name, email = parse_name_and_email(author_line.replace('From:', ''))
     if email is None:
-        return subject, None
+        return subject, None, None
 
     try:
         person = Person.objects.get(email__iexact=email)
+        new_person = False
     except Person.DoesNotExist:
         person = Person(name=name, email=email)
-        person.save()
-    return subject, person
+        new_person = True
+    return subject, person, new_person
 
 
 def find_patch_for_comment(project, mail):
@@ -493,6 +478,39 @@
     return None
 
 
+def is_valid_patch(submitter, author):
+    """A patch is valid if submitter or author are valid Linaro users.
+
+    :param submitter: The submitter to check, a Person object.
+    :param author: The author to check, a Person object.
+    :return True if one between submitter and author is a valid Linaro user.
+    """
+    valid_patch = False
+
+    if (settings.AUTH_CROWD_APPLICATION_USER is None and
+            settings.CROWD_USERS_DB_FILE is None):
+        # If none of the above settings is defined, fallback to an hardoceded
+        # check and accept only linaro.org email addresses.
+        if "@linaro.org" in submitter.email:
+            valid_patch |= True
+
+        if author is not None and "@linaro.org" in author.email:
+            valid_patch |= True
+    else:
+        crwd = Crowd(settings.AUTH_CROWD_APPLICATION_USER,
+                     settings.AUTH_CROWD_APPLICATION_PASSWORD,
+                     settings.AUTH_CROWD_SERVER_REST_URI)
+
+        cache_db = PatchworkDB(settings.CROWD_USERS_DB_FILE)
+        atexit.register(cache_db.close)
+
+        valid_patch |= is_linaro_user(submitter.email, cache_db, crwd)
+        if author is not None:
+            valid_patch |= is_linaro_user(author.email, cache_db, crwd)
+
+    return valid_patch
+
+
 def parse_mail(mail):
 
     # some basic sanity checks
@@ -517,8 +535,14 @@
     msgid = mail.get('Message-Id').strip()
 
     (submitter, save_required) = find_submitter(mail)
-
     (patch, comment) = find_content(project, mail)
+    (name, author, is_new_author) = find_patch_name_and_author(comment)
+
+    # If submitter nor author of a patch are not Linaro, we move on.
+    valid_patch = is_valid_patch(submitter, author)
+
+    if not valid_patch:
+        return 0
 
     if patch:
         # we delay the saving until we know we have a patch.
@@ -526,11 +550,14 @@
             submitter.save()
             save_required = False
         patch.submitter = submitter
-        name, author = find_patch_name_and_author(comment)
+
+        if is_new_author:
+            author.save()
         if author is not None:
             patch.author = author
         if name is not None:
             patch.name = name
+
         patch.msgid = msgid
         patch.project = project
         patch.state = get_state(mail.get('X-Patchwork-State', '').strip())

=== modified file 'apps/patchwork/utils.py'
--- apps/patchwork/utils.py	2013-10-16 13:25:16 +0000
+++ apps/patchwork/utils.py	2013-12-05 18:37:48 +0000
@@ -29,7 +29,6 @@
 from difflib import SequenceMatcher
 from django.conf import settings
 from django.shortcuts import get_object_or_404
-from patchwork.bin.parsemail import parse_name_and_email
 from patchwork.models import Bundle, BundlePatch, Patch, Person, State
 from patchwork.parser import hash_patch, parse_patch
 
@@ -83,6 +82,33 @@
     return ids
 
 
+def parse_name_and_email(text):
+    # tuple of (regex, fn)
+    #  - where fn returns a (name, email) tuple from the match groups resulting
+    #    from re.match().groups()
+    from_res = [
+        # for "Firstname Lastname" <[e-mail]> style addresses
+        (re.compile('"?(.*?)"?\s*<([^>]+)>'), (lambda g: (g[0], g[1]))),
+
+        # for [e-mail] (Firstname Lastname) style addresses
+        (re.compile('"?(.*?)"?\s*\(([^\)]+)\)'), (lambda g: (g[1], g[0]))),
+
+        # everything else
+        (re.compile('(.*)'), (lambda g: (None, g[0]))),
+    ]
+
+    for regex, fn in from_res:
+        match = regex.match(text)
+        if match:
+            name, email = fn(match.groups())
+            if name is not None:
+                name = name.strip()
+            if email is not None:
+                email = email.strip()
+            return name, email
+    return None, None
+
+
 class Order(object):
     order_map = {
         'date':         'date',