Code

Opened 7 years ago

Closed 6 years ago

Last modified 3 years ago

#5374 closed (fixed)

We need a validator for ModelAdmin classes

Reported by: jkocherhans Owned by: brosner
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 danielr 7 years ago.
Initial attempt at patch.
modeladmin_validation.dff (17.5 KB) - added by mrts 6 years ago.
Hooks into admin.site.register, more or less complete coverage of nfa options
modeladmin_validation.diff (17.5 KB) - added by mrts 6 years ago.
typo
modeladmin_validation2.diff (18.4 KB) - added by mrts 6 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 7 years ago by jkocherhans

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from django.core.validation.get_validation_errors does not work on ModelAdmin classes to We need a validator for ModelAdmin classes
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by anonymous

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:3 Changed 7 years ago by danielr

  • Owner changed from anonymous to danielr
  • Status changed from assigned to new

Changed 7 years ago by danielr

Initial attempt at patch.

comment:4 Changed 7 years ago by danielr

  • Has patch set
  • Status changed from new to assigned

comment:5 Changed 7 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 7 years ago by brosner

  • Keywords nfa-blocker added

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

comment:7 Changed 6 years ago by garcia_marc

  • milestone set to 1.0 alpha

comment:8 Changed 6 years ago by CollinAnderson

  • Cc cmawebsite@… added

comment:9 Changed 6 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 6 years ago by anonymous

  • Owner changed from danielr to mrts
  • Status changed from assigned to new

Changed 6 years ago by mrts

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

Changed 6 years ago by mrts

typo

Changed 6 years ago by mrts

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

comment:11 Changed 6 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 6 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 6 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 6 years ago by brosner

  • Owner set to brosner

comment:15 Changed 6 years ago by brosner

  • Status changed from new to assigned

comment:16 Changed 6 years ago by brosner

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:17 Changed 3 years ago by jacob

  • milestone 1.0 alpha deleted

Milestone 1.0 alpha deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.