Make it easy to add new views to a ModelAdmin2 subclass

[gregmuellegger]

As discussed with @pydanny during the djangocon.eu sprints, we said that it is
essential to have a very easy to use way to add additional views to a
ModelAdmin and to swap out existing ones.

[gregmuellegger]

As discussed with @pydanny during the djangocon.eu sprints, we said that it is essential to have a very easy to use way to add additional views to a ModelAdmin and to swap out existing ones.

My initial approach on that was to have a views list that would look something like this:

# in djadmin2/models.py
class ModelAdmin2(object):
    ...
    views = [
        (r'^$', ModelListView, 'index'),
        (r'^add/$', ModelAddFormView, 'create'),
        (r'^(?P<pk>\d+)$', ModelEditFormView, 'update'),
        ...
    ]
    ...

# in user code
class PostAdmin(ModelAdmin2):
    views = ModelAdmin2.views[:] + [
        (r'^(?P<pk>\d+)/preview/$', PreviewBlogPostView, 'preview'),
    ]

However I don't like that too much.

1. Exchanging existing views gets really hard
2. It's not very declarative.

So my second thought was to use more idiologies that we already have in django. I see a model as a container for db fields including some methods to work on those fields. The same is true for forms and form fields. If we see the ModelAdmin as a fancy wrapper around the views, we get something like this:

# in djadmin2/models.py
class ModelAdmin2Metaclass(type):
    # scan the ModelAdmin2 class definition of uses of `AdminView` instances,
    # like models do with modelfields, and put them in the `views` attribute.
    ...

class ModelAdmin2(object):
    __metaclass__ = ModelAdmin2Metaclass

    index = AdminView(r'^$', ModelListView)
    create = AdminView(r'^add/$', ModelAddFormView)
    update = AdminView(r'^(?P<pk>\d+)$', ModelEditFormView)

# in user code
class PostAdmin(ModelAdmin2):
    index = AdminView(r'^$', BlogPostDashboard)
    preview_post = AdminView(r'^$', PreviewBlogPostView, name='preview')

The semantics are:

1. The ModelAdmin2 has a metaclass (just like django models) that know about the views that were defined in this ModelAdmin2 (sub-)class. These will be placed and later discoverable in ModelAdmin2.views.
2. The AdminView is a wrapper around an ordinary CBV that is used to detect a view that is assigned to a ModelAdmin2.
3. AdminView takes additional arguments, including a url and name which are later used in the ModelAdmin2.get_urls() method to generate the urlconf.
4. The name argument on AdminView can be blank (like for index, create etc in the example above). Then the name will default to the attribute name that is used to hold the view on the ModelAdmin2 (just like django determines the db column from the name of a modelfield).

I'm keen to here your input on this.

[AndrewIngram]

I do like the metaclass approach and the API you've proposed, it's rather elegant. But I should add that you can already swap out any of the existing views, as all the view classes are stored as attributes on the ModelAdmin2.

[RaphaelKimmig]

Just for the sake of complexity I'd like to also mention the naive approach.

# replacing a view
class ModifiedModelAdmin2(ModelAdmin2):
    delete_view = CustomDeleteView

# adding a view
class CustomModelAdmin2(ModelAdmin2):
    extra_view = ExtraView

    def get_urls(self):
        patterns = super(CustomModelAdmin2, self).get_urls()
        return patterns + patterns('',
            url(
                regex=r'^extra_url$',
                view=self.extra_view.as_view(),
                name=self.get_prefixed_view_name('extra_view')
            ),
        )

