launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/no-proprietary-packagings into lp:launchpad.

Commit message:
No proprietary products for Upstream connections portlet.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1066905 in Launchpad itself: "OOPS on "Upstream connections" portlet with proprietary product"
  https://bugs.launchpad.net/launchpad/+bug/1066905

For more details, see:
https://code.launchpad.net/~abentley/launchpad/no-proprietary-packagings/+merge/132960

= Summary =
Fix bug #1066905: OOPS on "Upstream connections" portlet with proprietary product

== Proposed fix ==
Ensure that the vocabulary lists only Public products, disable information type slection if registering a new product.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of Private Products

== Implementation details ==
Removing non-Public products from the vocabulary has two effects: it prevents the item from being listed, and it also prevents it from being accepted if submitted.  (This could happen due to a race, as per test_link_upstream_handles_proprietary.)

The presence of a source_package_name becomes a criterion for hiding the information_type, in addition to the feature flag.

Fixed handling of vocab_filter for SQLObjectVocabularyBase and ProductVocabulary, so that I could specify Product._information_type == InformationType.PUBLIC as that filter.

Fixed name of "test_owner_is_requried_without_disclaim_maitainer" as a drive-by.

== Tests ==
bin/test -t test_register_upstream_forbids_proprietary -t test_link_upstream_handles_initial_proprietary -t test_link_upstream_handles_proprietary


== Demo and Q/A ==
Find a source package with no Upstream connection (or remove its connection).  Register a product from the portal.  You should not see the "Information Type" field.

Disconnect the product from the source package.

On the Upstream connections portal, the product should be listed as a potential connection.  In another window, make the product proprietary.

Click "Link to Upstream Project".  You should get "Invalid value", and the product should no longer be listed under "Upstream connections".

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/vocabularies.py
  lib/lp/registry/browser/sourcepackage.py
  lib/lp/services/webapp/vocabulary.py
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/tests/test_product.py
  lib/lp/registry/browser/tests/test_sourcepackage_views.py
-- 
https://code.launchpad.net/~abentley/launchpad/no-proprietary-packagings/+merge/132960
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/no-proprietary-packagings into lp:launchpad.

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-10-26 01:32:43 +0000
+++ lib/lp/registry/browser/product.py	2012-11-05 20:20:29 +0000
@@ -1984,14 +1984,18 @@
             'information_type': InformationType.PUBLIC,
         }
 
+    @property
+    def enable_information_type(self):
+        private_projects = bool(getFeatureFlag(PRIVATE_PROJECTS_FLAG))
+        return private_projects and not self.source_package_name
+
     def setUpFields(self):
         """See `LaunchpadFormView`."""
         super(ProjectAddStepTwo, self).setUpFields()
         hidden_names = ['__visited_steps__', 'license_info']
         hidden_fields = self.form_fields.select(*hidden_names)
 
-        private_projects = bool(getFeatureFlag(PRIVATE_PROJECTS_FLAG))
-        if not private_projects:
+        if not self.enable_information_type:
             hidden_names.extend([
                 'information_type', 'bug_supervisor', 'driver'])
 
@@ -2033,9 +2037,8 @@
         self.widgets['source_package_name'].visible = False
         self.widgets['distroseries'].visible = False
 
-        private_projects = bool(getFeatureFlag(PRIVATE_PROJECTS_FLAG))
-
-        if private_projects and IProductSet.providedBy(self.context):
+        if (self.enable_information_type and
+            IProductSet.providedBy(self.context)):
             self.widgets['information_type'].value = InformationType.PUBLIC
 
         # Set the source_package_release attribute on the licenses
@@ -2105,8 +2108,7 @@
             for error in errors:
                 self.errors.remove(error)
 
-        private_projects = bool(getFeatureFlag(PRIVATE_PROJECTS_FLAG))
-        if private_projects:
+        if self.enable_information_type:
             if data.get('information_type') != InformationType.PUBLIC:
                 for required_field in ('bug_supervisor', 'driver'):
                     if data.get(required_field) is None:

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2012-10-12 18:12:20 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2012-11-05 20:20:29 +0000
@@ -67,7 +67,7 @@
     StepView,
     )
 from lp.app.browser.tales import CustomizableFormatter
-from lp.app.enums import ServiceUsage
+from lp.app.enums import InformationType, ServiceUsage
 from lp.app.widgets.itemswidgets import LaunchpadRadioWidget
 from lp.bugs.browser.bugtask import BugTargetTraversalMixin
 from lp.registry.browser.product import ProjectAddStepOne
@@ -80,6 +80,7 @@
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.registry.model.product import Product
 from lp.services.webapp import (
     ApplicationMenu,
     canonical_url,
@@ -577,7 +578,9 @@
         # Find registered products that are similarly named to the source
         # package.
         product_vocab = getVocabularyRegistry().get(None, 'Product')
-        matches = product_vocab.searchForTerms(self.context.name)
+        matches = product_vocab.searchForTerms(self.context.name,
+            vocab_filter=[Product._information_type ==
+                          InformationType.PUBLIC])
         # Based upon the matching products, create a new vocabulary with
         # term descriptions that include a link to the product.
         self.product_suggestions = []

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-10-26 01:32:43 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-11-05 20:20:29 +0000
@@ -248,7 +248,7 @@
         product = self.product_set.getByName('fnord')
         self.assertEqual('registry', product.owner.name)
 
-    def test_owner_is_requried_without_disclaim_maitainer(self):
+    def test_owner_is_requried_without_disclaim_maintainer(self):
         # A valid owner name is required if disclaim_maintainer is
         # not selected.
         registrant = self.factory.makePerson()
@@ -496,7 +496,8 @@
                 product,
                 '+edit',
                 principal=product.owner)
