Merge lp:~sinzui/launchpad/oil-and-pigment into lp:launchpad

[Curtis Hovey]

This is my branch to fix from trivial issues while my brain is stuck in
first gear.

    lp:~sinzui/launchpad/oil-and-pigment
    Diff size: 489
    Launchpad bug: https://bugs.launchpad.net/bugs/436299
                   https://bugs.launchpad.net/bugs/568336
                   https://bugs.launchpad.net/bugs/130285
                   https://bugs.launchpad.net/bugs/557544
    Test command: ./bin/test -vv lp.registry.tests.test_person
        ./bin/test -t site-search -t xx-team-home -t xx-private-membership \
            -t person-karma
    Pre-implementation: no one
    Target release: 10.04

Fix from trivial issues while my brain is stuck in first gear
--------------------------------------------------------------------

Bug #436299 [Search results give wrong "Registered by" information]
    The maintainer is listed as the registrant. This is wrong. The
    problem is that distributions do not have a registrant.

Bug #568336 [Typo on profile workload page]
    There is a double 'or'

Bug #130285 [Disregard deleted projects from Most active in]
    Deactivated projects are listed in "Most active in" in the profile page
    and the links are a 404.

Bug #557544 [Bad plural on team page ("1 active members")]
    What: "1 active members" can appear on team page. This is incorrect use
    of a plural.

Rules
-----

Bug #436299 [Search results give wrong "Registered by" information]
    Verify there is a registrant using tales before appending the clause.
    tal:define="registrant view/pillar/registrant|nothing"
    tal:condition="registrant"

Bug #568336 [Typo on profile workload page]
    Remove the second 'or'.

Bug #130285 [Disregard deleted projects from lists]
    This issue in on the cusp of *not* trivial because a good understanding
    of pillar name behaviour and karma testing is needed. The fix can be
    easily placed into an existing doc test, but that is not the correct
    location.
    * pillar names (used by karma cache) have no concept of active/inactive.
      The method must filter deactivate pillars /after/ the query to get the
      names. The callsite must request more than is needed because of the
      filtering.
    * Generate the karma for a unit test of the method *and* the private
      methods that were never directly tested.
    * Test that deactivated pillars are not included.
    * ADDENDUM: While I knew how to generate the karma, the reason why
      the karma looks like it is inserted twice was not obvious from existing
      tests. I took an extra hour learning this and adding comments for
      future developers.

Bug #557544 [Bad plural on team page ("1 active members")]
    * Use the plural-message macro to switch between member and members

QA
--

Bug #436299 [Search results give wrong "Registered by" information]
    * Visit https://edge.launchpad.net/+search?field.text=wesnoth
    * Verify that Karianne Fog Heen is listed as the registrant
    * Visit https://edge.launchpad.net/+search?field.text=ubuntu
    * Verify that no registrant is listed:
      Registered on <date>

Bug #568336 [Typo on profile workload page]
    * Visit https://blueprints.edge.launchpad.net/~humphreybc/+specworkload
    * Verify text:
      Benjamin Humphrey is expected to work on, or is its creator

Bug #130285 [Disregard deleted projects from lists]
    * Visit https://edge.launchpad.net/~simon-zoellner
    * Verify that PHProute is not in "Most active in"

Bug #557544 [Bad plural on team page ("1 active members")]
    * Visit https://launchpad.net/~halfgeek.org-ixan
    * Verify the page says 1 active member

Lint
----

Linting changed files:
  lib/lp/app/stories/launchpad-root/site-search.txt
  lib/lp/app/templates/base-layout-macros.pt
  lib/lp/app/templates/launchpad-search.pt
  lib/lp/blueprints/templates/person-specworkload.pt
  lib/lp/registry/doc/person-karma.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/stories/team/xx-team-home.txt
  lib/lp/registry/stories/teammembership/xx-private-membership.txt
  lib/lp/registry/templates/team-portlet-membership.pt
  lib/lp/registry/tests/test_person.py

