Summary

Added support for extension admin sites

Review Request #2888 — Created Feb. 16, 2012 and submitted March 10, 2012, 4:37 p.m.

Information

Owner

smacleod

Repository

Djblets

Branch

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

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

Testing Done

Tested by creating admin site for rbwebhooks extension.

Screenshots

Extension list showing extension with admin site

Extension admin site with a single simple model

Issues

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 …	chipx86	Feb. 20, 2012, 7:07 a.m.
Blank line between these.	chipx86	Feb. 18, 2012, 6:27 a.m.
"Creates"	chipx86	Feb. 18, 2012, 5:53 a.m.
Period after "admin site."	chipx86	Feb. 18, 2012, 5:53 a.m.
Period at the end.	chipx86	Feb. 18, 2012, 5:54 a.m.
Comments should always be in sentence format. Casing and period.	chipx86	Feb. 18, 2012, 6:28 a.m.
You only use this in the exception case, so you probably don't want to import it yet.	chipx86	Feb. 18, 2012, 6:29 a.m.
I don't know that I'd bubble up an ImportError. That can be caused by any number of things. Rather, this …	chipx86	Feb. 20, 2012, 7:17 a.m.
You don't need this. It's implicit.	chipx86	March 10, 2012, 7:46 a.m.
Do we want to catch everything here? Seems like it's misleading to eventually raise an ImportError if this is, say, …	chipx86	March 10, 2012, 7:48 a.m.
Blank line between these.	chipx86	March 10, 2012, 7:49 a.m.
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_conley	March 10, 2012, 1:40 a.m.

djblets/extensions/base.py (Diff revision 1)

The issue has been resolved. 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.

smacleod Feb. 18, 2012, 6:30 a.m.

As we discussed in IRC, I'll fix this, and map the admin site to it's own url such as /admin/

djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Creates"
```
1. SM
  
  smacleod Feb. 18, 2012, 6:30 a.m.
  embarrassing :P
djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Period after "admin site."
```
djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Period at the end.
```
djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Comments should always be in sentence format. Casing and period.
```

djblets/extensions/base.py (Diff revision 1)

The issue has been resolved. Show all issues

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

djblets/extensions/base.py (Diff revision 1)

The issue has been resolved. 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.

smacleod Feb. 18, 2012, 6:30 a.m.

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?

chipx86 Feb. 18, 2012, 6:56 a.m.

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.

smacleod Feb. 18, 2012, 7:03 a.m.

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.

Change Summary:

Updated so that admin sites can be created with is_configurable = False. Fixed a bug where models were not being re-registered when the extensions was disabled and then re-enabled. Updated description to reflect changes.

Description:

		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
	~	1. extension.has_admin_site = True
-		2. extension.is_configurable = True (required for admin_urls.py to be loaded, this seems to make sense, but I'm open to input on alternatives)

~		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)
	~	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)
-		a url pattern must be added to admin_urls.py ex: url(r'^db/', include(extension.admin_site.urls) #adds the admin site at prefix/config/db

~		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. If anyone has ideas on how to fix this, I would appreciate input.
	~	The admin site will be generated and a link Database link will appear on the extension list.

~		Note: this change has a parent corresponding to /r/2886
	~	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).

Diff:

Revision 2 (+79 -15)

Show changes

	djblets/extensions/base.py
	djblets/extensions/templates/extensions/extension_list.html

Change Summary:

Updated description to fix link

Description:

		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

		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).
	~	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

Change Summary:

Added screenshots.

Screenshots:

+	Extension list showing extension with admin site
+	Extension admin site with a single simple model

Hey Steven,

The code looks fine to me - just one question and a possible issue below.

Thanks,

-Mike

djblets/extensions/base.py (Diff revision 2)

Just curious, why did we remove the conditional checking if urlconf has the attribute "urlpatterns"?

smacleod March 8, 2012, 11:50 a.m.

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.

mike_conley March 9, 2012, 11:22 a.m.
```
Gotcha, thanks for clearing that up.
```

djblets/extensions/base.py (Diff revision 2)

The issue has been dropped. 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?

smacleod March 8, 2012, 11:50 a.m.

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.

mike_conley March 9, 2012, 11:22 a.m.

Ah, good point. Feel free to drop this issue.

Ship it!

This looks good to me.

Good work, Steven.

djblets/extensions/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
You don't need this. It's implicit.
```

djblets/extensions/base.py (Diff revision 2)

The issue has been resolved. 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.

djblets/extensions/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Change Summary:

Updated based on Christian's review.

Diff:

Revision 3 (+79 -15)

Show changes

	djblets/extensions/base.py
	djblets/extensions/templates/extensions/extension_list.html

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (a91dce5)