Merge lp:~gmb/launchpad/cw-refactor-add-bwu-bug-567793 into lp:launchpad
- cw-refactor-add-bwu-bug-567793
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Gavin Panella |
Approved revision: | no longer in the source branch. |
Merged at revision: | not available |
Proposed branch: | lp:~gmb/launchpad/cw-refactor-add-bwu-bug-567793 |
Merge into: | lp:launchpad |
Diff against target: |
1044 lines (+407/-294) 8 files modified
lib/lp/bugs/doc/externalbugtracker-comment-imports.txt (+24/-12) lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt (+26/-19) lib/lp/bugs/doc/externalbugtracker-linking-back.txt (+22/-12) lib/lp/bugs/scripts/checkwatches/base.py (+7/-1) lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py (+284/-0) lib/lp/bugs/scripts/checkwatches/core.py (+14/-243) lib/lp/bugs/scripts/checkwatches/tests/test_base.py (+21/-7) utilities/less-oops.sh (+9/-0) |
To merge this branch: | bzr merge lp:~gmb/launchpad/cw-refactor-add-bwu-bug-567793 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+23946@code.launchpad.net |
Commit message
The BugWatch-specific functionality of CheckwatchesMaster has been refactored into a BugWatchUpdater class.
Description of the change
This branch is the first in a multi-branch series for refactoring the existing CheckwatchesMaster class into a series of smaller, better defined classes, and for untangling the code that lies within them.
As a first step, this branch moves the BugWatch-specific functionality out of CheckwatchesMaster and into BugWatchUpdater. The core of the functionality now resides in BugWatchUpdater
I've possibly done something perverse by having BugWatchUpdater extend WorkingBase, then take the transaction manager of CheckwatchesMaster upon instantiation. I don't know if it's the Wrong Thing to do (though the tests pass). However, I'm willing to either fix this here or, possibly more sensibly, refactor this relationship further in another branch (note that it's likely that BugWatchUpdater will in future be called by a RemoteBugUpdater, which in turn will be called by a BugTrackerUpdater and so on, so refactoring this sensibly may have to wait).
There's no lint and, as far as I can tell, everything passes.
Graham Binns (gmb) wrote : | # |
On Fri, Apr 23, 2010 at 01:56:40PM -0000, Gavin Panella wrote:
> Review: Approve
> You're my hero for taking this on.
Ta :)
> This is all good. I've made a few comments, and a bigger suggestion
> (including patch) to make the instantiation of BugWatchUpdater less of
> a pain (and to preserve the encapsulation that WorkingBase gives).
>
Cool. Replies in-line, etc...
>
> > === modified file 'lib/lp/
> > --- lib/lp/
> > +++ lib/lp/
> > @@ -85,10 +85,13 @@
> > comment-importing ExternalBugTracker instance will result in the three
> > comments in the comment_dict being imported into Launchpad.
> >
> > - >>> from lp.bugs.
> > - >>> bugwatch_updater = CheckwatchesMas
> > - >>> transaction.
> > - >>> bugwatch_
> > + >>> from canonical.
> > + >>> from lp.bugs.
> > + ... BugWatchUpdater)
> > + >>> bugwatch_updater = BugWatchUpdater(
> > + ... '<email address hidden>', LaunchpadZopele
>
> I think it's confusing to use LaunchpadZopele
> places and transaction in others. ZopelessTransac
> just calls through to transaction, so it's easier to just use
> transaction everywhere (or transaction.
> context manager needs the real manager). Also, I irrationally loathe
> the "txn" spelling, but that should have no bearing on this review :)
>
I'll switch everything to use transaction, then.
> '<email address hidden>' could be obtained from
> lp.bugs.
>
I'll leave it as-is for now, then.
> > + ... log, bug_watch, external_
> > + >>> bugwatch_
> > INFO:...:Imported 3 comments for remote bug 123456 on ...
> >
> > These three comments will be linked to the bug watch from which they
> > @@ -118,7 +121,7 @@
> >
> > >>> transaction.
> >
> > - >>> bugwatch_
> > + >>> bugwatch_
> > INFO:...:Imported 1 comments for remote bug 123456 on ...
> >
> > Once again, the newly-imported comment will be linked to the bug watch
> > @@ -169,7 +172,7 @@
> >
> > >>> transaction.
> >
> > - >>> bugwatch_
> > + >>> bugwatch_
> > INFO:...:Imported 1 comments for remote bug 123456 on ...
> >
> > >>> bug.messages[
> > @@ -187,7 +190,7 @@
> >
> > >>> transaction.
> >
> > - >>> bugwatch_
> > + >>> bugwatch_
> > INFO:...:Imported 1 comments for remote bug 123456 on ...
> >
> > >>> bug.messages[
Preview Diff
1 | === modified file 'lib/lp/bugs/doc/externalbugtracker-comment-imports.txt' |
2 | --- lib/lp/bugs/doc/externalbugtracker-comment-imports.txt 2010-04-21 10:30:24 +0000 |
3 | +++ lib/lp/bugs/doc/externalbugtracker-comment-imports.txt 2010-04-27 07:12:35 +0000 |
4 | @@ -85,10 +85,18 @@ |
5 | comment-importing ExternalBugTracker instance will result in the three |
6 | comments in the comment_dict being imported into Launchpad. |
7 | |
8 | - >>> from lp.bugs.scripts.checkwatches import CheckwatchesMaster |
9 | - >>> bugwatch_updater = CheckwatchesMaster(LaunchpadZopelessLayer.txn) |
10 | - >>> transaction.commit() |
11 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
12 | + >>> from canonical.launchpad.scripts.logger import log |
13 | + >>> from lp.bugs.scripts.checkwatches.base import ( |
14 | + ... WorkingBase) |
15 | + >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import ( |
16 | + ... BugWatchUpdater) |
17 | + |
18 | + >>> working_base = WorkingBase() |
19 | + >>> working_base.init( |
20 | + ... 'bugwatch@bugs.launchpad.net', transaction, log) |
21 | + >>> bugwatch_updater = BugWatchUpdater( |
22 | + ... working_base, bug_watch, external_bugtracker) |
23 | + >>> bugwatch_updater.importBugComments() |
24 | INFO:...:Imported 3 comments for remote bug 123456 on ... |
25 | |
26 | These three comments will be linked to the bug watch from which they |
27 | @@ -118,7 +126,7 @@ |
28 | |
29 | >>> transaction.commit() |
30 | |
31 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
32 | + >>> bugwatch_updater.importBugComments() |
33 | INFO:...:Imported 1 comments for remote bug 123456 on ... |
34 | |
35 | Once again, the newly-imported comment will be linked to the bug watch |
36 | @@ -169,7 +177,7 @@ |
37 | |
38 | >>> transaction.commit() |
39 | |
40 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
41 | + >>> bugwatch_updater.importBugComments() |
42 | INFO:...:Imported 1 comments for remote bug 123456 on ... |
43 | |
44 | >>> bug.messages[-1].owner.name |
45 | @@ -187,7 +195,7 @@ |
46 | |
47 | >>> transaction.commit() |
48 | |
49 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
50 | + >>> bugwatch_updater.importBugComments() |
51 | INFO:...:Imported 1 comments for remote bug 123456 on ... |
52 | |
53 | >>> bug.messages[-1].owner.name |
54 | @@ -212,7 +220,7 @@ |
55 | |
56 | >>> transaction.commit() |
57 | |
58 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
59 | + >>> bugwatch_updater.importBugComments() |
60 | WARNING:...:Unable to import remote comment author. No email address |
61 | or display name found. (OOPS-...) |
62 | INFO:...:Imported 0 comments for remote bug 123456 on ... |
63 | @@ -388,7 +396,9 @@ |
64 | >>> LaunchpadZopelessLayer.switchDbUser(config.checkwatches.dbuser) |
65 | >>> external_bugtracker.remote_comments = { |
66 | ... '5':"A comment containing a CVE entry: CVE-1991-9911."} |
67 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
68 | + >>> bugwatch_updater = BugWatchUpdater( |
69 | + ... working_base, bug_watch, external_bugtracker) |
70 | + >>> bugwatch_updater.importBugComments() |
71 | INFO:...:Imported 1 comments for remote bug 123456... |
72 | |
73 | >>> for cve in bug_watch.bug.cves: |
74 | @@ -410,7 +420,7 @@ |
75 | |
76 | >>> transaction.commit() |
77 | |
78 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
79 | + >>> bugwatch_updater.importBugComments() |
80 | INFO:...:Imported 1 comments for remote bug 123456... |
81 | |
82 | >>> for cve in sorted([cve.displayname for cve in bug_watch.bug.cves]): |
83 | @@ -460,7 +470,9 @@ |
84 | |
85 | >>> transaction.commit() |
86 | |
87 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
88 | + >>> bugwatch_updater = BugWatchUpdater( |
89 | + ... working_base, bug_watch, external_bugtracker) |
90 | + >>> bugwatch_updater.importBugComments() |
91 | INFO:...:Imported 2 comments for remote bug 42 ... |
92 | |
93 | >>> notifications = get_new_notifications(bug=bug_watch.bug) |
94 | @@ -516,7 +528,7 @@ |
95 | |
96 | >>> transaction.commit() |
97 | |
98 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
99 | + >>> bugwatch_updater.importBugComments() |
100 | INFO:...:Imported 2 comments for remote bug 42 ... |
101 | |
102 | >>> notifications = get_new_notifications(bug_watch.bug) |
103 | |
104 | === modified file 'lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt' |
105 | --- lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt 2010-04-21 10:30:24 +0000 |
106 | +++ lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt 2010-04-27 07:12:35 +0000 |
107 | @@ -95,12 +95,19 @@ |
108 | each Launchpad comment that needs to be pushed to the remote bug |
109 | tracker. |
110 | |
111 | - >>> from lp.bugs.scripts.checkwatches import CheckwatchesMaster |
112 | - >>> bugwatch_updater = CheckwatchesMaster(LaunchpadZopelessLayer.txn) |
113 | - |
114 | - >>> transaction.commit() |
115 | - |
116 | - >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch) |
117 | + >>> from canonical.launchpad.scripts.logger import log |
118 | + >>> from lp.bugs.scripts.checkwatches.base import ( |
119 | + ... WorkingBase) |
120 | + >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import ( |
121 | + ... BugWatchUpdater) |
122 | + |
123 | + >>> working_base = WorkingBase() |
124 | + >>> working_base.init( |
125 | + ... 'bugwatch@bugs.launchpad.net', transaction, log) |
126 | + >>> bugwatch_updater = BugWatchUpdater( |
127 | + ... working_base, bug_watch, external_bugtracker) |
128 | + |
129 | + >>> bugwatch_updater.pushBugComments() |
130 | Comment added as remote comment 1 |
131 | INFO:...:Pushed 1 comments to remote bug 1234 on ... |
132 | |
133 | @@ -122,7 +129,7 @@ |
134 | |
135 | >>> transaction.commit() |
136 | |
137 | - >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch) |
138 | + >>> bugwatch_updater.pushBugComments() |
139 | >>> transaction.commit() |
140 | |
141 | If we now check the bug watch's unpushed_comments property, we will |
142 | @@ -148,7 +155,7 @@ |
143 | >>> bugmessage_three = bug.linkMessage(message_three, bug_watch) |
144 | >>> transaction.commit() |
145 | |
146 | - >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch) |
147 | + >>> bugwatch_updater.pushBugComments() |
148 | Comment added as remote comment 2 |
149 | Comment added as remote comment 3 |
150 | INFO:...:Pushed 2 comments to remote bug 1234 on ... |
151 | @@ -171,7 +178,7 @@ |
152 | >>> bugmessage_four = bug.linkMessage(message_four) |
153 | >>> transaction.commit() |
154 | |
155 | - >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch) |
156 | + >>> bugwatch_updater.pushBugComments() |
157 | |
158 | >>> print_bug_messages(bug, bug_watch) |
159 | 1: Pushing, for the purpose of. |
160 | @@ -222,7 +229,8 @@ |
161 | |
162 | >>> transaction.commit() |
163 | |
164 | - >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch) |
165 | + >>> bugwatch_updater.external_bugtracker = external_bugtracker |
166 | + >>> bugwatch_updater.importBugComments() |
167 | INFO:...:Imported 3 comments for remote bug 1234 on ... |
168 | |
169 | Each of the imported comments has its remote_comment_id field set. |
170 | @@ -239,9 +247,7 @@ |
171 | Running pushBugComments() on the external bugtracker won't result in the |
172 | comments being pushed because they have already been imported. |
173 | |
174 | - >>> transaction.commit() |
175 | - |
176 | - >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch) |
177 | + >>> bugwatch_updater.pushBugComments() |
178 | |
179 | If the external bugtracker's addRemoteComment() method returns an |
180 | invalid remote comment ID, an error will be raised: |
181 | @@ -264,8 +270,10 @@ |
182 | >>> broken_external_bugtracker = ErroringExternalBugTracker( |
183 | ... 'http://example.com') |
184 | |
185 | - >>> bugwatch_updater.pushBugComments( |
186 | - ... broken_external_bugtracker, bug_watch) |
187 | + >>> bugwatch_updater = BugWatchUpdater( |
188 | + ... working_base, bug_watch, external_bugtracker) |
189 | + >>> bugwatch_updater.external_bugtracker = broken_external_bugtracker |
190 | + >>> bugwatch_updater.pushBugComments() |
191 | Traceback (most recent call last): |
192 | ... |
193 | AssertionError: A remote_comment_id must be specified. |
194 | @@ -310,8 +318,7 @@ |
195 | remote server. This allows us to include salient information, such as |
196 | the comment author, with the pushed comment. |
197 | |
198 | - >>> formatted_message = bugwatch_updater._formatRemoteComment( |
199 | - ... external_bugtracker, bug_watch, message) |
200 | + >>> formatted_message = bugwatch_updater._formatRemoteComment(message) |
201 | >>> print formatted_message |
202 | Sample Person added the following comment to Launchpad bug #...: |
203 | <BLANKLINE> |
204 | @@ -331,8 +338,8 @@ |
205 | ... dirname(__file__), '../tests/testfiles/test_comment_template.txt') |
206 | >>> external_bugtracker.comment_template = comment_template |
207 | |
208 | - >>> formatted_message = bugwatch_updater._formatRemoteComment( |
209 | - ... external_bugtracker, bug_watch, message) |
210 | + >>> bugwatch_updater.external_bugtracker = external_bugtracker |
211 | + >>> formatted_message = bugwatch_updater._formatRemoteComment(message) |
212 | >>> print formatted_message |
213 | Egg and bacon |
214 | Egg, sausage and bacon |
215 | |
216 | === modified file 'lib/lp/bugs/doc/externalbugtracker-linking-back.txt' |
217 | --- lib/lp/bugs/doc/externalbugtracker-linking-back.txt 2010-04-21 10:30:24 +0000 |
218 | +++ lib/lp/bugs/doc/externalbugtracker-linking-back.txt 2010-04-27 07:12:35 +0000 |
219 | @@ -30,7 +30,7 @@ |
220 | The methods are called by the CheckwatchesMaster class: |
221 | |
222 | >>> from canonical.testing import LaunchpadZopelessLayer |
223 | - >>> txn = LaunchpadZopelessLayer.txn |
224 | + >>> txn = transaction |
225 | |
226 | >>> LaunchpadZopelessLayer.switchDbUser('launchpad') |
227 | |
228 | @@ -47,13 +47,13 @@ |
229 | >>> LaunchpadZopelessLayer.switchDbUser('checkwatches') |
230 | |
231 | >>> from lp.bugs.scripts.checkwatches import CheckwatchesMaster |
232 | - >>> bug_watch_updater = CheckwatchesMaster(txn) |
233 | + >>> checkwatches_master = CheckwatchesMaster(txn) |
234 | >>> txn.commit() |
235 | |
236 | >>> external_bugtracker = BackLinkingExternalBugTracker( |
237 | ... 'http://example.com/') |
238 | |
239 | - >>> bug_watch_updater.updateBugWatches(external_bugtracker, [bug_watch]) |
240 | + >>> checkwatches_master.updateBugWatches(external_bugtracker, [bug_watch]) |
241 | Getting Launchpad id for bug 42 |
242 | Setting Launchpad id for bug 42 |
243 | |
244 | @@ -63,17 +63,17 @@ |
245 | For comment syncing and back-linking to be attempted, bug watches must |
246 | be related to a bug task, not just a bug. |
247 | |
248 | - >>> bug_watch_updater.updateBugWatches( |
249 | + >>> checkwatches_master.updateBugWatches( |
250 | ... external_bugtracker, [bug_watch_without_bugtask]) |
251 | |
252 | |
253 | -== CheckwatchesMaster.linkLaunchpadBug() == |
254 | +== BugWatchUpdater.linkLaunchpadBug() == |
255 | |
256 | -The CheckwatchesMaster method that does the work of setting the Launchpad |
257 | +The BugWatchUpdater method that does the work of setting the Launchpad |
258 | bug link is linkLaunchpadBug(). This method first retrieves the |
259 | current Launchpad bug ID for the remote bug. If the remote bug is |
260 | already linked to a Launchpad bug other than the one that we're trying |
261 | -to link it to, the CheckwatchesMaster will check that the bug that is |
262 | +to link it to, the BugWatchUpdater will check that the bug that is |
263 | already linked has a valid watch on the remote bug in question. If it |
264 | does, the link will remain unchanged. Otherwise it will be updated. |
265 | |
266 | @@ -91,13 +91,23 @@ |
267 | |
268 | >>> transaction.commit() |
269 | |
270 | - >>> bug_watch_updater.linkLaunchpadBug( |
271 | - ... external_bugtracker, bug_watch_2) |
272 | + >>> from canonical.launchpad.scripts.logger import log |
273 | + >>> from lp.bugs.scripts.checkwatches.base import ( |
274 | + ... WorkingBase) |
275 | + >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import ( |
276 | + ... BugWatchUpdater) |
277 | + |
278 | + >>> working_base = WorkingBase() |
279 | + >>> working_base.init( |
280 | + ... 'bugwatch@bugs.launchpad.net', transaction, log) |
281 | + >>> bug_watch_updater = BugWatchUpdater( |
282 | + ... working_base, bug_watch, external_bugtracker) |
283 | + >>> bug_watch_updater.linkLaunchpadBug() |
284 | Getting Launchpad id for bug 42 |
285 | |
286 | However, if we set the current Launchpad bug ID on our |
287 | BackLinkingExternalBugTracker to a Launchpad bug that doesn't link to |
288 | -the remote bug, CheckwatchesMaster.linkLaunchpadBug() will call |
289 | +the remote bug, BugWatchUpdater.linkLaunchpadBug() will call |
290 | getLaunchpadBugId() and then, when it discovers that the current |
291 | Launchpad bug ID isn't valid, setLaunchpadBugId() to correct the error. |
292 | |
293 | @@ -117,7 +127,7 @@ |
294 | |
295 | >>> transaction.commit() |
296 | |
297 | - >>> bug_watch_updater.linkLaunchpadBug(external_bugtracker, bug_watch) |
298 | + >>> bug_watch_updater.linkLaunchpadBug() |
299 | Getting Launchpad id for bug 42 |
300 | Setting Launchpad id for bug 42 |
301 | |
302 | @@ -127,6 +137,6 @@ |
303 | error. |
304 | |
305 | >>> external_bugtracker.last_launchpad_bug_id = 0 |
306 | - >>> bug_watch_updater.linkLaunchpadBug(external_bugtracker, bug_watch) |
307 | + >>> bug_watch_updater.linkLaunchpadBug() |
308 | Getting Launchpad id for bug 42 |
309 | Setting Launchpad id for bug 42 |
310 | |
311 | === modified file 'lib/lp/bugs/scripts/checkwatches/base.py' |
312 | --- lib/lp/bugs/scripts/checkwatches/base.py 2010-04-21 08:58:25 +0000 |
313 | +++ lib/lp/bugs/scripts/checkwatches/base.py 2010-04-27 07:12:35 +0000 |
314 | @@ -119,7 +119,7 @@ |
315 | class WorkingBase: |
316 | """A base class for writing a long-running process.""" |
317 | |
318 | - def __init__(self, login, transaction_manager, logger): |
319 | + def init(self, login, transaction_manager, logger): |
320 | self._login = login |
321 | self._principal = ( |
322 | getUtility(IPlacelessAuthUtility).getPrincipalByLogin( |
323 | @@ -127,6 +127,12 @@ |
324 | self._transaction_manager = transaction_manager |
325 | self.logger = logger |
326 | |
327 | + def initFromParent(self, parent): |
328 | + self._login = parent._login |
329 | + self._principal = parent._principal |
330 | + self._transaction_manager = parent._transaction_manager |
331 | + self.logger = parent.logger |
332 | + |
333 | @property |
334 | @contextmanager |
335 | def interaction(self): |
336 | |
337 | === added file 'lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py' |
338 | --- lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py 1970-01-01 00:00:00 +0000 |
339 | +++ lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py 2010-04-27 07:12:35 +0000 |
340 | @@ -0,0 +1,284 @@ |
341 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
342 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
343 | + |
344 | +"""Classes and logic for the checkwatches BugWatchUpdater.""" |
345 | + |
346 | +from __future__ import with_statement |
347 | + |
348 | +__metaclass__ = type |
349 | +__all__ = [ |
350 | + 'BugWatchUpdater', |
351 | + ] |
352 | + |
353 | +import sys |
354 | + |
355 | +from zope.component import getUtility |
356 | +from zope.event import notify |
357 | + |
358 | +from canonical.launchpad.helpers import get_email_template |
359 | +from canonical.launchpad.interfaces.launchpad import ( |
360 | + ILaunchpadCelebrities, NotFoundError) |
361 | +from canonical.launchpad.interfaces.message import IMessageSet |
362 | +from canonical.launchpad.webapp.publisher import canonical_url |
363 | + |
364 | +from lazr.lifecycle.event import ObjectCreatedEvent |
365 | + |
366 | +from lp.bugs.interfaces.bug import IBugSet |
367 | +from lp.bugs.scripts.checkwatches.base import ( |
368 | + WorkingBase, commit_before) |
369 | +from lp.registry.interfaces.person import PersonCreationRationale |
370 | + |
371 | + |
372 | +class BugWatchUpdater(WorkingBase): |
373 | + """Handles the updating of a single BugWatch for checkwatches.""" |
374 | + |
375 | + def __init__(self, parent, bug_watch, external_bugtracker): |
376 | + self.initFromParent(parent) |
377 | + self.bug_watch = bug_watch |
378 | + self.external_bugtracker = external_bugtracker |
379 | + |
380 | + @commit_before |
381 | + def updateBugWatch(self, new_remote_status, new_malone_status, |
382 | + new_remote_importance, new_malone_importance, |
383 | + can_import_comments, can_push_comments, can_back_link, |
384 | + error, oops_id): |
385 | + """Update the BugWatch.""" |
386 | + with self.transaction: |
387 | + self.bug_watch.last_error_type = error |
388 | + if new_malone_status is not None: |
389 | + self.bug_watch.updateStatus( |
390 | + new_remote_status, new_malone_status) |
391 | + if new_malone_importance is not None: |
392 | + self.bug_watch.updateImportance( |
393 | + new_remote_importance, new_malone_importance) |
394 | + # Only sync comments and backlink if there was no |
395 | + # earlier error, the local bug isn't a duplicate, |
396 | + # *and* if the bug watch is associated with a bug |
397 | + # task. This helps us to avoid spamming upstream. |
398 | + do_sync = ( |
399 | + error is None and |
400 | + self.bug_watch.bug.duplicateof is None and |
401 | + len(self.bug_watch.bugtasks) > 0 |
402 | + ) |
403 | + |
404 | + # XXX: Gavin Panella 2010-04-19 bug=509223: |
405 | + # Exception handling is all wrong! If any of these |
406 | + # throw an exception, *all* the watches in |
407 | + # self.bug_watches, even those that have not errored, |
408 | + # will have negative activity added. |
409 | + if do_sync: |
410 | + if can_import_comments: |
411 | + self.importBugComments() |
412 | + if can_push_comments: |
413 | + self.pushBugComments() |
414 | + if can_back_link: |
415 | + self.linkLaunchpadBug() |
416 | + |
417 | + with self.transaction: |
418 | + self.bug_watch.addActivity( |
419 | + result=error, oops_id=oops_id) |
420 | + |
421 | + @commit_before |
422 | + def importBugComments(self): |
423 | + """Import all the comments from the remote bug.""" |
424 | + # Avoid circularity. |
425 | + from lp.bugs.scripts.checkwatches.core import ( |
426 | + get_remote_system_oops_properties) |
427 | + |
428 | + with self.transaction: |
429 | + local_bug_id = self.bug_watch.bug.id |
430 | + remote_bug_id = self.bug_watch.remotebug |
431 | + |
432 | + # Construct a list of the comment IDs we want to import; i.e. |
433 | + # those which we haven't already imported. |
434 | + all_comment_ids = self.external_bugtracker.getCommentIds( |
435 | + remote_bug_id) |
436 | + |
437 | + with self.transaction: |
438 | + comment_ids_to_import = [ |
439 | + comment_id for comment_id in all_comment_ids |
440 | + if not self.bug_watch.hasComment(comment_id)] |
441 | + |
442 | + self.external_bugtracker.fetchComments( |
443 | + remote_bug_id, comment_ids_to_import) |
444 | + |
445 | + with self.transaction: |
446 | + previous_imported_comments = ( |
447 | + self.bug_watch.getImportedBugMessages()) |
448 | + is_initial_import = previous_imported_comments.count() == 0 |
449 | + imported_comments = [] |
450 | + |
451 | + for comment_id in comment_ids_to_import: |
452 | + displayname, email = ( |
453 | + self.external_bugtracker.getPosterForComment( |
454 | + remote_bug_id, comment_id)) |
455 | + |
456 | + if displayname is None and email is None: |
457 | + # If we don't have a displayname or an email address |
458 | + # then we can't create a Launchpad Person as the author |
459 | + # of this comment. We raise an OOPS and continue. |
460 | + self.warning( |
461 | + "Unable to import remote comment author. No email " |
462 | + "address or display name found.", |
463 | + get_remote_system_oops_properties( |
464 | + self.external_bugtracker), |
465 | + sys.exc_info()) |
466 | + continue |
467 | + |
468 | + poster = self.bug_watch.bugtracker.ensurePersonForSelf( |
469 | + displayname, email, PersonCreationRationale.BUGIMPORT, |
470 | + "when importing comments for %s." % self.bug_watch.title) |
471 | + |
472 | + comment_message = ( |
473 | + self.external_bugtracker.getMessageForComment( |
474 | + remote_bug_id, comment_id, poster)) |
475 | + |
476 | + bug_message = self.bug_watch.addComment( |
477 | + comment_id, comment_message) |
478 | + imported_comments.append(bug_message) |
479 | + |
480 | + if len(imported_comments) > 0: |
481 | + self.bug_watch_updater = ( |
482 | + getUtility(ILaunchpadCelebrities).bug_watch_updater) |
483 | + if is_initial_import: |
484 | + notification_text = get_email_template( |
485 | + 'bugwatch-initial-comment-import.txt') % dict( |
486 | + num_of_comments=len(imported_comments), |
487 | + bug_watch_url=self.bug_watch.url) |
488 | + comment_text_template = get_email_template( |
489 | + 'bugwatch-comment.txt') |
490 | + |
491 | + for bug_message in imported_comments: |
492 | + comment = bug_message.message |
493 | + notification_text += comment_text_template % dict( |
494 | + comment_date=comment.datecreated.isoformat(), |
495 | + commenter=comment.owner.displayname, |
496 | + comment_text=comment.text_contents, |
497 | + comment_reply_url=canonical_url(comment)) |
498 | + notification_message = getUtility(IMessageSet).fromText( |
499 | + subject=self.bug_watch.bug.followup_subject(), |
500 | + content=notification_text, |
501 | + owner=self.bug_watch_updater) |
502 | + self.bug_watch.bug.addCommentNotification( |
503 | + notification_message) |
504 | + else: |
505 | + for bug_message in imported_comments: |
506 | + notify(ObjectCreatedEvent( |
507 | + bug_message, |
508 | + user=self.bug_watch_updater)) |
509 | + |
510 | + self.logger.info("Imported %(count)i comments for remote bug " |
511 | + "%(remotebug)s on %(bugtracker_url)s into Launchpad bug " |
512 | + "%(bug_id)s." % |
513 | + {'count': len(imported_comments), |
514 | + 'remotebug': remote_bug_id, |
515 | + 'bugtracker_url': self.external_bugtracker.baseurl, |
516 | + 'bug_id': local_bug_id}) |
517 | + |
518 | + def _formatRemoteComment(self, message): |
519 | + """Format a comment for a remote bugtracker and return it.""" |
520 | + comment_template = get_email_template( |
521 | + self.external_bugtracker.comment_template) |
522 | + |
523 | + return comment_template % { |
524 | + 'launchpad_bug': self.bug_watch.bug.id, |
525 | + 'comment_author': message.owner.displayname, |
526 | + 'comment_body': message.text_contents, |
527 | + } |
528 | + |
529 | + @commit_before |
530 | + def pushBugComments(self): |
531 | + """Push Launchpad comments to the remote bug. |
532 | + |
533 | + :param self.external_bugtracker: An external bugtracker which |
534 | + implements `ISupportsCommentPushing`. |
535 | + :param self.bug_watch: The bug watch to which the comments should be |
536 | + pushed. |
537 | + """ |
538 | + pushed_comments = 0 |
539 | + |
540 | + with self.transaction: |
541 | + local_bug_id = self.bug_watch.bug.id |
542 | + remote_bug_id = self.bug_watch.remotebug |
543 | + unpushed_comments = list(self.bug_watch.unpushed_comments) |
544 | + |
545 | + # Loop over the unpushed comments for the bug watch. |
546 | + # We only push those comments that haven't been pushed |
547 | + # already. We don't push any comments not associated with |
548 | + # the bug watch. |
549 | + for unpushed_comment in unpushed_comments: |
550 | + with self.transaction: |
551 | + message = unpushed_comment.message |
552 | + message_rfc822msgid = message.rfc822msgid |
553 | + # Format the comment so that it includes information |
554 | + # about the Launchpad bug. |
555 | + formatted_comment = self._formatRemoteComment(message) |
556 | + |
557 | + remote_comment_id = ( |
558 | + self.external_bugtracker.addRemoteComment( |
559 | + remote_bug_id, formatted_comment, |
560 | + message_rfc822msgid)) |
561 | + |
562 | + assert remote_comment_id is not None, ( |
563 | + "A remote_comment_id must be specified.") |
564 | + with self.transaction: |
565 | + unpushed_comment.remote_comment_id = remote_comment_id |
566 | + |
567 | + pushed_comments += 1 |
568 | + |
569 | + if pushed_comments > 0: |
570 | + self.logger.info("Pushed %(count)i comments to remote bug " |
571 | + "%(remotebug)s on %(bugtracker_url)s from Launchpad bug " |
572 | + "%(bug_id)s" % |
573 | + {'count': pushed_comments, |
574 | + 'remotebug': remote_bug_id, |
575 | + 'bugtracker_url': self.external_bugtracker.baseurl, |
576 | + 'bug_id': local_bug_id}) |
577 | + |
578 | + @commit_before |
579 | + def linkLaunchpadBug(self): |
580 | + """Link a Launchpad bug to a given remote bug.""" |
581 | + with self.transaction: |
582 | + local_bug_id = self.bug_watch.bug.id |
583 | + local_bug_url = canonical_url(self.bug_watch.bug) |
584 | + remote_bug_id = self.bug_watch.remotebug |
585 | + |
586 | + current_launchpad_id = self.external_bugtracker.getLaunchpadBugId( |
587 | + remote_bug_id) |
588 | + |
589 | + if current_launchpad_id is None: |
590 | + # If no bug is linked to the remote bug, link this one and |
591 | + # then stop. |
592 | + self.external_bugtracker.setLaunchpadBugId( |
593 | + remote_bug_id, local_bug_id, local_bug_url) |
594 | + return |
595 | + |
596 | + elif current_launchpad_id == local_bug_id: |
597 | + # If the current_launchpad_id is the same as the ID of the bug |
598 | + # we're trying to link, we can stop. |
599 | + return |
600 | + |
601 | + else: |
602 | + # If the current_launchpad_id isn't the same as the one |
603 | + # we're trying to link, check that the other bug actually |
604 | + # links to the remote bug. If it does, we do nothing, since |
605 | + # the first valid link wins. Otherwise we link the bug that |
606 | + # we've been passed, overwriting the previous value of the |
607 | + # Launchpad bug ID for this remote bug. |
608 | + try: |
609 | + with self.transaction: |
610 | + other_launchpad_bug = getUtility(IBugSet).get( |
611 | + current_launchpad_id) |
612 | + other_bug_watch = other_launchpad_bug.getBugWatch( |
613 | + self.bug_watch.bugtracker, remote_bug_id) |
614 | + except NotFoundError: |
615 | + # If we can't find the bug that's referenced by |
616 | + # current_launchpad_id we simply set other_self.bug_watch to |
617 | + # None so that the Launchpad ID of the remote bug can be |
618 | + # set correctly. |
619 | + other_bug_watch = None |
620 | + |
621 | + if other_bug_watch is None: |
622 | + self.external_bugtracker.setLaunchpadBugId( |
623 | + remote_bug_id, local_bug_id, local_bug_url) |
624 | + |
625 | |
626 | === modified file 'lib/lp/bugs/scripts/checkwatches/core.py' |
627 | --- lib/lp/bugs/scripts/checkwatches/core.py 2010-04-21 10:33:55 +0000 |
628 | +++ lib/lp/bugs/scripts/checkwatches/core.py 2010-04-27 07:12:35 +0000 |
629 | @@ -33,21 +33,15 @@ |
630 | from twisted.python.threadpool import ThreadPool |
631 | |
632 | from zope.component import getUtility |
633 | -from zope.event import notify |
634 | |
635 | from canonical.database.constants import UTC_NOW |
636 | from canonical.database.sqlbase import flush_database_updates |
637 | -from lazr.lifecycle.event import ObjectCreatedEvent |
638 | -from canonical.launchpad.helpers import get_email_template |
639 | from canonical.launchpad.interfaces import ( |
640 | BugTaskStatus, BugWatchActivityStatus, CreateBugParams, |
641 | IBugTrackerSet, IBugWatchSet, IDistribution, ILaunchpadCelebrities, |
642 | IPersonSet, ISupportsCommentImport, ISupportsCommentPushing, |
643 | PersonCreationRationale, UNKNOWN_REMOTE_STATUS) |
644 | -from canonical.launchpad.interfaces.launchpad import NotFoundError |
645 | -from canonical.launchpad.interfaces.message import IMessageSet |
646 | from canonical.launchpad.scripts.logger import log as default_log |
647 | -from canonical.launchpad.webapp.publisher import canonical_url |
648 | |
649 | from lp.bugs import externalbugtracker |
650 | from lp.bugs.externalbugtracker import ( |
651 | @@ -55,10 +49,10 @@ |
652 | BugWatchUpdateError, InvalidBugId, PrivateRemoteBug, |
653 | UnknownBugTrackerTypeError, UnknownRemoteStatusError, UnparseableBugData, |
654 | UnparseableBugTrackerVersion, UnsupportedBugTrackerVersion) |
655 | -from lp.bugs.interfaces.bug import IBugSet |
656 | from lp.bugs.interfaces.externalbugtracker import ISupportsBackLinking |
657 | from lp.bugs.scripts.checkwatches.base import ( |
658 | WorkingBase, commit_before, with_interaction) |
659 | +from lp.bugs.scripts.checkwatches.bugwatchupdater import BugWatchUpdater |
660 | from lp.services.scripts.base import LaunchpadCronScript |
661 | |
662 | |
663 | @@ -166,8 +160,7 @@ |
664 | provides a similar interface. |
665 | |
666 | """ |
667 | - super(CheckwatchesMaster, self).__init__( |
668 | - LOGIN, transaction_manager, logger) |
669 | + self.init(LOGIN, transaction_manager, logger) |
670 | |
671 | # Override SYNCABLE_GNOME_PRODUCTS if necessary. |
672 | if syncable_gnome_products is not None: |
673 | @@ -792,44 +785,19 @@ |
674 | ('URL', remote_bug_url), |
675 | ('bug_id', remote_bug_id), |
676 | ('local_ids', local_ids), |
677 | - ] + get_remote_system_oops_properties(remotesystem), |
678 | + ] + get_remote_system_oops_properties( |
679 | + remotesystem), |
680 | info=sys.exc_info()) |
681 | |
682 | for bug_watch in bug_watches: |
683 | - with self.transaction: |
684 | - bug_watch.last_error_type = error |
685 | - if new_malone_status is not None: |
686 | - bug_watch.updateStatus( |
687 | - new_remote_status, new_malone_status) |
688 | - if new_malone_importance is not None: |
689 | - bug_watch.updateImportance( |
690 | - new_remote_importance, new_malone_importance) |
691 | - # Only sync comments and backlink if there was no |
692 | - # earlier error, the local bug isn't a duplicate, |
693 | - # *and* if the bug watch is associated with a bug |
694 | - # task. This helps us to avoid spamming upstream. |
695 | - do_sync = ( |
696 | - error is None and |
697 | - bug_watch.bug.duplicateof is None and |
698 | - len(bug_watch.bugtasks) > 0 |
699 | - ) |
700 | - |
701 | - # XXX: Gavin Panella 2010-04-19 bug=509223: |
702 | - # Exception handling is all wrong! If any of these |
703 | - # throw an exception, *all* the watches in |
704 | - # bug_watches, even those that have not errored, |
705 | - # will have negative activity added. |
706 | - if do_sync: |
707 | - if can_import_comments: |
708 | - self.importBugComments(remotesystem, bug_watch) |
709 | - if can_push_comments: |
710 | - self.pushBugComments(remotesystem, bug_watch) |
711 | - if can_back_link: |
712 | - self.linkLaunchpadBug(remotesystem, bug_watch) |
713 | - |
714 | - with self.transaction: |
715 | - bug_watch.addActivity( |
716 | - result=error, oops_id=oops_id) |
717 | + bug_watch_updater = BugWatchUpdater( |
718 | + self, bug_watch, remotesystem) |
719 | + |
720 | + bug_watch_updater.updateBugWatch( |
721 | + new_remote_status, new_malone_status, |
722 | + new_remote_importance, new_malone_importance, |
723 | + can_import_comments, can_push_comments, |
724 | + can_back_link, error, oops_id) |
725 | |
726 | except (KeyboardInterrupt, SystemExit): |
727 | # We should never catch KeyboardInterrupt or SystemExit. |
728 | @@ -873,8 +841,8 @@ |
729 | """ |
730 | assert IDistribution.providedBy(bug_target), ( |
731 | 'Only imports of bugs for a distribution is implemented.') |
732 | - reporter_name, reporter_email = external_bugtracker.getBugReporter( |
733 | - remote_bug) |
734 | + reporter_name, reporter_email = ( |
735 | + external_bugtracker.getBugReporter(remote_bug)) |
736 | reporter = getUtility(IPersonSet).ensurePerson( |
737 | reporter_email, reporter_name, PersonCreationRationale.BUGIMPORT, |
738 | comment='when importing bug #%s from %s' % ( |
739 | @@ -907,203 +875,6 @@ |
740 | |
741 | return bug |
742 | |
743 | - def importBugComments(self, external_bugtracker, bug_watch): |
744 | - """Import all the comments from a remote bug. |
745 | - |
746 | - :param external_bugtracker: An external bugtracker which |
747 | - implements `ISupportsCommentImport`. |
748 | - :param bug_watch: The bug watch for which the comments should be |
749 | - imported. |
750 | - """ |
751 | - with self.transaction: |
752 | - local_bug_id = bug_watch.bug.id |
753 | - remote_bug_id = bug_watch.remotebug |
754 | - |
755 | - # Construct a list of the comment IDs we want to import; i.e. |
756 | - # those which we haven't already imported. |
757 | - all_comment_ids = external_bugtracker.getCommentIds(remote_bug_id) |
758 | - |
759 | - with self.transaction: |
760 | - comment_ids_to_import = [ |
761 | - comment_id for comment_id in all_comment_ids |
762 | - if not bug_watch.hasComment(comment_id)] |
763 | - |
764 | - external_bugtracker.fetchComments( |
765 | - remote_bug_id, comment_ids_to_import) |
766 | - |
767 | - with self.transaction: |
768 | - previous_imported_comments = bug_watch.getImportedBugMessages() |
769 | - is_initial_import = previous_imported_comments.count() == 0 |
770 | - imported_comments = [] |
771 | - |
772 | - for comment_id in comment_ids_to_import: |
773 | - displayname, email = external_bugtracker.getPosterForComment( |
774 | - remote_bug_id, comment_id) |
775 | - |
776 | - if displayname is None and email is None: |
777 | - # If we don't have a displayname or an email address |
778 | - # then we can't create a Launchpad Person as the author |
779 | - # of this comment. We raise an OOPS and continue. |
780 | - self.warning( |
781 | - "Unable to import remote comment author. No email " |
782 | - "address or display name found.", |
783 | - get_remote_system_oops_properties(external_bugtracker), |
784 | - sys.exc_info()) |
785 | - continue |
786 | - |
787 | - poster = bug_watch.bugtracker.ensurePersonForSelf( |
788 | - displayname, email, PersonCreationRationale.BUGIMPORT, |
789 | - "when importing comments for %s." % bug_watch.title) |
790 | - |
791 | - comment_message = external_bugtracker.getMessageForComment( |
792 | - remote_bug_id, comment_id, poster) |
793 | - |
794 | - bug_message = bug_watch.addComment( |
795 | - comment_id, comment_message) |
796 | - imported_comments.append(bug_message) |
797 | - |
798 | - if len(imported_comments) > 0: |
799 | - bug_watch_updater = ( |
800 | - getUtility(ILaunchpadCelebrities).bug_watch_updater) |
801 | - if is_initial_import: |
802 | - notification_text = get_email_template( |
803 | - 'bugwatch-initial-comment-import.txt') % dict( |
804 | - num_of_comments=len(imported_comments), |
805 | - bug_watch_url=bug_watch.url) |
806 | - comment_text_template = get_email_template( |
807 | - 'bugwatch-comment.txt') |
808 | - |
809 | - for bug_message in imported_comments: |
810 | - comment = bug_message.message |
811 | - notification_text += comment_text_template % dict( |
812 | - comment_date=comment.datecreated.isoformat(), |
813 | - commenter=comment.owner.displayname, |
814 | - comment_text=comment.text_contents, |
815 | - comment_reply_url=canonical_url(comment)) |
816 | - notification_message = getUtility(IMessageSet).fromText( |
817 | - subject=bug_watch.bug.followup_subject(), |
818 | - content=notification_text, |
819 | - owner=bug_watch_updater) |
820 | - bug_watch.bug.addCommentNotification(notification_message) |
821 | - else: |
822 | - for bug_message in imported_comments: |
823 | - notify(ObjectCreatedEvent( |
824 | - bug_message, |
825 | - user=bug_watch_updater)) |
826 | - |
827 | - self.logger.info("Imported %(count)i comments for remote bug " |
828 | - "%(remotebug)s on %(bugtracker_url)s into Launchpad bug " |
829 | - "%(bug_id)s." % |
830 | - {'count': len(imported_comments), |
831 | - 'remotebug': remote_bug_id, |
832 | - 'bugtracker_url': external_bugtracker.baseurl, |
833 | - 'bug_id': local_bug_id}) |
834 | - |
835 | - def _formatRemoteComment(self, external_bugtracker, bug_watch, message): |
836 | - """Format a comment for a remote bugtracker and return it.""" |
837 | - comment_template = get_email_template( |
838 | - external_bugtracker.comment_template) |
839 | - |
840 | - return comment_template % { |
841 | - 'launchpad_bug': bug_watch.bug.id, |
842 | - 'comment_author': message.owner.displayname, |
843 | - 'comment_body': message.text_contents, |
844 | - } |
845 | - |
846 | - def pushBugComments(self, external_bugtracker, bug_watch): |
847 | - """Push Launchpad comments to the remote bug. |
848 | - |
849 | - :param external_bugtracker: An external bugtracker which |
850 | - implements `ISupportsCommentPushing`. |
851 | - :param bug_watch: The bug watch to which the comments should be |
852 | - pushed. |
853 | - """ |
854 | - pushed_comments = 0 |
855 | - |
856 | - with self.transaction: |
857 | - local_bug_id = bug_watch.bug.id |
858 | - remote_bug_id = bug_watch.remotebug |
859 | - unpushed_comments = list(bug_watch.unpushed_comments) |
860 | - |
861 | - # Loop over the unpushed comments for the bug watch. |
862 | - # We only push those comments that haven't been pushed |
863 | - # already. We don't push any comments not associated with |
864 | - # the bug watch. |
865 | - for unpushed_comment in unpushed_comments: |
866 | - with self.transaction: |
867 | - message = unpushed_comment.message |
868 | - message_rfc822msgid = message.rfc822msgid |
869 | - # Format the comment so that it includes information |
870 | - # about the Launchpad bug. |
871 | - formatted_comment = self._formatRemoteComment( |
872 | - external_bugtracker, bug_watch, message) |
873 | - |
874 | - remote_comment_id = ( |
875 | - external_bugtracker.addRemoteComment( |
876 | - remote_bug_id, formatted_comment, |
877 | - message_rfc822msgid)) |
878 | - |
879 | - assert remote_comment_id is not None, ( |
880 | - "A remote_comment_id must be specified.") |
881 | - with self.transaction: |
882 | - unpushed_comment.remote_comment_id = remote_comment_id |
883 | - |
884 | - pushed_comments += 1 |
885 | - |
886 | - if pushed_comments > 0: |
887 | - self.logger.info("Pushed %(count)i comments to remote bug " |
888 | - "%(remotebug)s on %(bugtracker_url)s from Launchpad bug " |
889 | - "%(bug_id)s" % |
890 | - {'count': pushed_comments, |
891 | - 'remotebug': remote_bug_id, |
892 | - 'bugtracker_url': external_bugtracker.baseurl, |
893 | - 'bug_id': local_bug_id}) |
894 | - |
895 | - def linkLaunchpadBug(self, remotesystem, bug_watch): |
896 | - """Link a Launchpad bug to a given remote bug.""" |
897 | - with self.transaction: |
898 | - local_bug_id = bug_watch.bug.id |
899 | - local_bug_url = canonical_url(bug_watch.bug) |
900 | - remote_bug_id = bug_watch.remotebug |
901 | - |
902 | - current_launchpad_id = remotesystem.getLaunchpadBugId(remote_bug_id) |
903 | - |
904 | - if current_launchpad_id is None: |
905 | - # If no bug is linked to the remote bug, link this one and |
906 | - # then stop. |
907 | - remotesystem.setLaunchpadBugId( |
908 | - remote_bug_id, local_bug_id, local_bug_url) |
909 | - return |
910 | - |
911 | - elif current_launchpad_id == local_bug_id: |
912 | - # If the current_launchpad_id is the same as the ID of the bug |
913 | - # we're trying to link, we can stop. |
914 | - return |
915 | - |
916 | - else: |
917 | - # If the current_launchpad_id isn't the same as the one |
918 | - # we're trying to link, check that the other bug actually |
919 | - # links to the remote bug. If it does, we do nothing, since |
920 | - # the first valid link wins. Otherwise we link the bug that |
921 | - # we've been passed, overwriting the previous value of the |
922 | - # Launchpad bug ID for this remote bug. |
923 | - try: |
924 | - with self.transaction: |
925 | - other_launchpad_bug = getUtility(IBugSet).get( |
926 | - current_launchpad_id) |
927 | - other_bug_watch = other_launchpad_bug.getBugWatch( |
928 | - bug_watch.bugtracker, remote_bug_id) |
929 | - except NotFoundError: |
930 | - # If we can't find the bug that's referenced by |
931 | - # current_launchpad_id we simply set other_bug_watch to |
932 | - # None so that the Launchpad ID of the remote bug can be |
933 | - # set correctly. |
934 | - other_bug_watch = None |
935 | - |
936 | - if other_bug_watch is None: |
937 | - remotesystem.setLaunchpadBugId( |
938 | - remote_bug_id, local_bug_id, local_bug_url) |
939 | - |
940 | |
941 | class BaseScheduler: |
942 | """Run jobs according to a policy.""" |
943 | |
944 | === modified file 'lib/lp/bugs/scripts/checkwatches/tests/test_base.py' |
945 | --- lib/lp/bugs/scripts/checkwatches/tests/test_base.py 2010-04-21 08:42:13 +0000 |
946 | +++ lib/lp/bugs/scripts/checkwatches/tests/test_base.py 2010-04-27 07:12:35 +0000 |
947 | @@ -48,7 +48,8 @@ |
948 | def test_interaction(self): |
949 | # The WorkingBase.interaction context manager will begin an |
950 | # interaction on entry and end it on exit. |
951 | - base = WorkingBase(self.email, transaction.manager, self.logger) |
952 | + base = WorkingBase() |
953 | + base.init(self.email, transaction.manager, self.logger) |
954 | endInteraction() |
955 | self.assertIs(None, queryInteraction()) |
956 | with base.interaction: |
957 | @@ -59,7 +60,8 @@ |
958 | # If an interaction is already in progress, the interaction |
959 | # context manager will not begin a new interaction on entry, |
960 | # nor will it end the interaction on exit. |
961 | - base = WorkingBase(self.email, transaction.manager, self.logger) |
962 | + base = WorkingBase() |
963 | + base.init(self.email, transaction.manager, self.logger) |
964 | endInteraction() |
965 | self.assertIs(None, queryInteraction()) |
966 | with base.interaction: |
967 | @@ -74,7 +76,8 @@ |
968 | # transaction is in progress on entry, commits on a successful |
969 | # exit, or aborts the transaction on failure. |
970 | transaction_manager = StubTransactionManager() |
971 | - base = WorkingBase(self.email, transaction_manager, self.logger) |
972 | + base = WorkingBase() |
973 | + base.init(self.email, transaction_manager, self.logger) |
974 | transaction.commit() |
975 | with base.transaction: |
976 | self.assertFalse(is_transaction_in_progress()) |
977 | @@ -88,7 +91,8 @@ |
978 | # On entry, WorkingBase.transaction will raise an exception if |
979 | # a transaction is in progress. |
980 | transaction_manager = StubTransactionManager() |
981 | - base = WorkingBase(self.email, transaction_manager, self.logger) |
982 | + base = WorkingBase() |
983 | + base.init(self.email, transaction_manager, self.logger) |
984 | self.assertTrue(is_transaction_in_progress()) |
985 | self.assertRaises(TransactionInProgress, base.transaction.__enter__) |
986 | |
987 | @@ -97,7 +101,8 @@ |
988 | # manager is active, the transaction will be aborted and the |
989 | # exception re-raised. |
990 | transaction_manager = StubTransactionManager() |
991 | - base = WorkingBase(self.email, transaction_manager, self.logger) |
992 | + base = WorkingBase() |
993 | + base.init(self.email, transaction_manager, self.logger) |
994 | transaction.commit() |
995 | try: |
996 | with base.transaction: |
997 | @@ -111,7 +116,8 @@ |
998 | def test_statement_logging(self): |
999 | # The WorkingBase.statement_logging context manager starts |
1000 | # statement logging on entry and stops it on exit. |
1001 | - base = WorkingBase(self.email, transaction.manager, self.logger) |
1002 | + base = WorkingBase() |
1003 | + base.init(self.email, transaction.manager, self.logger) |
1004 | self.factory.makeEmail('numpty1@example.com', self.person) |
1005 | self.assertEqual( |
1006 | 0, len(get_request_statements()), |
1007 | @@ -130,6 +136,13 @@ |
1008 | "SQL statement log not cleared on exit " |
1009 | "from base.statement_logging.") |
1010 | |
1011 | + def test_initFromParent(self): |
1012 | + base1 = WorkingBase() |
1013 | + base1.init(self.email, transaction.manager, self.logger) |
1014 | + base2 = WorkingBase() |
1015 | + base2.initFromParent(base1) |
1016 | + self.failUnlessEqual(base1.__dict__, base2.__dict__) |
1017 | + |
1018 | |
1019 | class TestWorkingBaseErrorReporting(TestCaseWithFactory): |
1020 | |
1021 | @@ -140,7 +153,8 @@ |
1022 | person = self.factory.makePerson() |
1023 | email = person.preferredemail.email |
1024 | logger = QuietFakeLogger() |
1025 | - base = WorkingBase(email, transaction.manager, logger) |
1026 | + base = WorkingBase() |
1027 | + base.init(email, transaction.manager, logger) |
1028 | with base.statement_logging: |
1029 | self.factory.makeEmail('numpty@example.com', person) |
1030 | self.assertTrue( |
1031 | |
1032 | === added file 'utilities/less-oops.sh' |
1033 | --- utilities/less-oops.sh 1970-01-01 00:00:00 +0000 |
1034 | +++ utilities/less-oops.sh 2010-04-27 07:12:35 +0000 |
1035 | @@ -0,0 +1,9 @@ |
1036 | +#!/bin/bash |
1037 | +# |
1038 | +# Copyright 2009 Canonical Ltd. This software is licensed under the |
1039 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
1040 | +# |
1041 | +# Outputs the contents of the OOPS report for a given OOPS ID. |
1042 | + |
1043 | +OOPS_ID=$1 |
1044 | +grep -rl "$OOPS_ID" /var/tmp/lperr* | xargs less |
You're my hero for taking this on.
This is all good. I've made a few comments, and a bigger suggestion
(including patch) to make the instantiation of BugWatchUpdater less of
a pain (and to preserve the encapsulation that WorkingBase gives).
Gavin.
> === modified file 'lib/lp/ bugs/doc/ externalbugtrac ker-comment- imports. txt' bugs/doc/ externalbugtrac ker-comment- imports. txt 2010-04-21 10:30:24 +0000 bugs/doc/ externalbugtrac ker-comment- imports. txt 2010-04-23 09:29:42 +0000 scripts. checkwatches import CheckwatchesMaster ter(LaunchpadZo pelessLayer. txn) commit( ) updater. importBugCommen ts(external_ bugtracker, bug_watch) launchpad. scripts. logger import log scripts. checkwatches. bugwatchupdater import ( ssLayer. txn,
> --- lib/lp/
> +++ lib/lp/
> @@ -85,10 +85,13 @@
> comment-importing ExternalBugTracker instance will result in the three
> comments in the comment_dict being imported into Launchpad.
>
> - >>> from lp.bugs.
> - >>> bugwatch_updater = CheckwatchesMas
> - >>> transaction.
> - >>> bugwatch_
> + >>> from canonical.
> + >>> from lp.bugs.
> + ... BugWatchUpdater)
> + >>> bugwatch_updater = BugWatchUpdater(
> + ... '<email address hidden>', LaunchpadZopele
I think it's confusing to use LaunchpadZopele ssLayer. txn in some tionManager actually manager; the statement_logging
places and transaction in others. ZopelessTransac
just calls through to transaction, so it's easier to just use
transaction everywhere (or transaction.
context manager needs the real manager). Also, I irrationally loathe
the "txn" spelling, but that should have no bearing on this review :)
'<email address hidden>' could be obtained from scripts. checkwatches. core.LOGIN, but it's not necessary.
lp.bugs.
> + ... log, bug_watch, external_ bugtracker) updater. importBugCommen ts() commit( ) updater. importBugCommen ts(external_ bugtracker, bug_watch) updater. importBugCommen ts() commit( ) updater. importBugCommen ts(external_ bugtracker, bug_watch) updater. importBugCommen ts() -1].owner. name commit( ) updater. importBugCommen ts(external_ bugtracker, bug_watch) updater. importBugCommen ts() -1].owner. name commit( ) updater. importBugCommen ts(external_ bugtracker, bug_watch) updater. importBugCommen ts()
> + >>> bugwatch_
> INFO:...:Imported 3 comments for remote bug 123456 on ...
>
> These three comments will be linked to the bug watch from which they
> @@ -118,7 +121,7 @@
>
> >>> transaction.
>
> - >>> bugwatch_
> + >>> bugwatch_
> INFO:...:Imported 1 comments for remote bug 123456 on ...
>
> Once again, the newly-imported comment will be linked to the bug watch
> @@ -169,7 +172,7 @@
>
> >>> transaction.
>
> - >>> bugwatch_
> + >>> bugwatch_
> INFO:...:Imported 1 comments for remote bug 123456 on ...
>
> >>> bug.messages[
> @@ -187,7 +190,7 @@
>
> >>> transaction.
>
> - >>> bugwatch_
> + >>> bugwatch_
> INFO:...:Imported 1 comments for remote bug 123456 on ...
>
> >>> bug.messages[
> @@ -212,7 +215,7 @@
>
> >>> transaction.
>
> - >>> bugwatch_
> + >>> bugwatch_
> WARNING:...:Unable to import remote comment author. No email address
> or display name found. (OOPS-...)
> INFO:...:Impor...