launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/bugtracker-redhat-fixes into lp:launchpad.

Commit message:
Fix various failures to sync from Red Hat's Bugzilla instance.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1678486 in Launchpad itself: "Enable watching Red Hat Bugzilla bugs"
  https://bugs.launchpad.net/launchpad/+bug/1678486

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/bugtracker-redhat-fixes/+merge/332559

I'm sure there'll be more; this is just what I noticed when syncing a test batch of 10.  All these seem likely to be common.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/bugtracker-redhat-fixes into lp:launchpad.

=== modified file 'lib/lp/bugs/doc/externalbugtracker-bugzilla-api.txt'
--- lib/lp/bugs/doc/externalbugtracker-bugzilla-api.txt	2017-01-06 22:38:06 +0000
+++ lib/lp/bugs/doc/externalbugtracker-bugzilla-api.txt	2017-10-20 11:30:23 +0000
@@ -533,20 +533,28 @@
     >>> remote_bug = bug_watch_two.remotebug
     >>> transaction.commit()
 
-    >>> bugzilla.fetchComments(remote_bug, ['2', '4'])
-    >>> displayname, email = bugzilla.getPosterForComment(
-    ...     bug_watch_two.remotebug, '4')
+    >>> bugzilla.fetchComments(remote_bug, ['2', '4', '5'])
+    >>> displayname, email = bugzilla.getPosterForComment(remote_bug, '4')
     >>> print displayname, email
     Ford Prefect ford.prefect@xxxxxxxx
 
-If the author's name as stored in the comment isn't a valid email
-address then the method will return displayname = None and will return
-the whole of the author's name in the email field.
-
-    >>> displayname, email = bugzilla.getPosterForComment(
-    ...     bug_watch_two.remotebug, '2')
-    >>> print displayname, email
-    None trillian
+getPosterForComment() handles situations in which only an email address
+is supplied for the 'user' field by returning None as the user's
+displayname. When this is passed to IPersonSet.ensurePerson() a display
+name will be generated for the user from their email address.
+
+    >>> displayname, email = bugzilla.getPosterForComment(remote_bug, '5')
+    >>> print displayname, email
+    None arthur.dent@xxxxxxxxxxxxxxxxx
+
+getPosterForComment() will also return displayname, email tuples in
+cases where the 'user' field is set to a plain username (e.g. 'foo').
+However, in these cases it is the email address that will be set to
+None.
+
+    >>> displayname, email = bugzilla.getPosterForComment(remote_bug, '2')
+    >>> print displayname, email
+    trillian None
 
 
 getMessageForComment()
@@ -609,20 +617,20 @@
         'id': 1})
 
     >>> comment_id
-    '5'
+    '6'
 
 The comment will be stored on the remote server with the other comments.
 
     >>> bugzilla.xmlrpc_transport.print_method_calls = False
     >>> print sorted(bugzilla.getCommentIds(bug_watch.remotebug))
-    ['1', '3', '5']
+    ['1', '3', '6']
 
     >>> remote_bug = bug_watch.remotebug
     >>> transaction.commit()
 
-    >>> bugzilla.fetchComments(remote_bug, ['5'])
+    >>> bugzilla.fetchComments(remote_bug, ['6'])
     >>> message = bugzilla.getMessageForComment(
-    ...     bug_watch.remotebug, '5', sample_person)
+    ...     bug_watch.remotebug, '6', sample_person)
     >>> print message.text_contents
     This is a new remote comment.
     <BLANKLINE>

=== modified file 'lib/lp/bugs/doc/externalbugtracker-bugzilla-lp-plugin.txt'
--- lib/lp/bugs/doc/externalbugtracker-bugzilla-lp-plugin.txt	2016-09-21 02:49:42 +0000
+++ lib/lp/bugs/doc/externalbugtracker-bugzilla-lp-plugin.txt	2017-10-20 11:30:23 +0000
@@ -460,7 +460,7 @@
         'id': 1})
 
     >>> comment_id
-    '5'
+    '6'
 
 The comment will be stored on the remote server with the other comments.
 
@@ -469,13 +469,13 @@
 
     >>> bugzilla.xmlrpc_transport.print_method_calls = False
     >>> print sorted(bugzilla.getCommentIds(remote_bug))
-    ['1', '3', '5']
+    ['1', '3', '6']
 
     >>> transaction.commit()
 
-    >>> bugzilla.fetchComments(remote_bug, ['5'])
+    >>> bugzilla.fetchComments(remote_bug, ['6'])
     >>> message = bugzilla.getMessageForComment(
-    ...     remote_bug, '5', sample_person)
+    ...     remote_bug, '6', sample_person)
     >>> print message.text_contents
     This is a new remote comment.
     <BLANKLINE>