Test
----

    * lib/lp/app/stories/launchpad-root/site-search.txt
      * Verified that distributions do not get a "registered by" line.
    * lib/lp/registry/doc/person-karma.txt
      * Moved test from doctest to unittest.
    * lib/lp/registry/stories/team/xx-team-home.txt
      * Verified singural and plural cases.
    * lib/lp/registry/stories/teammembership/xx-private-membership.txt
      * Verified singural and plural cases.
    * lib/lp/registry/tests/test_person.py
      * Lint hated this file. I removed a lot of unused imports and vars.
      * I switched the tests to run on the faster DatabaseFunctionalLayer.
      * Added a test for Person.getProjectsAndCategoriesContributedTo()
        and its private helpers. This also tests the new rule to not include
        deactivate projects.

Implementation
--------------

    * lib/lp/app/templates/base-layout-macros.pt
      * Discovered that the macro generated white-space that is inappropirate
        for anchors and punctuation.
    * lib/lp/app/templates/launchpad-search.pt
      * Updated the template to use registrant, not owner, and only print
        the "registered by" line if the pillar has a registrant.
    * lib/lp/blueprints/templates/person-specworkload.pt
      * Removed redundant "or".
    * lib/lp/registry/model/person.py
      * Request 5 extra pillar names and built a list of 5 active pillars.
    * lib/lp/registry/templates/team-portlet-membership.pt
      * Fixed plural rule for active, proposed, and pending membership.
        Sorry that this makes the template hard to read.

[Abel Deuring (adeuring)]

Hi Curtis,

overall, a nice branch. I have though a few qestions regarding the
filtering of inactive projects for the karma display, so I'm marking
the MP as "needs information".

> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py 2010-04-19 21:16:12 +0000
> +++ lib/lp/registry/model/person.py 2010-04-22 18:05:27 +0000
> @@ -850,12 +850,16 @@
> def getProjectsAndCategoriesContributedTo(self, limit=5):
> """See `IPerson`."""
> contributions = []
> - results = self._getProjectsWithTheMostKarma(limit=limit)
> - for pillar_name, karma in results:
> + # Pillars names have no concept of active. Extra pillars names are
> + # requested because deactivated pillars will be filtered out.
> + extra_limit = limit + 5
> + results = self._getProjectsWithTheMostKarma(limit=extra_limit)
> + for pillar_name, karma in results[:limit]:

While you retrieve 5 extra items calling _getProjectsWithTheMostKarma(),
you remove these extra items here ;)

> pillar = getUtility(IPillarNameSet).getByName(pillar_name)
> - contributions.append(
> - {'project': pillar,
> - 'categories': self._getContributedCategories(pillar)})
> + if pillar.active:
> + contributions.append(
> + {'project': pillar,
> + 'categories': self._getContributedCategories(pillar)})
> return contributions

I think that should be

          return contributions[:limit]

combined with a loop over the full result set.

But: Did you consider to change the SQL query in the method
_getProjectsWithTheMostKarma() itself so that only activ projects
are returned (if necessary, using an optional method parameter
"active_only=False")?

While adding five extra items to the result set looks reasonable,
there is the (admittedly largely theoretical) situation that
the result set contains more than five inactive projects, so that
the length of the "effective" list is shorter than expected.

And I would prefer it anyway to do the filtering in
_getProjectsWithTheMostKarma(), even if it would be too cumbersome
to implement in SQL.

[...]

> === modified file 'lib/lp/registry/tests/test_person.py'
> --- lib/lp/registry/tests/test_person.py 2010-02-11 19:26:26 +0000
> +++ lib/lp/registry/tests/test_person.py 2010-04-22 18:05:27 +0000

[...]

