Merge lp:~rvb/launchpad/branches-timeout-bug-827935 into lp:launchpad

[Raphaël Badin]

[r=gmb][bug=827935] Add a new simplified menu branch without the counts.

This branch adds a new "simplified" branch menu. The new menu does not include the number of {owned,registered,subscribed} branches which is the main reason why the page https://code.launchpad.net/~ubuntu-branches times out. The display of the new menu is protected by a feature flag so that we will be able to see (with real data) if the performance boost is worth the simplification of the design.

= Details =

The performance for this page was analysed by lifeless, jtv and myself (https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-2124AO73). Obviously, only the teams/persons with a very large number of owned branches is a problem (~ubuntu-branches has ~300k branches). As usual, most of the time is SQL time (~10s out of a total of ~11s for the whole page rendering).

After trying to see if the individual queries could be improved lifeless suggested this —rather dramatic— approach because it was clear that the performance problems were coming from the count queries used to display the number of {owned,registered,subscribed} branches: one of the count queries is taking ~3s and the others ~1s.

= UI =

The new UI will look like this: http://people.canonical.com/~rvb/new_menu_branches.png where the old menu looks like this http://people.canonical.com/~rvb/prev_menu_branches.png.

On the bright side, this will unify the branch menu looks with the bug menu looks (this is how the bug menu looks atm http://people.canonical.com/~rvb/current_bug_menu.png).

= Testing =

Right now, the display of this new menu is protected by a feature flag: code.simplified_branches_menu.enabled. My goal is to:
a) make sure that the performance boost is worth the change
b) ask the list to see if everyone is okay with the change

Also, I've recoded the testing done in lib/lp/code/stories/branches/xx-person-branches.txt so if we decide that this can land for real (i.e. without the FF), this doctest can be removed.

[Graham Binns (gmb)]

Hi Raphaël,

Nice branch! I've got a few comments but they're all cosmetic really. Otherwise, r=me.

[1]

62 +
63 + if self.simplified_branch_menu:
64 + return (
65 + self.registered_branch_not_empty or
66 + self.owned_branch_not_empty or
67 + self.subscribed_branch_not_empty or
68 + self.active_review_not_empty
69 + )
70 + else:
71 + return (self.owned_branch_count or
72 + self.registered_branch_count or
73 + self.subscribed_branch_count or
74 + self.active_review_count)

Slightly incosistent formatting here: The form you've used from
64-69 (newline after the opening parenthesis) is the correct one, though
the closing paren should go on the last line as you've done on line 74.

[2]

89 + @cachedproperty
90 + def owned_branch_not_empty(self):
91 + """False if the number of branches owned by self.person is zero."""
92 + return not self._getCountCollection().ownedBy(self.person).is_empty()

I get the feeling this should be named "owned_branches_not_empty" since
you're talking about a collection but the property name suggests you're
talking about a single branch.

[3]

94 + @cachedproperty
95 + def subscribed_branch_not_empty(self):

... And by the same token this should be subscribed_branches_not_empty.

[4]

103 + @cachedproperty
104 + def active_review_not_empty(self):

active_reviews_not_empty

[5]

224 + registered_branches_matcher = soupmatchers.HTMLContains(
225 + soupmatchers.Tag(
226 + 'Registered link', 'a', text='Registered branches',
227 + attrs={'href': 'http://launchpad.dev/~barney'
228 + '/+registeredbranches'}))
229 +

By convention, if you're going to declare a class attribute, you should
do it at the top of the class, since otherwise the reader (i.e. your
humble bearded reviewer) gets a bit confused :).

review: Approve (code)

[Raphaël Badin (rvb)]

> [1]
>
[...]
> Slightly incosistent formatting here: The form you've used from
> 64-69 (newline after the opening parenthesis) is the correct one, though
> the closing paren should go on the last line as you've done on line 74.

Right, I've only written the first part, but I should have harmonized it with the copied code indeed.
Fixed.

> [2]
> [3]
> [4]
> [5]

Right. Done.

