Merge lp:~thumper/launchpad/bmp-index-faster into lp:launchpad
- bmp-index-faster
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Stuart Bishop | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 11030 | ||||
Proposed branch: | lp:~thumper/launchpad/bmp-index-faster | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
428 lines (+191/-118) 3 files modified
lib/lp/code/browser/branch.py (+2/-108) lib/lp/code/browser/branchmergeproposal.py (+6/-10) lib/lp/code/browser/decorations.py (+183/-0) |
||||
To merge this branch: | bzr merge lp:~thumper/launchpad/bmp-index-faster | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Stuart Bishop (community) | Approve | ||
Review via email: mp+27788@code.launchpad.net |
Commit message
Cache the bzr_identity of the source branch used when rendering new revisions on merge proposal pages.
Description of the change
Many of the slow merge proposal renderings are due to having many revisions to show, and each revision has a link to the source branch. During my investigations last week I found that every query to bzr_identity on a branch causes two DB queries. The branch wraps the branch used for the rendering of the new revisions as a DecoratedBranch - which has a cached property for the bzr_identity.
Both the DecoratedBranch and DecoratedBug were moved into their own module as they were now both being used in the branch and branchmergeproposal browser modules.
The xx-code-
Tim Penhey (thumper) wrote : | # |
Stuart Bishop (stub) wrote : | # |
I notice neither DecoratedBug nor DecoratedBranch have docstrings, nor do they define an interface making them inscrutable. This is particularly confusing for DecoratedBug which provides self.tasks, self.bugtasks, self.bugtask, self.getBugTask(), self.default_
We can let this slide on this branch though since this is moving code to a better location, so a net win. It would be nice to merge the synonyms and document things though if possible.
Otherwise all fine.
Tim Penhey (thumper) wrote : | # |
On Thu, 17 Jun 2010 17:07:26 you wrote:
> Review: Approve
> I notice neither DecoratedBug nor DecoratedBranch have docstrings, nor do
> they define an interface making them inscrutable. This is particularly
> confusing for DecoratedBug which provides self.tasks, self.bugtasks,
> self.bugtask, self.getBugTask(), self.default_
> synonyms and some of them (getBugTask() and bugtask) return different
> things despite their names indicating the are synonyms.
>
> We can let this slide on this branch though since this is moving code to a
> better location, so a net win. It would be nice to merge the synonyms and
> document things though if possible.
>
> Otherwise all fine.
Thanks.
Did some docstring additions and simplified the bugtasks.
Tim
1 | === modified file 'lib/lp/code/browser/decorations.py' | |||
2 | --- lib/lp/code/browser/decorations.py 2010-06-17 01:16:30 +0000 | |||
3 | +++ lib/lp/code/browser/decorations.py 2010-06-17 05:57:00 +0000 | |||
4 | @@ -21,25 +21,38 @@ | |||
5 | 21 | 21 | ||
6 | 22 | 22 | ||
7 | 23 | class DecoratedBug: | 23 | class DecoratedBug: |
9 | 24 | """Provide some additional attributes to a normal bug.""" | 24 | """Provide some cached attributes to a normal bug. |
10 | 25 | |||
11 | 26 | We provide cached properties where sensible, and override default bug | ||
12 | 27 | behaviour where the cached properties can be used to avoid extra database | ||
13 | 28 | queries. | ||
14 | 29 | """ | ||
15 | 25 | delegates(IBug, 'bug') | 30 | delegates(IBug, 'bug') |
16 | 26 | 31 | ||
17 | 27 | def __init__(self, bug, branch, tasks=None): | 32 | def __init__(self, bug, branch, tasks=None): |
18 | 28 | self.bug = bug | 33 | self.bug = bug |
19 | 29 | self.branch = branch | 34 | self.branch = branch |
27 | 30 | self.tasks = tasks | 35 | if tasks is None: |
28 | 31 | if self.tasks is None: | 36 | tasks = self.bug.bugtasks |
29 | 32 | self.tasks = self.bug.bugtasks | 37 | self.bugtasks = tasks |
23 | 33 | |||
24 | 34 | @property | ||
25 | 35 | def bugtasks(self): | ||
26 | 36 | return self.tasks | ||
30 | 37 | 38 | ||
31 | 38 | @property | 39 | @property |
32 | 39 | def default_bugtask(self): | 40 | def default_bugtask(self): |
34 | 40 | return self.tasks[0] | 41 | """Use the first bugtask. |
35 | 42 | |||
36 | 43 | Use the cached tasks as calling default_bugtask on the bug object | ||
37 | 44 | causes a DB query. | ||
38 | 45 | """ | ||
39 | 46 | return self.bugtasks[0] | ||
40 | 41 | 47 | ||
41 | 42 | def getBugTask(self, target): | 48 | def getBugTask(self, target): |
42 | 49 | """Get the bugtask for a specific target. | ||
43 | 50 | |||
44 | 51 | This method is overridden rather than letting it fall through to the | ||
45 | 52 | underlying bug as the underlying implementation gets the bugtasks from | ||
46 | 53 | self, which would in that case be the normal bug model object, which | ||
47 | 54 | would then hit the database to get the tasks. | ||
48 | 55 | """ | ||
49 | 43 | # Copied from Bug.getBugTarget to avoid importing. | 56 | # Copied from Bug.getBugTarget to avoid importing. |
50 | 44 | for bugtask in self.bugtasks: | 57 | for bugtask in self.bugtasks: |
51 | 45 | if bugtask.target == target: | 58 | if bugtask.target == target: |
52 | @@ -49,6 +62,9 @@ | |||
53 | 49 | @property | 62 | @property |
54 | 50 | def bugtask(self): | 63 | def bugtask(self): |
55 | 51 | """Return the bugtask for the branch project, or the default bugtask. | 64 | """Return the bugtask for the branch project, or the default bugtask. |
56 | 65 | |||
57 | 66 | This method defers the identitication of the appropriate task to the | ||
58 | 67 | branch target. | ||
59 | 52 | """ | 68 | """ |
60 | 53 | return self.branch.target.getBugTask(self) | 69 | return self.branch.target.getBugTask(self) |
61 | 54 | 70 | ||
62 | @@ -65,6 +81,13 @@ | |||
63 | 65 | 81 | ||
64 | 66 | @cachedproperty | 82 | @cachedproperty |
65 | 67 | def linked_bugs(self): | 83 | def linked_bugs(self): |
66 | 84 | """Override the default behaviour of the branch object. | ||
67 | 85 | |||
68 | 86 | The default behaviour is just to get the bugs. We want to display the | ||
69 | 87 | tasks however, and to avoid the extra database queries to get the | ||
70 | 88 | tasks, we get them all at once, and provide decorated bugs (that have | ||
71 | 89 | their tasks cached). | ||
72 | 90 | """ | ||
73 | 68 | bugs = defaultdict(list) | 91 | bugs = defaultdict(list) |
74 | 69 | for bug, task in self.branch.getLinkedBugsAndTasks(): | 92 | for bug, task in self.branch.getLinkedBugsAndTasks(): |
75 | 70 | bugs[bug].append(task) | 93 | bugs[bug].append(task) |
76 | @@ -74,14 +97,26 @@ | |||
77 | 74 | 97 | ||
78 | 75 | @property | 98 | @property |
79 | 76 | def displayname(self): | 99 | def displayname(self): |
80 | 100 | """Override the default model property. | ||
81 | 101 | |||
82 | 102 | If left to the underlying model, it would call the bzr_identity on the | ||
83 | 103 | underlying branch rather than the cached bzr_identity on the decorated | ||
84 | 104 | branch. And that would cause two database queries. | ||
85 | 105 | """ | ||
86 | 77 | return self.bzr_identity | 106 | return self.bzr_identity |
87 | 78 | 107 | ||
88 | 79 | @cachedproperty | 108 | @cachedproperty |
89 | 80 | def bzr_identity(self): | 109 | def bzr_identity(self): |
90 | 110 | """Cache the result of the bzr identity. | ||
91 | 111 | |||
92 | 112 | The property is defined in the bzrIdentityMixin class. This uses the | ||
93 | 113 | associatedProductSeries and associatedSuiteSourcePackages methods. | ||
94 | 114 | """ | ||
95 | 81 | return super(DecoratedBranch, self).bzr_identity | 115 | return super(DecoratedBranch, self).bzr_identity |
96 | 82 | 116 | ||
97 | 83 | @cachedproperty | 117 | @cachedproperty |
98 | 84 | def is_series_branch(self): | 118 | def is_series_branch(self): |
99 | 119 | """A simple property to see if there are any series links.""" | ||
100 | 85 | # True if linked to a product series or suite source package. | 120 | # True if linked to a product series or suite source package. |
101 | 86 | return ( | 121 | return ( |
102 | 87 | len(self.associated_product_series) > 0 or | 122 | len(self.associated_product_series) > 0 or |
103 | @@ -97,21 +132,31 @@ | |||
104 | 97 | 132 | ||
105 | 98 | @cachedproperty | 133 | @cachedproperty |
106 | 99 | def associated_product_series(self): | 134 | def associated_product_series(self): |
107 | 135 | """Cache the realized product series links.""" | ||
108 | 100 | return list(self.branch.associatedProductSeries()) | 136 | return list(self.branch.associatedProductSeries()) |
109 | 101 | 137 | ||
110 | 102 | @cachedproperty | 138 | @cachedproperty |
111 | 103 | def suite_source_packages(self): | 139 | def suite_source_packages(self): |
112 | 140 | """Cache the realized suite source package links.""" | ||
113 | 104 | return list(self.branch.associatedSuiteSourcePackages()) | 141 | return list(self.branch.associatedSuiteSourcePackages()) |
114 | 105 | 142 | ||
115 | 106 | @cachedproperty | 143 | @cachedproperty |
116 | 107 | def upgrade_pending(self): | 144 | def upgrade_pending(self): |
117 | 145 | """Cache the result as the property hits the database.""" | ||
118 | 108 | return self.branch.upgrade_pending | 146 | return self.branch.upgrade_pending |
119 | 109 | 147 | ||
120 | 110 | @cachedproperty | 148 | @cachedproperty |
121 | 111 | def subscriptions(self): | 149 | def subscriptions(self): |
122 | 150 | """Cache the realized branch subscription objects.""" | ||
123 | 112 | return list(self.branch.subscriptions) | 151 | return list(self.branch.subscriptions) |
124 | 113 | 152 | ||
125 | 114 | def hasSubscription(self, user): | 153 | def hasSubscription(self, user): |
126 | 154 | """Override the default branch implementation. | ||
127 | 155 | |||
128 | 156 | The default implementation hits the database. Since we have a full | ||
129 | 157 | list of subscribers anyway, a simple check over the list is | ||
130 | 158 | sufficient. | ||
131 | 159 | """ | ||
132 | 115 | for sub in self.subscriptions: | 160 | for sub in self.subscriptions: |
133 | 116 | if sub.person == user: | 161 | if sub.person == user: |
134 | 117 | return True | 162 | return True |
135 | @@ -119,6 +164,11 @@ | |||
136 | 119 | 164 | ||
137 | 120 | @cachedproperty | 165 | @cachedproperty |
138 | 121 | def latest_revisions(self): | 166 | def latest_revisions(self): |
139 | 167 | """Cache the query result. | ||
140 | 168 | |||
141 | 169 | When a tal:repeat is used, the method is called twice. Firstly to | ||
142 | 170 | check that there is something to iterate over, and secondly for the | ||
143 | 171 | actual iteration. Without the cached property, the database is hit | ||
144 | 172 | twice. | ||
145 | 173 | """ | ||
146 | 122 | return list(self.branch.latest_revisions()) | 174 | return list(self.branch.latest_revisions()) |
147 | 123 | |||
148 | 124 |
Preview Diff
1 | === modified file 'lib/lp/code/browser/branch.py' | |||
2 | --- lib/lp/code/browser/branch.py 2010-06-14 02:37:06 +0000 | |||
3 | +++ lib/lp/code/browser/branch.py 2010-06-18 02:09:26 +0000 | |||
4 | @@ -25,14 +25,11 @@ | |||
5 | 25 | 'BranchURL', | 25 | 'BranchURL', |
6 | 26 | 'BranchView', | 26 | 'BranchView', |
7 | 27 | 'BranchSubscriptionsView', | 27 | 'BranchSubscriptionsView', |
8 | 28 | 'DecoratedBranch', | ||
9 | 29 | 'DecoratedBug', | ||
10 | 30 | 'RegisterBranchMergeProposalView', | 28 | 'RegisterBranchMergeProposalView', |
11 | 31 | 'TryImportAgainView', | 29 | 'TryImportAgainView', |
12 | 32 | ] | 30 | ] |
13 | 33 | 31 | ||
14 | 34 | import cgi | 32 | import cgi |
15 | 35 | from collections import defaultdict | ||
16 | 36 | from datetime import datetime, timedelta | 33 | from datetime import datetime, timedelta |
17 | 37 | 34 | ||
18 | 38 | import pytz | 35 | import pytz |
19 | @@ -47,7 +44,6 @@ | |||
20 | 47 | from zope.publisher.interfaces import NotFound | 44 | from zope.publisher.interfaces import NotFound |
21 | 48 | from zope.schema import Bool, Choice, Text | 45 | from zope.schema import Bool, Choice, Text |
22 | 49 | from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary | 46 | from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary |
23 | 50 | from lazr.delegates import delegates | ||
24 | 51 | from lazr.enum import EnumeratedType, Item | 47 | from lazr.enum import EnumeratedType, Item |
25 | 52 | from lazr.lifecycle.event import ObjectModifiedEvent | 48 | from lazr.lifecycle.event import ObjectModifiedEvent |
26 | 53 | from lazr.lifecycle.snapshot import Snapshot | 49 | from lazr.lifecycle.snapshot import Snapshot |
27 | @@ -79,11 +75,11 @@ | |||
28 | 79 | from canonical.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription | 75 | from canonical.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription |
29 | 80 | from canonical.widgets.lazrjs import vocabulary_to_choice_edit_items | 76 | from canonical.widgets.lazrjs import vocabulary_to_choice_edit_items |
30 | 81 | 77 | ||
31 | 82 | from lp.bugs.interfaces.bug import IBug | ||
32 | 83 | from lp.bugs.interfaces.bugtask import UNRESOLVED_BUGTASK_STATUSES | 78 | from lp.bugs.interfaces.bugtask import UNRESOLVED_BUGTASK_STATUSES |
33 | 84 | from lp.code.browser.branchref import BranchRef | 79 | from lp.code.browser.branchref import BranchRef |
34 | 85 | from lp.code.browser.branchmergeproposal import ( | 80 | from lp.code.browser.branchmergeproposal import ( |
35 | 86 | latest_proposals_for_each_branch) | 81 | latest_proposals_for_each_branch) |
36 | 82 | from lp.code.browser.decorations import DecoratedBranch | ||
37 | 87 | from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin | 83 | from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin |
38 | 88 | from lp.code.enums import ( | 84 | from lp.code.enums import ( |
39 | 89 | BranchLifecycleStatus, BranchType, | 85 | BranchLifecycleStatus, BranchType, |
40 | @@ -93,7 +89,7 @@ | |||
41 | 93 | CodeImportAlreadyRequested, CodeImportAlreadyRunning, | 89 | CodeImportAlreadyRequested, CodeImportAlreadyRunning, |
42 | 94 | CodeImportNotInReviewedState, InvalidBranchMergeProposal) | 90 | CodeImportNotInReviewedState, InvalidBranchMergeProposal) |
43 | 95 | from lp.code.interfaces.branch import ( | 91 | from lp.code.interfaces.branch import ( |
45 | 96 | BranchCreationForbidden, BranchExists, BzrIdentityMixin, IBranch, | 92 | BranchCreationForbidden, BranchExists, IBranch, |
46 | 97 | user_has_special_branch_access) | 93 | user_has_special_branch_access) |
47 | 98 | from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal | 94 | from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal |
48 | 99 | from lp.code.interfaces.branchtarget import IBranchTarget | 95 | from lp.code.interfaces.branchtarget import IBranchTarget |
49 | @@ -328,108 +324,6 @@ | |||
50 | 328 | return Link('+new-recipe', text, enabled=enabled, icon='add') | 324 | return Link('+new-recipe', text, enabled=enabled, icon='add') |
51 | 329 | 325 | ||
52 | 330 | 326 | ||
53 | 331 | class DecoratedBug: | ||
54 | 332 | """Provide some additional attributes to a normal bug.""" | ||
55 | 333 | delegates(IBug, 'bug') | ||
56 | 334 | |||
57 | 335 | def __init__(self, bug, branch, tasks=None): | ||
58 | 336 | self.bug = bug | ||
59 | 337 | self.branch = branch | ||
60 | 338 | self.tasks = tasks | ||
61 | 339 | if self.tasks is None: | ||
62 | 340 | self.tasks = self.bug.bugtasks | ||
63 | 341 | |||
64 | 342 | @property | ||
65 | 343 | def bugtasks(self): | ||
66 | 344 | return self.tasks | ||
67 | 345 | |||
68 | 346 | @property | ||
69 | 347 | def default_bugtask(self): | ||
70 | 348 | return self.tasks[0] | ||
71 | 349 | |||
72 | 350 | def getBugTask(self, target): | ||
73 | 351 | # Copied from Bug.getBugTarget to avoid importing. | ||
74 | 352 | for bugtask in self.bugtasks: | ||
75 | 353 | if bugtask.target == target: | ||
76 | 354 | return bugtask | ||
77 | 355 | return None | ||
78 | 356 | |||
79 | 357 | @property | ||
80 | 358 | def bugtask(self): | ||
81 | 359 | """Return the bugtask for the branch project, or the default bugtask. | ||
82 | 360 | """ | ||
83 | 361 | return self.branch.target.getBugTask(self) | ||
84 | 362 | |||
85 | 363 | |||
86 | 364 | class DecoratedBranch(BzrIdentityMixin): | ||
87 | 365 | """Wrap a number of the branch accessors to cache results. | ||
88 | 366 | |||
89 | 367 | This avoids repeated db queries. | ||
90 | 368 | """ | ||
91 | 369 | delegates(IBranch, 'branch') | ||
92 | 370 | |||
93 | 371 | def __init__(self, branch): | ||
94 | 372 | self.branch = branch | ||
95 | 373 | |||
96 | 374 | @cachedproperty | ||
97 | 375 | def linked_bugs(self): | ||
98 | 376 | bugs = defaultdict(list) | ||
99 | 377 | for bug, task in self.branch.getLinkedBugsAndTasks(): | ||
100 | 378 | bugs[bug].append(task) | ||
101 | 379 | return [DecoratedBug(bug, self.branch, tasks) | ||
102 | 380 | for bug, tasks in bugs.iteritems() | ||
103 | 381 | if check_permission('launchpad.View', bug)] | ||
104 | 382 | |||
105 | 383 | @property | ||
106 | 384 | def displayname(self): | ||
107 | 385 | return self.bzr_identity | ||
108 | 386 | |||
109 | 387 | @cachedproperty | ||
110 | 388 | def bzr_identity(self): | ||
111 | 389 | return super(DecoratedBranch, self).bzr_identity | ||
112 | 390 | |||
113 | 391 | @cachedproperty | ||
114 | 392 | def is_series_branch(self): | ||
115 | 393 | # True if linked to a product series or suite source package. | ||
116 | 394 | return ( | ||
117 | 395 | len(self.associated_product_series) > 0 or | ||
118 | 396 | len(self.suite_source_packages) > 0) | ||
119 | 397 | |||
120 | 398 | def associatedProductSeries(self): | ||
121 | 399 | """Override the IBranch.associatedProductSeries.""" | ||
122 | 400 | return self.associated_product_series | ||
123 | 401 | |||
124 | 402 | def associatedSuiteSourcePackages(self): | ||
125 | 403 | """Override the IBranch.associatedSuiteSourcePackages.""" | ||
126 | 404 | return self.suite_source_packages | ||
127 | 405 | |||
128 | 406 | @cachedproperty | ||
129 | 407 | def associated_product_series(self): | ||
130 | 408 | return list(self.branch.associatedProductSeries()) | ||
131 | 409 | |||
132 | 410 | @cachedproperty | ||
133 | 411 | def suite_source_packages(self): | ||
134 | 412 | return list(self.branch.associatedSuiteSourcePackages()) | ||
135 | 413 | |||
136 | 414 | @cachedproperty | ||
137 | 415 | def upgrade_pending(self): | ||
138 | 416 | return self.branch.upgrade_pending | ||
139 | 417 | |||
140 | 418 | @cachedproperty | ||
141 | 419 | def subscriptions(self): | ||
142 | 420 | return list(self.branch.subscriptions) | ||
143 | 421 | |||
144 | 422 | def hasSubscription(self, user): | ||
145 | 423 | for sub in self.subscriptions: | ||
146 | 424 | if sub.person == user: | ||
147 | 425 | return True | ||
148 | 426 | return False | ||
149 | 427 | |||
150 | 428 | @cachedproperty | ||
151 | 429 | def latest_revisions(self): | ||
152 | 430 | return list(self.branch.latest_revisions()) | ||
153 | 431 | |||
154 | 432 | |||
155 | 433 | class BranchView(LaunchpadView, FeedsMixin): | 327 | class BranchView(LaunchpadView, FeedsMixin): |
156 | 434 | 328 | ||
157 | 435 | __used_for__ = IBranch | 329 | __used_for__ = IBranch |
158 | 436 | 330 | ||
159 | === modified file 'lib/lp/code/browser/branchmergeproposal.py' | |||
160 | --- lib/lp/code/browser/branchmergeproposal.py 2010-05-16 23:56:51 +0000 | |||
161 | +++ lib/lp/code/browser/branchmergeproposal.py 2010-06-18 02:09:26 +0000 | |||
162 | @@ -70,6 +70,7 @@ | |||
163 | 70 | from lp.app.browser.stringformatter import FormattersAPI | 70 | from lp.app.browser.stringformatter import FormattersAPI |
164 | 71 | from lp.code.adapters.branch import BranchMergeProposalDelta | 71 | from lp.code.adapters.branch import BranchMergeProposalDelta |
165 | 72 | from lp.code.browser.codereviewcomment import CodeReviewDisplayComment | 72 | from lp.code.browser.codereviewcomment import CodeReviewDisplayComment |
166 | 73 | from lp.code.browser.decorations import DecoratedBranch, DecoratedBug | ||
167 | 73 | from lp.code.enums import ( | 74 | from lp.code.enums import ( |
168 | 74 | BranchMergeProposalStatus, BranchType, CodeReviewNotificationLevel, | 75 | BranchMergeProposalStatus, BranchType, CodeReviewNotificationLevel, |
169 | 75 | CodeReviewVote) | 76 | CodeReviewVote) |
170 | @@ -212,7 +213,6 @@ | |||
171 | 212 | @enabled_with_permission('launchpad.Edit') | 213 | @enabled_with_permission('launchpad.Edit') |
172 | 213 | def edit_status(self): | 214 | def edit_status(self): |
173 | 214 | text = 'Change status' | 215 | text = 'Change status' |
174 | 215 | status = self.context.queue_status | ||
175 | 216 | return Link('+edit-status', text, icon='edit') | 216 | return Link('+edit-status', text, icon='edit') |
176 | 217 | 217 | ||
177 | 218 | @enabled_with_permission('launchpad.Edit') | 218 | @enabled_with_permission('launchpad.Edit') |
178 | @@ -459,7 +459,6 @@ | |||
179 | 459 | 459 | ||
180 | 460 | def _createStatusVocabulary(self): | 460 | def _createStatusVocabulary(self): |
181 | 461 | # Create the vocabulary that is used for the status widget. | 461 | # Create the vocabulary that is used for the status widget. |
182 | 462 | curr_status = self.context.queue_status | ||
183 | 463 | possible_next_states = ( | 462 | possible_next_states = ( |
184 | 464 | BranchMergeProposalStatus.WORK_IN_PROGRESS, | 463 | BranchMergeProposalStatus.WORK_IN_PROGRESS, |
185 | 465 | BranchMergeProposalStatus.NEEDS_REVIEW, | 464 | BranchMergeProposalStatus.NEEDS_REVIEW, |
186 | @@ -590,7 +589,7 @@ | |||
187 | 590 | start_date = self.context.date_review_requested | 589 | start_date = self.context.date_review_requested |
188 | 591 | if start_date is None: | 590 | if start_date is None: |
189 | 592 | start_date = self.context.date_created | 591 | start_date = self.context.date_created |
191 | 593 | source = self.context.source_branch | 592 | source = DecoratedBranch(self.context.source_branch) |
192 | 594 | resultset = source.getMainlineBranchRevisions( | 593 | resultset = source.getMainlineBranchRevisions( |
193 | 595 | start_date, self.revision_end_date, oldest_first=True) | 594 | start_date, self.revision_end_date, oldest_first=True) |
194 | 596 | # Now group by date created. | 595 | # Now group by date created. |
195 | @@ -663,8 +662,6 @@ | |||
196 | 663 | @cachedproperty | 662 | @cachedproperty |
197 | 664 | def linked_bugs(self): | 663 | def linked_bugs(self): |
198 | 665 | """Return DecoratedBugs linked to the source branch.""" | 664 | """Return DecoratedBugs linked to the source branch.""" |
199 | 666 | # Avoid import loop | ||
200 | 667 | from lp.code.browser.branch import DecoratedBug | ||
201 | 668 | return [DecoratedBug(bug, self.context.source_branch) | 665 | return [DecoratedBug(bug, self.context.source_branch) |
202 | 669 | for bug in self.context.related_bugs] | 666 | for bug in self.context.related_bugs] |
203 | 670 | 667 | ||
204 | @@ -839,7 +836,7 @@ | |||
205 | 839 | reviewer = copy_field(ICodeReviewVoteReference['reviewer']) | 836 | reviewer = copy_field(ICodeReviewVoteReference['reviewer']) |
206 | 840 | 837 | ||
207 | 841 | review_type = copy_field( | 838 | review_type = copy_field( |
209 | 842 | ICodeReviewVoteReference['review_type'], | 839 | ICodeReviewVoteReference['review_type'], |
210 | 843 | description=u'Lowercase keywords describing the type of review you ' | 840 | description=u'Lowercase keywords describing the type of review you ' |
211 | 844 | 'would like to be performed.') | 841 | 'would like to be performed.') |
212 | 845 | 842 | ||
213 | @@ -874,8 +871,7 @@ | |||
214 | 874 | 871 | ||
215 | 875 | def requestReview(self, candidate, review_type): | 872 | def requestReview(self, candidate, review_type): |
216 | 876 | """Request a `review_type` review from `candidate` and email them.""" | 873 | """Request a `review_type` review from `candidate` and email them.""" |
219 | 877 | vote_reference = self.context.nominateReviewer( | 874 | self.context.nominateReviewer(candidate, self.user, review_type) |
218 | 878 | candidate, self.user, review_type) | ||
220 | 879 | 875 | ||
221 | 880 | @action('Request Review', name='review') | 876 | @action('Request Review', name='review') |
222 | 881 | @notify | 877 | @notify |
223 | @@ -1334,7 +1330,7 @@ | |||
224 | 1334 | vote = copy_field(ICodeReviewComment['vote'], required=True) | 1330 | vote = copy_field(ICodeReviewComment['vote'], required=True) |
225 | 1335 | 1331 | ||
226 | 1336 | review_type = copy_field( | 1332 | review_type = copy_field( |
228 | 1337 | ICodeReviewVoteReference['review_type'], | 1333 | ICodeReviewVoteReference['review_type'], |
229 | 1338 | description=u'Lowercase keywords describing the type of review you ' | 1334 | description=u'Lowercase keywords describing the type of review you ' |
230 | 1339 | 'are performing.') | 1335 | 'are performing.') |
231 | 1340 | 1336 | ||
232 | @@ -1428,7 +1424,7 @@ | |||
233 | 1428 | # team. | 1424 | # team. |
234 | 1429 | vote_ref.claimReview(self.user) | 1425 | vote_ref.claimReview(self.user) |
235 | 1430 | 1426 | ||
237 | 1431 | comment = self.context.createComment( | 1427 | self.context.createComment( |
238 | 1432 | self.user, subject=None, content=data['comment'], | 1428 | self.user, subject=None, content=data['comment'], |
239 | 1433 | vote=data['vote'], review_type=review_type) | 1429 | vote=data['vote'], review_type=review_type) |
240 | 1434 | 1430 | ||
241 | 1435 | 1431 | ||
242 | === added file 'lib/lp/code/browser/decorations.py' | |||
243 | --- lib/lp/code/browser/decorations.py 1970-01-01 00:00:00 +0000 | |||
244 | +++ lib/lp/code/browser/decorations.py 2010-06-18 02:09:26 +0000 | |||
245 | @@ -0,0 +1,183 @@ | |||
246 | 1 | # Copyright 2010 Canonical Ltd. This software is licensed under the | ||
247 | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). | ||
248 | 3 | |||
249 | 4 | """Decorated model objects used in the browser code.""" | ||
250 | 5 | |||
251 | 6 | __metaclass__ = type | ||
252 | 7 | __all__ = [ | ||
253 | 8 | 'DecoratedBranch', | ||
254 | 9 | 'DecoratedBug', | ||
255 | 10 | ] | ||
256 | 11 | |||
257 | 12 | |||
258 | 13 | from collections import defaultdict | ||
259 | 14 | |||
260 | 15 | from canonical.cachedproperty import cachedproperty | ||
261 | 16 | from canonical.launchpad.webapp.authorization import check_permission | ||
262 | 17 | from lazr.delegates import delegates | ||
263 | 18 | |||
264 | 19 | from lp.bugs.interfaces.bug import IBug | ||
265 | 20 | from lp.code.interfaces.branch import BzrIdentityMixin, IBranch | ||
266 | 21 | |||
267 | 22 | |||
268 | 23 | class DecoratedBug: | ||
269 | 24 | """Provide some cached attributes to a normal bug. | ||
270 | 25 | |||
271 | 26 | We provide cached properties where sensible, and override default bug | ||
272 | 27 | behaviour where the cached properties can be used to avoid extra database | ||
273 | 28 | queries. | ||
274 | 29 | """ | ||
275 | 30 | delegates(IBug, 'bug') | ||
276 | 31 | |||
277 | 32 | def __init__(self, bug, branch, tasks=None): | ||
278 | 33 | self.bug = bug | ||
279 | 34 | self.branch = branch | ||
280 | 35 | if tasks is None: | ||
281 | 36 | tasks = self.bug.bugtasks | ||
282 | 37 | self.tasks = tasks | ||
283 | 38 | |||
284 | 39 | @property | ||
285 | 40 | def bugtasks(self): | ||
286 | 41 | """This needs to be a property rather than an attribute. | ||
287 | 42 | |||
288 | 43 | If we try to assign to self.bugtasks, the lazr.delegates things we are | ||
289 | 44 | trying to assign to the property of the bug. | ||
290 | 45 | """ | ||
291 | 46 | return self.tasks | ||
292 | 47 | |||
293 | 48 | @property | ||
294 | 49 | def default_bugtask(self): | ||
295 | 50 | """Use the first bugtask. | ||
296 | 51 | |||
297 | 52 | Use the cached tasks as calling default_bugtask on the bug object | ||
298 | 53 | causes a DB query. | ||
299 | 54 | """ | ||
300 | 55 | return self.bugtasks[0] | ||
301 | 56 | |||
302 | 57 | def getBugTask(self, target): | ||
303 | 58 | """Get the bugtask for a specific target. | ||
304 | 59 | |||
305 | 60 | This method is overridden rather than letting it fall through to the | ||
306 | 61 | underlying bug as the underlying implementation gets the bugtasks from | ||
307 | 62 | self, which would in that case be the normal bug model object, which | ||
308 | 63 | would then hit the database to get the tasks. | ||
309 | 64 | """ | ||
310 | 65 | # Copied from Bug.getBugTarget to avoid importing. | ||
311 | 66 | for bugtask in self.bugtasks: | ||
312 | 67 | if bugtask.target == target: | ||
313 | 68 | return bugtask | ||
314 | 69 | return None | ||
315 | 70 | |||
316 | 71 | @property | ||
317 | 72 | def bugtask(self): | ||
318 | 73 | """Return the bugtask for the branch project, or the default bugtask. | ||
319 | 74 | |||
320 | 75 | This method defers the identitication of the appropriate task to the | ||
321 | 76 | branch target. | ||
322 | 77 | """ | ||
323 | 78 | return self.branch.target.getBugTask(self) | ||
324 | 79 | |||
325 | 80 | |||
326 | 81 | class DecoratedBranch(BzrIdentityMixin): | ||
327 | 82 | """Wrap a number of the branch accessors to cache results. | ||
328 | 83 | |||
329 | 84 | This avoids repeated db queries. | ||
330 | 85 | """ | ||
331 | 86 | delegates(IBranch, 'branch') | ||
332 | 87 | |||
333 | 88 | def __init__(self, branch): | ||
334 | 89 | self.branch = branch | ||
335 | 90 | |||
336 | 91 | @cachedproperty | ||
337 | 92 | def linked_bugs(self): | ||
338 | 93 | """Override the default behaviour of the branch object. | ||
339 | 94 | |||
340 | 95 | The default behaviour is just to get the bugs. We want to display the | ||
341 | 96 | tasks however, and to avoid the extra database queries to get the | ||
342 | 97 | tasks, we get them all at once, and provide decorated bugs (that have | ||
343 | 98 | their tasks cached). | ||
344 | 99 | """ | ||
345 | 100 | bugs = defaultdict(list) | ||
346 | 101 | for bug, task in self.branch.getLinkedBugsAndTasks(): | ||
347 | 102 | bugs[bug].append(task) | ||
348 | 103 | return [DecoratedBug(bug, self.branch, tasks) | ||
349 | 104 | for bug, tasks in bugs.iteritems() | ||
350 | 105 | if check_permission('launchpad.View', bug)] | ||
351 | 106 | |||
352 | 107 | @property | ||
353 | 108 | def displayname(self): | ||
354 | 109 | """Override the default model property. | ||
355 | 110 | |||
356 | 111 | If left to the underlying model, it would call the bzr_identity on the | ||
357 | 112 | underlying branch rather than the cached bzr_identity on the decorated | ||
358 | 113 | branch. And that would cause two database queries. | ||
359 | 114 | """ | ||
360 | 115 | return self.bzr_identity | ||
361 | 116 | |||
362 | 117 | @cachedproperty | ||
363 | 118 | def bzr_identity(self): | ||
364 | 119 | """Cache the result of the bzr identity. | ||
365 | 120 | |||
366 | 121 | The property is defined in the bzrIdentityMixin class. This uses the | ||
367 | 122 | associatedProductSeries and associatedSuiteSourcePackages methods. | ||
368 | 123 | """ | ||
369 | 124 | return super(DecoratedBranch, self).bzr_identity | ||
370 | 125 | |||
371 | 126 | @cachedproperty | ||
372 | 127 | def is_series_branch(self): | ||
373 | 128 | """A simple property to see if there are any series links.""" | ||
374 | 129 | # True if linked to a product series or suite source package. | ||
375 | 130 | return ( | ||
376 | 131 | len(self.associated_product_series) > 0 or | ||
377 | 132 | len(self.suite_source_packages) > 0) | ||
378 | 133 | |||
379 | 134 | def associatedProductSeries(self): | ||
380 | 135 | """Override the IBranch.associatedProductSeries.""" | ||
381 | 136 | return self.associated_product_series | ||
382 | 137 | |||
383 | 138 | def associatedSuiteSourcePackages(self): | ||
384 | 139 | """Override the IBranch.associatedSuiteSourcePackages.""" | ||
385 | 140 | return self.suite_source_packages | ||
386 | 141 | |||
387 | 142 | @cachedproperty | ||
388 | 143 | def associated_product_series(self): | ||
389 | 144 | """Cache the realized product series links.""" | ||
390 | 145 | return list(self.branch.associatedProductSeries()) | ||
391 | 146 | |||
392 | 147 | @cachedproperty | ||
393 | 148 | def suite_source_packages(self): | ||
394 | 149 | """Cache the realized suite source package links.""" | ||
395 | 150 | return list(self.branch.associatedSuiteSourcePackages()) | ||
396 | 151 | |||
397 | 152 | @cachedproperty | ||
398 | 153 | def upgrade_pending(self): | ||
399 | 154 | """Cache the result as the property hits the database.""" | ||
400 | 155 | return self.branch.upgrade_pending | ||
401 | 156 | |||
402 | 157 | @cachedproperty | ||
403 | 158 | def subscriptions(self): | ||
404 | 159 | """Cache the realized branch subscription objects.""" | ||
405 | 160 | return list(self.branch.subscriptions) | ||
406 | 161 | |||
407 | 162 | def hasSubscription(self, user): | ||
408 | 163 | """Override the default branch implementation. | ||
409 | 164 | |||
410 | 165 | The default implementation hits the database. Since we have a full | ||
411 | 166 | list of subscribers anyway, a simple check over the list is | ||
412 | 167 | sufficient. | ||
413 | 168 | """ | ||
414 | 169 | for sub in self.subscriptions: | ||
415 | 170 | if sub.person == user: | ||
416 | 171 | return True | ||
417 | 172 | return False | ||
418 | 173 | |||
419 | 174 | @cachedproperty | ||
420 | 175 | def latest_revisions(self): | ||
421 | 176 | """Cache the query result. | ||
422 | 177 | |||
423 | 178 | When a tal:repeat is used, the method is called twice. Firstly to | ||
424 | 179 | check that there is something to iterate over, and secondly for the | ||
425 | 180 | actual iteration. Without the cached property, the database is hit | ||
426 | 181 | twice. | ||
427 | 182 | """ | ||
428 | 183 | return list(self.branch.latest_revisions()) |
Oh, there was also some drive by lint fixes in the browser branchmergeproposal module.