Added support for extension admin sites

Review Request #2888 — Created Feb. 16, 2012 and submitted

Information

Djblets

Reviewers

This adds support for extensions to have their own admin site by setting extension.has_admin_site = True.

In order to add an admin site to an extension, the following steps must be taken:
  1. extension.has_admin_site = True

  2. Models registered in admin.py
    - ex:
      extension_manager = get_extension_manager()
      extension = extension_manager.get_enabled_extension(ExtensionClassName.id)
      extension.admin_site.register(ModelName, ModelAdminName)

The admin site will be generated and a link Database link will appear on the extension list.

Currently there is a bug due to interaction between action logs from the main admin site, and the extension admin sites. Since the models for Review Board are not registered with the extension admin site, and vice versa, clicking a recent action on the admin site page which originated from the other admin site will link to an invalid url. This is a django bug, and I have filed an issue: https://code.djangoproject.com/ticket/17726
Tested by creating admin site for rbwebhooks extension.
Description From Last Updated

I don't know that I agree that is_configurable must be set. I can imagine extensions that don't need any configuration …

chipx86chipx86

Blank line between these.

chipx86chipx86

"Creates"

chipx86chipx86

Period after "admin site."

chipx86chipx86

Period at the end.

chipx86chipx86

Comments should always be in sentence format. Casing and period.

chipx86chipx86

You only use this in the exception case, so you probably don't want to import it yet.

chipx86chipx86

I don't know that I'd bubble up an ImportError. That can be caused by any number of things. Rather, this …

chipx86chipx86

You don't need this. It's implicit.

chipx86chipx86

Do we want to catch everything here? Seems like it's misleading to eventually raise an ImportError if this is, say, …

chipx86chipx86

Blank line between these.

chipx86chipx86

Hm...so, if I understand this correctly, if I've set up my Extension incorrectly, and had has_admin_site set to true, but …

mike_conleymike_conley
chipx86
  1. 
      
  2. djblets/extensions/base.py (Diff revision 1)
     
     
     
     
    Show all issues
    I don't know that I agree that is_configurable must be set. I can imagine extensions that don't need any configuration to do anything, and thus won't need to present a config UI to the user, but an administration UI may be useful for tweaking stored data or performing extra queries, or just debugging.
    1. As we discussed in IRC, I'll fix this, and map the admin site to it's own url such as /admin/
  3. djblets/extensions/base.py (Diff revision 1)
     
     
     
    Show all issues
    Blank line between these.
  4. djblets/extensions/base.py (Diff revision 1)
     
     
    Show all issues
    "Creates"
  5. djblets/extensions/base.py (Diff revision 1)
     
     
    Show all issues
    Period after "admin site."
  6. djblets/extensions/base.py (Diff revision 1)
     
     
    Show all issues
    Period at the end.
  7. djblets/extensions/base.py (Diff revision 1)
     
     
    Show all issues
    Comments should always be in sentence format. Casing and period.
  8. djblets/extensions/base.py (Diff revision 1)
     
     
    Show all issues
    You only use this in the exception case, so you probably don't want to import it yet.
  9. djblets/extensions/base.py (Diff revision 1)
     
     
    Show all issues
    I don't know that I'd bubble up an ImportError. That can be caused by any number of things. Rather, this is a configuration error. Django's ImproperlyConfigured error, with message text clearly explaining the problem, would be more appropriate.
    1. I was copying how the django admin module takes care of this in django/contrib/admin/__init__.py - autodiscover(). I only bubble up if the extension has an admin.py, but I still couldn't import it. I don't know why or how this could happen, so I just followed what was done there. Knowing this do you still believe it would be better to change to an ImpoperlyConfigured error?
    2. Yeah, because they wrote the extension wrong. It's saying "Make me an admin interface" without having the right file. So the configuration of that flag is wrong.
      
      Though, this could be looked at another way. It *is* failing to import. So I'm actually okay with it being an ImportError, but I do want it to have a custom message so it's clear what's wrong here.
    3. I think I'm seeing it a little differently. To me it's saying "Make me an admin interface" with the right file existing.
      if module_has_submodule(mod, 'admin'):
                      raise
      So admin.py should exist, but importing it is failing for some reason. I'm not even sure what could cause this.
  10. 
      
SM
SM
SM
mike_conley
  1. Hey Steven,
    
    The code looks fine to me - just one question and a possible issue below.
    
    Thanks,
    
    -Mike
  2. djblets/extensions/base.py (Diff revision 2)
     
     
    Just curious, why did we remove the conditional checking if urlconf has the attribute "urlpatterns"?
    1. It hasn't been removed, just moved down a few lines, and put inside a conditional. This was done since the method now installs the URL for the admin site as well, so the is_configurable check is done inside the method.
    2. Gotcha, thanks for clearing that up.
  3. djblets/extensions/base.py (Diff revision 2)
     
     
     
    Show all issues
    Hm...so, if I understand this correctly, if I've set up my Extension incorrectly, and had has_admin_site set to true, but I don't have an admin module...the error gets swallowed?
    
    I'm not sure I agree with that.  Is there a good reason?
    1. This is just following the convention from Django. It's actually based off the Django code which does this for each app. It is possible that you would register AdminModels elsewhere, and not use the admin.py file.
    2. Ah, good point. Feel free to drop this issue.
  4. 
      
mike_conley
  1. This looks good to me.
    
    Good work, Steven.
    
  2. 
      
chipx86
  1. 
      
  2. djblets/extensions/base.py (Diff revision 2)
     
     
    Show all issues
    You don't need this. It's implicit.
  3. djblets/extensions/base.py (Diff revision 2)
     
     
    Show all issues
    Do we want to catch everything here? Seems like it's misleading to eventually raise an ImportError if this is, say, a SyntaxError or something else.
    
    I'd rather we catch ImportError and let other things fall through.
  4. djblets/extensions/base.py (Diff revision 2)
     
     
     
    Show all issues
    Blank line between these.
  5. 
      
SM
chipx86
  1. Ship It!
  2. 
      
SM
Review request changed
Status:
Completed
Change Summary:
Pushed to master (a91dce5)