Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#5374 closed (fixed)

We need a validator for ModelAdmin classes

Reported by: jkocherhans Owned by: Brian Rosner
Component: Core (Other) Version: newforms-admin
Severity: Keywords: nfa-blocker
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description (last modified by jkocherhans)

get_validation_errors needs to be updated to deal with ModelAdmin classes instead of the old inline Admin classes.

It may be desirable for ModelAdmin to know how to validate itself (and by 'validate' I basically mean catching invalid attribute names).

Attachments (4)

modelamin.patch (5.4 KB) - added by Daniel Roseman 9 years ago.
Initial attempt at patch.
modeladmin_validation.dff (17.5 KB) - added by mrts 8 years ago.
Hooks into admin.site.register, more or less complete coverage of nfa options
modeladmin_validation.diff (17.5 KB) - added by mrts 8 years ago.
typo
modeladmin_validation2.diff (18.4 KB) - added by mrts 8 years ago.
form and formset validation added, also comments on validation hook by jkocherhans

Download all attachments as: .zip

Change History (21)

comment:1 Changed 9 years ago by jkocherhans

Description: modified (diff)
Summary: django.core.validation.get_validation_errors does not work on ModelAdmin classesWe need a validator for ModelAdmin classes
Triage Stage: UnreviewedAccepted

comment:2 Changed 9 years ago by anonymous

Owner: changed from nobody to anonymous
Status: newassigned

comment:3 Changed 9 years ago by Daniel Roseman

Owner: changed from anonymous to Daniel Roseman
Status: assignednew

Changed 9 years ago by Daniel Roseman

Attachment: modelamin.patch added

Initial attempt at patch.

comment:4 Changed 9 years ago by Daniel Roseman

Has patch: set
Status: newassigned

comment:5 Changed 9 years ago by jkocherhans

I just thought of a possible issue here, this should work fine for the default admin site, but it won't work for any additional ones unless we use some sort of global registry of all AdminSite instnaces or ModelAdmin subclasses... yuck. I'm starting to think that it would make sense for ModelAdmin classes to know how to validate themselves.

comment:6 Changed 9 years ago by Brian Rosner

Keywords: nfa-blocker added

This does need to be apart of newforms-admin before merging. Tagging with nfa-blocker.

comment:7 Changed 8 years ago by Marc Garcia

milestone: 1.0 alpha

comment:8 Changed 8 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:9 Changed 8 years ago by mrts

Needs documentation: set
Needs tests: set
Patch needs improvement: set

I'm championing this currently. Will upload a patch shortly.

comment:10 Changed 8 years ago by anonymous

Owner: changed from Daniel Roseman to mrts
Status: assignednew

Changed 8 years ago by mrts

Attachment: modeladmin_validation.dff added

Hooks into admin.site.register, more or less complete coverage of nfa options

Changed 8 years ago by mrts

Attachment: modeladmin_validation.diff added

typo

Changed 8 years ago by mrts

Attachment: modeladmin_validation2.diff added

form and formset validation added, also comments on validation hook by jkocherhans

comment:11 Changed 8 years ago by mrts

Note the following:

<jkocherhans> mrts: we're probably not going to be able to design a nice 
system for validation before 1.0, so a function that does the validation is 
probably fine... no need to over-engieer anything or provide hook for custom 
classes to allow their own validation. 

<mrts> it doesn't hurt though... 

<jkocherhans> I'd argue that it *does* hurt though, cause then we're 
committing to an api even if it isn't documented, people will use it 

<mrts> should I remove the calls to _call_validation_hook(cls, model) then? 

<jkocherhans> mrts: I'd say yes, but you should probably get a second opinion

<jkocherhans> mrts: I'm not arguing that they're not useful, I'm arguing that
it's a design decision, and I know that malcolm and some other people have opinions about what should happen

comment:12 Changed 8 years ago by mrts

Owner: mrts deleted

