stephenmcd / mezzanine

CMS framework for Django
http://mezzanine.jupo.org
BSD 2-Clause "Simplified" License
4.76k stars 1.65k forks source link

Problem with ADMIN_REMOVAL and mezzanine.boot.lazy_admin.lazy_registration #1313

Closed jammon closed 9 years ago

jammon commented 9 years ago

I have problems with making ADMIN_REMOVAL work. It looks like this results from mezzanine.boot.lazy_admin.lazy_registration being called before the code in mezzanine/urls.py that processes ADMIN_REMOVAL. I started this as a question on stackoverflow, which I copy here:

In my new mezzanine site I want to remove some models from the admin menu. So I defined ADMIN_MENU_ORDER with the models I want to see in the admin menu. This results in the other models, that are not in the list (BlogPost, ThreadedComment, Site, Redirect), being listed below my desired models.

So I used this to remove them:

ADMIN_REMOVAL = ('blog.BlogPost',
                 'generic.ThreadedComment',
                 'sites.Site',
                 'redirects.Redirect',)

(I simply cut and pasted from the default ADMIN_MENU_ORDER.) But this had no effect because of an ImportError in mezzanine.urls (line 19-31):

# Remove unwanted models from the admin that are installed by default with
# third-party apps.
for model in settings.ADMIN_REMOVAL:
    try:
        model = tuple(model.rsplit(".", 1))
        exec("from %s import %s" % model)
    except ImportError:
        pass
    else:
        try:
            admin.site.unregister(eval(model[1]))
        except NotRegistered:
            pass

So I changed ADMIN_REMOVAL like this:

ADMIN_REMOVAL = ('blog.models.BlogPost',
                 'generic.models.ThreadedComment',
                 'django.contrib.sites.models.Site',
                 'django.contrib.redirects.models.Redirect',)

But this still has no effect although there is no ImportError or NotRegistered in this code lines.

Then I did some debugging. The line

            admin.site.unregister(eval(model[1]))

from mezzanine/urls.py (cited above) actually calls mezzanine.boot.lazy_admin.unregister, which looks like this:

def unregister(self, *args, **kwargs):
    self._deferred.append(("unregister", args, kwargs))

It seems like after the boot process mezzanine.boot.lazy_admin.lazy_registration should be called, to apply all the deferred registering and unregistering:

def lazy_registration(self):
    for name, deferred_args, deferred_kwargs in self._deferred:
        getattr(AdminSite, name)(self, *deferred_args, **deferred_kwargs)

But actually mezzanine.boot.lazy_admin.lazy_registration is called before the code in mezzanine/urls.py, that tries to unregister the models from ADMIN_REMOVAL.

(I added print statements like this and got the following output:)

mezzanine/urls.py

for model in settings.ADMIN_REMOVAL:
    [...]
        try:
            print 'unregistering', model[1]
            admin.site.unregister(eval(model[1]))
        except NotRegistered:
            pass

mezzanine/boot/lazy_admin.py

def lazy_registration(self):
    print 'lazy_registration occurs'
    for name, deferred_args, deferred_kwargs in self._deferred:
        getattr(AdminSite, name)(self, *deferred_args, **deferred_kwargs)

Output

lazy_registration occurs
unregistering BlogPost
unregistering ThreadedComment
unregistering Site
unregistering Redirect
jammon commented 9 years ago

When there is an ADMIN_MENU_ORDER that somehow defines the admin menu, is it reasonable that mezzanine adds all the registered models/modeladmins after the given menu order, that are not included there in the first place? Maybe make it configurable?

jerivas commented 9 years ago

Perhaps Mezzanine's default ModelAdmins should all support the in_menu() method so ADMIN_MENU_ORDER is the single point of configuration regarding admin menus. This would only hide them from the menu (as opposed to actually unregistering them), so a particularly knowledgeable user could find them by URL. Would this be considered an issue?

We could come up with a mixin class with a smart in_menu() method that looks for it's own concrete class in ADMIN_MENU_ORDER, so any ModelAdmin can inherit from it and get that capability for free, and with the option to override it, as PageAdmin does.

Kniyl commented 9 years ago

A quick fix, if you want it to work right away, is to add admin.site.lazy_registration() at line 32 of mezzanine/urls.py like so:

# Remove unwanted models from the admin that are installed by default with
# third-party apps.
for model in settings.ADMIN_REMOVAL:
    try:
        model = tuple(model.rsplit(".", 1))
        exec("from %s import %s" % model)
    except ImportError:
        pass
    else:
        try:
            admin.site.unregister(eval(model[1]))
        except NotRegistered:
            pass
        admin.site.lazy_registration()

For a proper fix… the mixin with a custom in_menu seems great, but what about models that are not part of mezzanine? Such as django.contrib.sites.models.Site, or django.contrib.redirects.models.Redirect that @jammon is trying to remove?

stephenmcd commented 9 years ago

I've a fix incoming that just moves the ADMIN_REMOVAL handling from Mezzanine's internal urls.py, to inside Mezzanine's LazyAdminSite, where we can directly control the order of what goes on. Seems to resolve it.

BTW this setting has been in Mezzanine from the very start - it's a thing I used to by default on all my Django projects back then. I think originally the code would live inside a project's urls.py module, and get called directly after admin.site.autodiscover() in the same file - which meant the ordering was correct. I guess that must have fallen by the wayside as the project got larger and refactored over the years.

jammon commented 9 years ago

Great, thanks!

sjkingo commented 9 years ago

I've been trying to use this setting myself, and it doesn't appear to be working. I have tried a clean project with Mezzanine 4.0.1 and while the code is called in mezzanine/boot/lazy_admin.py, the AdminSite.unregister call doesn't appear to do anything (the model is still in the left-hand navigation).

Stephen, are you able to verify you're getting the same behaviour?

mathisonian commented 9 years ago

fwiw I was able to get this to work by using the fully qualified name

ADMIN_REMOVAL = ('mezzanine.blog.models.BlogPost',)

instead of ADMIN_REMOVAL = ('blog.BlogPost', ) or ADMIN_REMOVAL = ('blog.models.BlogPost',)

dszmaj commented 8 years ago

@mathisonian +1 issue still do not resolved

jpmelos commented 8 years ago

This is not working @stephenmcd, we have to use the fully qualified name still.

stephenmcd commented 8 years ago

Using the full import path has always been the case - I've clarified in the docs in 125ed890b1676ffb9a97c1db1bafa358d0e447e1

skabbit commented 6 years ago

Not working in 4.2.3 for django.contrib.auth.models.Group or other non-mezzannine models. Here is the solution for it in lazy_admin.py (I just changed blocks in places):

    def lazy_registration(self):
        # Pick up any admin classes registered via decorator to the
        # default admin site.
        for model, admin in default_site._registry.items():
            self._deferred.append(("register", (model, admin.__class__), {}))
        # Call register/unregister.
        for name, args, kwargs in self._deferred:
            try:
                getattr(AdminSite, name)(self, *args, **kwargs)
            except (AlreadyRegistered, NotRegistered):
                pass

        # First, directly handle models we don't want at all,
        # as per the ``ADMIN_REMOVAL`` setting.
        for model in getattr(settings, "ADMIN_REMOVAL", []):
            try:
                model = tuple(model.rsplit(".", 1))
                exec("from %s import %s" % model)
            except ImportError:
                pass
            else:
                try:
                    AdminSite.unregister(self, eval(model[1]))
                except NotRegistered:
                    pass
tiktuk commented 6 years ago

Thanks @skabbit, created a pr with your fix in #1867.