WebHooks Extension

Review Request #2855 — Created Feb. 10, 2012 and submitted

Information

rb-extension-pack

Reviewers

The start of a WebHooks extension.

Currently configuration for URL targets is in the Admin Site for the extension. To modify the targets, click the Database button on the extension list.

Only one hook has been implemented so far.
Tested on local dev by targeting a local instance of ReviewBot
Description From Last Updated

Just being nitpicky here, but shouldn't it be more like (empty lines are relevant): imports of urllib and urllib2 django …

AM ammok

What is this global used for?

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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 …

mike_conleymike_conley

We'll need to make sure this gets taken out. Also, it might be a good idea to try hooking into …

mike_conleymike_conley

Do we go over 80 chars if we put these all on line 31? It just looks a bit odd …

mike_conleymike_conley

Why the extra newlines on line 11 / 14?

mike_conleymike_conley

This comment can probably be more clear. "Get the enabled RBWebHooksExtension instance (since we can assume if we've reached this …

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

This comment is probably superfluous.

mike_conleymike_conley

nit: Maybe just put that last parenthesis on the same line as the last argument? I guess I don't really …

mike_conleymike_conley

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

mike_conleymike_conley

Space here.

ME medanat

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

ME medanat

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

ME medanat

is_new = changedesc is None if not is_new: ...

ME medanat

Space here.

ME medanat
mike_conley
  1. 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
    1. Thanks, it's good to know I wasn't completely off the mark!
      
      I actually did run all the code through PEP8, and didn't find any style errors... Was your style comment a suggestion, and you didn't actually find errors, or am I doing something wrong with PEP8?
  2. rbwebhooks/rbwebhooks/extension.py (Diff revision 1)
     
     
    What is this global used for?
    1. I was using it to access the extension from other modules. It's not really necessary any more due to other changes. If needed in the future, I'll use what Anthony suggested in his review.
  3. rbwebhooks/rbwebhooks/extension.py (Diff revision 1)
     
     
    I like how the connection / dispatching of signals is separated into its own class here.
    1. Yay! for modularization
  4. rbwebhooks/rbwebhooks/extension.py (Diff revision 1)
     
     
    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.
  5. rbwebhooks/rbwebhooks/extension.py (Diff revision 1)
     
     
     
     
    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?
    1. The reason behind this is: When an extension is first run, it has an empty settings dictionary. Unless someone configures the extension, settings won't exist, and I'll get exceptions when trying to access them (This could be a problem since you have to enable an extension before you can configure it).
      
      That is why if I detect a missing key (Only when the extension has not been configured), I'll write the default value, and pretend it was there all along.
      
      I'm not really happy with this method. I'll probably switch to something which defaults the empty settings as soon as the extension is loaded. I'd love suggestions if you can think of a good way to do this, or something better. It would be nice if the extension system had something built in.
  6. rbwebhooks/rbwebhooks/models.py (Diff revision 1)
     
     
     
     
    Maybe this should be connected to the SignalHandlers class?  So that we can define new signals in a single place?
    1. Moved to SignalHandlers. I had planned to do that but it slipped my mind before I posted, thanks for catching that.
  7. rbwebhooks/rbwebhooks/models.py (Diff revision 1)
     
     
    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()
    1. Fixing. Nice catch, I didn't notice that.
  8. 
      
AM
  1. 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)
    1. Didn't even realize that. I might stick a doc string in there to avoid this.
  2. rbwebhooks/rbwebhooks/extension.py (Diff revision 1)
     
     
    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
    1. Thanks for bringing this up. The imports do need to be cleaned up.
      
      PEP8 specifies
      
      1. standard library imports
      2. related third party imports
      3. local application/library specific imports
      
      From that I feel like I should go
      1.urllib urllib2
      2.django
      3.reviewboard
      4.rbwebhooks
      
      Does that make sense?
  3. rbwebhooks/rbwebhooks/extension.py (Diff revision 1)
     
     
    Another nitpick, but would this make more sense as the singular "_get_setting()"?
  4. rbwebhooks/rbwebhooks/forms.py (Diff revision 1)
     
     
    Extra line?
  5. rbwebhooks/rbwebhooks/forms.py (Diff revision 1)
     
     
    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?
    1. I wanted a quick way to be able to turn off actually sending the requests without disabling the extension. After the configuration is more developed I might bump this up.
  6. rbwebhooks/rbwebhooks/views.py (Diff revision 1)
     
     
    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) 
    1. Thanks for the tip, I will move away from the global.
  7. 
      
