mezzanine

mezzanine copied to clipboard

Add a lazy function that retrieves all models for a given app, identified by it's app_label. This makes it possible to include all models for an app in ADMIN_MENU_ORDER setting, so that it's at the right spot and no models are "missing" (moved down) when upgrading the dependency.

Usage:

ADMIN_MENU_ORDER = (
(
    "Content", (
        "pages.Page",
        "blog.BlogPost",
        "generic.ThreadedComment",
        (_("Media Library"), "media-library"),)
),
# ... more here ...
(
    _("Social Accounts"), get_models_for_app('socialaccount')
),
# ... more here ...

A tuple of classes can also be returned, which may or may not prove to be useful in the future.

Will do docs if feature accepted. Not sure where to put tests. mezzanine.conf.tests deals with injecting settings, not with settings in general and mezzanine.utils.tests only provides the base test case.

P.S. I've considered changing ADMIN_MENU_ORDER parsing to allow for second element being an app_label, but this would break existing code based on blog.admin.BlogCategoryAdmin.has_module_permission.

Why does this need to be in Mezzanine?

To have a standardized way of organizing your menu. It becomes tedious and a maintenance hassle to look up all models of an app if your INSTALLED_APPS is anything of reasonable size.

Sure, it's easy to add this code to a project in it's own module, but the same can be said of ADMIN_MENU_ORDER itself or the bit to import local settings. So basically boils down to "batteries included".

I'm sorry I didn't understand the use-case, I actually missed the example you posted.

Looking at it properly now, I think it's a good idea but the implementation isn't right. The developer shouldn't need to import a function to apply it to the setting. What if the value for the sequence of models could also just be a string that contains the name of an app, and that would be the interface for achieving what you're trying to do here, so:

ADMIN_MENU_ORDER = (
(
    "Content", (
        "pages.Page",
        "blog.BlogPost",
        "generic.ThreadedComment",
        (_("Media Library"), "media-library"),)
),
# ... more here ...
(
    _("Social Accounts"), 'socialaccount'
),
# ... more here ...

I think that would be a much simpler API for the developer, and it would remove the need for any kind of lazy loading, since the code for dealing with this could occur at runtime (not import time) in the same spot that deals with the ADMIN_MENU_ORDER setting.

What do you think?

Except for this:

P.S. I've considered changing ADMIN_MENU_ORDER parsing to allow for second element being an app_label, but this would break existing code based on blog.admin.BlogCategoryAdmin.has_module_permission.

I have no problem with it. And yes, it makes things a lot easier implementation wise.

I'm really sorry, I keep rushing and not reading everything entirely.

I guess we could tweak blog.admin.BlogCategoryAdmin to do different checks based on whether the value is a string or not, right?

Yes, we could. It's just that it's mentioned in the documentation as an example. So it's logical (and I'm one of them) to take that code and copy it for apps of your own. Incompatibilities between versions are the life of software, but I saw a way to do it without disrupting this, so I went for it. I'm happy to change the implementation as suggested.

Ultimately, that's up to you to decide and factor in what else breaks in the next release.

Don't worry about skipping things. We all have other things in our lives and I know it's not intentional.

As for implementation, I think it's easier to have one "unwrapping" method that remembers state for ADMIN_MENU_ORDER.

That keeps code that uses it for what it's made for deal with actual data and not references it needs to resolve. So settings.ADMIN_MENU_ORDER would be what the administrator/developer configured and unwrap_admin_menu_order() would return the version with all references resolved, which caches result and only does the work once.