Django

Code

Ticket #5374 (closed: fixed)

Opened 1 year ago

Last modified 5 months ago

We need a validator for ModelAdmin classes

Reported by: jkocherhans Assigned to: brosner
Milestone: 1.0 alpha Component: Core framework
Version: newforms-admin Keywords: nfa-blocker
Cc: cmawebsite@gmail.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 1
Needs tests: 1 Patch needs improvement: 1

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

modelamin.patch (5.4 kB) - added by danielr on 09/14/07 19:11:56.
Initial attempt at patch.
modeladmin_validation.dff (17.5 kB) - added by mrts on 07/11/08 08:47:22.
Hooks into admin.site.register, more or less complete coverage of nfa options
modeladmin_validation.diff (17.5 kB) - added by mrts on 07/11/08 08:48:40.
typo
modeladmin_validation2.diff (18.4 kB) - added by mrts on 07/11/08 10:18:18.
form and formset validation added, also comments on validation hook by jkocherhans

Change History

09/14/07 14:50:15 changed by jkocherhans

  • description changed.
  • needs_better_patch changed.
  • needs_tests changed.
  • summary changed from django.core.validation.get_validation_errors does not work on ModelAdmin classes to We need a validator for ModelAdmin classes.
  • needs_docs changed.
  • stage changed from Unreviewed to Accepted.

09/14/07 19:10:09 changed by anonymous

  • owner changed from nobody to anonymous.
  • status changed from new to assigned.

09/14/07 19:11:04 changed by danielr

  • owner changed from anonymous to danielr.
  • status changed from assigned to new.

09/14/07 19:11:56 changed by danielr

  • attachment modelamin.patch added.

Initial attempt at patch.

09/14/07 19:15:31 changed by danielr

  • status changed from new to assigned.
  • has_patch set to 1.

09/15/07 14:10:03 changed 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.

12/10/07 11:03:18 changed by brosner

  • keywords set to nfa-blocker.

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

06/16/08 10:45:39 changed by garcia_marc

  • milestone set to 1.0 alpha.

07/05/08 14:53:41 changed by CollinAnderson

  • cc set to cmawebsite@gmail.com.

07/11/08 04:10:54 changed by mrts

  • needs_better_patch set to 1.
  • needs_tests set to 1.
  • needs_docs set to 1.

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

07/11/08 04:47:54 changed by anonymous

  • owner changed from danielr to mrts.
  • status changed from assigned to new.

07/11/08 08:47:22 changed by mrts

  • attachment modeladmin_validation.dff added.

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

07/11/08 08:48:40 changed by mrts

  • attachment modeladmin_validation.diff added.

typo

07/11/08 10:18:18 changed by mrts

  • attachment modeladmin_validation2.diff added.

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

07/11/08 10:20:38 changed 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

07/11/08 10:23:41 changed by mrts

  • owner 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.

07/11/08 12:31:15 changed 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.

07/12/08 11:23:35 changed by brosner

  • owner set to brosner.

07/12/08 11:23:48 changed by brosner

  • status changed from new to assigned.

07/15/08 16:43:24 changed by brosner

  • status changed from assigned to closed.
  • resolution set to fixed.

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


Add/Change #5374 (We need a validator for ModelAdmin classes)




Change Properties
Action