-            info_types = [t.name for t in view.widgets['information_type'].vocabulary]
+            vocabulary = view.widgets['information_type'].vocabulary
+            info_types = [t.name for t in vocabulary]
             expected = ['PUBLIC', 'PROPRIETARY', 'EMBARGOED']
             self.assertEqual(expected, info_types)
 

=== modified file 'lib/lp/registry/browser/tests/test_sourcepackage_views.py'
--- lib/lp/registry/browser/tests/test_sourcepackage_views.py	2012-10-18 12:40:40 +0000
+++ lib/lp/registry/browser/tests/test_sourcepackage_views.py	2012-11-05 20:20:29 +0000
@@ -8,6 +8,12 @@
 import cgi
 import urllib
 
+from soupmatchers import (
+    HTMLContains,
+    Tag,
+    )
+from testtools.matchers import Not
+from testtools.testcase import ExpectedException
 from zope.component import getUtility
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
@@ -24,6 +30,7 @@
     IDistroSeriesSet,
     )
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.services.features.testing import FeatureFixture
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
     BrowserTestCase,
@@ -152,6 +159,60 @@
             url, 'field.homepageurl', 'http://eg.dom/bonkers')
 
 
+class TestSourcePackageView(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_register_upstream_forbids_proprietary(self):
+        # Cannot specify information_type if registering for sourcepackage.
+        self.useFixture(FeatureFixture({'disclosure.private_projects.enabled':
+            'on'}))
+        sourcepackage = self.factory.makeSourcePackage()
+        browser = self.getViewBrowser(sourcepackage)
+        browser.getControl("Register the upstream project").click()
+        browser.getControl("Link to Upstream Project").click()
+        browser.getControl("Summary").value = "summary"
+        browser.getControl("Continue").click()
+        t = Tag('info_type', 'input', attrs={'name': 'field.information_type'})
+        self.assertThat(browser.contents, Not(HTMLContains(t)))
+
+    def test_link_upstream_handles_initial_proprietary(self):
+        # Proprietary product is not listed as an option.
+        owner = self.factory.makePerson()
+        sourcepackage = self.factory.makeSourcePackage()
+        product_name = sourcepackage.name
+        product_displayname = self.factory.getUniqueString()
+        self.factory.makeProduct(
+            name=product_name, owner=owner,
+            information_type=InformationType.PROPRIETARY,
+            displayname=product_displayname)
+        browser = self.getViewBrowser(sourcepackage, user=owner)
+        with ExpectedException(LookupError):
+            browser.getControl(product_displayname)
+
+    def test_link_upstream_handles_proprietary(self):
+        # Proprietary products produce an 'invalid value' error.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        product_name = product.name
+        product_displayname = product.displayname
+        sourcepackage = self.factory.makeSourcePackage(
+            sourcepackagename=product_name)
+        with person_logged_in(None):
+            browser = self.getViewBrowser(sourcepackage, user=owner)
+            with person_logged_in(owner):
+                product.information_type = InformationType.PROPRIETARY
+            browser.getControl(product_displayname).click()
+            browser.getControl("Link to Upstream Project").click()
+        error = Tag(
+            'error', 'div', attrs={'class': 'message'},
+            text='Invalid value')
+        self.assertThat(browser.contents, HTMLContains(error))
+        self.assertNotIn(
+            'The project %s was linked to this source package.' %
+            str(product_displayname), browser.contents)
+
+
 class TestSourcePackageUpstreamConnectionsView(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -299,7 +360,7 @@
         """Packaging cannot be created for PROPRIETARY products"""
         product_owner = self.factory.makePerson()
         product_name = 'proprietary-product'
-        product = self.factory.makeProduct(
+        self.factory.makeProduct(
             name=product_name, owner=product_owner,
             information_type=InformationType.PROPRIETARY)
         ubuntu_series = self.factory.makeUbuntuDistroSeries()

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-10-19 10:34:55 +0000
+++ lib/lp/registry/vocabularies.py	2012-11-05 20:20:29 +0000
@@ -296,12 +296,14 @@
             like_query = query.lower()
             like_query = "'%%%%' || %s || '%%%%'" % quote_like(like_query)
             fti_query = quote(query)
+            if vocab_filter is None:
+                vocab_filter = []
             where_clause = And(
                 SQL(
                     "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
                         like_query, fti_query)),
                 ProductSet.getProductPrivacyFilter(
-                    getUtility(ILaunchBag).user))
+                    getUtility(ILaunchBag).user), *vocab_filter)
             order_by = SQL(
                 '(CASE name WHEN %s THEN 1 '
                 ' ELSE rank(fti, ftq(%s)) END) DESC, displayname, name'

=== modified file 'lib/lp/services/webapp/vocabulary.py'
--- lib/lp/services/webapp/vocabulary.py	2011-12-30 06:14:56 +0000
+++ lib/lp/services/webapp/vocabulary.py	2012-11-05 20:20:29 +0000
@@ -307,7 +307,7 @@
     # search functionality produce a new vocabulary restricted to the
     # desired subset.
     def searchForTerms(self, query=None, vocab_filter=None):
-        results = self.search(query)
+        results = self.search(query, vocab_filter)
         return CountableIterator(results.count(), results, self.toTerm)
 
     def search(self, query, vocab_filter=None):