launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Needs Fixing code
Hi Ian.

I reviewed your branch minus the IRC code. I think the implementation is good. I like the IAffilaition stub code. This branch is only missing feature flags:

=== modified file 'lib/canonical/launchpad/browser/vocabulary.py'
--- lib/canonical/launchpad/browser/vocabulary.py	2011-01-18 18:33:21 +0000
+++ lib/canonical/launchpad/browser/vocabulary.py	2011-05-24 02:25:53 +0000
...

> +    def getPickerEntry(self, associated_object):
> +        person = self.context
> +        extra = super(PersonPickerEntryAdapter, self).getPickerEntry(
> +            associated_object)
> +
> +        # If the person is affiliated with the associated_object then we can
> +        # display a badge.
> +        badge_name = IHasAffiliation(
> +            associated_object).getAffiliationBadge(person)
> +        if badge_name is not None:
> +            extra.image = "/@@/%s" % badge_name
> +        if person.preferredemail is not None:
> +            try:
> +                extra.description = person.preferredemail.email
> +            except Unauthorized:
> +                extra.description = '<email address hidden>'

I am surprised this works. Clearly it must since we use this code all the
time, but I expected a check of IPerson.hide_email_addresses. This code looks
like a team without an email address will be listed as '<email address
hidden>', which is not true.

> +        # We will display the person's irc nick(s) after their email address
> +        # in the description text.
> +        irc_nicks = None
> +        if person.ircnicknames:
> +            irc_nicks = ", ".join(
> +                [IRCNicknameFormatterAPI(ircid).displayname()
> +                for ircid in person.ircnicknames])
> +        if irc_nicks:
> +            extra.description = "%s (%s)" % (extra.description, irc_nicks)
> +        return extra

The new rules here must be guarded with a feature flag to ensure only
testers see our mistakes.

> === added file 'lib/lp/registry/model/pillaraffiliation.py'
> --- lib/lp/registry/model/pillaraffiliation.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/model/pillaraffiliation.py	2011-05-24 02:25:53 +0000
> @@ -0,0 +1,66 @@
> +# Copyright 2011 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Adaptors to figure out affiliations between people and pillars/bugs etc.
> +
> +When using a person in a given context, for example as a selection item in a
> +picker used to choose a bug task assignee, it is important to provide an
> +indication as to how that person may be affiliated with the context. Amongst
> +other reasons, this provides a visual cue that the correct person is being
> +selected for example.
> +
> +The adaptors herein are provided for various contexts so that for a given
> +person, the relevant affiliation details may be determined.
> +
> +"""

Match the spelling of used by the code below:
s/Adaptors/Adapters/
s/adaptors/adapters/


> === modified file 'lib/lp/registry/vocabularies.py'
> --- lib/lp/registry/vocabularies.py	2011-05-14 15:02:13 +0000
> +++ lib/lp/registry/vocabularies.py	2011-05-24 02:25:53 +0000
> ...
> @@ -186,7 +189,9 @@
>      def toTerm(self, obj):
>          """Return the term for this object."""
>          try:
> -            return SimpleTerm(obj, obj.name, obj.displayname)
> +            # Display the person's Launchpad id next to their name.
> +            title = "%s (~%s)" % (obj.displayname, obj.name)
> +            return SimpleTerm(obj, obj.name, title)

Only alpha testers should see this.
-- 
https://code.launchpad.net/~wallyworld/launchpad/person-picker-extra-detail/+merge/61973
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/person-picker-extra-detail into lp:launchpad.