Hey Steven,

This is a really good start.  I think you're on the right path.  I've avoided doing any stylistic criticism, and instead, focused on some higher-level architectural ideas / issues.

For the stylistic business - you'd benefit from running these scripts through the pep8 script.  That'll clear up a ton of them.

-Mike

What is this global used for?

I like how the connection / dispatching of signals is separated into its own class here.

I brought up my concern about this being a blocking function in IRC.  I think we're going to just accept that for now.  Later down the line, we might have a proxy function that we can fork out so that RB doesn't block while sending this request...but I think that can wait for now.

Hm - you're not just returning the default, you're also *writing* the default setting so that it's no longer a default.

Is this intentional?  If so, why?

Maybe this should be connected to the SignalHandlers class?  So that we can define new signals in a single place?

If I'm not mistaken, verify_exists defaults to False, and is going to be deprecated in Django 1.5 (https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.URLField)

So I think you can just have:

url = models.URLField()

This review request is missing your __init__.py, which, I presume, is because it is blank. (this is more of a heads-up for those who might wish to try this locally)

Just being nitpicky here, but shouldn't it be more like (empty lines are relevant):

imports of urllib and urllib2

django imports

rbwebhooks imports

reviewboard imports

Another nitpick, but would this make more sense as the singular "_get_setting()"?

Extra line?

I realize this might be more in the vein of giving people enough rope to hang themselves, but would 1 make a better min value?

Instead of using a global extension variable, I'm wondering if it would make sense to go through the ExtensionManager? i.e.,

from reviewboard.extensions.base import get_extension_manager 

    extension_manager = get_extension_manager()                                    
    extension = extension_manager.get_enabled_extension(RBWebHooksExtension.id) 

This continues to look good.

We'll need to make sure this gets taken out.

Also, it might be a good idea to try hooking into Review Board's logging mechanism for this sort of thing.

Do we go over 80 chars if we put these all on line 31?  It just looks a bit odd broken up like this.

Hey Steven,

So I downloaded the patch for this and started fiddling with it, and I'm not seeing the admin site for WebHooks showing up in the admin interface.  What am I doing wrong?

-Mike

Hey Steven,

Just a few minor comments, but overall this looks pretty good.

-Mike

Why the extra newlines on line 11 / 14?

This comment can probably be more clear.

"Get the enabled RBWebHooksExtension instance (since we can assume if we've reached this code branch that the extension is enabled), and register WebHookTargetAdmin with the admin site."

Something like that, perhaps.

nit: I think this needs to be indented one more space, so that the curly braces line up.

This comment can probably be fleshed out a bit, explaining what this extension does.

This comment is probably superfluous.

nit: Maybe just put that last parenthesis on the same line as the last argument?  I guess I don't really like that last parenthesis hanging out in space.  :p

So I guess this defaults to POST?

If we failed, we should probably log that we failed.

Does putting this on line 6 put us over 80 chars?

I can't find anything wrong with this.

Thumbs up from me.

-Mike

Small coding style issues, but otherwise this looks good! Well done.

Yazan Medanat

Space here.

This could be:

while attempts:
    try:
        return urlib2.urlopen(request) 
    except ...:
        attempts -= 1

A strict return False may not be needed, as a failed urlopen will return None.

is_new = changedesc is None

if not is_new:
    ...

One nit, good job!

Space here.