On Wed, Jul 08, 2015 at 04:00:31PM -0000, Colin Watson wrote:
> Review: Approve
> 
> 
> 
> Diff comments:
> 
> > === modified file 'manual-status.py'
> > --- manual-status.py	2013-12-25 22:01:26 +0000
> > +++ manual-status.py	2015-07-07 21:29:21 +0000
> > @@ -244,23 +244,31 @@
> >      for uploaded, priority, package, user, uploader, source, \
> >              left_version, right_version in merges:
> >          if user is not None:
> > -            who = user
> > -            who = who.replace("&", "&")
> > -            who = who.replace("<", "&lt;")
> > -            who = who.replace(">", "&gt;")
> > +            (usr_name, usr_mail) = parseaddr(user)
> > +            user_lp_page = get_person_lp_page(usr_mail)
> > +            user = user.replace("&", "&amp;")
> > +            user = user.replace("<", "&lt;")
> > +            user = user.replace(">", "&gt;")
> > +            if user_lp_page:
> > +                who = "<a href='%s'>%s</a>" % (user_lp_page.encode("utf-8"), user)
> > +            else:
> > +                who = "%s" % user
> 
> user is already a string, so no need for the format operator here.

Fixed.

> >  
> >              if uploader is not None:
> > -                (usr_name, usr_mail) = parseaddr(user)
> >                  (upl_name, upl_mail) = parseaddr(uploader)
> > +                upl_lp_page = get_person_lp_page(upl_mail)
> >  
> >                  if len(usr_name) and usr_name != upl_name:
> >                      u_who = uploader
> >                      u_who = u_who.replace("&", "&amp;")
> >                      u_who = u_who.replace("<", "&lt;")
> >                      u_who = u_who.replace(">", "&gt;")
> > -
> > -                    who = "%s<br><small><em>Uploader:</em> %s</small>" \
> > -                            % (who, u_who)
> > +                    if upl_lp_page:
> > +                        who = "%s<br><small><em>Uploader:</em> <a href='%s'>%s</a></small>" \
> > +                                % (who, upl_lp_page.encode("utf-8"), u_who)
> > +                    else:
> > +                        who = "%s<br><small><em>Uploader:</em> %s</small>" \
> > +                               % (who, u_who)
> >          else:
> >              who = "&nbsp;"
> >  
> > === modified file 'merge-status.py'
> > --- merge-status.py	2013-08-02 10:09:52 +0000
> > +++ merge-status.py	2015-07-07 21:29:21 +0000
> > @@ -266,23 +266,31 @@
> >      for uploaded, priority, package, user, uploader, source, \
> >              base_version, left_version, right_version in merges:
> >          if user is not None:
> > -            who = user
> > -            who = who.replace("&", "&amp;")
> > -            who = who.replace("<", "&lt;")
> > -            who = who.replace(">", "&gt;")
> > +            (usr_name, usr_mail) = parseaddr(user)
> > +            user_lp_page = get_person_lp_page(usr_mail)
> > +            user = user.replace("&", "&amp;")
> > +            user = user.replace("<", "&lt;")
> > +            user = user.replace(">", "&gt;")
> > +            if user_lp_page:
> > +                who = "<a href='%s'>%s</a>" % (user_lp_page.encode("utf-8"), user)
> > +            else:
> > +                who = "%s" % user
> >  
> 
> user is already a string, so no need for the format operator here.

Also fixed.

> >              if uploader is not None:
> > -                (usr_name, usr_mail) = parseaddr(user)
> >                  (upl_name, upl_mail) = parseaddr(uploader)
> > +                upl_lp_page = get_person_lp_page(upl_mail)
> >  
> >                  if len(usr_name) and usr_name != upl_name:
> >                      u_who = uploader
> >                      u_who = u_who.replace("&", "&amp;")
> >                      u_who = u_who.replace("<", "&lt;")
> >                      u_who = u_who.replace(">", "&gt;")
> > -
> > -                    who = "%s<br><small><em>Uploader:</em> %s</small>" \
> > -                            % (who, u_who)
> > +                    if upl_lp_page:
> > +                        who = "%s<br><small><em>Uploader:</em> <a href='%s'>%s</a></small>" \
> > +                                % (who, upl_lp_page.encode("utf-8"), u_who)
> > +                    else:
> > +                        who = "%s<br><small><em>Uploader:</em> %s</small>" \
> > +                               % (who, u_who)
> >          else:
> >              who = "&nbsp;"
> >  
> > === modified file 'momlib.py'
> > --- momlib.py	2015-05-05 08:59:52 +0000
> > +++ momlib.py	2015-07-07 21:29:21 +0000
> > @@ -33,9 +33,12 @@
> >  import logging
> >  import datetime
> >  import stat
> > +import json
> >  
> >  from cgi import escape
> >  from optparse import OptionParser
> > +from urllib2 import urlopen, quote
> > +from contextlib import closing
> >  
> 
> urllib2.urlopen is as per my earlier feedback, thanks; but quote isn't documented as being exported from urllib2, even though it happens to be as an implementation detail.  Strictly I think this should be:
> 
>   from urllib import quote
>   from urllib2 import urlopen
> 
> ... instead.

Done, thanks!

> >  from deb.controlfile import ControlFile
> >  from deb.version import Version
> > @@ -169,6 +175,23 @@
> >      """Return an md5sum."""
> >      return md5(open(filename).read()).hexdigest()
> >  
> > +def get_person_lp_page(person_email):
> > +    if person_email in person_lp_page_mapping:
> > +        return person_lp_page_mapping[person_email]
> > +    email = quote(person_email)
> > +    find_person = "https://api.launchpad.net/devel/people/?ws.op=findPerson&text=%s" % email
> > +    try:
> > +        with closing(urlopen(find_person)) as response:
> > +            content = response.read()
> > +    except IOError:
> > +        return None
> > +    data = json.loads(content)["entries"]
> > +    if len(data) != 1:
> > +        person_lp_page_mapping[person_email] = None
> > +    else:
> > +        person_lp_page_mapping[person_email] = data[0]["web_link"]
> > +    return person_lp_page_mapping[person_email]
> > +
> 
> people.findPerson in fact, and somewhat inexplicably, does a startswith match, so it will return the collection of people with a name, display name, or e-mail address which (case-insensitively) starts with the address you provide.  So this query is somewhat imprecise.  Perhaps it would be best to scan through the list of returned entries and return the first, if any, where the e-mail address is an exact match.

To do this one would need to examine confirmed_email_addresses for each
person and this does not return entries if you are not logged in.
Since linking to the uploader's Launchpad web page is just a nice to
have, I'll risk the imprecision but add a comment.

> >  
> >  # --------------------------------------------------------------------------- #
> >  # Location functions
> 
> 
> -- 
> https://code.launchpad.net/~brian-murray/merge-o-matic/linkify-uploader/+merge/264066
> You are the owner of lp:~brian-murray/merge-o-matic/linkify-uploader.
> 
--
Brian Murray