Merge lp:~abentley/launchpad/proprietary-karma into lp:launchpad

[Aaron Bentley]

Filter karma for product privacy.

= Summary =
Fix bug #1078470: Cannot see user's profile page because it lists a proprietary project

== Proposed fix ==
Use standard product privacy filtering code.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of Private Projects

== Implementation details ==
Removed iterTopProjectsContributedTo, since it was unused and wanted inactive, as well as active, products. (It probably actually didn't, but hadn't been updated because it was unused.)

Stormified the query, then changed it to use Products and Distributions directly, since that's what the calling code really wants. Then changed it to return only active products on the query side. Then added the privacy filter. This reduces the query count slightly, since it no longer needs to repeatedly call IPillarNameSet).getByName. Since now every row returned by the query can be used, there is no need to return an extra 5 rows, which will reduce the number of rows returned.

Updated tests to reflect the fact that getProjectsAndCategoriesContributedTo and _getProjectsWithTheMostKarma now take a user parameter.

test_karma_category_sort was failing because it was running as the 'karma' user, which did not have permission on 'distribution'. Using the 'karma' user was not intentional; it was a side-effect of _makeKarmaCache. Changed _makeKarmaCache to use "with dbuser('karma')", so that the test runs under the default user.

== Tests ==
bin/test -t test_karma_category_sort -t person-karma.txt -t TestPersonKarma

== Demo and Q/A ==
Hard to QA because karma cache is updated by a script and staging doesn't have updated data on ~a.rosales

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/doc/person-karma.txt
  lib/lp/registry/browser/tests/test_person.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py

[Richard Harding (rharding)]

#208

Multiple imports on the one line.

review: Approve

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-11-05 02:30:49 +0000
+++ lib/lp/registry/browser/person.py	2012-11-20 16:44:21 +0000
@@ -1644,7 +1644,7 @@
     def contributions(self):
         """Cache the results of getProjectsAndCategoriesContributedTo()."""
         return self.context.getProjectsAndCategoriesContributedTo(
-            limit=5)
+            self.user, limit=5)
 
     @cachedproperty
     def contributed_categories(self):

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2012-11-19 05:54:50 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2012-11-20 16:44:21 +0000
@@ -67,7 +67,10 @@
     StormStatementRecorder,
     TestCaseWithFactory,
     )
-from lp.testing.dbuser import switch_dbuser
+from lp.testing.dbuser import (
+    dbuser,
+    switch_dbuser,
+    )
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -263,29 +266,22 @@
                          'Categories are not sorted correctly')
 
     def _makeKarmaCache(self, person, product, category, value=10):
-        """ Create and return a KarmaCache entry with the given arguments.
+        """Create and return a KarmaCache entry with the given arguments.
 
-        In order to create the KarmaCache record we must switch to the DB
-        user 'karma', so tests that need a different user after calling
-        this method should run switch_dbuser() themselves.
+        A commit is implicitly triggered because the 'karma' dbuser is used.
         """
-
-        switch_dbuser('karma')
-
-        cache_manager = getUtility(IKarmaCacheManager)
-        karmacache = cache_manager.new(
-            value, person.id, category.id, product_id=product.id)
-
-        try:
-            cache_manager.updateKarmaValue(
-                value, person.id, category_id=None, product_id=product.id)
-        except NotFoundError:
-            cache_manager.new(
-                value, person.id, category_id=None, product_id=product.id)
-
-        # We must commit here so that the change is seen in other transactions
-        # (e.g. when the callsite issues a switch_dbuser() after we return).
-        transaction.commit()
+        with dbuser('karma'):
+            cache_manager = getUtility(IKarmaCacheManager)
+            karmacache = cache_manager.new(
+                value, person.id, category.id, product_id=product.id)
+
+            try:
+                cache_manager.updateKarmaValue(
+                    value, person.id, category_id=None, product_id=product.id)
+            except NotFoundError:
+                cache_manager.new(
+                    value, person.id, category_id=None, product_id=product.id)
+
         return karmacache
 
 

=== modified file 'lib/lp/registry/doc/person-karma.txt'
--- lib/lp/registry/doc/person-karma.txt	2011-12-30 06:14:56 +0000
+++ lib/lp/registry/doc/person-karma.txt	2012-11-20 16:44:21 +0000
@@ -23,7 +23,8 @@
     >>> from lp.registry.interfaces.karma import IKarma
     >>> from lp.registry.interfaces.person import IPersonSet
     >>> from lp.registry.interfaces.product import IProductSet
-    >>> from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
+    >>> from lp.registry.interfaces.sourcepackagename import (
+    ...     ISourcePackageNameSet)
     >>> salgado = getUtility(IPersonSet).getByName('salgado')
     >>> firefox = getUtility(IProductSet).getByName('firefox')
     >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
@@ -94,7 +95,7 @@
 show the 5 most active projects.
 
     >>> foobar = getUtility(IPersonSet).getByName('name16')
-    >>> for contrib in foobar.getProjectsAndCategoriesContributedTo():
+    >>> for contrib in foobar.getProjectsAndCategoriesContributedTo(None):
     ...     categories = sorted(cat.name for cat in contrib['categories'])
     ...     print contrib['project'].title, categories
     The Evolution Groupware Application [u'bugs', u'translations']

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-11-01 03:41:36 +0000
+++ lib/lp/registry/interfaces/person.py	2012-11-20 16:44:21 +0000
@@ -1127,10 +1127,12 @@
     mailing_list = Attribute(
         _("The team's mailing list, if it has one, otherwise None."))
 
-    def getProjectsAndCategoriesContributedTo(limit=10):
+    def getProjectsAndCategoriesContributedTo(user, limit=10):
         """Return a list of dicts with projects and the contributions made
         by this person on that project.
 
