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-4 into lp:launchpad with lp:~rvb/launchpad/branches-timeout-bug-827935-3 as a prerequisite.

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-4/+merge/81290

Another simplification to the simplified menu (which is protected by a FF) on https://code.launchpad.net/~me.  Checking for the presence of active reviews (to choose whether or not we want to display a link) leads to this kind of request: http://paste.ubuntu.com/728236/ which takes 1s-2s on ~https://code.launchpad.net/~ubuntu-branches.  You might think that the duplication of the block http://paste.ubuntu.com/728238/ is responsible for the bad performance but that's not the case;  the problem is the enormous number of branches satisfying "Branch.transitively_private = FALSE" (300k on ~ubuntu-branches).  The easy solution is to always display the link to the "Active reviews" page.

= Tests =
(Modified)
./bin/test -vvc test_branchlisting test_branch_list_activereviews_link
-- 
https://code.launchpad.net/~rvb/launchpad/branches-timeout-bug-827935-4/+merge/81290
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/branches-timeout-bug-827935-4 into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2011-11-04 15:31:33 +0000
+++ lib/lp/code/browser/branchlisting.py	2011-11-04 15:31:38 +0000
@@ -922,12 +922,6 @@
             not self._getCountCollection().registeredBy(
                 self.person).is_empty())
 
-    @cachedproperty
-    def active_reviews_not_empty(self):
-        """Return the number of active reviews for self.person's branches."""
-        active_reviews = PersonActiveReviewsView(self.context, self.request)
-        return not active_reviews.getProposals().is_empty()
-
     def simplified_owned(self):
         return Link(
             canonical_url(self.context, rootsite='code'),
@@ -950,8 +944,7 @@
     def simplified_active_reviews(self):
         return Link(
             '+activereviews',
-            'Active reviews',
-            enabled=self.active_reviews_not_empty)
+            'Active reviews')
 
     @cachedproperty
     def registered_branch_count(self):

=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2011-11-02 14:04:31 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2011-11-04 15:31:38 +0000
@@ -36,7 +36,6 @@
     SourcePackageBranchesView,
     )
 from lp.code.enums import (
-    BranchMergeProposalStatus,
     BranchVisibilityRule,
     )
 from lp.code.model.branch import Branch
@@ -336,15 +335,12 @@
         self.assertThat(page, subscribed_branches_matcher)
 
     def test_branch_list_activereviews_link(self):
+        # The link to the actives reviews is always displayed.
         active_review_matcher = soupmatchers.HTMLContains(
             soupmatchers.Tag(
                 'Active reviews link', 'a', text='Active reviews',
                 attrs={'href': 'http://launchpad.dev/~barney'
                                '/+activereviews'}))
-        branch = self.factory.makeAnyBranch(owner=self.person)
-        self.factory.makeBranchMergeProposal(
-            target_branch=branch, registrant=self.person,
-            set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
         page = self.get_branch_list_page()
         self.assertThat(page, active_review_matcher)