Decouple Admin inner class from Model

[daekharel@…]

django.db.models.Model's add_to_class method contains logic for handling Admin inner classes:

    def add_to_class(cls, name, value):
        if name == 'Admin':
            assert type(value) == types.ClassType, "%r attribute of %s model must be a class, not a %s object" % (name, cls.__name__, type(value))
            value = AdminOptions(**dict([(k, v) for k, v in value.__dict__.items() if not k.startswith('_')]))
        if hasattr(value, 'contribute_to_class'):
            value.contribute_to_class(cls, name)
        else:
            setattr(cls, name, value)

Ideally, this logic would be separated from the Model class. I don't know how this could be done without changing the way Admin inner classes work (e.g. by requiring them to subclass some other class which did the work that AdminOptions does here using metaclassing).

[Malcolm Tredinnick <malcolm@…>]

What are you trying to achieve here (what does decoupling gain)? Using an Admin class is pretty simple at the moment and you seem to be proposing making that a bit harder (by requiring subclassing or extra machinery), so is there some particular user case that you have in mind?

[daekharel@…]

Decoupling gains what decoupling always gains: modularity. It means that the logic in the Model is no longer dependent on the Admin and vice-versa, and this makes implementing new functionality similar to that of an Admin system easier. As things currently stand, if one wanted to create a system that also used inner classes to add functionality, as the Admin system does, one would have to modify the Model class' add_to_class method. I suppose one plausible user case might be if someone wanted to use inner classes to implement some sort of fine-grained permissions policy (as opposed to Django's built-in but very broad permissions) with characteristics dependent upon the models themselves.

Admittedly, this isn't a huge problem, being a rather minor corner case, and perhaps it should be ignored, but it seems rather out-of-step with Django's apparent style of decoupling different layers.

[James Bennett]

I don't really see it as an issue of decoupling -- the contents of the administrative interface for a model should be coupled to the definition of that model. I do see this proposal raising problems with DRY, however, because it would mean that the model definition would no longer be the single place for obtaining information about the model.

[daekharel@…]

Perhaps the name of the ticket is misleading; I'm not proposing that you remove Admin inner classes from model definitions (in other words, make them non-inner classes), thus violating DRY. I'm proposing that the behind-the-scenes logic that deals with actually handling Admin inner classes be taken out of the base Model class, since this currently couples (albeit weakly) the basic database model to a specific application (the administrative interface). One way to implement this (probably not the only way, and possibly undesirable, since it would break backwards compatibility / require code changes on the user end) is to require Admin inner classes to subclass a class that provides, via metaclassing, the same features as the AdminOptions class invoked in the add_to_class method, and also implements contribute_to_class. If this were done, the first three lines of Models' add_to_class classmethod could be deleted, eliminating the coupling between the Model class and the administrative interface application.

[Luke Plant]

Joseph Kocherhans (sp?) and I have both come across this issue. The way I ended up hacking it was doing:

MyModel._meta.admin.__class__ = MyModelsSpecialAdminClass

A nicer way of doing this would be good. Perhaps for backwards compatibility (and simplicity) the current 'class Admin:' would work as is, but if you did 'class Admin(MyAdminClass):' it will instantiate MyAdminClass instead.

As for using inner classes for other purposes, isn't it already possible? I would have thought the 'setattr(cls, name, value)' line would have handled it.

[Gary Wilson <gary.wilson@…>]

It's possible that the admin changes discussed ​here might clean this up.

[James Bennett]

The newforms-admin changes will accomplish this fairly cleanly.

[Malcolm Tredinnick]

This is fixed by newforms-admin. Nothing further needs to be done here.