=== modified file 'lib/lp/bugs/doc/externalbugtracker-bugzilla.txt'
--- lib/lp/bugs/doc/externalbugtracker-bugzilla.txt	2016-07-04 17:08:29 +0000
+++ lib/lp/bugs/doc/externalbugtracker-bugzilla.txt	2017-10-20 11:30:23 +0000
@@ -307,6 +307,10 @@
     'Invalid'
     >>> external_bugzilla.convertRemoteStatus('CLOSED UPSTREAM').title
     "Won't Fix"
+    >>> external_bugzilla.convertRemoteStatus('CLOSED EOL').title
+    'Expired'
+    >>> external_bugzilla.convertRemoteStatus('CLOSED DEFERRED').title
+    'Expired'
 
 If the status can't be converted an UnknownRemoteStatusError will be
 returned.
@@ -406,6 +410,18 @@
     >>> external_bugzilla.convertRemoteImportance('P1').title
     'Low'
 
+A priority or severity of 'UNSPECIFIED' turns into
+BugTaskImportance.UNDECIDED unless the other field gives us something
+better.
+
+    >>> external_bugzilla.convertRemoteImportance('URGENT UNSPECIFIED').title
+    'Critical'
+    >>> external_bugzilla.convertRemoteImportance(
+    ...     'UNSPECIFIED UNSPECIFIED').title
+    'Undecided'
+    >>> external_bugzilla.convertRemoteImportance('UNSPECIFIED').title
+    'Undecided'
+
 Some bugzillas don't provide a value, resulting in blank strings for
 priority and severity.  We simply leave the importance unknown in this
 case.