> 224 + registered_branches_matcher = soupmatchers.HTMLContains(
> 225 + soupmatchers.Tag(
> 226 + 'Registered link', 'a', text='Registered branches',
> 227 + attrs={'href': 'http://launchpad.dev/~barney'
> 228 + '/+registeredbranches'}))
> 229 +
>
> By convention, if you're going to declare a class attribute, you should
> do it at the top of the class, since otherwise the reader (i.e. your
> humble bearded reviewer) gets a bit confused :).

Okay. I thought it might be a good idea to declare it near where it's used but I suppose it's much better to have a sane default for these.

Thanks for the review Graham!

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2011-10-14 02:12:06 +0000
+++ lib/lp/code/browser/branchlisting.py	2011-11-01 14:15:30 +0000
@@ -31,6 +31,7 @@
     ]
 
 from operator import attrgetter
+import urlparse
 
 from lazr.delegates import delegates
 from lazr.enum import (
@@ -41,7 +42,6 @@
     Asc,
     Desc,
     )
-import urlparse
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.component import getUtility
 from zope.formlib import form
@@ -135,6 +135,7 @@
 from lp.registry.interfaces.sourcepackage import ISourcePackageFactory
 from lp.registry.model.sourcepackage import SourcePackage
 from lp.services.browser_helpers import get_plural_text
+from lp.services.features import getFeatureFlag
 from lp.services.propertycache import cachedproperty
 
 
@@ -850,7 +851,9 @@
     usedfor = IPerson
     facet = 'branches'
     links = ['registered', 'owned', 'subscribed', 'addbranch',
-             'active_reviews', 'mergequeues']
+             'active_reviews', 'mergequeues',
+             'simplified_subscribed', 'simplified_registered',
+             'simplified_owned', 'simplified_active_reviews']
     extra_attributes = [
         'active_review_count',
         'owned_branch_count',
@@ -858,6 +861,7 @@
         'show_summary',
         'subscribed_branch_count',
         'mergequeue_count',
+        'simplified_branches_menu',
         ]
 
     def _getCountCollection(self):
@@ -881,13 +885,88 @@
         """
         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."""
-        return (self.owned_branch_count or
+
+        if self.simplified_branches_menu:
+            return (
+                self.registered_branches_not_empty or
+                self.owned_branches_not_empty or
+                self.subscribed_branches_not_empty or
+                self.active_reviews_not_empty
+                )
+        else:
+            return (
+                self.owned_branch_count or
                 self.registered_branch_count or
                 self.subscribed_branch_count or
-                self.active_review_count)
+                self.active_review_count
+                )
+
+    @cachedproperty
+    def simplified_branches_menu(self):
+        return getFeatureFlag('code.simplified_branches_menu.enabled')
+
+    @cachedproperty
+    def registered_branches_not_empty(self):
+        """False if the number of branches registered by self.person
+        is zero.
+        """
+        return (
+            not self._getCountCollection().registeredBy(
+                self.person).is_empty())
+
+    @cachedproperty
+    def owned_branches_not_empty(self):
+        """False if the number of branches owned by self.person is zero."""
+        return not self._getCountCollection().ownedBy(self.person).is_empty()
+
+    @cachedproperty
+    def subscribed_branches_not_empty(self):
+        """False if the number of branches subscribed to by self.person
+        is zero.
+        """
+        return (
+            not self._getCountCollection().subscribedBy(
+                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'),
+            'Owned branches',
+            enabled=self.owned_branches_not_empty)
+
+    def simplified_registered(self):
+        person_is_individual = (not self.person.is_team)
+        return Link(
+            '+registeredbranches',
+            'Registered branches',
+            enabled=(
+                person_is_individual and
+                self.registered_branches_not_empty))
+
+    def simplified_subscribed(self):
+        return Link(
+            '+subscribedbranches',
+            'Subscribed branches',
+            enabled=self.subscribed_branches_not_empty)
+
+    def simplified_active_reviews(self):
+        return Link(
+            '+activereviews',
+            'Active reviews',
+            enabled=self.active_reviews_not_empty)
 
     @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-10-14 02:12:06 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2011-11-01 14:15:30 +0000
@@ -11,18 +11,22 @@
 import re
 
 from lazr.uri import URI
+import soupmatchers
 from storm.expr import (
     Asc,
     Desc,
     )
+from testtools.matchers import Not
 from zope.component import getUtility
 
 from canonical.launchpad.testing.pages import (
     extract_text,
+    find_main_content,
     find_tag_by_id,
-    find_main_content)
+    )
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing import LaunchpadFunctionalLayer
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.code.browser.branchlisting import (
     BranchListingSort,
@@ -31,7 +35,10 @@
     PersonProductSubscribedBranchesView,
     SourcePackageBranchesView,
     )
