← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wallyworld/launchpad/incorrect-picker-label-830935 into lp:launchpad

 

Ian Booth has proposed merging lp:~wallyworld/launchpad/incorrect-picker-label-830935 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #830935 in Launchpad itself: "Incorrect label on picker filter"
  https://bugs.launchpad.net/launchpad/+bug/830935

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/incorrect-picker-label-830935/+merge/72513

== Implementation ==

Fix the title attribute of the picker entries for projects and project groups so that they use the correct terminology. Introduce a pillar_category property to Product and ProjectGroup model classes to provide the relevant text.

The picker search results display-
Before:
Firefox (Product)
Mozilla (ProjectGroup)

After:
Firefox (Project)
Mozilla (Project Group)

The filter text has also been changed to match.

Also did some drive-by lint fixes and a little test cleanup.

== Demo/QA ==

http://people.canonical.com/~ianb/picker-filter-bug-fixed.png

== Tests ==

Added new tests to:
lp/app/widgets/tests/test_popup.py
lp/registry/tests/test_pillar_vocabularies.py
lp/registry/tests/test_product.py
lp/registry/tests/test_projectgroup.py

== Lint ==
Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/tests/test_popup.py
  lib/lp/registry/configure.zcml
  lib/lp/registry/vocabularies.py
  lib/lp/registry/model/product.py
  lib/lp/registry/model/projectgroup.py
  lib/lp/registry/tests/test_pillar_vocabularies.py
  lib/lp/registry/tests/test_product.py
  lib/lp/registry/tests/test_projectgroup.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/incorrect-picker-label-830935/+merge/72513
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/incorrect-picker-label-830935 into lp:launchpad.
=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py	2011-08-22 13:01:14 +0000
+++ lib/lp/app/widgets/tests/test_popup.py	2011-08-23 00:36:29 +0000
@@ -103,9 +103,9 @@
              'title': 'All',
              'description': 'Display all search results'},
             {'name': 'PROJECT',
-             'title': 'Product',
+             'title': 'Project',
              'description':
-                 'Display search results associated with products'},
+                 'Display search results associated with projects'},
             {'name': 'DISTRO',
              'title': 'Distribution',
              'description':

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2011-08-18 20:35:41 +0000
+++ lib/lp/registry/configure.zcml	2011-08-23 00:36:29 +0000
@@ -367,6 +367,9 @@
     <class
         class="lp.registry.model.projectgroup.ProjectGroup">
         <allow
+            attributes="
+                pillar_category"/>
+        <allow
             interface="lp.registry.interfaces.projectgroup.IProjectGroupPublic"/>
         <allow interface="lp.bugs.interfaces.bugsummary.IBugSummaryDimension"/>
         <allow
@@ -1189,6 +1192,9 @@
     <class
         class="lp.registry.model.product.Product">
         <allow
+            attributes="
+                pillar_category"/>
+        <allow
             interface="lp.registry.interfaces.product.IProductPublic"/>
         <allow interface="lp.bugs.interfaces.bugsummary.IBugSummaryDimension"/>
         <allow

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2011-06-21 01:34:08 +0000
+++ lib/lp/registry/model/product.py	2011-08-23 00:36:29 +0000
@@ -386,6 +386,10 @@
     date_next_suggest_packaging = UtcDateTimeCol(default=None)
 
     @property
+    def pillar_category(self):
+        return "Project"
+
+    @property
     def official_codehosting(self):
         # XXX Need to remove official_codehosting column from Product
         # table.

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2011-06-21 00:04:12 +0000
+++ lib/lp/registry/model/projectgroup.py	2011-08-23 00:36:29 +0000
@@ -76,7 +76,6 @@
     HasBugHeatMixin,
     OfficialBugTag,
     )
-from lp.bugs.model.bugtask import BugTask
 from lp.bugs.model.structuralsubscription import (
     StructuralSubscriptionTargetMixin,
     )
@@ -171,6 +170,10 @@
     max_bug_heat = Int()
 
     @property
+    def pillar_category(self):
+        return "Project Group"
+
+    @property
     def products(self):
         return Product.selectBy(
             project=self, active=True, orderBy='displayname')

=== modified file 'lib/lp/registry/tests/test_pillar_vocabularies.py'
--- lib/lp/registry/tests/test_pillar_vocabularies.py	2011-08-18 00:46:19 +0000
+++ lib/lp/registry/tests/test_pillar_vocabularies.py	2011-08-23 00:36:29 +0000
@@ -35,14 +35,22 @@
             self.vocabulary.supportedFilters()
         )
 
-    def test_toTerm(self):
+    def test_Product_toTerm(self):
         # Product terms are composed of title, name, and the object.
         term = self.vocabulary.toTerm(self.product)
-        title = '%s (Product)' % self.product.title
+        title = '%s (Project)' % self.product.title
         self.assertEqual(title, term.title)
         self.assertEqual(self.product.name, term.token)
         self.assertEqual(self.product, term.value)
 
+    def test_ProjectGroup_toTerm(self):
+        # ProductGroup terms are composed of title, name, and the object.
+        term = self.vocabulary.toTerm(self.project_group)
+        title = '%s (Project Group)' % self.project_group.title
+        self.assertEqual(title, term.title)
+        self.assertEqual(self.project_group.name, term.token)
+        self.assertEqual(self.project_group, term.value)
+
     def test_getTermByToken(self):
         # Tokens are case insentive because the product name is lowercase.
         term = self.vocabulary.getTermByToken('ORCHID-SNARK')

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2011-06-28 15:04:29 +0000
+++ lib/lp/registry/tests/test_product.py	2011-08-23 00:36:29 +0000
@@ -5,7 +5,6 @@
 
 from cStringIO import StringIO
 import datetime
