Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unquote object_id when calling get_change_actions #137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nshafer
Copy link

@nshafer nshafer commented Jun 6, 2022

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 crccheck changed the title Unquote object_id when calling get_change_actions fix: unquote object_id when calling get_change_actions Jun 28, 2022
@crccheck
Copy link
Owner

crccheck commented Nov 4, 2022

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
Copy link
Author

nshafer commented Nov 4, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants