newforms-admin: Fixed #5490 -- Properly quote special characters in primary keys in the admin. Added tests to ensure functionality. This also moves quote and unquote to django/contrib/admin/util.py. Thanks jdetaeye and shanx for all your help.

git-svn-id: http://code.djangoproject.com/svn/django/branches/newforms-admin@7935 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/contrib/admin/models.py

@@ -1,6 +1,7 @@
 from django.db import models
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.auth.models import User
+from django.contrib.admin.util import quote
 from django.utils.translation import ugettext_lazy as _
 from django.utils.encoding import smart_unicode
 from django.utils.safestring import mark_safe
@@ -50,4 +51,4 @@ class LogEntry(models.Model):
         Returns the admin URL to edit the object represented by this log entry.
         This is relative to the Django admin index page.
         """
-        return mark_safe(u"%s/%s/%s/" % (self.content_type.app_label, self.content_type.model, self.object_id))
+        return mark_safe(u"%s/%s/%s/" % (self.content_type.app_label, self.content_type.model, quote(self.object_id)))

django/contrib/admin/options.py

@@ -5,7 +5,7 @@ from django.newforms.models import modelform_factory, inlineformset_factory
 from django.newforms.models import BaseInlineFormset
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.admin import widgets
-from django.contrib.admin.util import get_deleted_objects
+from django.contrib.admin.util import quote, unquote, get_deleted_objects
 from django.core.exceptions import ImproperlyConfigured, PermissionDenied
 from django.db import models, transaction
 from django.http import Http404, HttpResponse, HttpResponseRedirect
@@ -24,26 +24,6 @@ get_ul_class = lambda x: 'radiolist%s' % ((x == HORIZONTAL) and ' inline' or '')
 class IncorrectLookupParameters(Exception):
     pass
 
-def unquote(s):
-    """
-    Undo the effects of quote(). Based heavily on urllib.unquote().
-    """
-    mychr = chr
-    myatoi = int
-    list = s.split('_')
-    res = [list[0]]
-    myappend = res.append
-    del list[0]
-    for item in list:
-        if item[1:2]:
-            try:
-                myappend(mychr(myatoi(item[:2], 16)) + item[2:])
-            except ValueError:
-                myappend('_' + item)
-        else:
-            myappend('_' + item)
-    return "".join(res)
-
 def flatten_fieldsets(fieldsets):
     """Returns a list of field names from an admin fieldsets structure."""
     field_names = []
@@ -656,7 +636,7 @@ class ModelAdmin(BaseModelAdmin):
 
         # Populate deleted_objects, a data structure of all related objects that
         # will also be deleted.
-        deleted_objects = [mark_safe(u'%s: <a href="../../%s/">%s</a>' % (escape(force_unicode(capfirst(opts.verbose_name))), force_unicode(object_id), escape(obj))), []]
+        deleted_objects = [mark_safe(u'%s: <a href="../../%s/">%s</a>' % (escape(force_unicode(capfirst(opts.verbose_name))), quote(object_id), escape(obj))), []]
         perms_needed = sets.Set()
         get_deleted_objects(deleted_objects, perms_needed, request.user, obj, opts, 1, self.admin_site)
 

django/contrib/admin/util.py

@@ -6,6 +6,43 @@ from django.utils.text import capfirst
 from django.utils.encoding import force_unicode
 from django.utils.translation import ugettext as _
 
+
+def quote(s):
+    """
+    Ensure that primary key values do not confuse the admin URLs by escaping
+    any '/', '_' and ':' characters. Similar to urllib.quote, except that the
+    quoting is slightly different so that it doesn't get automatically
+    unquoted by the Web browser.
+    """
+    if not isinstance(s, basestring):
+        return s
+    res = list(s)
+    for i in range(len(res)):
+        c = res[i]
+        if c in """:/_#?;@&=+$,"<>%\\""":
+            res[i] = '_%02X' % ord(c)
+    return ''.join(res)
+
+def unquote(s):
+    """
+    Undo the effects of quote(). Based heavily on urllib.unquote().
+    """
+    mychr = chr
+    myatoi = int
+    list = s.split('_')
+    res = [list[0]]
+    myappend = res.append
+    del list[0]
+    for item in list:
+        if item[1:2]:
+            try:
+                myappend(mychr(myatoi(item[:2], 16)) + item[2:])
+            except ValueError:
+                myappend('_' + item)
+        else:
+            myappend('_' + item)
+    return "".join(res)
+
 def _nest_help(obj, depth, val):
     current = obj
     for i in range(depth):

django/contrib/admin/views/main.py

@@ -1,5 +1,6 @@
 from django.contrib.admin.filterspecs import FilterSpec
 from django.contrib.admin.options import IncorrectLookupParameters
+from django.contrib.admin.util import quote
 from django.core.paginator import Paginator, InvalidPage
 from django.db import models
 from django.db.models.query import QuerySet
@@ -30,22 +31,6 @@ ERROR_FLAG = 'e'
 # Text to display within change-list table cells if the value is blank.
 EMPTY_CHANGELIST_VALUE = '(None)'
 
