launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/product-all-branches-dies into lp:launchpad.

Commit message:
Merge Product:+all-branches into Product:+branches.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/product-all-branches-dies/+merge/260793

Product:+all-branches and Product:+branches are separate because +branches defaults to a custom sort order that can't be batch-navigated. But it turns out that most programming languages support conditional statements, so we can actually have them in a single view.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/product-all-branches-dies into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2015-06-01 22:39:30 +0000
+++ lib/lp/code/browser/branchlisting.py	2015-06-02 06:53:37 +0000
@@ -1250,10 +1250,6 @@
     show_set_development_focus = True
 
     @property
-    def form_action(self):
-        return "+all-branches"
-
-    @property
     def initial_values(self):
         return {
             'lifecycle': BranchLifecycleStatusFilter.CURRENT,
@@ -1314,42 +1310,14 @@
     def _branches(self, lifecycle_status):
         """Return the series branches, followed by most recently changed."""
         # The params are ignored, and only used by the listing view.
-        return self.initial_branches
-
-    def hasAnyBranchesVisibleByUser(self):
-        """See `BranchListingView`."""
-        return self.branch_count > 0
-
-    @property
-    def has_development_focus_branch(self):
-        """Is there a branch assigned as development focus?"""
-        return self.development_focus_branch is not None
-
-
-class ProductAllBranchesView(ProductBranchListingView):
-    """View for branch listing for a product."""
-
-    def initialize(self):
-        """Conditionally redirect to the default view.
-
-        If the branch listing requests the default listing, redirect to the
-        default view for the product.
-        """
-        ProductBranchListingView.initialize(self)
         if self.sort_by == BranchListingSort.DEFAULT:
-            redirect_url = canonical_url(self.context)
-            widget = self.widgets['lifecycle']
-            if widget.hasValidInput():
-                redirect_url += (
-                    '?field.lifecycle=' + widget.getInputValue().name)
-            self.request.response.redirect(redirect_url)
+            return self.initial_branches
+        return super(ProductBranchesView, self)._branches(lifecycle_status)
 
     @property
-    def initial_values(self):
-        return {
-            'lifecycle': BranchLifecycleStatusFilter.CURRENT,
-            'sort_by': BranchListingSort.LIFECYCLE,
-            }
+    def has_development_focus_branch(self):
+        """Is there a branch assigned as development focus?"""
+        return self.development_focus_branch is not None
 
 
 class ProjectBranchesView(BranchListingView):

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2015-06-01 08:43:09 +0000
+++ lib/lp/code/browser/configure.zcml	2015-06-02 06:53:37 +0000
@@ -712,12 +712,6 @@
 
     <browser:page
         for="lp.registry.interfaces.product.IProduct"
-        class="lp.code.browser.branchlisting.ProductAllBranchesView"
-        permission="zope.Public"
-        name="+all-branches"
-        template="../templates/product-branches.pt"/>
-    <browser:page
-        for="lp.registry.interfaces.product.IProduct"
         class="lp.code.browser.branchlisting.ProductBranchesView"
         permission="zope.Public"
         name="+branches"

=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2015-01-29 14:14:01 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2015-06-02 06:53:37 +0000
@@ -688,7 +688,7 @@
     def test_branch_listing_last_modified(self):
         branch = self.factory.makeProductBranch()
         view = create_initialized_view(
-            branch.product, name="+all-branches", rootsite='code')
+            branch.product, name="+branches", rootsite='code')
         self.assertIn('a moment ago', view())
 
     def test_no_branch_message_escaped(self):
@@ -740,7 +740,7 @@
         # A search request with a 'batch_request' query parameter causes the
         # view to just render the next batch of results.
         product = self.factory.makeProduct(projectgroup=self.projectgroup)
-        self._test_search_batch_request(product, view_name='+all-branches')
+        self._test_search_batch_request(product, view_name='+branches')
 
     def test_ajax_batch_navigation_feature_flag(self):
         # The Javascript to wire up the ajax batch navigation behaviour is
@@ -749,7 +749,7 @@
         for i in range(10):
             self.factory.makeProductBranch(product=product)
         self._test_ajax_batch_navigation_feature_flag(
-            product, view_name='+all-branches')
+            product, view_name='+branches')
 
     def test_non_batch_template(self):
         # The correct template is used for non batch requests.

=== modified file 'lib/lp/code/browser/tests/test_product.py'
--- lib/lp/code/browser/tests/test_product.py	2015-06-01 08:43:09 +0000
+++ lib/lp/code/browser/tests/test_product.py	2015-06-02 06:53:37 +0000
@@ -440,11 +440,11 @@
 
     def test_cannot_configure_branches_product_no_edit_permission(self):
         product = self.factory.makeProduct()