-from lp.code.enums import BranchVisibilityRule
+from lp.code.enums import (
+    BranchMergeProposalStatus,
+    BranchVisibilityRule,
+    )
 from lp.code.model.branch import Branch
 from lp.code.model.seriessourcepackagebranch import (
     SeriesSourcePackageBranchSet,
@@ -59,8 +66,8 @@
     COMMERCIAL_ADMIN_EMAIL,
     )
 from lp.testing.views import (
+    create_initialized_view,
     create_view,
-    create_initialized_view,
     )
 
 
@@ -257,6 +264,96 @@
         self._test_batch_template(self.barney)
 
 
+SIMPLIFIED_BRANCHES_MENU_FLAG = {
+    'code.simplified_branches_menu.enabled': 'on'}
+
+
+class TestSimplifiedPersonOwnedBranchesView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    registered_branches_matcher = soupmatchers.HTMLContains(
+        soupmatchers.Tag(
+            'Registered link', 'a', text='Registered branches',
+            attrs={'href': 'http://launchpad.dev/~barney'
+                           '/+registeredbranches'}))
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.user = self.factory.makePerson()
+        self.person = self.factory.makePerson(name='barney')
+        self.team = self.factory.makeTeam(owner=self.person)
+        self.product = self.factory.makeProduct(name='bambam')
+
+    def get_branch_list_page(self, page_name='+branches', user=None):
+        if user is None:
+            user = self.person
+        with FeatureFixture(SIMPLIFIED_BRANCHES_MENU_FLAG):
+            with person_logged_in(self.user):
+                return create_initialized_view(
+                    user, page_name, rootsite='code',
+                    principal=self.user)()
+
+    def test_branch_list_h1(self):
+        page = self.get_branch_list_page()
+        h1_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Title', 'h1', text='Bazaar branches owned by Barney'))
+        self.assertThat(page, h1_matcher)
+
+    def test_branch_list_empty(self):
+        page = self.get_branch_list_page()
+        empty_message_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Empty message', 'p',
+                text='There are no branches related to Barney '
+                     'in Launchpad today.'))
+        self.assertThat(page, empty_message_matcher)
+        self.assertThat(page, Not(self.registered_branches_matcher))
+
+    def test_branch_list_registered_link(self):
+        self.factory.makeAnyBranch(owner=self.person)
+        page = self.get_branch_list_page()
+        self.assertThat(page, self.registered_branches_matcher)
+
+    def test_branch_list_owned_link(self):
+        owned_branches_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Owned link', 'a', text='Owned branches',
+                attrs={'href': 'http://code.launchpad.dev/~barney'}))
+        self.factory.makeAnyBranch(owner=self.person)
+        page = self.get_branch_list_page('+subscribedbranches')
+        self.assertThat(page, owned_branches_matcher)
+
+    def test_branch_list_subscribed_link(self):
+        subscribed_branches_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Subscribed link', 'a', text='Subscribed branches',
+                attrs={'href': 'http://launchpad.dev/~barney'
+                               '/+subscribedbranches'}))
+        self.factory.makeAnyBranch(owner=self.person)
+        page = self.get_branch_list_page()
+        self.assertThat(page, subscribed_branches_matcher)
+
+    def test_branch_list_activereviews_link(self):
+        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)
+
+    def test_branch_list_no_registered_link_team(self):
+        self.factory.makeAnyBranch(owner=self.person)
+        page = self.get_branch_list_page(user=self.team)
+        self.assertThat(page, Not(self.registered_branches_matcher))
+
+
 class TestSourcePackageBranchesView(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py	2011-08-25 20:37:02 +0000
+++ lib/lp/code/interfaces/branchcollection.py	2011-11-01 14:15:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211, E0213
@@ -50,6 +50,9 @@
     def count():
         """The number of branches in this collection."""
 
+    def is_empty():
+        """Is this collection empty?"""
+
     def ownerCounts():
         """Return the number of different branch owners.
 

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-10-09 23:35:43 +0000
+++ lib/lp/code/model/branchcollection.py	2011-11-01 14:15:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementations of `IBranchCollection`."""
@@ -33,31 +33,31 @@
     DecoratedResultSet,
     )
 from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.launchpad.searchbuilder import any
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
     IStoreSelector,
     MAIN_STORE,
     )
