Ticket #5490: urlquote_string_primarekey_with_tests.diff

django/contrib/admin/options.py

@@ -15 +15 @@
 from django.utils.text import capfirst, get_text_list
 from django.utils.translation import ugettext as _
 from django.utils.encoding import force_unicode
+from django.utils.http import urlquote, URL_SAFE_CHARACTERS
 import sets
 
 HORIZONTAL, VERTICAL = 1, 2
@@ -680 +681 @@
 
         # 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))), urlquote(object_id, safe=URL_SAFE_CHARACTERS), escape(obj))), []]
         perms_needed = sets.Set()
         get_deleted_objects(deleted_objects, perms_needed, request.user, obj, opts, 1, self.admin_site)
 

django/contrib/admin/models.py

@@ -4 +4 @@
 from django.utils.translation import ugettext_lazy as _
 from django.utils.encoding import smart_unicode
 from django.utils.safestring import mark_safe
+from django.utils.http import urlquote, URL_SAFE_CHARACTERS
 
 ADDITION = 1
 CHANGE = 2
@@ -50 +51 @@
         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, urlquote(self.object_id, safe=URL_SAFE_CHARACTERS)))

django/contrib/admin/views/main.py

@@ -6 +6 @@
 from django.utils.encoding import force_unicode, smart_str
 from django.utils.translation import ugettext
 from django.utils.safestring import mark_safe
-from django.utils.http import urlencode
+from django.utils.http import urlencode, urlquote, URL_SAFE_CHARACTERS
 import operator
 
 try:
@@ -246 +246 @@
         return qs
 
     def url_for_result(self, result):
-        return "%s/" % quote(getattr(result, self.pk_attname))
+        return "%s/" % urlquote(getattr(result, self.pk_attname), safe=URL_SAFE_CHARACTERS)

django/contrib/admin/templates/admin/index.html

@@ -59 +59 @@
             {% else %}
             <ul class="actionlist">
             {% for entry in admin_log %}
-            <li class="{% if entry.is_addition %}addlink{% endif %}{% if entry.is_change %}changelink{% endif %}{% if entry.is_deletion %}deletelink{% endif %}">{% if not entry.is_deletion %}<a href="{{ entry.get_admin_url }}">{% endif %}{{ entry.object_repr|escape }}{% if not entry.is_deletion %}</a>{% endif %}<br /><span class="mini quiet">{% filter capfirst %}{% trans entry.content_type.name %}{% endfilter %}</span></li>
+            <li class="{% if entry.is_addition %}addlink{% endif %}{% if entry.is_change %}changelink{% endif %}{% if entry.is_deletion %}deletelink{% endif %}">{% if not entry.is_deletion %}<a href="{{ entry.get_admin_url }}">{% endif %}{{ entry.object_repr }}{% if not entry.is_deletion %}</a>{% endif %}<br /><span class="mini quiet">{% filter capfirst %}{% trans entry.content_type.name %}{% endfilter %}</span></li>
             {% endfor %}
             </ul>
             {% endif %}

django/utils/http.py

@@ -4 +4 @@
 from django.utils.encoding import smart_str, force_unicode
 from django.utils.functional import allow_lazy
 
+# These are the safe characters as defined by:
+# http://www.ietf.org/rfc/rfc2396.txt (see: 2.3. Unreserved Characters)
+# these characters are safe to be used in a URL part.
+URL_SAFE_CHARACTERS = "-_.!~*'()"
+
 def urlquote(url, safe='/'):
     """
     A version of Python's urllib.quote() function that can operate on unicode

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

@@ +1 @@
+<?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>
+ No newline at end of file

tests/regressiontests/admin_views/tests.py

@@ -2 +2 @@
 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
 
 # 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"""
@@ -320 +321 @@
         self.assertRedirects(post, '/test_admin/admin/')
         self.failUnlessEqual(Article.objects.all().count(), 0)
         self.client.get('/test_admin/admin/logout/')
-        
- No newline at end of file
+        
+class AdminViewStringPrimaryKeyTest(TestCase):
+    fixtures = ['admin-views-users.xml', 'string-primary-key.xml']
+    
+    def __init__(self, *args):
+        super(AdminViewStringPrimaryKeyTest, self).__init__(*args)
+        
+        # The primary key value used is just a string containing all possible characters
+        self.pk = """abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890 -_.!~*'() ;/?:@&=+$, <>#%" {}|\^[]`"""
+
+        self.pk_escaped = """abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890 -_.!~*&#39;() ;/?:@&amp;=+$, &lt;&gt;#%&quot; {}|\^[]`"""
+        
+        # The characters that should be quoted are defined in: 
+        # These are the safe characters as defined by:
+        # http://www.ietf.org/rfc/rfc2396.txt (see chapter 2)
+        # The safe characters are defined in django.util.http.URL_SAFE_CHARACTERS
+        self.pk_quoted = """abcdefghijklmnopqrstuvwxyz%20ABCDEFGHIJKLMNOPQRSTUVWXYZ%201234567890%20-_.!~*'()%20%3B%2F%3F%3A%40%26%3D%2B%24%2C%20%3C%3E%23%25%22%20%7B%7D%7C%5C%5E%5B%5D%60"""
+    
+    def setUp(self):
+        self.client.login(username='super', password='secret')
+        
+        # This entry refers to content type which is not known at the moment the fixture is imported
+        content_type_id = ContentType.objects.get_for_model(ModelWithStringPrimaryKey).id
+        ContentType.objects.all()
+        
+        LogEntry.objects.log_action(100, content_type_id, 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/' % self.pk_quoted)
+
+        self.assertContains(response, self.pk_escaped)
+    
+    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>""" % (self.pk_quoted, self.pk_escaped)
+        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>""" % (self.pk_quoted, self.pk_escaped)
+        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/' % self.pk_quoted)
+
+        should_contain = """<a href="../../%s/">%s</a>""" % (self.pk_quoted, self.pk_escaped)        
+        self.assertContains(response, should_contain)
+    
+ No newline at end of file

tests/regressiontests/admin_views/models.py

@@ -48 +48 @@
                 '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)