=== modified file 'lib/lp/bugs/externalbugtracker/bugzilla.py'
--- lib/lp/bugs/externalbugtracker/bugzilla.py	2017-01-14 07:41:41 +0000
+++ lib/lp/bugs/externalbugtracker/bugzilla.py	2017-10-20 11:30:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Bugzilla ExternalBugTracker utility."""
@@ -25,6 +25,7 @@
 from zope.component import getUtility
 from zope.interface import implementer
 
+from lp.app.validators.email import valid_email
 from lp.bugs.externalbugtracker.base import (
     BugNotFound,
     BugTrackerAuthenticationError,
@@ -263,19 +264,22 @@
         'p1': BugTaskImportance.LOW,
         'enhancement': BugTaskImportance.WISHLIST,
         'wishlist': BugTaskImportance.WISHLIST,
+        'unspecified': BugTaskImportance.UNDECIDED,
         }
 
     def convertRemoteImportance(self, remote_importance):
         """See `ExternalBugTracker`."""
         words = remote_importance.lower().split()
-        try:
-            return self._importance_lookup[words.pop()]
-        except KeyError:
-            raise UnknownRemoteImportanceError(remote_importance)
-        except IndexError:
-            return BugTaskImportance.UNKNOWN
-
-        return BugTaskImportance.UNKNOWN
+        importance = BugTaskImportance.UNKNOWN
+        while importance in (
+                BugTaskImportance.UNKNOWN, BugTaskImportance.UNDECIDED):
+            try:
+                importance = self._importance_lookup[words.pop()]
+            except KeyError:
+                raise UnknownRemoteImportanceError(remote_importance)
+            except IndexError:
+                break
+        return importance
 
     _status_lookup_titles = 'Bugzilla status', 'Bugzilla resolution'
     _status_lookup = LookupTree(
@@ -296,6 +300,7 @@
                 ('WONTFIX', 'WILL_NOT_FIX', 'NOTOURBUG', 'UPSTREAM',
                  BugTaskStatus.WONTFIX),
                 ('OBSOLETE', 'INSUFFICIENT_DATA', 'INCOMPLETE', 'EXPIRED',
+                 'EOL', 'DEFERRED',
                  BugTaskStatus.EXPIRED),
                 ('INVALID', 'WORKSFORME', 'NOTABUG', 'CANTFIX',
                  'UNREPRODUCIBLE', 'DUPLICATE',
@@ -839,12 +844,18 @@
         comment = self._bugs[actual_bug_id]['comments'][comment_id]
         display_name, email = parseaddr(comment['author'])
 
-        # If the name is empty then we return None so that
-        # IPersonSet.ensurePerson() can actually do something with it.
-        if not display_name:
-            display_name = None
-
-        return (display_name, email)
+        # If the email isn't valid, return the email address as the
+        # display name (a Launchpad Person will be created with this
+        # name).
+        if not valid_email(email):
+            return email, None
+        # If the display name is empty, set it to None so that it's
+        # useable by IPersonSet.ensurePerson().
+        elif display_name == '':
+            return None, email
+        # Both displayname and email are valid, return both.
+        else:
+            return display_name, email
 
     def getMessageForComment(self, remote_bug_id, comment_id, poster):
         """See `ISupportsCommentImport`."""

=== modified file 'lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt'
--- lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt	2017-01-06 22:38:06 +0000
+++ lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt	2017-10-20 11:30:23 +0000
@@ -346,6 +346,13 @@
         text: I appear to have become a perfectly safe penguin.
         time: 2008-06-17 20:28:40
     <BLANKLINE>
+        author: arthur.dent@xxxxxxxxxxxxxxxxx
+        bug_id: 2
+        id: 5
+        is_private: False
+        text: I never could get the hang of Thursdays.
+        time: 2008-06-19 09:30:00
+    <BLANKLINE>
     <BLANKLINE>
 
 Passing a list of comment IDs to Bug.comments will cause it to return
@@ -404,6 +411,9 @@
     <BLANKLINE>
         author: Ford Prefect <ford.prefect@xxxxxxxx>
         id: 4
+    <BLANKLINE>
+        author: arthur.dent@xxxxxxxxxxxxxxxxx
+        id: 5
 
     >>> return_dict = server.Bug.comments(
     ...     {'comment_ids': [1, 4], 'include_fields': ('id', 'author')})
@@ -445,12 +455,12 @@
     >>> bugzilla_transport.auth_cookie = 'open sesame'
     >>> return_dict = server.Bug.add_comment({'id': 1, 'comment': comment})
     >>> return_dict
-    {'id': 5}
+    {'id': 6}
 
 The comment will be stored with the other comments on the remote server.
 
     >>> return_dict = server.Bug.comments({
-    ...     'id': [1], 'comment_ids': [5]})
+    ...     'id': [1], 'comment_ids': [6]})
     >>> comments_dict = return_dict['comments']
 
     >>> for comment_id, comment in comments_dict.items():
@@ -458,10 +468,10 @@
     ...     for comment_key in sorted(comment):
     ...         print "    %s: %s" % (
     ...             comment_key, comment[comment_key])
-    Comment 5:
+    Comment 6:
         author: launchpad
         bug_id: 1
-        id: 5
+        id: 6
         is_private: False
         text: I'm supposed to write something apposite here.
         time: ...

=== modified file 'lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt'
--- lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt	2016-09-21 02:49:42 +0000
+++ lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt	2017-10-20 11:30:23 +0000
@@ -451,17 +451,17 @@
     >>> return_dict = server.Launchpad.add_comment(
     ...     {'id': 1, 'comment': comment})
     >>> print return_dict['comment_id']
-    5
+    6
 
 The comment will be stored with the other comments on the remote server.
 
-    >>> return_dict = server.Launchpad.comments({'bug_ids': [1], 'ids': [5]})
+    >>> return_dict = server.Launchpad.comments({'bug_ids': [1], 'ids': [6]})
     >>> bugs_dict = return_dict['bugs']
 
     >>> print_bug_comments(bugs_dict, sort_key='id')
     Bug 1:
         author: launchpad
-        id: 5
+        id: 6
         number: 3
         text: Didn't we have a lovely time the day we went to Bangor?
         time: ...

=== modified file 'lib/lp/bugs/tests/externalbugtracker.py'
--- lib/lp/bugs/tests/externalbugtracker.py	2017-01-06 22:38:06 +0000
+++ lib/lp/bugs/tests/externalbugtracker.py	2017-10-20 11:30:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper classes for testing ExternalSystem."""
@@ -450,7 +450,7 @@
         }
 
     # Comments are mapped to bug IDs.
-    comment_id_index = 4
+    comment_id_index = 5
     new_comment_time = datetime(2008, 6, 20, 11, 42, 42)
     _bug_comments = {
         1: {
@@ -854,13 +854,20 @@
                 'text': "Bring the passengers to the bridge please Marvin.",
                 'time': datetime(2008, 6, 16, 13, 8, 8),
                 },
-             2: {'author': 'Ford Prefect <ford.prefect@xxxxxxxx>',
+            2: {'author': 'Ford Prefect <ford.prefect@xxxxxxxx>',
                 'bug_id': 2,
                 'id': 4,
                 'is_private': False,
                 'text': "I appear to have become a perfectly safe penguin.",
                 'time': datetime(2008, 6, 17, 20, 28, 40),
                 },
+            3: {'author': 'arthur.dent@xxxxxxxxxxxxxxxxx',
+                'bug_id': 2,
+                'id': 5,
+                'is_private': False,
+                'text': "I never could get the hang of Thursdays.",
+                'time': datetime(2008, 6, 19, 9, 30, 0),
+                },
             },
         }