launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/productset-translation-related-queries into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/productset-translation-related-queries/+merge/131216

This branch removes th method ProductSet.featuredTranslatables(): It is not used at all, as noted in bug 253583 -- but if would now be used, it would likely break if it returned embagoed or proprietary products.

The second change: The method ProductSet.getTranslatables() now retuns only those products a user can see.

Running "make lint", I noticed that an unrelated test which created a product but did not use it in any assert or in another way, so I added an assert call.

tests:

./bin/test -vvt lp.registry.tests.test_product.TestProductSet.test_getTranslatables_filters_private_products
./bin/test -vvt lp.registry.tests.test_product.TestProductSet.test_users_private_products

no lint
-- 
https://code.launchpad.net/~adeuring/launchpad/productset-translation-related-queries/+merge/131216
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/productset-translation-related-queries into lp:launchpad.

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-10-22 10:09:48 +0000
+++ lib/lp/registry/configure.zcml	2012-10-24 15:32:28 +0000
@@ -1451,7 +1451,6 @@
                 search
                 latest
                 getTranslatables
-                featuredTranslatables
                 count_all
                 count_translatables
                 count_buggy

=== modified file 'lib/lp/registry/doc/product.txt'
--- lib/lp/registry/doc/product.txt	2012-09-28 14:35:35 +0000
+++ lib/lp/registry/doc/product.txt	2012-10-24 15:32:28 +0000
@@ -186,29 +186,6 @@
     alsa-utils
     evolution
 
-A similar list of "featured translatables" picks a random set of these,
-so we can present a short list of arbitrary projects that a user can
-help translate.
-
-XXX JeroenVermeulen 2008-07-31 bug-253583: We're not using this method
-at the moment.  Remove it or start using it?
-
-    >>> for product in productset.featuredTranslatables():
-    ...     print product.name
-    alsa-utils
-    evolution
-
-In featuredTranslatables() we can also set how many products we want to
-show at maximum.  We can pick just one random translatable product, for
-example.
-
-    >>> features = [
-    ...     product.name for product in productset.featuredTranslatables(1)]
-    >>> len(features)
-    1
-    >>> features[0] in ('alsa-utils', 'evolution')
-    True
-
 Only active products are listed as translatables.
 
     # Unlink the source packages so the project can be deactivated.
@@ -219,10 +196,6 @@
     ...    print product.name
     alsa-utils
 
-    >>> for product in productset.featuredTranslatables():
-    ...    print product.name
-    alsa-utils
-
     >>> evo.active = True
 
 

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-10-19 18:14:36 +0000
+++ lib/lp/registry/interfaces/product.py	2012-10-24 15:32:28 +0000
@@ -1087,19 +1087,6 @@
         Launchpad, as well as non-active ones.
         """
 
-    def featuredTranslatables(maximumproducts=8):
-        """Return an iterator over a random sample of translatable products.
-
-        Similar to `getTranslatables`, except the number of results is
-        limited and they are randomly chosen.
-
-        :param maximum_products: Maximum number of products to be
-            returned.
-        :return: An iterator over active, translatable products.
-        """
-        # XXX JeroenVermeulen 2008-07-31 bug=253583: this is not
-        # currently used!
-
     def count_all():
         """Return a count of the total number of products registered in
         Launchpad."""

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-10-24 09:42:11 +0000
+++ lib/lp/registry/model/product.py	2012-10-24 15:32:28 +0000
@@ -203,6 +203,7 @@
     get_property_cache,
     )
 from lp.services.statistics.interfaces.statistic import ILaunchpadStatisticSet
+from lp.services.webapp.interfaces import ILaunchBag
 from lp.translations.enums import TranslationPermission
 from lp.translations.interfaces.customlanguagecode import (
     IHasCustomLanguageCodes,
@@ -2008,40 +2009,22 @@
 
     def getTranslatables(self):
         """See `IProductSet`"""
+        user = getUtility(ILaunchBag).user
+        privacy_clause = self.getProductPrivacyFilter(user)
         results = IStore(Product).find(
             (Product, Person),
             Product.active == True,
             Product.id == ProductSeries.productID,
             POTemplate.productseriesID == ProductSeries.id,
             Product.translations_usage == ServiceUsage.LAUNCHPAD,
-            Person.id == Product._ownerID).config(
+            Person.id == Product._ownerID,
+            privacy_clause).config(
                 distinct=True).order_by(Product.title)
 
         # We only want Product - the other tables are just to populate
         # the cache.
         return DecoratedResultSet(results, operator.itemgetter(0))
 
-    def featuredTranslatables(self, maximumproducts=8):
-        """See `IProductSet`"""
-        return Product.select('''
-            id IN (
-                SELECT DISTINCT product_id AS id
-                FROM (
-                    SELECT Product.id AS product_id, random() AS place
-                    FROM Product
-                    JOIN ProductSeries ON
-                        ProductSeries.Product = Product.id
-                    JOIN POTemplate ON
-                        POTemplate.productseries = ProductSeries.id
-                    WHERE Product.active AND Product.translations_usage = %s
-                    ORDER BY place
-                ) AS randomized_products
-                LIMIT %s
-            )
-            ''' % sqlvalues(ServiceUsage.LAUNCHPAD, maximumproducts),
-            distinct=True,
-            orderBy='Product.title')
-
     @cachedproperty
     def stats(self):
         return getUtility(ILaunchpadStatisticSet)

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-10-22 14:35:30 +0000
+++ lib/lp/registry/tests/test_product.py	2012-10-24 15:32:28 +0000
@@ -87,6 +87,7 @@
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.authorization import check_permission
 from lp.testing import (
+    ANONYMOUS,
     celebrity_logged_in,
     login,
     person_logged_in,
@@ -1974,3 +1975,31 @@
         self.assertIn(public, result)
         self.assertIn(embargoed, result)
         self.assertIn(proprietary, result)
+
+    def test_getTranslatables_filters_private_products(self):
+        # ProductSet.getTranslatables() returns rivate translatable
+        # products only for user that have grants for these products.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner, translations_usage=ServiceUsage.LAUNCHPAD)
+        series = self.factory.makeProductSeries(product)
+        po_template = self.factory.makePOTemplate(productseries=series)
+        with person_logged_in(owner):
+            product.information_type=InformationType.PROPRIETARY
+        # Anonymous users do not see private products.
+        with person_logged_in(ANONYMOUS):
+            translatables = getUtility(IProductSet).getTranslatables()
+            self.assertNotIn(product, list(translatables))
+        # Ordinary users do not see private products.
+        user = self.factory.makePerson()
+        with person_logged_in(user):
+            translatables = getUtility(IProductSet).getTranslatables()
+            self.assertNotIn(product, list(translatables))
+        # Users with policy grants on private products see them.
+        with person_logged_in(owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                product, user, owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        with person_logged_in(user):
+            translatables = getUtility(IProductSet).getTranslatables()
+            self.assertIn(product, list(translatables))