From c2bbcf38ad2cde88728d4000059d3ab15fce8359 Mon Sep 17 00:00:00 2001 From: crccheck Date: Mon, 9 Sep 2024 02:19:10 +0000 Subject: [PATCH 1/8] delint --- django_object_actions/utils.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py index 6bd3b60..05c09d6 100644 --- a/django_object_actions/utils.py +++ b/django_object_actions/utils.py @@ -315,9 +315,14 @@ def decorated_function(self, request, queryset): def action( - function=None, *, permissions=None, description=None, label=None, attrs=None, - methods=('GET', 'POST'), - button_type='a', + function=None, + *, + permissions=None, + description=None, + label=None, + attrs=None, + methods=("GET", "POST"), + button_type="a", ): """ Conveniently add attributes to an action function: From 2e6c5fb9187de65d1b459d4f777aae67f213a2ee Mon Sep 17 00:00:00 2001 From: crccheck Date: Mon, 9 Sep 2024 03:09:44 +0000 Subject: [PATCH 2/8] fix adding defaults and access to new methods and button_type --- .../django_object_actions/action_trigger.html | 18 +++++++++--------- django_object_actions/utils.py | 6 ++++-- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/django_object_actions/templates/django_object_actions/action_trigger.html b/django_object_actions/templates/django_object_actions/action_trigger.html index 34e1587..bbb5b1f 100644 --- a/django_object_actions/templates/django_object_actions/action_trigger.html +++ b/django_object_actions/templates/django_object_actions/action_trigger.html @@ -1,14 +1,6 @@ {% load add_preserved_filters from admin_urls %} -{% if tool.button_type == 'a' %} - {{ tool.label|capfirst }} -{% elif tool.button_type == 'form' %} +{% if tool.button_type == 'form' %}
{% csrf_token %} {{ tool.label|capfirst }}
+{% else %} + {{ tool.label|capfirst }} {% endif %} diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py index 05c09d6..5cb00da 100644 --- a/django_object_actions/utils.py +++ b/django_object_actions/utils.py @@ -2,6 +2,7 @@ from itertools import chain from django.contrib import messages +from django.contrib.admin import ModelAdmin from django.contrib.admin.utils import unquote from django.db.models.query import QuerySet from django.http import Http404, HttpResponseRedirect @@ -159,7 +160,7 @@ def _get_tool_dict(self, tool_name): label=getattr(tool, "label", tool_name.replace("_", " ").capitalize()), standard_attrs=standard_attrs, custom_attrs=custom_attrs, - button_type=tool.button_type, + button_type=getattr(tool, "button_type", "a"), ) def _get_button_attrs(self, tool): @@ -218,6 +219,7 @@ class BaseActionView(View): model = None actions = None current_app = None + methods = ("GET", "POST") @property def view_args(self): @@ -249,7 +251,7 @@ def dispatch(self, request, tool, **kwargs): except KeyError: raise Http404("Action does not exist") - if request.method not in view.methods: + if request.method not in self.methods: return HttpResponseNotAllowed(view.methods) ret = view(request, *self.view_args) From 2d80a5b7dd23818217d76bcc37801aacd13a5bdb Mon Sep 17 00:00:00 2001 From: crccheck Date: Mon, 9 Sep 2024 03:13:30 +0000 Subject: [PATCH 3/8] update test patch path --- django_object_actions/tests/test_admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/django_object_actions/tests/test_admin.py b/django_object_actions/tests/test_admin.py index 3dffa07..f4db02c 100644 --- a/django_object_actions/tests/test_admin.py +++ b/django_object_actions/tests/test_admin.py @@ -25,7 +25,7 @@ def test_action_on_a_model_with_uuid_pk_works(self): response = self.client.get(action_url) self.assertRedirects(response, comment_url) - @patch("django_object_actions.utils.ChangeActionView.get") + @patch("django_object_actions.utils.ChangeActionView.dispatch") def test_action_on_a_model_with_arbitrary_pk_works(self, mock_view): mock_view.return_value = HttpResponse() action_url = "/admin/polls/comment/{0}/actions/hodor/".format(" i am a pk ") @@ -35,7 +35,7 @@ def test_action_on_a_model_with_arbitrary_pk_works(self, mock_view): self.assertTrue(mock_view.called) self.assertEqual(mock_view.call_args[1]["pk"], " i am a pk ") - @patch("django_object_actions.utils.ChangeActionView.get") + @patch("django_object_actions.utils.ChangeActionView.dispatch") def test_action_on_a_model_with_slash_in_pk_works(self, mock_view): mock_view.return_value = HttpResponse() action_url = "/admin/polls/comment/{0}/actions/hodor/".format("pk/slash") From 92ee519725b0d605109e08c01336202db992d372 Mon Sep 17 00:00:00 2001 From: crccheck Date: Mon, 9 Sep 2024 03:20:26 +0000 Subject: [PATCH 4/8] nm, let's be strict and assume tool.button_type exists --- .../django_object_actions/action_trigger.html | 18 +++++++++--------- django_object_actions/utils.py | 1 - 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/django_object_actions/templates/django_object_actions/action_trigger.html b/django_object_actions/templates/django_object_actions/action_trigger.html index bbb5b1f..34e1587 100644 --- a/django_object_actions/templates/django_object_actions/action_trigger.html +++ b/django_object_actions/templates/django_object_actions/action_trigger.html @@ -1,6 +1,14 @@ {% load add_preserved_filters from admin_urls %} -{% if tool.button_type == 'form' %} +{% if tool.button_type == 'a' %} + {{ tool.label|capfirst }} +{% elif tool.button_type == 'form' %}
{% csrf_token %} {{ tool.label|capfirst }}
-{% else %} - {{ tool.label|capfirst }} {% endif %} diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py index 5cb00da..b1d90c6 100644 --- a/django_object_actions/utils.py +++ b/django_object_actions/utils.py @@ -2,7 +2,6 @@ from itertools import chain from django.contrib import messages -from django.contrib.admin import ModelAdmin from django.contrib.admin.utils import unquote from django.db.models.query import QuerySet from django.http import Http404, HttpResponseRedirect From 37d56e1fe00260ffb60d167f4a0677ddeb2a46b3 Mon Sep 17 00:00:00 2001 From: crccheck Date: Mon, 9 Sep 2024 04:50:06 +0000 Subject: [PATCH 5/8] doc using POST --- README.md | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e2038ef..6cb35c5 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ class ArticleAdmin(DjangoObjectActions, admin.ModelAdmin): ## Usage -Defining new &_tool actions_ is just like defining regular [admin actions]. The +Defining new _tool actions_ is just like defining regular [admin actions]. The major difference is the functions for `django-object-actions` will take an object instance instead of a queryset (see _Re-using Admin Actions_ below). @@ -176,6 +176,24 @@ def get_change_actions(self, request, object_id, form_url): The same is true for changelist actions with `get_changelist_actions`. +### Using POST instead of GET for actions + +⚠️ This is a beta feature and subject to change + +Since actions usually change data, for safety and semantics, it would be +preferable that actions use a HTTP POST instead of a GET. + +You can configure an action to only use POST with: + +```python +@action(methods=("post",), button_type="form") +``` + +One caveat is Django's styling is pinned to anchor tags[^1], so to maintain visual +consistency with other actions, we have to use anchor tags too. + +[^1]: https://github.com/django/django/blob/826ef006681eae1e9b4bd0e4f18fa13713025cba/django/contrib/admin/static/admin/css/base.css#L786 + ### Alternate Installation You don't have to add this to `INSTALLED_APPS`, all you need to to do From 6c6e6f0b5f4dbee165ecf2b13ac8a0cd53790734 Mon Sep 17 00:00:00 2001 From: crccheck Date: Mon, 9 Sep 2024 05:47:25 +0000 Subject: [PATCH 6/8] add test coverage and example of a POST only tool --- README.md | 8 ++++---- django_object_actions/tests/test_admin.py | 8 +++++++- django_object_actions/tests/tests.py | 1 + django_object_actions/utils.py | 8 +++++--- example_project/polls/admin.py | 2 +- 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 6cb35c5..615c024 100644 --- a/README.md +++ b/README.md @@ -186,11 +186,12 @@ preferable that actions use a HTTP POST instead of a GET. You can configure an action to only use POST with: ```python -@action(methods=("post",), button_type="form") +@action(methods=("POST",), button_type="form") ``` -One caveat is Django's styling is pinned to anchor tags[^1], so to maintain visual -consistency with other actions, we have to use anchor tags too. +One caveat is Django's styling is pinned to anchor tags[^1], so to maintain +visual consistency with other actions, we have to use anchor tags too and use +JavaScript to turn make it act like a form. [^1]: https://github.com/django/django/blob/826ef006681eae1e9b4bd0e4f18fa13713025cba/django/contrib/admin/static/admin/css/base.css#L786 @@ -267,4 +268,3 @@ open a simple form in a modal dialog. If you want an actions menu for each row of your changelist, check out [Django Admin Row Actions](https://github.com/DjangoAdminHackers/django-admin-row-actions). - diff --git a/django_object_actions/tests/test_admin.py b/django_object_actions/tests/test_admin.py index f4db02c..70e268f 100644 --- a/django_object_actions/tests/test_admin.py +++ b/django_object_actions/tests/test_admin.py @@ -76,10 +76,16 @@ def test_changelist_template_context(self): self.assertIn("foo", response.context_data) def test_changelist_action_view(self): - url = "/admin/polls/choice/actions/delete_all/" + url = reverse("admin:polls_choice_actions", args=("delete_all",)) response = self.client.get(url) self.assertRedirects(response, "/admin/polls/choice/") + def test_changelist_action_post_only_tool_rejects_get(self): + poll = PollFactory.create() + url = reverse("admin:polls_choice_actions", args=(poll.pk, "reset_vote")) + response = self.client.get(url) + self.assertEqual(response.status_code, 405) + def test_changelist_nonexistent_action(self): url = "/admin/polls/choice/actions/xyzzy/" response = self.client.get(url) diff --git a/django_object_actions/tests/tests.py b/django_object_actions/tests/tests.py index bee08b3..e78fa4b 100644 --- a/django_object_actions/tests/tests.py +++ b/django_object_actions/tests/tests.py @@ -46,6 +46,7 @@ def test_can_return_template(self): # it's good to document that this is something we can do. url = reverse("admin:polls_poll_actions", args=(1, "delete_all_choices")) response = self.client.get(url) + print(url, response) self.assertTemplateUsed(response, "clear_choices.html") def test_message_user_sends_message(self): diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py index b1d90c6..99afb40 100644 --- a/django_object_actions/utils.py +++ b/django_object_actions/utils.py @@ -218,7 +218,6 @@ class BaseActionView(View): model = None actions = None current_app = None - methods = ("GET", "POST") @property def view_args(self): @@ -250,8 +249,11 @@ def dispatch(self, request, tool, **kwargs): except KeyError: raise Http404("Action does not exist") - if request.method not in self.methods: - return HttpResponseNotAllowed(view.methods) + # TODO move ('get', 'post' config default someplace else) + allowed_methods = getattr(view, "methods", ("GET", "POST")) + print("dispatch", request.method, allowed_methods) + if request.method.upper() not in allowed_methods: + return HttpResponseNotAllowed(allowed_methods) ret = view(request, *self.view_args) if isinstance(ret, HttpResponseBase): diff --git a/example_project/polls/admin.py b/example_project/polls/admin.py index 8d784a8..3c2ffdb 100644 --- a/example_project/polls/admin.py +++ b/example_project/polls/admin.py @@ -45,7 +45,7 @@ def decrement_vote(self, request, obj): def delete_all(self, request, queryset): self.message_user(request, "just kidding!") - @action(description="0") + @action(description="0", methods=("POST",), button_type="form") def reset_vote(self, request, obj): obj.votes = 0 obj.save() From f52099ff06b5bb1a55a786e5c413d1cd8a882ff3 Mon Sep 17 00:00:00 2001 From: crccheck Date: Mon, 9 Sep 2024 05:58:04 +0000 Subject: [PATCH 7/8] refactor: move defaults to a const --- django_object_actions/tests/tests.py | 1 - django_object_actions/utils.py | 13 +++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/django_object_actions/tests/tests.py b/django_object_actions/tests/tests.py index e78fa4b..bee08b3 100644 --- a/django_object_actions/tests/tests.py +++ b/django_object_actions/tests/tests.py @@ -46,7 +46,6 @@ def test_can_return_template(self): # it's good to document that this is something we can do. url = reverse("admin:polls_poll_actions", args=(1, "delete_all_choices")) response = self.client.get(url) - print(url, response) self.assertTemplateUsed(response, "clear_choices.html") def test_message_user_sends_message(self): diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py index 99afb40..2635d07 100644 --- a/django_object_actions/utils.py +++ b/django_object_actions/utils.py @@ -11,6 +11,9 @@ from django.views.generic.list import MultipleObjectMixin from django.urls import re_path, reverse +DEFAULT_METHODS_ALLOWED = ("GET", "POST") +DEFAULT_BUTTON_TYPE = "a" + class BaseDjangoObjectActions(object): """ @@ -159,7 +162,7 @@ def _get_tool_dict(self, tool_name): label=getattr(tool, "label", tool_name.replace("_", " ").capitalize()), standard_attrs=standard_attrs, custom_attrs=custom_attrs, - button_type=getattr(tool, "button_type", "a"), + button_type=getattr(tool, "button_type", DEFAULT_BUTTON_TYPE), ) def _get_button_attrs(self, tool): @@ -249,9 +252,7 @@ def dispatch(self, request, tool, **kwargs): except KeyError: raise Http404("Action does not exist") - # TODO move ('get', 'post' config default someplace else) - allowed_methods = getattr(view, "methods", ("GET", "POST")) - print("dispatch", request.method, allowed_methods) + allowed_methods = getattr(view, "methods", DEFAULT_METHODS_ALLOWED) if request.method.upper() not in allowed_methods: return HttpResponseNotAllowed(allowed_methods) @@ -324,8 +325,8 @@ def action( description=None, label=None, attrs=None, - methods=("GET", "POST"), - button_type="a", + methods=DEFAULT_METHODS_ALLOWED, + button_type=DEFAULT_BUTTON_TYPE, ): """ Conveniently add attributes to an action function: From b16e50595e254e7dd749436c0e93135192f03426 Mon Sep 17 00:00:00 2001 From: crccheck Date: Tue, 10 Sep 2024 03:22:34 +0000 Subject: [PATCH 8/8] docs: wording cleanup --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 615c024..8548a71 100644 --- a/README.md +++ b/README.md @@ -190,8 +190,8 @@ You can configure an action to only use POST with: ``` One caveat is Django's styling is pinned to anchor tags[^1], so to maintain -visual consistency with other actions, we have to use anchor tags too and use -JavaScript to turn make it act like a form. +visual consistency, we have to use anchor tags and use JavaScript to make it act +like the submit button of the form. [^1]: https://github.com/django/django/blob/826ef006681eae1e9b4bd0e4f18fa13713025cba/django/contrib/admin/static/admin/css/base.css#L786