crccheck / django-object-actions

A Django app for easily adding object tools in the Django admin
https://pypi.org/project/django-object-actions/
Apache License 2.0
680 stars 82 forks source link

fix: unquote object_id when calling get_change_actions #137

Open nshafer opened 2 years ago

nshafer commented 2 years ago

I was recently overriding get_change_actions() as instructed in the documentation, however I was having problems due to a model having a CharField for the primary key, and specifically values that included underscore characters. I was receiving what seemed to be invalid values in object_id which extra characters.

By default the django admin quotes primary key values via the quote() function in "django/contrib/admin/utils.py". This is to prevent certain characters, such as underscores, in the primary key, especially if it's a CharField, from messing up the URL. Therefore, in the admin the object_id is unquoted before calling get_object() and in other cases.

Therefore, I think object_id should also be unquoted before calling get_change_actions so that object_id is not url quoted any more, and operations such as obj = self.model.objects.get(pk=object_id) will succeed as expected.

This PR simply does that. I tried for a short period to get tests running, but I've never installed poetry and was having issues, and just don't have time to debug it right now, so I wasn't able to write a test to confirm the behavior, apologies.

crccheck commented 1 year ago

I was unable to get a test case that recreated the original problem, I'll have to try it with the sample project next. Here's what I tried:

diff --git a/django_object_actions/tests/test_admin.py b/django_object_actions/tests/test_admin.py
index db6e7da..472212c 100644
--- a/django_object_actions/tests/test_admin.py
+++ b/django_object_actions/tests/test_admin.py
@@ -47,7 +47,7 @@ class CommentTests(LoggedInTestCase):

 class ExtraTests(LoggedInTestCase):
     def test_action_on_a_model_with_complex_id(self):
-        related_data = RelatedDataFactory()
+        related_data = RelatedDataFactory(id="t)e_st id€")
         related_data_url = reverse(
             "admin:polls_relateddata_change", args=(related_data.pk,)
         )
@@ -56,6 +56,7 @@ class ExtraTests(LoggedInTestCase):
         )

         response = self.client.get(action_url)
+
         self.assertNotEqual(response.status_code, 404)
         self.assertRedirects(response, related_data_url)
nshafer commented 1 year ago

Sorry for lack of details, I was under a tight deadline at that time and just needed a fix ASAP. Anyways, I have created a new project that reproduces the error at https://github.com/nshafer/oatest. Steps to install and reproduce are in the README. What it really boils down to is that for an object with an ID such as "key_with_underscores", instead of that being passed to the get_change_actions method, the id is showing up as "key_5Fwith_5Funderscores" due to the django admin quoting it for the URL. Now the question is, should it be unquoted before calling get_change_actions as I've done in this PR, or should it be the responsibility of the user of this module that's overriding the method. If you look at the django admin in "django/contrib/admin/options.py" you see that they tend to call unquote(object_id) as late as possible, generally right as they are calling self.get_object(). So I could see an argument for that. If that's the case, maybe just add that call to unquote() in your sample docs in the README or make a general note about it, and then close this PR.