launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/skip-private-dev-focus into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #947195 in Launchpad itself: "LocationError: (None, 'branch_type') at +code-index"
  https://bugs.launchpad.net/launchpad/+bug/947195

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/skip-private-dev-focus/+merge/127629

Currently, Product:+code-index assumes that import branches are public, which is wrong. I have cleaned up the TAL and written a function to decide whether to show the section only if the branch is visible as well.

I have cleaned up some whitespace as well. The net-positive in this branch will be wiped out by a branch I'm preparing for merge now.
-- 
https://code.launchpad.net/~stevenk/launchpad/skip-private-dev-focus/+merge/127629
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/skip-private-dev-focus into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2012-09-28 06:15:58 +0000
+++ lib/lp/code/browser/branchlisting.py	2012-10-03 03:45:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Base class view for branch listings."""
@@ -61,7 +61,10 @@
     LaunchpadFormView,
     )
 from lp.app.browser.tales import MenuAPI
-from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.app.enums import (
+    PRIVATE_INFORMATION_TYPES,
+    ServiceUsage,
+    )
 from lp.app.widgets.itemswidgets import LaunchpadDropdownWidget
 from lp.blueprints.interfaces.specificationbranch import (
     ISpecificationBranchSet,
@@ -1406,6 +1409,12 @@
         set_branch.text = 'Configure code hosting'
         return set_branch
 
+    @property
+    def external_visible(self):
+        return (
+            self.context.codehosting_usage == ServiceUsage.EXTERNAL
+            and self.branch)
+
 
 class ProductBranchesView(ProductBranchListingView):
     """View for branch listing for a product."""

=== modified file 'lib/lp/code/browser/tests/test_product.py'
--- lib/lp/code/browser/tests/test_product.py	2012-09-19 01:19:35 +0000
+++ lib/lp/code/browser/tests/test_product.py	2012-10-03 03:45:23 +0000
@@ -31,6 +31,7 @@
     login,
     login_person,
     logout,
+    person_logged_in,
     TestCaseWithFactory,
     time_counter,
     )
@@ -196,6 +197,20 @@
         content = view()
         self.assertIn('bzr push lp:~', content)
 
+    def test_product_code_index_with_private_imported_branch(self):
+        # Product:+code-index will not crash if the devfoocs is an private
+        # imported branch.
+        product, branch = self.makeProductAndDevelopmentFocusBranch(
+            information_type=InformationType.USERDATA,
+            branch_type=BranchType.IMPORTED)
+        user = self.factory.makePerson()
+        with person_logged_in(user):
+            view = create_initialized_view(
+                product, '+code-index', rootsite='code', principal=user)
+            html = view()
+        expected = 'There are no branches for %s' % product.displayname
+        self.assertIn(expected, html)
+
 
 class TestProductCodeIndexServiceUsages(ProductTestBase, BrowserTestCase):
     """Tests for the product code page, especially the usage messasges."""
@@ -247,10 +262,8 @@
         # A remote branch says code is hosted externally, and displays
         # upstream data.
         product, branch = self.makeProductAndDevelopmentFocusBranch(
-            branch_type=BranchType.REMOTE,
-            url="http://example.com/mybranch";)
-        self.assertEqual(ServiceUsage.EXTERNAL,
-                         product.codehosting_usage)
+            branch_type=BranchType.REMOTE, url="http://example.com/mybranch";)
+        self.assertEqual(ServiceUsage.EXTERNAL, product.codehosting_usage)
         browser = self.getUserBrowser(canonical_url(product, rootsite='code'))
         login(ANONYMOUS)
         content = find_tag_by_id(browser.contents, 'external')
@@ -305,10 +318,8 @@
         # Mirrors show the correct upstream url as the mirror location.
         url = "http://example.com/mybranch";
         product, branch = self.makeProductAndDevelopmentFocusBranch(
-            branch_type=BranchType.MIRRORED,
-            url=url)
-        view = create_initialized_view(product, '+code-index',
-                                       rootsite='code')
+            branch_type=BranchType.MIRRORED, url=url)
+        view = create_initialized_view(product, '+code-index', rootsite='code')
         self.assertEqual(url, view.mirror_location)
 
 
@@ -343,8 +354,7 @@
         # If the BranchUsage is EXTERNAL then the portlets are shown.
         url = "http://example.com/mybranch";
         product, branch = self.makeProductAndDevelopmentFocusBranch(
-            branch_type=BranchType.MIRRORED,
-            url=url)
+            branch_type=BranchType.MIRRORED, url=url)
         browser = self.getUserBrowser(canonical_url(product, rootsite='code'))
         contents = browser.contents
         self.assertIsNot(None, find_tag_by_id(contents, 'branch-portlet'))
@@ -397,4 +407,4 @@
         product = self.factory.makeProduct()
         login_person(product.owner)
         view = create_view(product, '+branches', layer=CodeLayer)
-        self.assertEqual(True, view.can_configure_branches())
+        self.assertTrue(view.can_configure_branches())

=== modified file 'lib/lp/code/templates/product-branch-summary.pt'
--- lib/lp/code/templates/product-branch-summary.pt	2012-07-12 13:15:10 +0000
+++ lib/lp/code/templates/product-branch-summary.pt	2012-10-03 03:45:23 +0000
@@ -19,8 +19,7 @@
   </div>
 
   <div id="external"
-       tal:condition="context/codehosting_usage/enumvalue:EXTERNAL"
-       tal:define="branch view/branch">
+       tal:condition="view/external_visible" tal:define="branch view/branch">
     <strong>
       <p tal:condition="not: branch/branch_type/enumvalue:IMPORTED">
         <tal:project_title replace="context/title" /> hosts its code at
@@ -43,16 +42,16 @@
       You can learn more at the project's
       <a tal:attributes="href context/homepageurl">web page</a>.
     </p>
-    <p tal:condition="view/branch/branch_type/enumvalue:MIRRORED">
+    <p tal:condition="branch/branch_type/enumvalue:MIRRORED">
       Launchpad has a mirror of the master branch and you can create branches
       from it.
     </p>
-    <p tal:condition="view/branch/branch_type/enumvalue:IMPORTED">
+    <p tal:condition="branch/branch_type/enumvalue:IMPORTED">
       Launchpad imports the master branch and you can create branches from
       it.
     </p>
 
-    <p tal:condition="view/branch/branch_type/enumvalue:REMOTE">
+    <p tal:condition="branch/branch_type/enumvalue:REMOTE">
       Launchpad does not have a copy of the remote branch.
     </p>
   </div>