+        Only entries visible to the specified user will be shown.
+
         The list is limited to the :limit: projects this person is most
         active.
 
@@ -1198,12 +1200,6 @@
         Return no more than the number given as quantity.
         """
 
-    def iterTopProjectsContributedTo(limit=10):
-        """Iterate over the top projects contributed to.
-
-        Iterate no more than the given limit.
-        """
-
     # XXX: salgado, 2008-08-01: Unexported because this method doesn't take
     # into account whether or not a team's memberships are private.
     # @operation_parameters(team=copy_field(ITeamMembership['team']))

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-11-19 05:54:50 +0000
+++ lib/lp/registry/model/person.py	2012-11-20 16:44:21 +0000
@@ -205,7 +205,6 @@
     )
 from lp.registry.interfaces.personnotification import IPersonNotificationSet
 from lp.registry.interfaces.persontransferjob import IPersonMergeJobSource
-from lp.registry.interfaces.pillar import IPillarNameSet
 from lp.registry.interfaces.product import (
     IProduct,
     IProductSet,
@@ -264,7 +263,6 @@
     SQLBase,
     sqlvalues,
     )
-from lp.services.features import getFeatureFlag
 from lp.services.helpers import (
     ensure_unicode,
     shortlist,
@@ -989,27 +987,21 @@
             return getUtility(IBugTaskSet).search(
                 search_params, *args, prejoins=prejoins)
 
-    def getProjectsAndCategoriesContributedTo(self, limit=5):
+    def getProjectsAndCategoriesContributedTo(self, user, limit=5):
         """See `IPerson`."""
         contributions = []
-        # 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, ignore_inactive=True)
-            if pillar is not None:
-                contributions.append(
-                    {'project': pillar,
-                     'categories': self._getContributedCategories(pillar)})
-            if len(contributions) == limit:
-                break
+        results = self._getProjectsWithTheMostKarma(user, limit=limit)
+        for product, distro, karma in results:
+            pillar = (product or distro)
+            contributions.append(
+                {'project': pillar,
+                 'categories': self._getContributedCategories(pillar)})
         return contributions
 
-    def _getProjectsWithTheMostKarma(self, limit=10):
-        """Return the names and karma points of this person on the
-        product/distribution with that name.
+    def _getProjectsWithTheMostKarma(self, user, limit=10):
+        """Return the product/distribution and karma points of this person.
+
+        Inactive products are ignored.
 
         The results are ordered descending by the karma points and limited to
         the given limit.
@@ -1017,24 +1009,27 @@
         # We want this person's total karma on a given context (that is,
         # across all different categories) here; that's why we use a
         # "KarmaCache.category IS NULL" clause here.
-        query = """
-            SELECT PillarName.name, KarmaCache.karmavalue
-            FROM KarmaCache
-            JOIN PillarName ON
-                COALESCE(KarmaCache.distribution, -1) =
-                COALESCE(PillarName.distribution, -1)
-                AND
-                COALESCE(KarmaCache.product, -1) =
-                COALESCE(PillarName.product, -1)
-            WHERE person = %(person)s
-                AND KarmaCache.category IS NULL
-                AND KarmaCache.project IS NULL
-            ORDER BY karmavalue DESC, name
-            LIMIT %(limit)s;
-            """ % sqlvalues(person=self, limit=limit)
-        cur = cursor()
-        cur.execute(query)
-        return cur.fetchall()
+        from lp.registry.model.product import (
+            Product,
+            ProductSet,
+        )
+        from lp.registry.model.distribution import Distribution
+        tableset = Store.of(self).using(
+            KarmaCache, LeftJoin(Product, Product.id == KarmaCache.productID),
+            LeftJoin(Distribution, Distribution.id ==
+                     KarmaCache.distributionID))
+        result = tableset.find(
+            (Product, Distribution, KarmaCache.karmavalue),
+             KarmaCache.personID == self.id,
+             KarmaCache.category == None,
+             KarmaCache.project == None,
+             Or(
+                And(Product.id != None, Product.active == True,
+                    ProductSet.getProductPrivacyFilter(user)),
+                Distribution.id != None))
+        result.order_by(Desc(KarmaCache.karmavalue),
+                        Coalesce(Product.name, Distribution.name))
+        return result[:limit]
 
     def _genAffiliatedProductSql(self, user=None):
         """Helper to generate the product sql for getAffiliatePillars"""
@@ -1238,11 +1233,6 @@
                 Person.id == self.id)
         return not person.is_empty()
 
-    def iterTopProjectsContributedTo(self, limit=10):
-        getByName = getUtility(IPillarNameSet).getByName
-        for name, ignored in self._getProjectsWithTheMostKarma(limit=limit):
-            yield getByName(name)
-
     def _getContributedCategories(self, pillar):
         """Return the KarmaCategories to which this person has karma on the
         given pillar.

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-10-26 07:15:49 +0000
+++ lib/lp/registry/tests/test_person.py	2012-11-20 16:44:21 +0000
@@ -748,11 +748,11 @@
     def test_canDeactivateAccount_private_projects(self):
         """A user owning non-public products cannot be deactivated."""
         user = self.factory.makePerson()