SM
SM
mike_conley
  1. This continues to look good.
  2. rbwebhooks/rbwebhooks/extension.py (Diff revision 2)
     
     
    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.
  3. rbwebhooks/rbwebhooks/handlers.py (Diff revision 2)
     
     
     
     
    Do we go over 80 chars if we put these all on line 31?  It just looks a bit odd broken up like this.
  4. 
      
SM
mike_conley
  1. 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
    1. At the moment you need to go to the Extensions list page in the Review Board admin panel, and then click database on the RBWebHooks extension.
  2. 
      
mike_conley
  1. Hey Steven,
    
    Just a few minor comments, but overall this looks pretty good.
    
    -Mike
  2. rbwebhooks/rbwebhooks/admin.py (Diff revision 3)
     
     
    Why the extra newlines on line 11 / 14?
  3. rbwebhooks/rbwebhooks/admin.py (Diff revision 3)
     
     
    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.
  4. rbwebhooks/rbwebhooks/admin_urls.py (Diff revision 3)
     
     
    nit: I think this needs to be indented one more space, so that the curly braces line up.
  5. rbwebhooks/rbwebhooks/extension.py (Diff revision 3)
     
     
    This comment can probably be fleshed out a bit, explaining what this extension does.
  6. rbwebhooks/rbwebhooks/extension.py (Diff revision 3)
     
     
    This comment is probably superfluous.
    1. Looks like a code place holder I never removed.
  7. rbwebhooks/rbwebhooks/extension.py (Diff revision 3)
     
     
    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
  8. rbwebhooks/rbwebhooks/extension.py (Diff revision 3)
     
     
    So I guess this defaults to POST?
    1. It will actually choose GET by default, but later when I call request.add_data(arguments) it automatically switches the request to POST. Maybe I'll put a comment in to make that clear.
  9. rbwebhooks/rbwebhooks/extension.py (Diff revision 3)
     
     
    If we failed, we should probably log that we failed.
  10. rbwebhooks/rbwebhooks/views.py (Diff revision 3)
     
     
    Does putting this on line 6 put us over 80 chars?
    1. Nope, Fixed :D. Actually I've decided to nuke the file, since it is unused :|
  11. 
      
SM
SM
mike_conley
  1. I can't find anything wrong with this.
    
    Thumbs up from me.
    
    -Mike
  2. 
      
ME
  1. Small coding style issues, but otherwise this looks good! Well done.
    
    Yazan Medanat
  2. rbwebhooks/rbwebhooks/extension.py (Diff revision 4)
     
     
     
    Space here.
  3. rbwebhooks/rbwebhooks/extension.py (Diff revision 4)
     
     
     
     
     
     
     
    This could be:
    
    while attempts:
        try:
            return urlib2.urlopen(request) 
        except ...:
            attempts -= 1
  4. rbwebhooks/rbwebhooks/extension.py (Diff revision 4)
     
     
    A strict return False may not be needed, as a failed urlopen will return None.
    1. Not exactly true. If urlopen fails, it will except, and we won't return. I'll switch this to None now that I'm returning the result which makes more sense.
  5. rbwebhooks/rbwebhooks/handlers.py (Diff revision 4)
     
     
     
     
     
     
    is_new = changedesc is None
    
    if not is_new:
        ...
  6. 
      
SM
ME
  1. One nit, good job!
  2. rbwebhooks/rbwebhooks/handlers.py (Diff revision 5)
     
     
     
    Space here.
  3. 
      
SM
SM
Review request changed

Status: Closed (submitted)

Change Summary:

Committed as 83c0ecfb. Thanks for your work!
Loading...