-        view = create_view(product, '+all-branches')
+        view = create_view(product, '+branches')
         self.assertEqual(False, view.can_configure_branches())
 
     def test_can_configure_branches_product_with_edit_permission(self):
         product = self.factory.makeProduct()
         login_person(product.owner)
-        view = create_view(product, '+all-branches')
+        view = create_view(product, '+branches')
         self.assertTrue(view.can_configure_branches())

=== modified file 'lib/lp/code/stories/branches/xx-branch-listings-merge-proposal-badge.txt'
--- lib/lp/code/stories/branches/xx-branch-listings-merge-proposal-badge.txt	2014-02-25 06:38:58 +0000
+++ lib/lp/code/stories/branches/xx-branch-listings-merge-proposal-badge.txt	2015-06-02 06:53:37 +0000
@@ -24,14 +24,14 @@
     >>> proposal = source.addLandingTarget(bob, target)
     >>> logout()
 
-    >>> browser.open('http://code.launchpad.dev/firefox/+all-branches')
+    >>> browser.open('http://code.launchpad.dev/firefox/+branches')
     >>> branchSummary(browser)
     /~bob/firefox/review
     Has a merge proposal
     /~bob/firefox/target
-    /~mark/firefox/release--0.9.1
     /~mark/firefox/release-0.8
     /~mark/firefox/release-0.9
+    /~mark/firefox/release--0.9.1
     /~mark/firefox/release-0.9.2
     Linked to a bug
 

=== modified file 'lib/lp/code/stories/branches/xx-branch-listings.txt'
--- lib/lp/code/stories/branches/xx-branch-listings.txt	2014-12-31 13:24:43 +0000
+++ lib/lp/code/stories/branches/xx-branch-listings.txt	2015-06-02 06:53:37 +0000
@@ -213,6 +213,8 @@
     >>> def branchSummary(browser):
     ...     table = find_tag_by_id(browser.contents, 'branchtable')
     ...     for row in table.tbody.fetch('tr'):
+    ...         if row.getText().startswith('A development focus branch'):
+    ...             continue
     ...         cells = row.findAll('td')
     ...         first_cell = cells[0]
     ...         anchors = first_cell.fetch('a')
@@ -221,7 +223,9 @@
     ...         for img in cells[1].findAll('img'):
     ...             print img['title']
 
-    >>> browser.open('http://code.launchpad.dev/firefox/+all-branches')
+    >>> browser.open(
+    ...     'http://code.launchpad.dev/firefox/+branches'
+    ...     '?field.sort_by=LIFECYCLE')
     >>> branchSummary(browser)
     /~mark/firefox/release--0.9.1
     /~mark/firefox/release-0.8
@@ -240,7 +244,7 @@
 
 Now the badge is still shown for Sample Person...
 
-    >>> browser.open('http://code.launchpad.dev/firefox/+all-branches')
+    >>> browser.open('http://code.launchpad.dev/firefox/+branches')
     >>> branchSummary(browser)
     /~mark/firefox/release--0.9.1
     /~mark/firefox/release-0.8
@@ -252,7 +256,7 @@
 
 ... but not for an anonymous user.
 
-    >>> anon_browser.open('http://code.launchpad.dev/firefox/+all-branches')
+    >>> anon_browser.open('http://code.launchpad.dev/firefox/+branches')
     >>> branchSummary(anon_browser)
     /~mark/firefox/release--0.9.1
     /~mark/firefox/release-0.8
@@ -286,13 +290,13 @@
     oldest first
 
 On a listing that can only have branches from one product, the default
-is to sort by lifecycle status, and product name is not one of the
+is to sort by most interesting, and product name is not one of the
 options that can be selected.
 
-    >>> browser.open('http://code.launchpad.dev/gnome-terminal/+all-branches')
+    >>> browser.open('http://code.launchpad.dev/gnome-terminal/+branches')
     >>> sort_by_control = browser.getControl(name='field.sort_by')
     >>> sort_by_control.value
-    ['by status']
+    ['by most interesting']
     >>> sort_by_control.value = ['by project name']
     Traceback (most recent call last):
       ...
@@ -311,7 +315,7 @@
 If you filter by a specific lifecycle status then ordering by
 lifecycle ceases to be relevant, and the default is to sort by registrant.
 
-    >>> browser.open('http://code.launchpad.dev/gnome-terminal/+all-branches')
+    >>> browser.open('http://code.launchpad.dev/gnome-terminal/+branches')
     >>> status_control = browser.getControl(name='field.lifecycle')
     >>> status_control.value = ['MATURE']
     >>> browser.getControl('Filter').click()
@@ -328,10 +332,11 @@
 
 Finally, sorting by a particular criterion has the desired effect.
 