-        public_product = self.factory.makeProduct(
+        self.factory.makeProduct(
             information_type=InformationType.PUBLIC,
             name="public",
             owner=user)
-        public_product = self.factory.makeProduct(
+        self.factory.makeProduct(
             information_type=InformationType.PROPRIETARY,
             name="private",
             owner=user)
@@ -1101,7 +1101,9 @@
     def test__getProjectsWithTheMostKarma_ordering(self):
         # Verify that pillars are ordered by karma.
         results = removeSecurityProxy(
-            self.person)._getProjectsWithTheMostKarma()
+            self.person)._getProjectsWithTheMostKarma(None)
+        results = [((distro or product).name, karma)
+                   for product, distro, karma in results]
         self.assertEqual(
             [('cc', 150), ('bb', 50), ('aa', 10)], results)
 
@@ -1116,7 +1118,7 @@
         # Verify that a list of projects and contributed karma categories
         # is returned.
         results = removeSecurityProxy(
-            self.person).getProjectsAndCategoriesContributedTo()
+            self.person).getProjectsAndCategoriesContributedTo(None)
         names = [entry['project'].name for entry in results]
         self.assertEqual(
             ['cc', 'bb', 'aa'], names)
@@ -1132,7 +1134,7 @@
         a_product = getUtility(IProductSet).getByName('cc')
         a_product.active = False
         results = removeSecurityProxy(
-            self.person).getProjectsAndCategoriesContributedTo()
+            self.person).getProjectsAndCategoriesContributedTo(None)
         names = [entry['project'].name for entry in results]
         self.assertEqual(
             ['bb', 'aa'], names)
@@ -1149,7 +1151,32 @@
         self._makeKarmaCache(
             self.person, f_product, [('bugs', 3)])
         results = removeSecurityProxy(
-            self.person).getProjectsAndCategoriesContributedTo()
+            self.person).getProjectsAndCategoriesContributedTo(None)
+        names = [entry['project'].name for entry in results]
+        self.assertEqual(
+            ['cc', 'bb', 'aa', 'dd', 'ee'], names)
+
+    def test_getProjectsAndCategoriesContributedTo_privacy(self):
+        # Verify privacy is honored.
+        d_owner = self.factory.makePerson()
+        d_product = self.factory.makeProduct(
+            name='dd', information_type=InformationType.PROPRIETARY,
+            owner=d_owner)
+        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(None)
+        names = [entry['project'].name for entry in results]
+        self.assertEqual(
+            ['cc', 'bb', 'aa', 'ee', 'ff'], names)
+        results = removeSecurityProxy(
+            self.person).getProjectsAndCategoriesContributedTo(d_owner)
         names = [entry['project'].name for entry in results]
         self.assertEqual(
             ['cc', 'bb', 'aa', 'dd', 'ee'], names)