openhatch

Issue367

Title Migrate Launchpad bug importer to Twisted
Milestone 0.11.12 Priority feature
Waiting On Status resolved
Superseder Nosy List armooo, paulproteus, pythonian4000
Assigned To armooo Keywords

Created on 2011-04-05.14:04:35 by pythonian4000, last changed 2011-12-22.22:04:04 by paulproteus.

Files
File name Uploaded Type Edit Remove
0001-Create-a-launchpad-bugimporter.patch armooo, 2011-12-21.03:07:54 text/x-patch
0002-Add-Launchpad-form-and-adjust-views-accordingly.patch armooo, 2011-12-21.03:08:03 text/x-patch
0003-Added-bitesized-and-doc-tag-processing.patch armooo, 2011-12-21.03:08:11 text/x-patch
0004-Removed-the-old-launchpad-bugtracker.patch armooo, 2011-12-21.03:08:19 text/x-patch
asheesh-additions.patch paulproteus, 2011-12-06.00:10:12 text/x-patch
launchpad_review2.patch armooo, 2011-12-22.02:43:14 text/x-patch
Messages
msg2837 (view) Author: paulproteus Date: 2011-12-22.22:04:01
I just deployed this.

I made some small additions -- primarily, removing the LaunchpadQueryForm. I
agree that the Model is needed, although I determined the Form is not.

Also the LaunchpadTrackerForm now causes a QueryModel to be created. That way,
the customs_twist.Command.update_trackers code will detect that it should
refresh that Launchpad tracker (as you pointed out).

This is totally great. Thanks!.
msg2832 (view) Author: armooo Date: 2011-12-22.02:43:14
*  It looks like asheesh-additions.patch it making the TrackerQueryModel optional 
from the UI, but customs_twist.Command.update_trackers is based on the last 
polled date of the TrackerQueryModel. I could directly use TrackerQueryModel and 
not LaunchpadQueryModel (if the django ORM is ok with it). Is this your suggestion?

* I split the mysite/customs/bugimporters/base.py changes in to a new commit.

* I removed self.bug_urls, but changed it to a call to process_bugs not 
handle_task_data_json. This way the LaunchpadBug creation stays in one place and 
we have the to call determine_if_finished which seems to be required to stop the 
reactor if no new bugs are found. The other process_bugs methods also take the 
same list of (url, data).
msg2831 (view) Author: paulproteus Date: 2011-12-22.00:59:33
This looks quite good. Just a few nitpicks:

* You don't need a LaunchpadQueryModel at all since you don't use it

* The changes to mysite/customs/bugimporters/base.py should be in their own
commit. (Good changes, though!)

* This delta:

+        # The bug data that show up in bug_collection['entries']
+        # is equivalent to what we get back if we asked for the
+        # data on that bug explicitly.
+        for bug in bug_collection['entries']:
+            self.bug_urls.append((bug['web_link'], bug))

arguably reproduces logic available in:

+    def handle_task_data_json(self, data, lp_bug):

So why don't you just call the handle_task_data_json() method?

In asheesh-additions.patch there are some changes that, effectively, make URL
models optional. I suggest adding those to your patchset.

Also, it's easier to review if you do:

git format-patch --stdout master > entire-series-in-one-file.patch

Can you address those items? Beyond those things, this looks great, and I'm
thrilled that we can likely land this quite soon.
msg2799 (view) Author: paulproteus Date: 2011-12-06.00:20:02
A further thought:

The way we're modeling things now, there is one Launchpad BugTracker per project.

This is a little odd for the following case: if someone adds a bug to their
project, but then within Launchpad the same bug ID is reassigned to some other
project, we might handle things wrong.

It's something we should just make sure to cook up a test for, and be
happy-enough with the results at some point.
msg2797 (view) Author: paulproteus Date: 2011-12-06.00:14:02
Three more notes:

1. I might be wrong about when a call to self.push_urls_onto_reactor() is needed.

2. Feel free to make liberal use of the FakeGetPage() object at the top of
mysite/customs/tests.py. If you use @mock.patch() to patch out the real getPage
with that one, then that will let you download arbitrary URLs (so long as you
save a copy of them somewhere in the repo) during a run of a test.

3. I can totally see how crazy this code is now. Luckily there are some ideas
for cleaning up our use of Twisted, which would de-insane it.

I don't think this is going to be done before 0.11.11, but we've made a lot of
really good progress on it! Marking as "later," but I'm hoping we can land it
for 0.11.12.
msg2796 (view) Author: paulproteus Date: 2011-12-06.00:10:12
Here's the progress of work on this so far:

* armooo has a branch on Gitorious with some work:
https://gitorious.org/~armooo/openhatch/armooos-oh-mainline/commits/launchpad ,
started at the Dec 4 OpenHatch sprint.

* I took a look at it, and have some remarks as well as patches.

In general, you don't have to call self.process_queries() with the Launchpad
BugImporter class; that should be handled by the framework.

I notice the new Launchpad importer has no subclass of "BugParser". This is a
convention; it seems it's okay to not follow it. The most idiomatic use of
BugParser can be seen in bugimporters/bugzilla.py 

I added a test and some sample data and did a little bit of cleanup in the
attached patches. They should apply cleanly on top of the current state of
armooo's gitorious branch; armooo, feel free to push them on top your branch
immediately.
msg2701 (view) Author: paulproteus Date: 2011-11-14.02:27:16
If we do this, we have to write our own consumer of the Launchpad API, I think.

That's life. Let's do it. Marking as 0.11.11.
msg1428 (view) Author: pythonian4000 Date: 2011-04-05.14:04:35
This issue is tracked in issue260.
History
Date User Action Args
2011-12-22 22:04:04paulproteussetstatus: in-progress -> resolved
messages: + msg2837
milestone: later -> 0.11.12
2011-12-22 02:43:19armooosetfiles: + launchpad_review2.patch
messages: + msg2832
2011-12-22 00:59:38paulproteussetstatus: need-review -> in-progress
assignedto: armooo
messages: + msg2831
2011-12-21 03:08:19armooosetstatus: chatting -> need-review
files: + 0004-Removed-the-old-launchpad-bugtracker.patch
2011-12-21 03:08:11armooosetfiles: + 0003-Added-bitesized-and-doc-tag-processing.patch
2011-12-21 03:08:03armooosetfiles: + 0002-Add-Launchpad-form-and-adjust-views-accordingly.patch
2011-12-21 03:07:54armooosetfiles: + 0001-Create-a-launchpad-bugimporter.patch
2011-12-06 00:20:02paulproteussetmessages: + msg2799
2011-12-06 00:16:53paulproteusunlinkissue260 blockers
2011-12-06 00:14:07paulproteussetmilestone: 0.11.11 -> later
2011-12-06 00:14:02paulproteussetmessages: + msg2797
2011-12-06 00:10:13paulproteussetfiles: + asheesh-additions.patch
nosy: + armooo
messages: + msg2796
2011-11-14 02:27:17paulproteussetstatus: unread -> chatting
messages: + msg2701
milestone: 0.11.11
2011-04-05 14:04:54pythonian4000linkissue260 blockers
2011-04-05 14:04:35pythonian4000create