> @@ -597,5 +601,103 @@
> assignee=self.user)
>
>
> +class TestPersonKarma(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(TestPersonKarma, self).setUp()
> + self.person = self.factory.makePerson()
> + a_product = self.factory.makeProduct(name='aa')
> + b_product = self.factory.makeProduct(name='bb')
> + self.c_product = self.factory.makeProduct(name='cc')
> + self.cache_manager = getUtility(IKarmaCacheManager)
> + self._makeKarmaCache(
> + self.person, a_product, [('bugs', 10)])
> + self._makeKarmaCache(
> + self.person, b_product, [('answers', 50)])
> + self._makeKarmaCache(
> + self....

review: Needs Information (code)

[Curtis Hovey (sinzui)]

On Fri, 2010-04-23 at 13:41 +0000, Abel Deuring wrote:
> Review: Needs Information code
> Hi Curtis,
>
> overall, a nice branch. I have though a few qestions regarding the
> filtering of inactive projects for the karma display, so I'm marking
> the MP as "needs information".
>
> > === modified file 'lib/lp/registry/model/person.py'
> > --- lib/lp/registry/model/person.py 2010-04-19 21:16:12 +0000
> > +++ lib/lp/registry/model/person.py 2010-04-22 18:05:27 +0000
> > @@ -850,12 +850,16 @@
> > def getProjectsAndCategoriesContributedTo(self, limit=5):
> > """See `IPerson`."""
> > contributions = []
> > - results = self._getProjectsWithTheMostKarma(limit=limit)
> > - for pillar_name, karma in results:
> > + # Pillars names have no concept of active. Extra pillars names are
> > + # requested because deactivated pillars will be filtered out.
> > + extra_limit = limit + 5
> > + results = self._getProjectsWithTheMostKarma(limit=extra_limit)
> > + for pillar_name, karma in results[:limit]:
>
> While you retrieve 5 extra items calling _getProjectsWithTheMostKarma(),
> you remove these extra items here ;)

I am incompetent. I removed the limit on the iterator. I added a guard
in the loop to break at the limit.

> > pillar = getUtility(IPillarNameSet).getByName(pillar_name)
> > - contributions.append(
> > - {'project': pillar,
> > - 'categories': self._getContributedCategories(pillar)})
> > + if pillar.active:
> > + contributions.append(
> > + {'project': pillar,
> > + 'categories': self._getContributedCategories(pillar)})
> > return contributions
>
> I think that should be
>
> return contributions[:limit]
>
> combined with a loop over the full result set.

I prefer a guard in the loop. The loop may make 5 extra SQL queries for
data that will not be used. _getContributedCategories() is a call to the
db.

> But: Did you consider to change the SQL query in the method
> _getProjectsWithTheMostKarma() itself so that only activ projects
> are returned (if necessary, using an optional method parameter
> "active_only=False")?

Yes, the query would insane because it is often nothing to join on. We
are querying pillar names, not objects. that is why our code always
filters *after* the pillar_name is used to get the pillar (product,
distribution, project group)

> While adding five extra items to the result set looks reasonable,
> there is the (admittedly largely theoretical) situation that
> the result set contains more than five inactive projects, so that
> the length of the "effective" list is shorter than expected.

Yes. This hack is the same approach taken when getting a short list of
bugs that may have private ones filtered out. The complaints from users
is about 1 deactivated project in the list. We are providing coverage
for 5 deactivated projects.

> And I would prefer it anyway to do the filtering in
> _getProjectsWithTheMostKarma(), even if it would be too cumbersome
> to implement in SQL.

I do not think that is practical. As I said above, ...

[Abel Deuring (adeuring)]

OK, looks good now. (Though I would still prefer to move the filtering to _getprojectsWithMostKarma(), even if it is implemented in Python)

review: Approve (code)

=== modified file 'lib/lp/app/stories/launchpad-root/site-search.txt'
--- lib/lp/app/stories/launchpad-root/site-search.txt	2010-04-16 15:06:55 +0000
+++ lib/lp/app/stories/launchpad-root/site-search.txt	2010-04-23 15:21:38 +0000
@@ -173,8 +173,7 @@
     Ubuntu
     Ubuntu is a new approach to Linux Distribution that includes regular
     releases, and a simplified single-CD installation system.
-    Registered by Ubuntu Team
-    on 2006-10-16
+    Registered on 2006-10-16
 
 The user enters the number 1, and he sees a bug and a question in the
 "Exact matches" section.

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2010-04-09 12:30:19 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2010-04-23 15:21:38 +0000
@@ -438,11 +438,9 @@
   </tal:comment>
   <tal:singular
     condition="python: count == 1"
-    replace="singular" />
-  <tal:plural
+    replace="singular" /><tal:plural
      define="l_default string:s;
              l_plural plural | string:$singular$l_default;"
     condition="python: count != 1"
-    replace="l_plural" />
-</metal:plural-msg>
+    replace="l_plural" /></metal:plural-msg>
 </macros>

=== modified file 'lib/lp/app/templates/launchpad-search.pt'
--- lib/lp/app/templates/launchpad-search.pt	2010-04-16 15:06:55 +0000
+++ lib/lp/app/templates/launchpad-search.pt	2010-04-23 15:21:38 +0000
@@ -97,8 +97,13 @@
                       Project summary
                   </div>
                   <div>
-                    Registered by <a
-                      tal:replace="structure view/pillar/owner/fmt:link">Foo Bar</a>
+                    Registered
+                    <tal:registrant
+                      define="registrant view/pillar/registrant|nothing"
+                      condition="registrant">
+                      by
+                      <a tal:replace="structure registrant/fmt:link">Foo Bar</a>
+                    </tal:registrant>
                     <tal:XXX condition="nothing">
                       # XXX sinzui 2008-05-27:
                       # Product and ProjectGroup still use the wrong name for

=== modified file 'lib/lp/blueprints/templates/person-specworkload.pt'
--- lib/lp/blueprints/templates/person-specworkload.pt	2009-09-15 21:24:28 +0000
+++ lib/lp/blueprints/templates/person-specworkload.pt	2010-04-23 15:21:38 +0000
@@ -13,7 +13,7 @@
   <p>
     This page lists the specifications that
     <tal:name replace="context/displayname" /> is expected to work on, or
-    or is its creator.
+    is its creator.
   </p>
 
   <tal:team_definition define="is_team context/isTeam" >

=== modified file 'lib/lp/registry/doc/person-karma.txt'
--- lib/lp/registry/doc/person-karma.txt	2010-03-15 12:32:37 +0000
+++ lib/lp/registry/doc/person-karma.txt	2010-04-23 15:21:38 +0000
@@ -101,14 +101,6 @@
     Mozilla Thunderbird [u'bugs']
     Mozilla Firefox [u'bugs']
 
-As we can see, the results are ordered descending by the karma of the person
-on that project.
-
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> print removeSecurityProxy(foobar)._getProjectsWithTheMostKarma()
-    [(u'evolution', 175), (u'ubuntu', 26), (u'gnomebaker', 15),
-     (u'thunderbird', 15), (u'firefox', 8), (u'bazaar', 2)]
-
 
 Karma Updater
 =============

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2010-04-18 18:17:05 +0000
+++ lib/lp/registry/model/mailinglist.py	2010-04-23 15:21:38 +0000
@@ -294,7 +294,9 @@
             if email.id == self.team.preferredemail.id:
                 self.team.setContactAddress(None)
         assert email.personID == self.teamID, 'Incorrectly linked email.'
-        email.status = EmailAddressStatus.NEW
+        # Anyone with permission to deactivate a list can also set the
+        # email address status to NEW.
+        removeSecurityProxy(email).status = EmailAddressStatus.NEW
 
     def reactivate(self):
         """See `IMailingList`."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-04-19 21:16:12 +0000
+++ lib/lp/registry/model/person.py	2010-04-23 15:21:38 +0000
@@ -850,12 +850,19 @@
     def getProjectsAndCategoriesContributedTo(self, limit=5):
         """See `IPerson`."""
         contributions = []
-        results = self._getProjectsWithTheMostKarma(limit=limit)
+        # Pillars names have no concept of active. Extra pillars names are
+        # requested because deactivated pillars will be filtered out.
+        extra_limit = limit + 5
+        results = self._getProjectsWithTheMostKarma(limit=extra_limit)
         for pillar_name, karma in results:
-            pillar = getUtility(IPillarNameSet).getByName(pillar_name)
-            contributions.append(
-                {'project': pillar,
-                 'categories': self._getContributedCategories(pillar)})
+            pillar = getUtility(IPillarNameSet).getByName(
+                pillar_name, ignore_inactive=True)
+            if pillar is not None:
+                contributions.append(
+                    {'project': pillar,
+                     'categories': self._getContributedCategories(pillar)})
+            if len(contributions) == limit:
+                break
         return contributions
 
     def _getProjectsWithTheMostKarma(self, limit=10):

=== modified file 'lib/lp/registry/stories/team/xx-team-home.txt'
--- lib/lp/registry/stories/team/xx-team-home.txt	2010-02-25 23:53:19 +0000
+++ lib/lp/registry/stories/team/xx-team-home.txt	2010-04-23 15:21:38 +0000
@@ -47,7 +47,7 @@
 
     >>> print extract_text(
     ...     find_tag_by_id(browser.contents, 'membership-summary'))
-    10 active members, 1 invited members, 2 proposed members...
+    10 active members, 1 invited member, 2 proposed members...
 
     >>> print extract_text(
     ...     find_tag_by_id(browser.contents, 'contact-email'))

=== modified file 'lib/lp/registry/stories/teammembership/xx-private-membership.txt'
--- lib/lp/registry/stories/teammembership/xx-private-membership.txt	2010-04-19 08:11:52 +0000
+++ lib/lp/registry/stories/teammembership/xx-private-membership.txt	2010-04-23 15:21:38 +0000
@@ -48,7 +48,7 @@
 
     >>> print extract_text(
     ...     find_tag_by_id(browser.contents, 'membership-summary'))
-    2 active members, 1 proposed members...
+    2 active members, 1 proposed member...
 
 
 == Team Participation ==

=== modified file 'lib/lp/registry/templates/team-portlet-membership.pt'
--- lib/lp/registry/templates/team-portlet-membership.pt	2010-02-25 23:53:19 +0000
+++ lib/lp/registry/templates/team-portlet-membership.pt	2010-04-23 15:21:38 +0000
@@ -16,27 +16,33 @@
   <div id="membership-summary" class="portletBody portletContent"
     style="margin-bottom: 1.5em;">
     <div>
-      <div id="membership-counts">
-        <img src="/@@/team" alt="team" />
-        <strong id="approved-member-count">
+      <div id="membership-counts"
+        tal:define="singular string:member; plural string:members">
+        <strong class="sprite team" id="approved-member-count">
           <tal:active content="context/all_member_count" />
         </strong>
         <a tal:attributes="href string:${context/fmt:url/+members}#active"
-          >active members</a><tal:invited
-            define="invited_member_count context/invited_member_count"
-            condition="invited_member_count">,
-          <strong id="invited-member-count">
-            <tal:invited_count content="context/invited_member_count" />
-          </strong>
-          <a tal:attributes="href string:${context/fmt:url/+members}#invited"
-            >invited members</a></tal:invited><tal:proposed
-              define="proposed_member_count context/proposed_member_count"
-              condition="proposed_member_count">,
-          <strong id="proposed-member-count">
-            <tal:proposed_count content="proposed_member_count" />
-          </strong>
-          <a tal:attributes="href string:${context/fmt:url/+members}#proposed"
-            >proposed members</a>
+          tal:define="count context/all_member_count">active
+          <tal:plural
+            metal:use-macro="context/@@+base-layout-macros/plural-message"
+            /></a><tal:invited
+          define="count context/invited_member_count" condition="count">,
+            <strong id="invited-member-count">
+              <tal:invited_count content="count" />
+            </strong>
+            <a tal:attributes="href string:${context/fmt:url/+members}#invited"
+              >invited
+              <tal:plural
+                metal:use-macro="context/@@+base-layout-macros/plural-message"
+              /></a></tal:invited><tal:proposed
+          define="count context/proposed_member_count" condition="count">,
+            <strong id="proposed-member-count">
+              <tal:proposed_count content="count" />
+            </strong>
+            <a tal:attributes="href string:${context/fmt:url/+members}#proposed"
+              >proposed
+              <tal:plural
+                metal:use-macro="context/@@+base-layout-macros/plural-message"/></a>
         </tal:proposed>
       </div>
     </div>

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-02-11 19:26:26 +0000
+++ lib/lp/registry/tests/test_person.py	2010-04-23 15:21:38 +0000
@@ -7,6 +7,8 @@
 from datetime import datetime
 import pytz
 
+import transaction
+
 from zope.component import getUtility
 from zope.interface import providedBy
 from zope.security.proxy import removeSecurityProxy
@@ -19,6 +21,7 @@
     EmailAddressAlreadyTaken, IEmailAddressSet, InvalidEmailAddress)
 from lazr.lifecycle.snapshot import Snapshot
 from lp.blueprints.interfaces.specification import ISpecificationSet
+from lp.registry.interfaces.karma import IKarmaCacheManager
 from lp.registry.interfaces.person import InvalidName
 from lp.registry.interfaces.product import IProductSet
 from lp.registry.interfaces.mailinglist import IMailingListSet
@@ -28,6 +31,7 @@
 from canonical.launchpad.database import Bug, BugTask, BugSubscription
 from lp.registry.model.structuralsubscription import (
     StructuralSubscription)
+from lp.registry.model.karma import KarmaCategory
 from lp.registry.model.person import Person
 from lp.bugs.model.bugtask import get_related_bugtasks_search_params
 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
@@ -37,8 +41,8 @@
 from lp.testing.views import create_initialized_view
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.registry.interfaces.person import PrivatePersonLinkageError
-from canonical.testing.layers import (
-    DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
+from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores
+
 
 class TestPerson(TestCaseWithFactory):
 
@@ -137,13 +141,12 @@
                 setattr, specification, attr_name, self.myteam)
 
     def test_visibility_validator_announcement(self):
-        announcement = self.bzr.announce(
+        self.bzr.announce(
             user = self.otherteam,
             title = 'title foo',
             summary = 'summary foo',
             url = 'http://foo.com',
-            publication_date = self.now
-            )
+            publication_date = self.now)
         try:
             self.otherteam.visibility = PersonVisibility.PRIVATE_MEMBERSHIP
         except ImmutableVisibilityError, exc:
@@ -166,7 +169,7 @@
         self.assertEqual(fake_warning, warning)
 
     def test_visibility_validator_answer_contact(self):
-        answer_contact = AnswerContact(
+        AnswerContact(
             person=self.otherteam,
             product=self.bzr,
             distribution=None,
@@ -180,7 +183,7 @@
                 'it is referenced by an answercontact.')
 
     def test_visibility_validator_archive(self):
-        archive = getUtility(IArchiveSet).new(
+        getUtility(IArchiveSet).new(
             owner=self.otherteam,
             description='desc foo',
             purpose=ArchivePurpose.PPA)
@@ -193,7 +196,7 @@
                 'it is referenced by an archive.')
 
     def test_visibility_validator_branch(self):
-        branch = self.factory.makeProductBranch(
+        self.factory.makeProductBranch(
             registrant=self.otherteam,
             owner=self.otherteam,
             product=self.bzr)
@@ -262,7 +265,7 @@
 
     def test_visibility_validator_team_mailinglist_public(self):
         self.otherteam.visibility = PersonVisibility.PRIVATE_MEMBERSHIP
-        mailinglist = getUtility(IMailingListSet).new(self.otherteam)
+        getUtility(IMailingListSet).new(self.otherteam)
         try:
             self.otherteam.visibility = PersonVisibility.PUBLIC
         except ImmutableVisibilityError, exc:
@@ -272,7 +275,7 @@
 
     def test_visibility_validator_team_mailinglist_public_view(self):
         self.otherteam.visibility = PersonVisibility.PRIVATE_MEMBERSHIP
-        mailinglist = getUtility(IMailingListSet).new(self.otherteam)
+        getUtility(IMailingListSet).new(self.otherteam)
         # The view should add an error notification.
         view = create_initialized_view(self.otherteam, '+edit', {
             'field.name': 'otherteam',
@@ -299,7 +302,7 @@
         self.assertEqual(self.otherteam.visibility, PersonVisibility.PUBLIC)
 
     def test_visibility_validator_team_mailinglist_private(self):
-        mailinglist = getUtility(IMailingListSet).new(self.otherteam)
+        getUtility(IMailingListSet).new(self.otherteam)
         try:
             self.otherteam.visibility = PersonVisibility.PRIVATE_MEMBERSHIP
         except ImmutableVisibilityError, exc:
@@ -310,7 +313,7 @@
 
     def test_visibility_validator_team_mailinglist_private_view(self):
         # The view should add a field error.
-        mailinglist = getUtility(IMailingListSet).new(self.otherteam)
+        getUtility(IMailingListSet).new(self.otherteam)
         view = create_initialized_view(self.otherteam, '+edit', {
             'field.name': 'otherteam',
             'field.displayname': 'Other Team',
@@ -329,14 +332,14 @@
         # A PRIVATE_MEMBERSHIP team with a mailing list may convert to a
         # PRIVATE.
         self.otherteam.visibility = PersonVisibility.PRIVATE_MEMBERSHIP
-        mailinglist = getUtility(IMailingListSet).new(self.otherteam)
+        getUtility(IMailingListSet).new(self.otherteam)
         self.otherteam.visibility = PersonVisibility.PRIVATE
 
     def test_visibility_validator_team_mailinglist_pmt_to_private_view(self):
         # A PRIVATE_MEMBERSHIP team with a mailing list may convert to a
         # PRIVATE.
         self.otherteam.visibility = PersonVisibility.PRIVATE_MEMBERSHIP
-        mailinglist = getUtility(IMailingListSet).new(self.otherteam)
+        getUtility(IMailingListSet).new(self.otherteam)
         view = create_initialized_view(self.otherteam, '+edit', {
             'field.name': 'otherteam',
             'field.displayname': 'Other Team',
@@ -377,7 +380,7 @@
         # A PUBLIC team with a structural subscription to a product can
         # convert to a PRIVATE team.
         foo_bar = Person.byName('name16')
-        sub = StructuralSubscription(
+        StructuralSubscription(
             product=self.bzr, subscriber=self.otherteam,
             subscribed_by=foo_bar)
         self.otherteam.visibility = PersonVisibility.PRIVATE
@@ -435,7 +438,7 @@
 
 class TestPersonSet(unittest.TestCase):
     """Test `IPersonSet`."""
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         login(ANONYMOUS)
@@ -463,7 +466,7 @@
 
 class TestCreatePersonAndEmail(unittest.TestCase):
     """Test `IPersonSet`.createPersonAndEmail()."""
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         login(ANONYMOUS)
@@ -499,7 +502,7 @@
 
 class TestPersonRelatedBugTaskSearch(TestCaseWithFactory):
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(TestPersonRelatedBugTaskSearch, self).setUp()
@@ -521,7 +524,8 @@
         # With no specified options, get_related_bugtasks_search_params()
         # returns 4 BugTaskSearchParams objects, each with a different
         # user field set.
-        search_params = get_related_bugtasks_search_params(self.user, self.context)
+        search_params = get_related_bugtasks_search_params(
+            self.user, self.context)
         self.assertEqual(len(search_params), 4)
         self.checkUserFields(
             search_params[0], assignee=self.context)
@@ -533,8 +537,8 @@
             search_params[3], bug_commenter=self.context)
 
     def test_get_related_bugtasks_search_params_with_assignee(self):
-        # With assignee specified, get_related_bugtasks_search_params() returns
-        # 3 BugTaskSearchParams objects.
+        # With assignee specified, get_related_bugtasks_search_params()
+        # returns 3 BugTaskSearchParams objects.
         search_params = get_related_bugtasks_search_params(
             self.user, self.context, assignee=self.user)
         self.assertEqual(len(search_params), 3)
@@ -597,5 +601,102 @@
             assignee=self.user)
 
 
+class TestPersonKarma(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonKarma, self).setUp()
+        self.person = self.factory.makePerson()
+        a_product = self.factory.makeProduct(name='aa')
+        b_product = self.factory.makeProduct(name='bb')
+        self.c_product = self.factory.makeProduct(name='cc')
+        self.cache_manager = getUtility(IKarmaCacheManager)
+        self._makeKarmaCache(
+            self.person, a_product, [('bugs', 10)])
+        self._makeKarmaCache(
+            self.person, b_product, [('answers', 50)])
+        self._makeKarmaCache(
+            self.person, self.c_product, [('code', 100), (('bugs', 50))])
+
+    def _makeKarmaCache(self, person, product, category_name_values):
+        """Create a KarmaCache entry with the given arguments.
+
+        In order to create the KarmaCache record we must switch to the DB
+        user 'karma'. This requires a commit and invalidates the product
+        instance.
+        """
+        transaction.commit()
+        reconnect_stores('karmacacheupdater')
+        total = 0
+        # Insert category total for person and project.
+        for category_name, value in category_name_values:
+            category = KarmaCategory.byName(category_name)
+            self.cache_manager.new(
+                value, person.id, category.id, product_id=product.id)
+            total += value
+        # Insert total cache for person and project.
+        self.cache_manager.new(
+            total, person.id, None, product_id=product.id)
+        transaction.commit()
+        reconnect_stores('launchpad')
+
+    def test__getProjectsWithTheMostKarma_ordering(self):
+        # Verify that pillars are ordered by karma.
+        results = removeSecurityProxy(
+            self.person)._getProjectsWithTheMostKarma()
+        self.assertEqual(
+            [('cc', 150), ('bb', 50), ('aa', 10)], results)
+
+    def test__getContributedCategories(self):
+        # Verify that a iterable of karma categories is returned.
+        categories = removeSecurityProxy(
+            self.person)._getContributedCategories(self.c_product)
+        names = sorted(category.name for category in categories)
+        self.assertEqual(['bugs', 'code'], names)
+
+    def test_getProjectsAndCategoriesContributedTo(self):
+        # Verify that a list of projects and contributed karma categories
+        # is returned.
+        results = removeSecurityProxy(
+            self.person).getProjectsAndCategoriesContributedTo()
+        names = [entry['project'].name for entry in results]
+        self.assertEqual(
+            ['cc', 'bb', 'aa'], names)
+        project_categories = results[0]
+        names = [
+            category.name for category in project_categories['categories']]
+        self.assertEqual(
+            ['code', 'bugs'], names)
+
+    def test_getProjectsAndCategoriesContributedTo_active_only(self):
+        # Verify that deactivated pillars are not included.
+        login('[e-mail]')
+        a_product = getUtility(IProductSet).getByName('cc')
+        a_product.active = False
+        results = removeSecurityProxy(
+            self.person).getProjectsAndCategoriesContributedTo()
+        names = [entry['project'].name for entry in results]
+        self.assertEqual(
+            ['bb', 'aa'], names)
+
+    def test_getProjectsAndCategoriesContributedTo_limit(self):
+        # Verify the limit of 5 is honored.
+        d_product = self.factory.makeProduct(name='dd')
+        self._makeKarmaCache(
+            self.person, d_product, [('bugs', 5)])
+        e_product = self.factory.makeProduct(name='ee')
+        self._makeKarmaCache(
+            self.person, e_product, [('bugs', 4)])
+        f_product = self.factory.makeProduct(name='ff')
+        self._makeKarmaCache(
+            self.person, f_product, [('bugs', 3)])
+        results = removeSecurityProxy(
+            self.person).getProjectsAndCategoriesContributedTo()
+        names = [entry['project'].name for entry in results]
+        self.assertEqual(
+            ['cc', 'bb', 'aa', 'dd', 'ee'], names)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)