launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 
> === 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
> @@ -296,6 +300,7 @@
>                  ('WONTFIX', 'WILL_NOT_FIX', 'NOTOURBUG', 'UPSTREAM',
>                   BugTaskStatus.WONTFIX),
>                  ('OBSOLETE', 'INSUFFICIENT_DATA', 'INCOMPLETE', 'EXPIRED',
> +                 'EOL', 'DEFERRED',
>                   BugTaskStatus.EXPIRED),

I'll sleep on it.  I'm probably happy either way, but yes, I might make at least the two new resolutions WONTFIX.

>                  ('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

Well, in this case it isn't an email address, it's just that:

    >>> from email.utils import parseaddr
    >>> parseaddr('username')
    ('', 'username')

Would you be more comfortable if this were just something like "if '@' not in email:", and we let the corner case of an email address that another system treats as valid but that Launchpad's validator doesn't remain as an OOPS?

And yes, we do: the same thing happens with Trac, and dogfood has 459 Person rows with creation_rationale = BUGIMPORT and no corresponding EmailAddress row.

> +        # 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`."""


-- 
https://code.launchpad.net/~cjwatson/launchpad/bugtracker-redhat-fixes/+merge/332559
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.