-from canonical.launchpad.searchbuilder import any
 from canonical.launchpad.webapp.vocabulary import CountableIterator
 from lp.bugs.interfaces.bugtask import (
+    BugTaskSearchParams,
     IBugTaskSet,
-    BugTaskSearchParams,
     )
 from lp.bugs.interfaces.bugtaskfilter import filter_bugtasks_by_context
 from lp.bugs.model.bugbranch import BugBranch
 from lp.bugs.model.bugtask import BugTask
+from lp.code.enums import BranchMergeProposalStatus
 from lp.code.interfaces.branch import user_has_special_branch_access
 from lp.code.interfaces.branchcollection import (
     IBranchCollection,
     InvalidFilter,
     )
+from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
 from lp.code.interfaces.seriessourcepackagebranch import (
     IFindOfficialBranchLinks,
     )
-from lp.code.enums import BranchMergeProposalStatus
-from lp.code.interfaces.branchlookup import IBranchLookup
-from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
 from lp.code.model.branch import Branch
 from lp.code.model.branchmergeproposal import BranchMergeProposal
 from lp.code.model.branchsubscription import BranchSubscription
@@ -132,6 +132,10 @@
         """See `IBranchCollection`."""
         return self.getBranches(eager_load=False).count()
 
+    def is_empty(self):
+        """See `IBranchCollection`."""
+        return self.getBranches(eager_load=False).is_empty()
+
     def ownerCounts(self):
         """See `IBranchCollection`."""
         is_team = Person.teamowner != None

=== modified file 'lib/lp/code/templates/person-codesummary.pt'
--- lib/lp/code/templates/person-codesummary.pt	2010-11-18 12:05:34 +0000
+++ lib/lp/code/templates/person-codesummary.pt	2011-11-01 14:15:30 +0000
@@ -8,7 +8,7 @@
       features request/features"
   tal:condition="menu/show_summary">
 
-  <table>
+  <table tal:condition="not: menu/simplified_branches_menu">
     <tr class="code-links">
       <td class="code-count" tal:content="menu/owned_branch_count">100</td>
       <td tal:content="structure menu/owned/render"
@@ -34,4 +34,29 @@
           />
     </tr>
   </table>
+
+  <table tal:condition="menu/simplified_branches_menu">
+    <tr class="code-links"
+        tal:condition="menu/simplified_owned/enabled">
+      <td tal:content="structure menu/simplified_owned/render" />
+    </tr>
+    <tr class="code-links"
+        tal:condition="menu/simplified_registered/enabled">
+      <td tal:content="structure menu/simplified_registered/render" />
+    </tr>
+    <tr class="code-links"
+        tal:condition="menu/simplified_subscribed/enabled">
+      <td tal:content="structure menu/simplified_subscribed/render" />
+    </tr>
+    <tr class="code-links"
+        tal:condition="menu/simplified_active_reviews/enabled">
+      <td tal:content="structure menu/simplified_active_reviews/render" />
+    </tr>
+    <tr tal:condition="features/code.branchmergequeue" id="mergequeue-counts">
+      <td class="code-count" tal:content="menu/mergequeue_count">5</td>
+      <td tal:condition="menu"
+          tal:content="structure menu/mergequeues/render"
+          />
+    </tr>
+  </table>
 </div>

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-10-27 01:11:39 +0000
+++ lib/lp/services/features/flags.py	2011-11-01 14:15:30 +0000
@@ -64,6 +64,10 @@
      'boolean',
      'Shows incremental diffs on merge proposals.',
      ''),
+    ('code.simplified_branches_menu.enabled',
+     'boolean',
+     ('Display a simplified version of the branch menu (omit the counts).'),
+     ''),
     ('hard_timeout',
      'float',
      'Sets the hard request timeout in milliseconds.',