Merge lp:~rvb/launchpad/activereviews-bug-867941-eagerload3 into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Raphaël Badin | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 14390 | ||||
Proposed branch: | lp:~rvb/launchpad/activereviews-bug-867941-eagerload3 | ||||
Merge into: | lp:launchpad | ||||
Prerequisite: | lp:~rvb/launchpad/activereviews-bug-893702 | ||||
Diff against target: |
255 lines (+113/-18) 5 files modified
lib/lp/code/browser/tests/test_branchmergeproposallisting.py (+3/-1) lib/lp/code/model/branchcollection.py (+55/-11) lib/lp/code/model/branchmergeproposal.py (+4/-6) lib/lp/code/model/tests/test_branchcollection.py (+45/-0) lib/lp/testing/factory.py (+6/-0) |
||||
To merge this branch: | bzr merge lp:~rvb/launchpad/activereviews-bug-867941-eagerload3 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Brad Crittenden (community) | code | Approve | |
Review via email: mp+83389@code.launchpad.net |
Commit message
[r=bac][bug=867941] Preload the chain of stacked on branches.
Description of the change
This branch is (hopefully) the last one in a series aimed at improving the performance of +activereviews. This branch improves the preloading of the chain of stacked on branches. It uses a recursive CTE to fetch in one query all the branches on which the branches to be displayed in {person,
Two things I'd be happy to have a reviewer's option about:
- since the list of branches provided to preloadVisibleS
- I would be happy to avoid using the name 'column1' (which is the default postgresql name for columns created by VALUES expressions but is not a sql standard) in 'SELECT column1 as id FROM (VALUES %s) AS temp[...]' but I could not find a way to do this. Here is a working query if you are wanting to give a try at modifying it: http://
=Tests=
(changed)
/bin/test -vvc test_branchmerg
(added)
/bin/test -vvc test_branchcoll
/bin/test -vvc test_branchcoll
/bin/test -vvc test_branchcoll
=Q/A=
https:/
Hi Raphaël,
* For createStackedOn Chain(self, product) it might be nice to add another argument for the number of branches, defaulting to 5.
* Can branch_ids = map(itemgetter(0), result.get_all()) be rewritten as
branch_ ids = [res[0] for res in result.get_all()] ? If so, all things being equal, I think it is more readable.
* Nice use of 'partial'. I learned something new!
* Actually now that I see createBranchChain it looks like createdStackedO nChain is redundant. Care to promote createBranchChain to the factory.py and re-use it?
* As to your question about visibility of stacked on branches, if you can write a test to prove your assertion then I'd be happy for you to delete the extra step. But the test must be there to ensure we don't have a regression that will allow private branches to leak.
* Sorry I have no ideas on your 'column1' question. Perhaps you can ask stub later.