-def quote(s):
-    """
-    Ensure that primary key values do not confuse the admin URLs by escaping
-    any '/', '_' and ':' characters. Similar to urllib.quote, except that the
-    quoting is slightly different so that it doesn't get automatically
-    unquoted by the Web browser.
-    """
-    if type(s) != type(''):
-        return s
-    res = list(s)
-    for i in range(len(res)):
-        c = res[i]
-        if c in ':/_':
-            res[i] = '_%02X' % ord(c)
-    return ''.join(res)
-
 class ChangeList(object):
     def __init__(self, request, model, list_display, list_display_links, list_filter, date_hierarchy, search_fields, list_select_related, list_per_page, model_admin):
         self.model = model

django/db/models/base.py

@@ -137,6 +137,10 @@ class ModelBase(type):
         return get_model(new_class._meta.app_label, name, False)
 
     def add_to_class(cls, name, value):
+        if name == 'Admin':
+            import warnings
+            warnings.warn("The inner Admin class for %s is no longer supported. "
+                "Please use a ModelAdmin instead." % cls.__name__)
         if hasattr(value, 'contribute_to_class'):
             value.contribute_to_class(cls, name)
         else:

tests/regressiontests/admin_views/fixtures/string-primary-key.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="utf-8"?>
+<django-objects version="1.0">
+    <object pk="1" model="admin_views.modelwithstringprimarykey">
+        <field type="CharField" name="id"><![CDATA[abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890 -_.!~*'() ;/?:@&=+$, <>#%" {}|\^[]`]]></field>
+    </object>    
+</django-objects>

tests/regressiontests/admin_views/models.py

@@ -48,7 +48,14 @@ class CustomArticleAdmin(admin.ModelAdmin):
                 'extra_var': 'Hello!'
             }
         )
+
+class ModelWithStringPrimaryKey(models.Model):
+    id = models.CharField(max_length=255, primary_key=True)
+    
+    def __unicode__(self):
+        return self.id
         
 admin.site.register(Article, ArticleAdmin)
 admin.site.register(CustomArticle, CustomArticleAdmin)
 admin.site.register(Section)
+admin.site.register(ModelWithStringPrimaryKey)

tests/regressiontests/admin_views/tests.py

@@ -2,10 +2,13 @@
 from django.test import TestCase
 from django.contrib.auth.models import User, Permission
 from django.contrib.contenttypes.models import ContentType
+from django.contrib.admin.models import LogEntry
 from django.contrib.admin.sites import LOGIN_FORM_KEY, _encode_post_data
+from django.contrib.admin.util import quote
+from django.utils.html import escape
 
 # local test models
-from models import Article, CustomArticle, Section
+from models import Article, CustomArticle, Section, ModelWithStringPrimaryKey
 
 def get_perm(Model, perm):
     """Return the permission object, for the Model"""
@@ -318,3 +321,42 @@ class AdminViewPermissionsTest(TestCase):
         self.assertRedirects(post, '/test_admin/admin/')
         self.failUnlessEqual(Article.objects.all().count(), 0)
         self.client.get('/test_admin/admin/logout/')
+
+class AdminViewStringPrimaryKeyTest(TestCase):
+    fixtures = ['admin-views-users.xml', 'string-primary-key.xml']
+    
+    def __init__(self, *args):
+        super(AdminViewStringPrimaryKeyTest, self).__init__(*args)
+        self.pk = """abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890 -_.!~*'() ;/?:@&=+$, <>#%" {}|\^[]`"""
+    
+    def setUp(self):
+        self.client.login(username='super', password='secret')
+        content_type_pk = ContentType.objects.get_for_model(ModelWithStringPrimaryKey).pk
+        LogEntry.objects.log_action(100, content_type_pk, self.pk, self.pk, 2, change_message='')
+    
+    def tearDown(self):
+        self.client.logout()
+    
+    def test_get_change_view(self):
+        "Retrieving the object using urlencoded form of primary key should work"
+        response = self.client.get('/test_admin/admin/admin_views/modelwithstringprimarykey/%s/' % quote(self.pk))
+        self.assertContains(response, escape(self.pk))
+        self.failUnlessEqual(response.status_code, 200)
+    
+    def test_changelist_to_changeform_link(self):
+        "The link from the changelist referring to the changeform of the object should be quoted"
+        response = self.client.get('/test_admin/admin/admin_views/modelwithstringprimarykey/')
+        should_contain = """<tr class="row1"><th><a href="%s/">%s</a></th></tr>""" % (quote(self.pk), escape(self.pk))
+        self.assertContains(response, should_contain)
+    
+    def test_recentactions_link(self):
+        "The link from the recent actions list referring to the changeform of the object should be quoted"
+        response = self.client.get('/test_admin/admin/')
+        should_contain = """<a href="admin_views/modelwithstringprimarykey/%s/">%s</a>""" % (quote(self.pk), escape(self.pk))
+        self.assertContains(response, should_contain)
+    
+    def test_deleteconfirmation_link(self):
+        "The link from the delete confirmation page referring back to the changeform of the object should be quoted"
+        response = self.client.get('/test_admin/admin/admin_views/modelwithstringprimarykey/%s/delete/' % quote(self.pk))
+        should_contain = """<a href="../../%s/">%s</a>""" % (quote(self.pk), escape(self.pk))
+        self.assertContains(response, should_contain)