Merge lp://staging/~rvb/launchpad/branches-timeout-bug-827935 into lp://staging/launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Raphaël Badin | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 14225 | ||||
Proposed branch: | lp://staging/~rvb/launchpad/branches-timeout-bug-827935 | ||||
Merge into: | lp://staging/launchpad | ||||
Diff against target: |
420 lines (+227/-15) 6 files modified
lib/lp/code/browser/branchlisting.py (+83/-4) lib/lp/code/browser/tests/test_branchlisting.py (+100/-3) lib/lp/code/interfaces/branchcollection.py (+4/-1) lib/lp/code/model/branchcollection.py (+10/-6) lib/lp/code/templates/person-codesummary.pt (+26/-1) lib/lp/services/features/flags.py (+4/-0) |
||||
To merge this branch: | bzr merge lp://staging/~rvb/launchpad/branches-timeout-bug-827935 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Graham Binns (community) | code | Approve | |
Review via email: mp+80875@code.staging.launchpad.net |
Commit message
[r=gmb][bug=827935] Add a new simplified menu branch without the counts.
Description of the change
This branch adds a new "simplified" branch menu. The new menu does not include the number of {owned,
= Details =
The performance for this page was analysed by lifeless, jtv and myself (https:/
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,
= UI =
The new UI will look like this: http://
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://
= Testing =
Right now, the display of this new menu is protected by a feature flag: code.simplified
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/
Hi Raphaël,
Nice branch! I've got a few comments but they're all cosmetic really. Otherwise, r=me.
[1]
62 + _branch_ menu: _branch_ not_empty or branch_ not_empty or _branch_ not_empty or review_ not_empty branch_ count or _branch_ count or _branch_ count or review_ count)
63 + if self.simplified
64 + return (
65 + self.registered
66 + self.owned_
67 + self.subscribed
68 + self.active_
69 + )
70 + else:
71 + return (self.owned_
72 + self.registered
73 + self.subscribed
74 + self.active_
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 not_empty( self): ollection( ).ownedBy( self.person) .is_empty( )
90 + def owned_branch_
91 + """False if the number of branches owned by self.person is zero."""
92 + return not self._getCountC
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 branch_ not_empty( self):
95 + def subscribed_
... And by the same token this should be subscribed_ branches_ not_empty.
[4]
103 + @cachedproperty review_ not_empty( self):
104 + def active_
active_ reviews_ not_empty
[5]
224 + registered_ branches_ matcher = soupmatchers. HTMLContains( launchpad. dev/~barney' anches' }))
225 + soupmatchers.Tag(
226 + 'Registered link', 'a', text='Registered branches',
227 + attrs={'href': 'http://
228 + '/+registeredbr
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 :).