I will be away until July 21, so if anyone can write the tests and add a minor documentation note that validation will only be run if settings.DEBUG = True I'd be grateful. If not, I'm back on 21. and will continue on this.

comment:13 Changed 8 years ago by mrts

Another note: the initial plan was to provide classmethods in the line of the following for easy overriding:

Index: django/contrib/admin/options.py
===================================================================
--- django/contrib/admin/options.py	(revision 7877)
+++ django/contrib/admin/options.py	(working copy)
@@ -221,6 +221,103 @@
         return None
     declared_fieldsets = property(_declared_fieldsets)
 
+    def validate(cls, model):
+        """Does basic option validation."""
+        # insert validation here
+    validate = classmethod(validate)
+
 class ModelAdmin(BaseModelAdmin):
     "Encapsulates all admin options and functionality for a given model."
     __metaclass__ = forms.MediaDefiningClass
@@ -236,7 +333,7 @@
     save_on_top = False
     ordering = None
     inlines = []
-    
+
     # Custom templates (designed to be over-ridden in subclasses)
     change_form_template = None
     change_list_template = None
@@ -735,6 +832,12 @@
             "admin/object_history.html"
         ], context, context_instance=template.RequestContext(request))
 
+    def validate(cls, model):
+        """Does basic option validation."""
+        super(ModelAdmin, cls).validate()
+        # insert validation here
+    validate = classmethod(validate)
+
 class InlineModelAdmin(BaseModelAdmin):
     """
     Options for inline editing of ``model`` instances.
@@ -779,6 +882,12 @@
         form = self.get_formset(request).form
         return [(None, {'fields': form.base_fields.keys()})]
 
+    def validate(cls, model):
+        """Does basic option validation."""
+        super(InlineModelAdmin, cls).validate()
+        # insert validation here
+    validate = classmethod(validate)
+
 class StackedInline(InlineModelAdmin):
     template = 'admin/edit_inline/stacked.html'
 
Index: django/contrib/admin/sites.py
===================================================================
--- django/contrib/admin/sites.py	(revision 7877)
+++ django/contrib/admin/sites.py	(working copy)
@@ -7,6 +7,7 @@
 from django.utils.text import capfirst
 from django.utils.translation import ugettext_lazy, ugettext as _
 from django.views.decorators.cache import never_cache
+from django.conf import settings
 import base64
 import cPickle as pickle
 import datetime
@@ -65,6 +66,7 @@
 
         If a model is already registered, this will raise AlreadyRegistered.
         """
+        do_validate = admin_class and settings.DEBUG
         admin_class = admin_class or ModelAdmin
         # TODO: Handle options
         if isinstance(model_or_iterable, ModelBase):
@@ -72,6 +74,8 @@
         for model in model_or_iterable:
             if model in self._registry:
                 raise AlreadyRegistered('The model %s is already registered' % model.__name__)
+            if do_validate:
+                admin_class.validate(model)
             self._registry[model] = admin_class(model, self)
 
     def unregister(self, model_or_iterable):

However, that would make options.py and ModelAdmin classes needlessly heavy-weight. Thus, validation was moved to a separate module that is loaded only in debug mode. Custom validation is still possible, as there are hooks that will call validate() classmethod if provided in the custom ModelAdmin class. Their usefulness or relevance remains a point of discussion, see previous note.

comment:14 Changed 8 years ago by Brian Rosner

Owner: set to Brian Rosner

comment:15 Changed 8 years ago by Brian Rosner

Status: newassigned

comment:16 Changed 8 years ago by Brian Rosner

Resolution: fixed
Status: assignedclosed

(In [7929]) newforms-admin: Fixed #5374 -- Added validation for ModelAdmin and InlineModelAdmin options including tests. Thanks mrts for initial legwork.

comment:17 Changed 5 years ago by Jacob

milestone: 1.0 alpha

Milestone 1.0 alpha deleted

Note: See TracTickets for help on using tickets.
Back to Top