launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03715
[Merge] lp:~wallyworld/launchpad/person-picker-extra-detail into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/person-picker-extra-detail into lp:launchpad.
Requested reviews:
Launchpad UI Reviewers (launchpad-ui-reviewers): ui
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #669930 in Launchpad itself: "person picker isn't clear and unambiguous about the person being picked"
https://bugs.launchpad.net/launchpad/+bug/669930
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/person-picker-extra-detail/+merge/61973
Bug 669930: person picker isn't clear and unambiguous about the person being picked
To solve the issue, add extra detail to what's displayed in the person picker:
Display Name (~lp username)
Email Address (irc nicks)
Also we need to overlay a badge/icon on the person/team sprite to indicate the affiliation the person/team has with the pillar associated with the current context. This part isn't done yet but the hooks are in place. What's required is the business logic to do the affiliation check. Once it is determined how a user is affiliated (if at all), the idea is to return the name of the badge/icon. This is used as the "image" attribute in the lazr-js picker entry object. lazr-js renders it to the left of the entry title (display name) which at the moment means it will overlay the css sprite used for team/person. It doesn't look too bad but we need to work out exactly what UI we want, which may well require a lazr-js update.
== Implementation ==
Modify the existing adapters defined in vocabulary.py to make them classes and allowing the required PickerEntry object to be constructed via an API call to getPickerEntry(associated_object). The associated_object parameter is the current context object for which the picker is being created (BugTask etc). A new IHasAffiliation interface has been created with some adapter infrastructure. The idea is that the associated_object mentioned before will be adapted to an IHasAffiliation instance and the relevant affiliation determined.
The relevant code from the PersonPickerEntryAdaptor is something like:
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)
So a simple one line call is made to figure out the affiliation and the business logic to do so can be delivered via a subsequent branch.
An IrcNicknameFormatterAPI was also created to standardise the display formatting of irc nicks. The current implementation is to strip out just the core domain part of the nick url:
"fred" from network "irc.canonical.com" -> fred@canonical
"joe" from network "irc.freenode.net" -> joe@freenode
This was suggested in one of the bug comments but it's easy to change to whatever is deemed necessary.
There's also quite a few changes due to lint cleanup.
== Demo ==
http://people.canonical.com/~ianb/person-picker-extra-detail.png
== Tests ==
test_tales.py: add new TestIRCNicknameFormatterAPI test case
vocabularies-json.txt: extend existing tests to cover the new picker display attributes
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical/launchpad/browser/vocabulary.py
lib/canonical/launchpad/doc/vocabulary-json.txt
lib/canonical/launchpad/zcml/launchpad.zcml
lib/lp/app/browser/configure.zcml
lib/lp/app/browser/tales.py
lib/lp/app/tests/test_tales.py
lib/lp/registry/configure.zcml
lib/lp/registry/vocabularies.py
lib/lp/registry/model/pillaraffiliation.py
./lib/canonical/launchpad/doc/vocabulary-json.txt
1: narrative uses a moin header.
83: want exceeds 78 characters.
./lib/lp/registry/vocabularies.py
594: E261 at least two spaces before inline comment
--
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.
=== 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
@@ -6,16 +6,13 @@
__metaclass__ = type
__all__ = [
- 'branch_to_vocabularyjson',
- 'default_vocabularyjson_adapter',
'HugeVocabularyJSONView',
'IPickerEntry',
- 'person_to_vocabularyjson',
- 'sourcepackagename_to_vocabularyjson',
]
+import simplejson
+
from lazr.restful.interfaces import IWebServiceClientRequest
-import simplejson
from zope.app.form.interfaces import MissingInputError
from zope.app.schema.vocabulary import IVocabularyFactory
from zope.component import (
@@ -25,7 +22,6 @@
from zope.component.interfaces import ComponentLookupError
from zope.interface import (
Attribute,
- implementer,
implements,
Interface,
)
@@ -34,12 +30,16 @@
from canonical.launchpad.webapp.batching import BatchNavigator
from canonical.launchpad.webapp.interfaces import NoCanonicalUrl
from canonical.launchpad.webapp.publisher import canonical_url
-from lp.app.browser.tales import ObjectImageDisplayAPI
+from lp.app.browser.tales import (
+ IRCNicknameFormatterAPI,
+ ObjectImageDisplayAPI,
+ )
from canonical.launchpad.webapp.vocabulary import IHugeVocabulary
from lp.app.errors import UnexpectedFormData
from lp.code.interfaces.branch import IBranch
from lp.registry.interfaces.person import IPerson
from lp.registry.interfaces.sourcepackagename import ISourcePackageName
+from lp.registry.model.pillaraffiliation import IHasAffiliation
from lp.registry.model.sourcepackagename import getSourcePackageDescriptions
# XXX: EdwinGrubbs 2009-07-27 bug=405476
@@ -68,51 +68,90 @@
self.css = css
-@implementer(IPickerEntry)
@adapter(Interface)
-def default_pickerentry_adapter(obj):
+class DefaultPickerEntryAdapter(object):
"""Adapts Interface to IPickerEntry."""
- extra = PickerEntry()
- if hasattr(obj, 'summary'):
- extra.description = obj.summary
- display_api = ObjectImageDisplayAPI(obj)
- extra.css = display_api.sprite_css()
- if extra.css is None:
- extra.css = 'sprite bullet'
- return extra
-
-
-@implementer(IPickerEntry)
+
+ implements(IPickerEntry)
+
+ def __init__(self, context):
+ self.context = context
+
+ def getPickerEntry(self, associated_object):
+ """ Construct a PickerEntry for the context of this adaptor.
+
+ The associated_object represents the context for which the picker is
+ being rendered. eg a picker used to select a bug task assignee will
+ have associated_object set to the bug task.
+ """
+ extra = PickerEntry()
+ if hasattr(self.context, 'summary'):
+ extra.description = self.context.summary
+ display_api = ObjectImageDisplayAPI(self.context)
+ extra.css = display_api.sprite_css()
+ if extra.css is None:
+ extra.css = 'sprite bullet'
+ return extra
+
+
@adapter(IPerson)
-def person_to_pickerentry(person):
+class PersonPickerEntryAdapter(DefaultPickerEntryAdapter):
"""Adapts IPerson to IPickerEntry."""
- extra = default_pickerentry_adapter(person)
- if person.preferredemail is not None:
- try:
- extra.description = person.preferredemail.email
- except Unauthorized:
- extra.description = '<email address hidden>'
- return extra
-
-
-@implementer(IPickerEntry)
+
+ 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>'
+
+ # 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
+
+
@adapter(IBranch)
-def branch_to_pickerentry(branch):
+class BranchPickerEntryAdapter(DefaultPickerEntryAdapter):
"""Adapts IBranch to IPickerEntry."""
- extra = default_pickerentry_adapter(branch)
- extra.description = branch.bzr_identity
- return extra
-
-
-@implementer(IPickerEntry)
+
+ def getPickerEntry(self, associated_object):
+ branch = self.context
+ extra = super(BranchPickerEntryAdapter, self).getPickerEntry(
+ associated_object)
+ extra.description = branch.bzr_identity
+ return extra
+
+
@adapter(ISourcePackageName)
-def sourcepackagename_to_pickerentry(sourcepackagename):
+class SourcePackageNamePickerEntryAdapter(DefaultPickerEntryAdapter):
"""Adapts ISourcePackageName to IPickerEntry."""
- extra = default_pickerentry_adapter(sourcepackagename)
- descriptions = getSourcePackageDescriptions([sourcepackagename])
- extra.description = descriptions.get(
- sourcepackagename.name, "Not yet built")
- return extra
+
+ def getPickerEntry(self, associated_object):
+ sourcepackagename = self.context
+ extra = super(
+ SourcePackageNamePickerEntryAdapter, self).getPickerEntry(
+ associated_object)
+ descriptions = getSourcePackageDescriptions([sourcepackagename])
+ extra.description = descriptions.get(
+ sourcepackagename.name, "Not yet built")
+ return extra
class HugeVocabularyJSONView:
@@ -168,11 +207,12 @@
# needed for inplace editing via a REST call. The
# form picker doesn't need the api_url.
entry['api_uri'] = 'Could not find canonical url.'
- picker_entry = IPickerEntry(term.value)
+ picker_entry = IPickerEntry(term.value).getPickerEntry(
+ self.context)
if picker_entry.description is not None:
if len(picker_entry.description) > MAX_DESCRIPTION_LENGTH:
entry['description'] = (
- picker_entry.description[:MAX_DESCRIPTION_LENGTH-3]
+ picker_entry.description[:MAX_DESCRIPTION_LENGTH - 3]
+ '...')
else:
entry['description'] = picker_entry.description
=== modified file 'lib/canonical/launchpad/doc/vocabulary-json.txt'
--- lib/canonical/launchpad/doc/vocabulary-json.txt 2011-04-21 10:24:47 +0000
+++ lib/canonical/launchpad/doc/vocabulary-json.txt 2011-05-24 02:25:53 +0000
@@ -52,7 +52,7 @@
{
"api_uri": "/~guadamen",
"css": "sprite team",
- "title": "GuadaMen",
+ "title": "GuadaMen (~guadamen)",
"value": "guadamen"
}
],
@@ -80,7 +80,7 @@
{
"api_uri": "/~commercial-admins",
"css": "sprite team",
- "title": "Commercial Subscription Admins",
+ "title": "Commercial Subscription Admins (~commercial-admins)",
"value": "commercial-admins"
}
],
@@ -97,7 +97,7 @@
"api_uri": "/~name16",
"css": "sprite person",
"description": "<email address hidden>",
- "title": "Foo Bar",
+ "title": "Foo Bar (~name16)",
"value": "name16"
}
],
@@ -135,9 +135,28 @@
"api_uri": "/~name12",
"css": "sprite person",
"description": "<email address hidden>",
- "title": "Sample Person",
+ "title": "Sample Person (~name12)",
"value": "name12"
}
],
"total_size": 1
}
+
+IRC nicknames should be displayed after any email address.
+
+ >>> form = dict(name='ValidPersonOrTeam', search_text='mark',
+ ... start='0', batch='1')
+ >>> view = create_vocabulary_view(form)
+ >>> print_json(view())
+ {
+ "entries": [
+ {
+ "api_uri": "/~mark",
+ "css": "sprite person",
+ "description": "<email address hidden> (mark@freenode)",
+ "title": "Mark Shuttleworth (~mark)",
+ "value": "mark"
+ }
+ ],
+ "total_size": 1
+ }
=== modified file 'lib/canonical/launchpad/zcml/launchpad.zcml'
--- lib/canonical/launchpad/zcml/launchpad.zcml 2011-04-21 02:10:29 +0000
+++ lib/canonical/launchpad/zcml/launchpad.zcml 2011-05-24 02:25:53 +0000
@@ -136,19 +136,19 @@
/>
<adapter
- factory="canonical.launchpad.browser.vocabulary.default_pickerentry_adapter"
- />
-
- <adapter
- factory="canonical.launchpad.browser.vocabulary.person_to_pickerentry"
- />
-
- <adapter
- factory="canonical.launchpad.browser.vocabulary.branch_to_pickerentry"
- />
-
- <adapter
- factory="canonical.launchpad.browser.vocabulary.sourcepackagename_to_pickerentry"
+ factory="canonical.launchpad.browser.vocabulary.DefaultPickerEntryAdapter"
+ />
+
+ <adapter
+ factory="canonical.launchpad.browser.vocabulary.PersonPickerEntryAdapter"
+ />
+
+ <adapter
+ factory="canonical.launchpad.browser.vocabulary.BranchPickerEntryAdapter"
+ />
+
+ <adapter
+ factory="canonical.launchpad.browser.vocabulary.SourcePackageNamePickerEntryAdapter"
/>
<facet facet="overview">
=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml 2011-03-09 20:39:15 +0000
+++ lib/lp/app/browser/configure.zcml 2011-05-24 02:25:53 +0000
@@ -273,6 +273,13 @@
/>
<adapter
+ for="lp.registry.interfaces.irc.IIrcID"
+ provides="zope.traversing.interfaces.IPathAdapter"
+ factory="lp.app.browser.tales.IRCNicknameFormatterAPI"
+ name="fmt"
+ />
+
+ <adapter
for="datetime.timedelta"
provides="zope.traversing.interfaces.IPathAdapter"
factory="lp.app.browser.tales.DurationFormatterAPI"
=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py 2011-03-27 22:53:50 +0000
+++ lib/lp/app/browser/tales.py 2011-05-24 02:25:53 +0000
@@ -12,6 +12,7 @@
from email.Utils import formatdate
import math
import os.path
+import re
import rfc822
import sys
import urllib
@@ -497,7 +498,7 @@
# We are interested in the default value (param2).
result = ''
for nm in self.allowed_names:
- if name.startswith(nm+":"):
+ if name.startswith(nm + ":"):
name_parts = name.split(":")
name = name_parts[0]
if len(name_parts) > 2:
@@ -2051,7 +2052,7 @@
delta = abs(delta)
days = delta.days
hours = delta.seconds / 3600
- minutes = (delta.seconds - (3600*hours)) / 60
+ minutes = (delta.seconds - (3600 * hours)) / 60
seconds = delta.seconds % 60
result = ''
if future:
@@ -2129,7 +2130,7 @@
parts = []
minutes, seconds = divmod(self._duration.seconds, 60)
hours, minutes = divmod(minutes, 60)
- seconds = seconds + (float(self._duration.microseconds) / 10**6)
+ seconds = seconds + (float(self._duration.microseconds) / 10 ** 6)
if self._duration.days > 0:
if self._duration.days == 1:
parts.append('%d day' % self._duration.days)
@@ -2175,7 +2176,7 @@
# including the decimal part.
seconds = self._duration.days * (3600 * 24)
seconds += self._duration.seconds
- seconds += (float(self._duration.microseconds) / 10**6)
+ seconds += (float(self._duration.microseconds) / 10 ** 6)
# First we'll try to calculate an approximate number of
# seconds up to a minute. We'll start by defining a sorted
@@ -2655,3 +2656,26 @@
return getattr(self, name)(furtherPath)
except AttributeError:
raise TraversalError(name)
+
+
+class IRCNicknameFormatterAPI(ObjectFormatterAPI):
+ """Adapter from IrcID objects to a formatted string."""
+
+ implements(ITraversable)
+
+ traversable_names = {
+ 'displayname': 'displayname',
+ }
+
+ def __init__(self, context):
+ self.context = context
+
+ def displayname(self, view_name=None):
+ # We shorten the full irc network to just the core network
+ # name. eg irc.freenode.net -> freenode
+ network = self.context.network
+ irc_match = re.search(r'irc\.(.*)\..*', network)
+ if irc_match:
+ network = irc_match.group(1)
+ # Then we return something like nick@network
+ return "%s@%s" % (self.context.nickname, network)
=== modified file 'lib/lp/app/tests/test_tales.py'
--- lib/lp/app/tests/test_tales.py 2011-04-21 01:44:06 +0000
+++ lib/lp/app/tests/test_tales.py 2011-05-24 02:25:53 +0000
@@ -5,7 +5,10 @@
from lxml import html
-from zope.component import getAdapter
+from zope.component import (
+ getAdapter,
+ getUtility
+ )
from zope.traversing.interfaces import (
IPathAdapter,
TraversalError,
@@ -19,6 +22,7 @@
format_link,
PersonFormatterAPI,
)
+from lp.registry.interfaces.irc import IIrcIDSet
from lp.testing import (
test_tales,
TestCaseWithFactory,
@@ -141,7 +145,8 @@
# The rendering of an object's link ignores any specified default
# value which would be used in the case where the object were None.
person = self.factory.makePerson()
- person_link = test_tales('person/fmt:link::default value', person=person)
+ person_link = test_tales(
+ 'person/fmt:link::default value', person=person)
self.assertEqual(PersonFormatterAPI(person).link(None), person_link)
person_link = test_tales(
'person/fmt:link:bugs:default value', person=person)
@@ -264,3 +269,17 @@
extra = ['1', '2']
self.assertEqual('', traverse('shorten', extra))
self.assertEqual(['1'], extra)
+
+
+class TestIRCNicknameFormatterAPI(TestCaseWithFactory):
+ """Tests for IRCNicknameFormatterAPI"""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_format_nick(self):
+ person = self.factory.makePerson(name='fred')
+ ircset = getUtility(IIrcIDSet)
+ ircID = ircset.new(person, "irc.canonical.com", "fred")
+ self.assertEqual(
+ 'fred@canonical',
+ test_tales('nick/fmt:displayname', nick=ircID))
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2011-05-14 15:02:13 +0000
+++ lib/lp/registry/configure.zcml 2011-05-24 02:25:53 +0000
@@ -862,6 +862,14 @@
factory="lp.registry.browser.person.TeamBreadcrumb"
permission="zope.Public"/>
+ <adapter
+ factory="lp.registry.model.pillaraffiliation.BugTaskPillarAffiliation"
+ />
+
+ <adapter
+ factory="lp.registry.model.pillaraffiliation.PillarAffiliation"
+ />
+
<!-- Using
permission="Public" here causes some tests to fail
=== 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.
+
+"""
+
+__metaclass__ = type
+
+__all__ = [
+ 'IHasAffiliation',
+ ]
+
+from zope.component import adapter
+from zope.interface import (
+ implements,
+ Interface,
+ )
+
+from lp.bugs.interfaces.bugtask import IBugTask
+
+
+class IHasAffiliation(Interface):
+ """The affiliation status of a person with a context."""
+
+ def getAffiliationBadge(person):
+ """Return the badge name for the type of affiliation the person has.
+
+ If the person has no affiliation with this object, return None.
+ """
+
+
+@adapter(Interface)
+class PillarAffiliation(object):
+ """Default affiliation adaptor.
+
+ No affiliation is returned.
+ """
+
+ implements(IHasAffiliation)
+
+ def __init__(self, context):
+ self.context = context
+
+ def getAffiliationBadge(self, person):
+ return None
+
+
+# XXX: wallyworld 2011-05-24 bug=81692: TODO Work is required to determine
+# exactly what is required in terms of figuring out affiliation..
+
+@adapter(IBugTask)
+class BugTaskPillarAffiliation(PillarAffiliation):
+ """An affiliation adaptor for bug tasks."""
+
+ def getAffiliationBadge(self, person):
+ return None
=== 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
@@ -50,7 +50,6 @@
'SourcePackageNameVocabulary',
'UserTeamsParticipationPlusSelfVocabulary',
'UserTeamsParticipationVocabulary',
- 'ValidPersonOrClosedTeamVocabulary',
'ValidPersonOrTeamVocabulary',
'ValidPersonVocabulary',
'ValidTeamMemberVocabulary',
@@ -100,6 +99,9 @@
SQLBase,
sqlvalues,
)
+from canonical.launchpad.components.decoratedresultset import (
+ DecoratedResultSet,
+ )
from canonical.launchpad.database.emailaddress import EmailAddress
from canonical.launchpad.helpers import (
ensure_unicode,
@@ -167,7 +169,7 @@
from lp.registry.model.karma import KarmaCategory
from lp.registry.model.mailinglist import MailingList
from lp.registry.model.milestone import Milestone
-from lp.registry.model.person import Person
+from lp.registry.model.person import Person, IrcID
from lp.registry.model.pillar import PillarName
from lp.registry.model.product import Product
from lp.registry.model.productrelease import ProductRelease
@@ -175,6 +177,7 @@
from lp.registry.model.projectgroup import ProjectGroup
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.teammembership import TeamParticipation
+from lp.services.database import bulk
from lp.services.propertycache import cachedproperty
@@ -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)
except Unauthorized:
return None
@@ -580,7 +585,6 @@
EmailAddressStatus.PREFERRED.value,
self.LIMIT))
-
public_result = self.store.using(*public_tables).find(
Person,
And(
@@ -644,7 +648,14 @@
else:
result.order_by(Person.displayname, Person.name)
result.config(limit=self.LIMIT)
- return result
+
+ # We will be displaying the person's irc nic(s) in the description
+ # so we need to bulk load them in one query for performance.
+ def pre_iter_hook(rows):
+ persons = set(obj for obj in rows)
+ bulk.load_referencing(IrcID, persons, ['personID'])
+
+ return DecoratedResultSet(result, pre_iter_hook=pre_iter_hook)
def search(self, text):
"""Return people/teams whose fti or email address match :text:."""
@@ -1433,7 +1444,7 @@
def __iter__(self):
series = self._table.select(
- DistroSeries.q.distributionID==Distribution.q.id,
+ DistroSeries.q.distributionID == Distribution.q.id,
orderBy=self._orderBy, clauseTables=self._clauseTables)
for series in sorted(series, key=attrgetter('sortkey')):
yield self.toTerm(series)
Follow ups