-    >>> browser.open('http://code.launchpad.dev/gnome-terminal/+all-branches')
+    >>> browser.open('http://code.launchpad.dev/gnome-terminal/+branches')
     >>> table = find_tag_by_id(browser.contents, 'branchtable')
     >>> for row in table.tbody.fetch('tr'):
     ...     print extract_text(row)
+    A development focus ...
     lp://dev/~name12/gnome-terminal/2.6            Mature       ...
     lp://dev/~launchpad/gnome-terminal/launchpad   Development  ...
     lp://dev/~name12/gnome-terminal/main           Development  ...
@@ -344,6 +349,7 @@
     >>> table = find_tag_by_id(browser.contents, 'branchtable')
     >>> for row in table.tbody.fetch('tr'):
     ...     print extract_text(row)
+    A development focus ...
     lp://dev/~name12/gnome-terminal/2.6             Mature          ...
     lp://dev/~vcs-imports/gnome-terminal/import     Development             ...
     lp://dev/~name12/gnome-terminal/klingon         Experimental    ...
@@ -439,8 +445,6 @@
 
     >>> browser.getControl(name='field.lifecycle').displayValue = ['Experimental']
     >>> browser.getControl('Filter').click()
-    >>> print browser.url
-    http://code.launchpad.dev/gnome-terminal?field.lifecycle=EXPERIMENTAL
     >>> table = find_tag_by_id(browser.contents, 'branchtable')
     >>> for row in table.tbody.fetch('tr'):
     ...     print extract_text(row)
@@ -451,8 +455,6 @@
 
     >>> browser.getControl(name='field.lifecycle').displayValue = ['Development']
     >>> browser.getControl('Filter').click()
-    >>> print browser.url
-    http://code.launchpad.dev/gnome-terminal?field.lifecycle=DEVELOPMENT
     >>> table = find_tag_by_id(browser.contents, 'branchtable')
     >>> for row in table.tbody.fetch('tr'):
     ...     print extract_text(row)
@@ -467,7 +469,7 @@
 shown in the column after the last commit to allow the user to sort on
 the revision author column.
 
-    >>> browser.open('http://code.launchpad.dev/firefox/+all-branches')
+    >>> browser.open('http://code.launchpad.dev/firefox/+branches')
     >>> for commit in find_tags_by_class(browser.contents, 'lastCommit'):
     ...     print extract_text(commit)
     1.  Import of Mozilla Firefox 0.9.1

=== modified file 'lib/lp/code/stories/branches/xx-private-branch-listings.txt'
--- lib/lp/code/stories/branches/xx-private-branch-listings.txt	2014-12-06 10:45:17 +0000
+++ lib/lp/code/stories/branches/xx-private-branch-listings.txt	2015-06-02 06:53:37 +0000
@@ -103,7 +103,7 @@
 listing tab.
 
     >>> def print_landscape_code_listing(browser):
-    ...     browser.open('http://code.launchpad.dev/landscape/+all-branches')
+    ...     browser.open('http://code.launchpad.dev/landscape/+branches')
     ...     table = find_tag_by_id(browser.contents, 'branchtable')
     ...     # If there are no branches, the table is not shown.
     ...     # So print the text shown in the application summary.
@@ -122,13 +122,15 @@
     lp://dev/~no-priv/landscape/testing-branch      Development   ...
 
     >>> print_landscape_code_listing(landscape_dev_browser)
+    A development focus ...
     lp://dev/~landscape-developers/landscape/trunk  Development   ...
     lp://dev/~name12/landscape/feature-x            Development   ...
 
     >>> print_landscape_code_listing(admin_browser)
+    A development focus ...
+    lp://dev/~no-priv/landscape/testing-branch      Development   ...
     lp://dev/~landscape-developers/landscape/trunk  Development   ...
     lp://dev/~name12/landscape/feature-x            Development   ...
-    lp://dev/~no-priv/landscape/testing-branch      Development   ...
 
 
 Person code listing pages

=== modified file 'lib/lp/code/stories/branches/xx-product-branches.txt'
--- lib/lp/code/stories/branches/xx-product-branches.txt	2015-06-01 08:43:09 +0000
+++ lib/lp/code/stories/branches/xx-product-branches.txt	2015-06-02 06:53:37 +0000
@@ -320,7 +320,7 @@
 we are given a different message.
 
     >>> browser.open(
-    ...     'http://code.launchpad.dev/firefox/+all-branches'
+    ...     'http://code.launchpad.dev/firefox/+branches'
     ...     '?field.lifecycle=Mature')
     >>> message = find_tag_by_id(browser.contents, 'no-branch-message')
     >>> print extract_text(message)