launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/launchpad/branches-timeout-bug-827935-3 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #827935 in Launchpad itself: "Person:+branches timeouts"
  https://bugs.launchpad.net/launchpad/+bug/827935

For more details, see:
https://code.launchpad.net/~rvb/launchpad/branches-timeout-bug-827935-3/+merge/81262

This branch is a small optimization that will (in some cases) save a query for the https://code.launchpad.net/~team page.  For https://code.launchpad.net/~ubuntu-branches, the query in question takes ~1s so that's a big win for a small modification.

The page displays a batch of a filtered collection (the filter is on branches' statuses) of branches.  We want to display a text if the filtered batch is empty but other branches might be displayed if the listing was not filtered.  Obviously, if the batch itself is not empty, we don't need to check the size of the whole displayable collection to know that.

Drive-by fix: remove an ill-defined (and used) function that I introduced in a previous branch…

= Tests =

This should not change existing behavior.
-- 
https://code.launchpad.net/~rvb/launchpad/branches-timeout-bug-827935-3/+merge/81262
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/branches-timeout-bug-827935-3 into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2011-11-02 14:04:31 +0000
+++ lib/lp/code/browser/branchlisting.py	2011-11-04 11:57:32 +0000
@@ -613,7 +613,7 @@
 
     def hasAnyBranchesVisibleByUser(self):
         """Does the context have any branches that are visible to the user?"""
-        return self.branch_count > 0
+        return not self.is_branch_count_zero
 
     def _getCollection(self):
         """Override this to say what branches will be in the listing."""
@@ -624,6 +624,16 @@
         """The number of total branches the user can see."""
         return self._getCollection().visibleByUser(self.user).count()
 
+    @property
+    def is_branch_count_zero(self):
+        """Is the number of total branches the user can see zero?."""
+        # If the batch itself is not empty, we don't need to check
+        # the whole collection count (it might be expensive to compute if the
+        # total number of branches is huge).
+        return (
+            len(self.branches().visible_branches_for_view) == 0 and
+            not self.branch_count)
+
     def _branches(self, lifecycle_status):
         """Return a sequence of branches.
 
@@ -885,10 +895,6 @@
         """
         return self.context
 
-    @cachedproperty
-    def has_branches(self):
-        """Should the template show the summary view with the links."""
-
     @property
     def show_summary(self):
         """Should the template show the summary view with the links."""

=== modified file 'lib/lp/code/templates/person-branches.pt'
--- lib/lp/code/templates/person-branches.pt	2010-10-26 10:32:35 +0000
+++ lib/lp/code/templates/person-branches.pt	2011-11-04 11:57:32 +0000
@@ -33,13 +33,13 @@
       replace="view/user/name"/>/+junk/<em>BRANCHNAME</em></tt>
   </p>
 
-  <div id="no-branch-message" tal:condition="not: view/branch_count">
+  <div id="no-branch-message" tal:condition="view/is_branch_count_zero">
     <p tal:content="view/no_branch_message">
       There are no branches related to Eric the Viking today.
     </p>
   </div>
 
-  <tal:has-branches condition="view/branch_count">
+  <tal:has-branches condition="not: view/is_branch_count_zero">
     <tal:branchlisting
         content="structure branches/@@+branch-listing" />
   </tal:has-branches>