The problem I see with this approach is that it will require more boilerplate
when things start getting more complex. If we end up implementing some menu
system (#27) we might end up with another get_menu_entries function that would
also need to be overriden and so on. This would in my mind be the strongest
reason for implementing some higher level of abstraction.

On the other hand it is a really simple approach, doesn't require any new
concepts and exposes the (clear and simple) internal workings of admin2 - which
imho is a good idea if we want to encourage customization.

What do you think are the strongest reasons for going with something more
complex? And do you think of exposing the internals directly as something
positive or rather something negative?

The meta class approach looks really clean but it also means that there are
more pieces to understand and limits to work within when extending the admin
(beyond just registering another view).

[AndrewIngram]

@RaphaelKimmig agreed that there's more to understand, but I think it bears enough similarities with how forms and models work to feel familiar. I think it's worth exploring further, even if it turns out to be a dead-end.

@gregmuellegger we currently pass in different kwargs to as_view() on a per-view basis, can you illustrate how you see this working?

[AndrewIngram]

Answering my own question, it could work like this:

class PostAdmin(ModelAdmin2):
    index = AdminView(r'^$', BlogPostDashboard)
    preview_post = AdminView(r'^$', PreviewBlogPostView, name='preview')

    def get_index_kwargs(self):
        return {}

    def get_preview_post_kwargs(self):
        return {}

ie, when calling as_view we check for the existence of 'get_{}_kwargs'.format(name)

[RaphaelKimmig]

@AndrewIngram I agree. Using the meta class approach (if you do not want to do anything beyond simply registering another view) would be really straight forward. I was thinking more about the additional complexity hidden beneath the surface.

A question that comes to my mind is: In what order will urls be registered? That is: given a list of url patterns it is easy to see which takes precedence. How would we build the list of patterns here? By simply taking the views in the order they are defined in, with the ones defined on a subclass prepended to the ones collected from the base classes?

[gregmuellegger]

objections against the naive approach

First, the naive approach as I used a dozen times with django.contrib.admin (@RaphaelKimmig thanks for illustrating that one) has its problems mainly in overwriting the get_urls method. Yes it is straightforward and not too errorprone to do. But it always bugs me because you have to pull in the url and patterns function to define the urlpattern, call the super function, deciding if you want to override a view (then you apply the new url before the urlpatterns returned) and so on...

It always ends up beeing around 10-15 lines of code just for adding one view to one modeladmin.

How to pass in extra arguments into the view?

I would have the get_view_kwargs methods rather on the (new) AdminView descriptor/class. That way the actual ModelAdmin class doesn't need to know anything about how the view is actually called. We could have different descriptors for the different index/create/... views:

class AdminView(object):
    def get_view_kwargs(self):
        return {
            'model': self.model_admin.model,
        }

class AdminIndexView(AdminView):
    def get_view_kwargs(self):
        kwargs = super(AdminIndexView)
        kwargs.update({
            'list_per_page': self.list_per_page,
        })


class PostAdmin(ModelAdmin2):
    index = AdminIndexView(r'^$', BlogPostDashboard)
    preview_post = AdminView(r'^$', PreviewBlogPostView, name='preview')

The problem with that is that it's not straightforward enough to just add one more parameter to the view. This would force you into subclassing AdminView. The solution is to have optional methods on the ModelAdmin (like we currently have and what @AndrewIngram suggested):

class AdminView(object):
    def get_view_kwargs(self):
        kwargs = {
            'model': self.model_admin.model,
        }
        get_extra_kwargs = getattr(self.model_admin, 'get_%s_kwargs' % self.name, None)
        if get_extra_kwargs:
            kwargs = get_extra_kwargs(kwargs)
        return kwargs

class AdminIndexView(object):
    ...

class PostAdmin(ModelAdmin2):
    index = AdminIndexView(r'^$', BlogPostDashboard)
    preview_post = AdminView(r'^$', PreviewBlogPostView, name='preview')

    def get_preview_post_kwargs(self, kwargs):
        kwargs.update({
            'mode_info': self.some_attribute,
        })
        return kwargs

That way you have the possibility of easily overriding kwargs for specific views, but we don't have get_*_kwargs methods for all default views implemented in the ModelAdmin. That pattern of calling methods that have names based on other attributes is also already familiar, see django's def clean_*(self) methods on forms.

keeping the ModelAdmin class as easy to read as possible

One of the goals I want achive (and confirms with what @pydanny expressed to me on the sprints) is to reduce the amount of code that lives in a subclassed ModelAdmin. As an example of using the current admin for something complex, have a look at the pageadmin of django-cms: https://github.com/divio/django-cms/blob/develop/cms/admin/pageadmin.py#L132
This class is more than 1400 lines of code!!!! I think we should really try to avoid this by moving as much logic out of the ModelAdmin class into swapable classes.

Also django.contrib.admin BaseModelAdmin and ModelAdmin classes are together around 1400 lines. Thats not nice to maintain, so that's another reason I think for the Metaclass/Descriptor approach.

[RaphaelKimmig]

@gregmuellegger Yes, having to write ~10 lines just for adding one view is not short. On the other hand overriding get_urls is only 2 or 3 lines more than registering a view in the urls.py. Also the ordering will be even more confusing given the meta class approach because here you don't ever touch the list of urls explicitly. Anyway. I do actually like the meta class approach, my questions and remarks are merely meant as constructive input towards finding a simple and beautiful solution.

Regarding the view kwargs

If I understand @pydanny correctly in #99 the views will have access to an immutable version of the model_admin. Given that: Why do we need things like get_{}_kwargs or the descriptor approach? What are the drawbacks of having the view pull out whatever context it needs on its own?
Subclassing the view and overriding it on the admin class are only two extra lines of code (+2 for spacing if you follow pep8).

[gregmuellegger]

@RaphaelKimmig urls will be registered in the order that the views are defined in ModelAdmin. Like form fields on forms (the definition defines the order during rendering with form.as_p or the order of calling the clean_* methods.

I see that the order of urls is quite important. But overriding an existing view (with the same url regex), will also remove it from the urlpatterns, so its very unlikely that clashing URL-regexes will show up.

[pydanny]

I like following Django's ClassThing.Meta approach proposed and endorsed by @gregmuellegger and @AndrewIngram. By sticking to the Django feel of things, it means it's easier for developers to pick up and use the tool. On the other hand, I will say that I do appreciate the simplicity of the code behind the naive approach.

Alright then, a few things before we start digging too deeply into code:

1. Let's come up with a pseudo-code API and docs written before we implement the code. It's good for us to have the same end-goal for this effort. If there is some way we can register new/replacement views that feels intuitive, that gives us a goal for coding.
2. We need to come up with a way to test and introspect deep view-adding inheritance so we can document it fully. For example, forms is good at inheritance, but deep chains in models are problematic.
3. Let's get version 0.3.0 out the door. 😉

[RaphaelKimmig]

@gregmuellegger I am just looking into #27 (providing a way to define menus).

Do you think that AdminView should also be used to register menu entries? Any thoughts regarding that issue?

class PostAdmin(ModelAdmin2):
    analytics = AdminView(r'^$', AnalyticsView, name='analytics', menu_title=_('Analytics'))

[gregmuellegger]

@RaphaelKimmig yes I think that's a good approach. It makes sense to give the view a name that can be reused in the frontend.

[andrewsmedina]

I will do this issue :)

[gregmuellegger]

Good to hear! I wasn't able to work on this due to private stuff occupying me.
Feel free to ping me if you want code-reviews or to discuss something in detail.

[andrewsmedina]

@gregmuellegger and @pydanny I have implemented the AdminView in https://github.com/andrewsmedina/django-admin2/compare/admin_view and I have a question.

To implement the AdminView.get_view_kwargs it's need to set a model_admin attribute into the AdminView instance:

class AdminView(object):
    def get_view_kwargs(self):
        return {
            'model': self.model_admin.model,
        }

How can I do it? I thinking in one approach: define it on ModelAdmin.get_urls:

def get_urls(self):
    pattern_list = []
    for view in self.views:
        view.model_admin = self
        ...   
        pattern_list.append(url(...))
    return patterns('', *pattern_list)

But I'm not finding it good enough. Any suggestions?

[pydanny]

This is one of those tricky UX questions. We want to get it right, so I'm
opening the floor to suggestions.

--Danny

On Sun, Jun 23, 2013 at 8:45 PM, Andrews Medina notifications@github.comwrote:

@gregmuellegger https://github.com/gregmuellegger and @pydannyhttps://github.com/pydannyI have implemented the AdminView in
https://github.com/andrewsmedina/django-admin2/compare/admin_view and I
have a question.

To implement the AdminView.get_view_kwargs it's need to set a model_adminattribute into the
AdminView instance:

class AdminView(object):
def get_view_kwargs(self):
return {
'model': self.model_admin.model,
}

How can I do it? I thinking in one approach: define it on
ModelAdmin.get_urls:

def get_urls(self):
pattern_list = []
for view in self.views:
view.model_admin = self
...
pattern_list.append(url(...))
return patterns('', *pattern_list)

But I'm not finding it good enough. Any suggestions?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/142#issuecomment-19878873
.

'Knowledge is Power'
Daniel Greenfeld
Principal at Cartwheel Web; co-author of Two Scoops of Django
cartwheelweb.com | pydanny.com | django.2scoops.org

[pydanny]

@andrewsmedina completed this in #218. ‼️ 🌴 💯