Merge lp:~jcsackett/launchpad/dont-show-obsolete-junk into lp:launchpad

[j.c.sackett]

Fixes the related-projects for a team so that only active projects &c are shown. Incidentally fixes the branding issues with how those projects are shown (i.e. branding wasn't shown before).

Summary

Fixes an issue with related projects show obsolete/inactive/invalid projects. Incidentally also fixes an issue with related projects not showing branding.

Proposed fix

If possible, change the model to not return inactive projects. Otherwise, change the view to iterate over the returned set and remove anything we don't want to show.

Pre-implementation notes

Spoke with Curtis Hovey (sinzui) about the problem and investigated the view and model with him.

Implementation details

The person model's getOwnedOrDrivenPillars now filters out inactive pillars where possible, and returns a DecoratedResultSet rather than a running a list comprehension before returning.

The template now users pillarname/pillar/fmt:link rather than a hand crafted url.

Demo and Q/A

Go to a team page (https://launchpad.dev/~ubuntu-team); you should see only active entities, and should see their branding.

lint
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/person.py
  lib/lp/registry/model/person.py
  lib/lp/registry/stories/person/xx-person-projects.txt
  lib/lp/registry/templates/person-portlet-related-projects.pt
  lib/lp/registry/tests/test_person.py

[Brad Crittenden (bac)]

Hi Jonathan,

This branch looks good. I think the following:

return len(list(self._related_projects()))

could be:

return self._related_projects().count()

Please give that a whirl.

I'm marking this Approved with the understanding you'll make that change.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-08-13 07:48:54 +0000
+++ lib/lp/registry/browser/person.py	2010-08-17 14:54:45 +0000
@@ -5168,7 +5168,7 @@
     @cachedproperty
     def related_projects_count(self):
         """The number of project owned or driven by this person."""
-        return len(self._related_projects())
+        return self._related_projects().count()
 
     @cachedproperty
     def has_more_related_projects(self):

=== modified file 'lib/lp/registry/doc/person-account.txt'
--- lib/lp/registry/doc/person-account.txt	2010-07-14 21:19:42 +0000
+++ lib/lp/registry/doc/person-account.txt	2010-08-17 14:54:45 +0000
@@ -185,7 +185,7 @@
 
 ...no owned or driven pillars...
 
-    >>> len(foobar.getOwnedOrDrivenPillars())
+    >>> foobar.getOwnedOrDrivenPillars().count()
     0
 
 ...and, finally, to not be considered a valid person in Launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-08-16 07:27:07 +0000
+++ lib/lp/registry/model/person.py	2010-08-17 14:54:45 +0000
@@ -61,11 +61,13 @@
 from canonical.database.sqlbase import (
     cursor, quote, quote_like, sqlvalues, SQLBase)
 
-from canonical.cachedproperty import cachedproperty, cache_property, clear_property
-
-from canonical.lazr.utils import get_current_browser_request, safe_hasattr
-
-from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
+from canonical.cachedproperty import (cachedproperty, cache_property,
+    clear_property)
+
+from canonical.lazr.utils import get_current_browser_request
+
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet)
 from canonical.launchpad.database.account import Account, AccountPassword
 from canonical.launchpad.interfaces.account import AccountSuspendedError
 from lp.bugs.model.bugtarget import HasBugsBase
@@ -890,14 +892,16 @@
                 SELECT name, 3 as kind, displayname
                 FROM product
                 WHERE
-                    driver = %(person)s
-                    OR owner = %(person)s
+                    active = True AND
+                    (driver = %(person)s
+                    OR owner = %(person)s)
                 UNION
                 SELECT name, 2 as kind, displayname
                 FROM project
                 WHERE
-                    driver = %(person)s
-                    OR owner = %(person)s
+                    active = True AND
+                    (driver = %(person)s
+                    OR owner = %(person)s)
                 UNION
                 SELECT name, 1 as kind, displayname
                 FROM distribution
@@ -909,7 +913,12 @@
             """ % sqlvalues(person=self))
         results = IStore(self).using(origin).find(find_spec)
         results = results.order_by('kind', 'displayname')
-        return [pillar_name for pillar_name, kind, displayname in results]
+
+        def get_pillar_name(result):
+            pillar_name, kind, displayname = result
+            return pillar_name
+
+        return DecoratedResultSet(results, get_pillar_name)
 
     def getOwnedProjects(self, match_name=None):
         """See `IPerson`."""
@@ -1447,7 +1456,7 @@
         need_location=False, need_archive=False, need_preferred_email=False,
         need_validity=False):
         """Lookup all members of the team with optional precaching.
-        
+
         :param need_karma: The karma attribute will be cached.
         :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
             cached.
@@ -1475,7 +1484,8 @@
         if need_karma:
             # New people have no karmatotalcache rows.
             origin.append(
-                LeftJoin(KarmaTotalCache, KarmaTotalCache.person == Person.id))
+                LeftJoin(KarmaTotalCache,
+                    KarmaTotalCache.person == Person.id))
             columns.append(KarmaTotalCache)
         if need_ubuntu_coc:
             columns.append(Alias(Exists(Select(SignedCodeOfConduct,
@@ -1488,9 +1498,9 @@
                 LeftJoin(PersonLocation, PersonLocation.person == Person.id))
             columns.append(PersonLocation)
         if need_archive:
-            # Not everyone has PPA's 
-            # It would be nice to cleanly expose the soyuz rules for this to avoid
-            # duplicating the relationships.
+            # Not everyone has PPA's
+            # It would be nice to cleanly expose the soyuz rules for this to
+            # avoid duplicating the relationships.
             origin.append(
                 LeftJoin(Archive, Archive.owner == Person.id))
             columns.append(Archive)
@@ -1519,6 +1529,7 @@
         # Adapt the result into a cached Person.
         columns = tuple(columns)
         raw_result = store.using(*origin).find(columns, conditions)
+
         def prepopulate_person(row):
             result = row[0]
             index = 1
@@ -1556,7 +1567,8 @@
                 index += 1
                 cache_property(result, '_is_valid_person_cached', valid)
             return result
-        return DecoratedResultSet(raw_result, result_decorator=prepopulate_person)
+        return DecoratedResultSet(raw_result,
+            result_decorator=prepopulate_person)
 
     def _getMembersWithPreferredEmails(self):
         """Helper method for public getMembersWithPreferredEmails.

=== modified file 'lib/lp/registry/stories/person/xx-person-projects.txt'
--- lib/lp/registry/stories/person/xx-person-projects.txt	2010-07-14 16:27:33 +0000
+++ lib/lp/registry/stories/person/xx-person-projects.txt	2010-08-17 14:54:45 +0000
@@ -9,8 +9,8 @@
     ...     anon_browser.contents, 'portlet-related-projects')
     >>> for tr in related_projects.findAll('tr'):
     ...     print extract_text(tr)
-    Ubuntu Linux
-    Ubuntu Test
+    Ubuntu
+    ubuntutest
     Tomcat
 
 The +related-software page displays a table with links to open bugs,

=== modified file 'lib/lp/registry/templates/person-portlet-related-projects.pt'
--- lib/lp/registry/templates/person-portlet-related-projects.pt	2009-08-25 02:01:55 +0000
+++ lib/lp/registry/templates/person-portlet-related-projects.pt	2010-08-17 14:54:45 +0000
@@ -25,9 +25,7 @@
     <tbody>
       <tr tal:repeat="pillarname pillarnames">
         <td>
-          <a tal:attributes="href string:/${pillarname/pillar/name};
-                             class pillarname/pillar/image:sprite_css"
-          tal:content="pillarname/pillar/title">Pillar title</a>
+          <a tal:replace="structure pillarname/pillar/fmt:link" />
         </td>
       </tr>
     </tbody>

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-08-17 02:13:54 +0000
+++ lib/lp/registry/tests/test_person.py	2010-08-17 14:54:45 +0000
@@ -44,6 +44,7 @@
     )
 from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
+from lp.testing import celebrity_logged_in
 from lp.testing._webservice import QueryCollector
 from lp.registry.interfaces.person import PrivatePersonLinkageError
 from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores
@@ -71,6 +72,22 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_getOwnedOrDrivenPillars(self):
+        user = self.factory.makePerson()
+        active_project = self.factory.makeProject(owner=user)
+        inactive_project = self.factory.makeProject(owner=user)
+        with celebrity_logged_in('admin'):
+            inactive_project.active = False
+        expected_pillars = [active_project.name]
+        received_pillars = [pillar.name for pillar in
+            user.getOwnedOrDrivenPillars()]
+        self.assertEqual(expected_pillars, received_pillars)
+
+
+class TestPersonStates(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
     def setUp(self):
         TestCaseWithFactory.setUp(self, '[e-mail]')
         person_set = getUtility(IPersonSet)
@@ -201,7 +218,7 @@
     def test_person_snapshot(self):
         omitted = (
             'activemembers', 'adminmembers', 'allmembers',
-            'all_members_prepopulated',  'approvedmembers',
+            'all_members_prepopulated', 'approvedmembers',
             'deactivatedmembers', 'expiredmembers', 'inactivemembers',
             'invited_members', 'member_memberships', 'pendingmembers',
             'proposedmembers', 'unmapped_participants',
@@ -583,7 +600,8 @@
         self.addCleanup(collector.unregister)
         url = "/~%s/participants" % team.name
         logout()
-        response = webservice.get(url, headers={'User-Agent':'AnonNeedsThis'})
+        response = webservice.get(url,
+            headers={'User-Agent': 'AnonNeedsThis'})
         self.assertEqual(response.status, 200,
             "Got %d for url %r with response %r" % (
             response.status, url, response.body))