From: Stephen Finucane <stephen.finucane@intel.com>
Date: Fri, 25 Mar 2016 15:42:04 +0000 (+0000)
Subject: parsemail: Always update Person.email
X-Git-Tag: v2.0.0-rc1~355
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d018cd5be5e007bebf3eaf219be7e19b4d331f67;p=thirdparty%2Fpatchwork.git

parsemail: Always update Person.email

Patches applied using pwclient will always use the first name that has
been seen for a particular email address. While this is usually fine,
ot actually causes issues for people changing name without changing
email addresses or people changing the capitalization of their name.
Resolve this by always updating the Person object when we receive a
new submission or comment.

Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
Reviewed-by: Andy Doan <andy.doan@linaro.org>
Closes: #24
---

diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py
index 388b629b..78a466e4 100755
--- a/patchwork/bin/parsemail.py
+++ b/patchwork/bin/parsemail.py
@@ -117,7 +117,7 @@ def find_project_by_header(mail):
 def find_author(mail):
 
     from_header = clean_header(mail.get('From'))
-    (name, email) = (None, None)
+    name, email = (None, None)
 
     # tuple of (regex, fn)
     #  - where fn returns a (name, email) tuple from the match groups resulting
@@ -150,14 +150,14 @@ def find_author(mail):
     if name is not None:
         name = name.strip()
 
-    save_required = False
     try:
         person = Person.objects.get(email__iexact=email)
+        if name:  # use the latest provided name
+            person.name = name
     except Person.DoesNotExist:
         person = Person(name=name, email=email)
-        save_required = True
 
-    return person, save_required
+    return person
 
 
 def find_date(mail):
@@ -478,7 +478,7 @@ def parse_mail(mail, list_id=None):
         return  # nothing to work with
 
     msgid = mail.get('Message-Id').strip()
-    author, save_required = find_author(mail)
+    author = find_author(mail)
     name, prefixes = clean_subject(mail.get('Subject'), [project.linkname])
     x, n = parse_series_marker(prefixes)
     refs = find_references(mail)
@@ -490,8 +490,7 @@ def parse_mail(mail, list_id=None):
 
     if diff or pull_url:  # patches or pull requests
         # we delay the saving until we know we have a patch.
-        if save_required:
-            author.save()
+        author.save()
 
         delegate = find_delegate(mail)
         if not delegate and diff:
@@ -528,8 +527,7 @@ def parse_mail(mail, list_id=None):
             is_cover_letter = True
 
         if is_cover_letter:
-            if save_required:
-                author.save()
+            author.save()
 
             cover_letter = CoverLetter(
                 msgid=msgid,
@@ -551,9 +549,7 @@ def parse_mail(mail, list_id=None):
     if not submission:
         return
 
-    # ...and we only save the author if we're saving the comment
-    if save_required:
-        author.save()
+    author.save()
 
     comment = Comment(
         submission=submission,
diff --git a/patchwork/tests/test_patchparser.py b/patchwork/tests/test_patchparser.py
index c935de63..7cc61eca 100644
--- a/patchwork/tests/test_patchparser.py
+++ b/patchwork/tests/test_patchparser.py
@@ -150,7 +150,7 @@ class SenderEncodingTest(TestCase):
                'Subject: test\n\n' + \
                'test'
         self.email = message_from_string(mail)
-        self.person, _ = find_author(self.email)
+        self.person = find_author(self.email)
         self.person.save()
 
     def tearDown(self):
@@ -213,6 +213,12 @@ class SubjectUTF8QPMultipleEncodingTest(SubjectEncodingTest):
 
 
 class SenderCorrelationTest(TestCase):
+    """Validate correct behavior of the find_author case.
+
+    Relies of checking the internal state of a Django model object.
+
+    http://stackoverflow.com/a/19379636/613428
+    """
     existing_sender = 'Existing Sender <existing@example.com>'
     non_existing_sender = 'Non-existing Sender <nonexisting@example.com>'
 
@@ -226,29 +232,29 @@ class SenderCorrelationTest(TestCase):
     def setUp(self):
         self.existing_sender_mail = self.mail(self.existing_sender)
         self.non_existing_sender_mail = self.mail(self.non_existing_sender)
-        self.person, _ = find_author(self.existing_sender_mail)
+        self.person = find_author(self.existing_sender_mail)
         self.person.save()
 
-    def testExisingSender(self):
-        person, save_required = find_author(self.existing_sender_mail)
-        self.assertEqual(save_required, False)
+    def testExistingSender(self):
+        person = find_author(self.existing_sender_mail)
+        self.assertEqual(person._state.adding, False)
         self.assertEqual(person.id, self.person.id)
 
-    def testNonExisingSender(self):
-        person, save_required = find_author(self.non_existing_sender_mail)
-        self.assertEqual(save_required, True)
+    def testNonExistingSender(self):
+        person = find_author(self.non_existing_sender_mail)
+        self.assertEqual(person._state.adding, True)
         self.assertEqual(person.id, None)
 
     def testExistingDifferentFormat(self):
         mail = self.mail('existing@example.com')
-        person, save_required = find_author(mail)
-        self.assertEqual(save_required, False)
+        person = find_author(mail)
+        self.assertEqual(person._state.adding, False)
         self.assertEqual(person.id, self.person.id)
 
     def testExistingDifferentCase(self):
         mail = self.mail(self.existing_sender.upper())
-        person, save_required = find_author(mail)
-        self.assertEqual(save_required, False)
+        person = find_author(mail)
+        self.assertEqual(person._state.adding, False)
         self.assertEqual(person.id, self.person.id)
 
     def tearDown(self):