-import unittest
 
 from lazr.lifecycle.snapshot import Snapshot
 import pytz
@@ -35,6 +34,7 @@
 from lp.testing import (
     login,
     login_person,
+    TestCase,
     TestCaseWithFactory,
     WebServiceTestCase,
     )
@@ -46,6 +46,11 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_pillar_category(self):
+        # Products are really called Projects
+        product = self.factory.makeProduct()
+        self.assertEqual("Project", product.pillar_category)
+
     def test_deactivation_failure(self):
         # Ensure that a product cannot be deactivated if
         # it is linked to source packages.
@@ -97,7 +102,7 @@
         # milestones should be included.
         product = self.factory.makeProduct(name='foo')
         for i in range(25):
-            milestone_list = self.factory.makeMilestone(
+            self.factory.makeMilestone(
                 product=product,
                 productseries=product.development_focus,
                 name=str(i))
@@ -181,7 +186,7 @@
             [series.name for series in active_series])
 
 
-class TestProductFiles(unittest.TestCase):
+class TestProductFiles(TestCase):
     """Tests for downloadable product files."""
 
     layer = LaunchpadFunctionalLayer
@@ -201,7 +206,7 @@
             foo_file, 'text/plain', filename)
         firefox_owner.getControl(name='field.signature').add_file(
             foo_signature, 'text/plain', '%s.asc' % filename)
-        firefox_owner.getControl('Description').value="Foo installer"
+        firefox_owner.getControl('Description').value = "Foo installer"
         firefox_owner.getControl(name="field.contenttype").displayValue = \
            ["Installer file"]
         firefox_owner.getControl("Upload").click()
@@ -235,20 +240,20 @@
         self.assertEqual(a_element.contents[0].strip(), u'sig')
 
 
-class ProductAttributeCacheTestCase(unittest.TestCase):
+class ProductAttributeCacheTestCase(TestCase):
     """Cached attributes must be cleared at the end of a transaction."""
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
+        super(ProductAttributeCacheTestCase, self).setUp()
         self.product = Product.selectOneBy(name='tomcat')
 
     def testLicensesCache(self):
         """License cache should be cleared automatically."""
         self.assertEqual(self.product.licenses,
                          (License.ACADEMIC, License.AFFERO))
-        product_license = ProductLicense(
-            product=self.product, license=License.PYTHON)
+        ProductLicense(product=self.product, license=License.PYTHON)
         # Cache doesn't see new value.
         self.assertEqual(self.product.licenses,
                          (License.ACADEMIC, License.AFFERO))
@@ -258,8 +263,7 @@
         # Cache is cleared and it sees database changes that occur
         # before the cache is populated.
         transaction.abort()
-        product_license = ProductLicense(
-            product=self.product, license=License.MIT)
+        ProductLicense(product=self.product, license=License.MIT)
         self.assertEqual(self.product.licenses,
                          (License.ACADEMIC, License.AFFERO, License.MIT))
 
@@ -267,7 +271,7 @@
         """commercial_subscription cache should not traverse transactions."""
         self.assertEqual(self.product.commercial_subscription, None)
         now = datetime.datetime.now(pytz.UTC)
-        subscription = CommercialSubscription(
+        CommercialSubscription(
             product=self.product,
             date_starts=now,
             date_expires=now,
@@ -286,7 +290,7 @@
 
         # Cache is cleared again.
         transaction.abort()
-        subscription = CommercialSubscription(
+        CommercialSubscription(
             product=self.product,
             date_starts=now,
             date_expires=now,
@@ -378,7 +382,3 @@
         ws_group = self.wsObject(group)
         ws_product.translationgroup = ws_group
         ws_product.lp_save()
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/tests/test_projectgroup.py'
--- lib/lp/registry/tests/test_projectgroup.py	2011-08-12 11:37:08 +0000
+++ lib/lp/registry/tests/test_projectgroup.py	2011-08-23 00:36:29 +0000
@@ -21,6 +21,17 @@
     )
 
 
+class TestProjectGroup(TestCaseWithFactory):
+    """Tests project group object."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_pillar_category(self):
+        # The pillar category is correct.
+        pg = self.factory.makeProject()
+        self.assertEqual("Project Group", pg.pillar_category)
+
+
 class ProjectGroupSearchTestCase(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2011-08-22 12:13:22 +0000
+++ lib/lp/registry/vocabularies.py	2011-08-23 00:36:29 +0000
@@ -1994,8 +1994,8 @@
 
     def __new__(cls):
         return super(VocabularyFilter, cls).__new__(
-            cls, 'PROJECT', 'Product',
-            'Display search results associated with products')
+            cls, 'PROJECT', 'Project',
+            'Display search results associated with projects')
 
     @property
     def filter_terms(self):
@@ -2007,8 +2007,8 @@
 
     def __new__(cls):
         return super(VocabularyFilter, cls).__new__(
-            cls, 'PROJECTGROUP', 'Project',
-            'Display search results associated with projects')
+            cls, 'PROJECTGROUP', 'Project Group',
+            'Display search results associated with project groups')
 
     @property
     def filter_terms(self):
@@ -2050,9 +2050,10 @@
                     obj.__class__.__name__, obj.id)
             obj = obj.pillar
 
-        # It is a hack using the class name here, but it works
-        # fine and avoids an ugly if statement.
-        title = '%s (%s)' % (obj.title, obj.__class__.__name__)
+        # We use the "pillar_category" property if it exists, otherwise we
+        # just use the class name.
+        category = getattr(obj, "pillar_category", obj.__class__.__name__)
+        title = '%s (%s)' % (obj.title, category)
 
         return SimpleTerm(obj, obj.name, title)
 


Follow ups