Define django_loaded signal so we can properly connect our own.

Review Request #968 — Created Aug. 16, 2009 and submitted

Information

Review Board SVN (deprecated)

Reviewers

Email notification wasn't getting properly connected to the
publish signals because the file wasn't loaded. And
connecting them in __init__.py breaks because django isn't
loaded yet.

With the django_loaded signal, we can delay connecting signals
until we're sure all we need is there.
All tests pass.
chipx86
  1. 
      
  2. /trunk/reviewboard/signals.py (Diff revision 1)
     
     
    I think I'd prefer to say this is when Review Board is "initialized" rather than "Django is loaded." These are two separate things. So perhaps just rename this to "initialized".
    1. Hm... It also doesn't mean Review Board is initialized in the full sense, because we use the signal to do part of the initialization (loading modules and binding signals). Maybe it's better to connect the signals explicitly in urls.py, like with logging:
      
          # urls.py
          from reviewboard import notifications
      
          notifications.connect_signals()
      
      In the future, connect_signals() can take parameters like 'email', 'webhooks', or get its configuration from siteconfig.
    2. I'd really like to get all the init stuff in urls.py out of there, really. It's been on my todo list for a while.
      
      I know it's not *exactly* initialized, but it's close enough. Maybe initializing is more correct.
  3. 
      
EH
  1. FYI, Python does not necessarily guarantee that a module is imported only once.  This can cause signals to be registered multiple times.  If you include the dispatch_uid parameter with a unique string in the connect method, that should avoid the problem.  
    
    See http://code.djangoproject.com/wiki/Signals and http://code.djangoproject.com/ticket/3951.
    1. That's why, in ReviewBoard, we have a canonical way of importing modules, using the full path relative to the root site. This way we guarantee that the module will be imported only once.
      
      If you look at the bug report, that is mentioned:
      
      >>> from testproj.testapp.models import test_function_A as full_path_A_function
      >>> from testapp.models import test_function_A as short_path_A_function
      
      >>> id(full_path_A_function), id(short_path_A_function)
      (18898800, 18960624)
      
      I think this will be a non-issue, since this project already have importing path figured out.
  2. 
      
HE
Review request changed
chipx86
  1. Thanks! Committed in r2112.
    1. Christian,
      
      It looks like you forgot to svn add reviewboard/signals.py.
      
      Nathan
    2. Oops... Fixed.
  2. 
      
Loading...