From a616d3747531aeb25c916c711c9c939165f2a379 Mon Sep 17 00:00:00 2001 From: Wiktor Latanowicz Date: Mon, 7 Nov 2022 09:30:36 +0100 Subject: [PATCH 1/3] Invoke actions on POST only --- CHANGELOG.md | 5 ++++ .../django_object_actions/change_form.html | 17 ++++++----- .../django_object_actions/change_list.html | 17 ++++++----- django_object_actions/tests/test_admin.py | 30 ++++++++++++------- django_object_actions/tests/test_urls.py | 2 +- django_object_actions/tests/tests.py | 16 +++++----- django_object_actions/utils.py | 5 +--- example_project/polls/admin.py | 2 +- .../polls/templates/clear_choices.html | 2 +- 9 files changed, 57 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8ca3aa..50e3d4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,11 @@ # Changelog +### Feature +* Drop support for GET method. All action are now invoked with POST method. + +### Breaking +* When dealing with a secondary form in action, you cannot simply check the http method to determine if the form should be rendered or processed. You need to check for specific form inputs in POST payload. ## v4.1.0 (2022-11-14) ### Feature diff --git a/django_object_actions/templates/django_object_actions/change_form.html b/django_object_actions/templates/django_object_actions/change_form.html index 1b0f80d..846fb7d 100644 --- a/django_object_actions/templates/django_object_actions/change_form.html +++ b/django_object_actions/templates/django_object_actions/change_form.html @@ -5,13 +5,16 @@ {% for tool in objectactions %}
  • {% url tools_view_name pk=object_id tool=tool.name as action_url %} - - {{ tool.label|capfirst }} - +
    + {% csrf_token %} + + {{ tool.label|capfirst }} + +
    • {% endfor %} {{ block.super }} diff --git a/django_object_actions/templates/django_object_actions/change_list.html b/django_object_actions/templates/django_object_actions/change_list.html index 2fd9082..b7c9b80 100644 --- a/django_object_actions/templates/django_object_actions/change_list.html +++ b/django_object_actions/templates/django_object_actions/change_list.html @@ -5,13 +5,16 @@ {% for tool in objectactions %}
  • {% url tools_view_name tool=tool.name as action_url %} - - {{ tool.label|capfirst }} - +
    + {% csrf_token %} + + {{ tool.label|capfirst }} + +
    • {% endfor %} {{ block.super }} diff --git a/django_object_actions/tests/test_admin.py b/django_object_actions/tests/test_admin.py index db6e7da..8fba3b4 100644 --- a/django_object_actions/tests/test_admin.py +++ b/django_object_actions/tests/test_admin.py @@ -21,25 +21,25 @@ def test_action_on_a_model_with_uuid_pk_works(self): action_url = "/admin/polls/comment/{0}/actions/hodor/".format(comment.pk) # sanity check that url has a uuid self.assertIn("-", action_url) - response = self.client.get(action_url) + response = self.client.post(action_url) self.assertRedirects(response, comment_url) - @patch("django_object_actions.utils.ChangeActionView.get") + @patch("django_object_actions.utils.ChangeActionView.post") 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 ") - self.client.get(action_url) + self.client.post(action_url) 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.post") 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") - self.client.get(action_url) + self.client.post(action_url) self.assertTrue(mock_view.called) self.assertEqual(mock_view.call_args[1]["pk"], "pk/slash") @@ -55,7 +55,7 @@ def test_action_on_a_model_with_complex_id(self): quote(related_data.pk) ) - response = self.client.get(action_url) + response = self.client.post(action_url) self.assertNotEqual(response.status_code, 404) self.assertRedirects(response, related_data_url) @@ -76,12 +76,12 @@ def test_changelist_template_context(self): def test_changelist_action_view(self): url = "/admin/polls/choice/actions/delete_all/" - response = self.client.get(url) + response = self.client.post(url) self.assertRedirects(response, "/admin/polls/choice/") def test_changelist_nonexistent_action(self): url = "/admin/polls/choice/actions/xyzzy/" - response = self.client.get(url) + response = self.client.post(url) self.assertEqual(response.status_code, 404) def test_get_changelist_can_remove_action(self): @@ -94,13 +94,23 @@ def test_get_changelist_can_remove_action(self): response = self.client.get(admin_change_url) self.assertIn(action_url, response.rendered_content) - response = self.client.get(action_url) # Click on the button + response = self.client.post(action_url) # Click on the button self.assertRedirects(response, admin_change_url) # button is not in the admin anymore response = self.client.get(admin_change_url) self.assertNotIn(action_url, response.rendered_content) + def test_changelist_get_method_action_view(self): + url = "/admin/polls/choice/actions/delete_all/" + response = self.client.get(url) + self.assertEqual(response.status_code, 405) + + def test_changelist_get_method_nonexistent_action(self): + url = "/admin/polls/choice/actions/xyzzy/" + response = self.client.get(url) + self.assertEqual(response.status_code, 405) + class ChangeListTests(LoggedInTestCase): def test_changelist_template_context(self): @@ -122,5 +132,5 @@ def test_redirect_back_from_secondary_admin(self): action_url = "/support/polls/poll/1/actions/question_mark/" self.assertTrue(admin_change_url.startswith("/support/")) - response = self.client.get(action_url) + response = self.client.post(action_url) self.assertRedirects(response, admin_change_url) diff --git a/django_object_actions/tests/test_urls.py b/django_object_actions/tests/test_urls.py index 251b8cb..04700ef 100644 --- a/django_object_actions/tests/test_urls.py +++ b/django_object_actions/tests/test_urls.py @@ -11,5 +11,5 @@ def test_changelist_action_is_rendered(self): response = self.client.get(reverse("admin:polls_choice_changelist")) self.assertEqual(response.status_code, 200) self.assertIn( - b'href="/admin/polls/choice/actions/delete_all/"', response.content + b'action="/admin/polls/choice/actions/delete_all/"', response.content ) diff --git a/django_object_actions/tests/tests.py b/django_object_actions/tests/tests.py index bee08b3..a64052b 100644 --- a/django_object_actions/tests/tests.py +++ b/django_object_actions/tests/tests.py @@ -22,7 +22,7 @@ class AppTests(LoggedInTestCase): def test_tool_func_gets_executed(self): c = Choice.objects.get(pk=1) votes = c.votes - response = self.client.get( + response = self.client.post( reverse("admin:polls_choice_actions", args=(1, "increment_vote")) ) self.assertEqual(response.status_code, 302) @@ -34,7 +34,7 @@ def test_tool_func_gets_executed(self): def test_tool_can_return_httpresponse(self): # we know this url works because of fixtures url = reverse("admin:polls_choice_actions", args=(2, "edit_poll")) - response = self.client.get(url) + response = self.client.post(url) # we expect a redirect self.assertEqual(response.status_code, 302) self.assertTrue( @@ -45,24 +45,24 @@ def test_can_return_template(self): # This is more of a test of render_to_response than the app, but I think # 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) + response = self.client.post(url) self.assertTemplateUsed(response, "clear_choices.html") def test_message_user_sends_message(self): url = reverse("admin:polls_poll_actions", args=(1, "delete_all_choices")) self.assertNotIn("messages", self.client.cookies) - self.client.get(url) + self.client.post(url) self.assertIn("messages", self.client.cookies) def test_intermediate_page_with_post_works(self): self.assertTrue(Choice.objects.filter(poll=1).count()) url = reverse("admin:polls_poll_actions", args=(1, "delete_all_choices")) - response = self.client.post(url) + response = self.client.post(url, data={"sure": "Sure"}) self.assertEqual(response.status_code, 302) self.assertEqual(Choice.objects.filter(poll=1).count(), 0) def test_undefined_tool_404s(self): - response = self.client.get( + response = self.client.post( reverse("admin:polls_poll_actions", args=(1, "weeeewoooooo")) ) self.assertEqual(response.status_code, 404) @@ -70,10 +70,10 @@ def test_undefined_tool_404s(self): def test_key_error_tool_500s(self): self.assertRaises( KeyError, - self.client.get, + self.client.post, reverse("admin:polls_choice_actions", args=(1, "raise_key_error")), ) def test_render_button(self): - response = self.client.get(reverse("admin:polls_choice_change", args=(1,))) + response = self.client.post(reverse("admin:polls_choice_change", args=(1,))) self.assertEqual(response.status_code, 200) diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py index 095ab11..6eca0e2 100644 --- a/django_object_actions/utils.py +++ b/django_object_actions/utils.py @@ -238,7 +238,7 @@ def back_url(self): """ raise NotImplementedError - def get(self, request, tool, **kwargs): + def post(self, request, tool, **kwargs): # Fix for case if there are special symbols in object pk for k, v in self.kwargs.items(): self.kwargs[k] = unquote(v) @@ -254,9 +254,6 @@ def get(self, request, tool, **kwargs): return HttpResponseRedirect(self.back_url) - # HACK to allow POST requests too - post = get - def message_user(self, request, message): """ Mimic Django admin actions's `message_user`. diff --git a/example_project/polls/admin.py b/example_project/polls/admin.py index 72c2e74..f99ecde 100644 --- a/example_project/polls/admin.py +++ b/example_project/polls/admin.py @@ -108,7 +108,7 @@ def change_view(self, request, object_id, form_url="", extra_context=None): def delete_all_choices(self, request, obj): from django.shortcuts import render - if request.method == "POST": + if "sure" in request.POST: obj.choice_set.all().delete() return diff --git a/example_project/polls/templates/clear_choices.html b/example_project/polls/templates/clear_choices.html index 6d4e8f2..fd352c8 100644 --- a/example_project/polls/templates/clear_choices.html +++ b/example_project/polls/templates/clear_choices.html @@ -1,5 +1,5 @@
    {% csrf_token %} Delete All Choices? - +
    From a7ad92a13e46deaec38136be45dae69ca4ee0e3a Mon Sep 17 00:00:00 2001 From: Wiktor Latanowicz Date: Mon, 7 Nov 2022 09:56:08 +0100 Subject: [PATCH 2/3] Support for inline forms for action buttons --- CHANGELOG.md | 1 + README.md | 19 ++++++ .../django_object_actions/css/style.css | 6 ++ .../django_object_actions/change_form.html | 61 +++++++++++++++---- .../django_object_actions/change_list.html | 61 +++++++++++++++---- django_object_actions/tests/test_admin.py | 34 +++++++++++ django_object_actions/tests/tests.py | 13 ++++ django_object_actions/utils.py | 18 +++++- example_project/polls/admin.py | 27 +++++++- 9 files changed, 212 insertions(+), 28 deletions(-) create mode 100644 django_object_actions/static/django_object_actions/css/style.css diff --git a/CHANGELOG.md b/CHANGELOG.md index 50e3d4f..917ca88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### Feature * Drop support for GET method. All action are now invoked with POST method. +* Add option to include inline forms with actions. ### Breaking * When dealing with a secondary form in action, you cannot simply check the http method to determine if the form should be rendered or processed. You need to check for specific form inputs in POST payload. diff --git a/README.md b/README.md index da479d0..9f826aa 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,25 @@ increment_vote.attrs = { } ``` +## Adding inline forms + +You can add parameters to the action button by adding Django [Form](https://docs.djangoproject.com/en/4.1/ref/forms/api/#django.forms.Form) object to it. Parameter values can be read form request's `POST` property. + +```python +from django import forms + +class ResetAllForm(forms.Form): + new_value = forms.IntegerField(initial=0) + +def reset_all(self, request, queryset): + new_value = int(request.POST["new_value"]) + queryset.update(value=new_value) +reset_all.form = ResetAllForm() +``` + +Each action with form assigned is rendered in it's own, separate row. + + ### Programmatically Disabling Actions You can programmatically disable registered actions by defining your own diff --git a/django_object_actions/static/django_object_actions/css/style.css b/django_object_actions/static/django_object_actions/css/style.css new file mode 100644 index 0000000..ef40852 --- /dev/null +++ b/django_object_actions/static/django_object_actions/css/style.css @@ -0,0 +1,6 @@ +ul.object-tools.django-object-actions { + margin-top: 0; + padding-top: 16px; + position: relative; + top: -24px; +} diff --git a/django_object_actions/templates/django_object_actions/change_form.html b/django_object_actions/templates/django_object_actions/change_form.html index 846fb7d..b33d787 100644 --- a/django_object_actions/templates/django_object_actions/change_form.html +++ b/django_object_actions/templates/django_object_actions/change_form.html @@ -1,21 +1,56 @@ {% extends "admin/change_form.html" %} {% load add_preserved_filters from admin_urls %} +{% load static %} + +{% block extrastyle %} +{{ block.super }} + +{% endblock %} {% block object-tools-items %} {% for tool in objectactions %} -
  • - {% url tools_view_name pk=object_id tool=tool.name as action_url %} -
    - {% csrf_token %} - - {{ tool.label|capfirst }} - -
    -
    • + {% if not tool.form %} +
  • + {% url tools_view_name pk=object_id tool=tool.name as action_url %} +
    + {% csrf_token %} + + {{ tool.label|capfirst }} + +
    +
    • + {% endif %} {% endfor %} {{ block.super }} {% endblock %} + +{% block object-tools %} + {{ block.super }} + {% for tool in objectactions %} + {% if tool.form %} + {% url tools_view_name pk=object_id tool=tool.name as action_url %} +
    +
    + {% csrf_token %} + +
    +
    + {% endif %} + {% endfor %} +
    +{% endblock %} diff --git a/django_object_actions/templates/django_object_actions/change_list.html b/django_object_actions/templates/django_object_actions/change_list.html index b7c9b80..4e6fba6 100644 --- a/django_object_actions/templates/django_object_actions/change_list.html +++ b/django_object_actions/templates/django_object_actions/change_list.html @@ -1,21 +1,56 @@ {% extends "admin/change_list.html" %} {% load add_preserved_filters from admin_urls %} +{% load static %} + +{% block extrastyle %} +{{ block.super }} + +{% endblock %} {% block object-tools-items %} {% for tool in objectactions %} -
  • - {% url tools_view_name tool=tool.name as action_url %} -
    - {% csrf_token %} - - {{ tool.label|capfirst }} - -
    -
    • + {% if not tool.form %} +
  • + {% url tools_view_name tool=tool.name as action_url %} +
    + {% csrf_token %} + + {{ tool.label|capfirst }} + +
    +
    • + {% endif %} {% endfor %} {{ block.super }} {% endblock %} + +{% block object-tools %} + {{ block.super }} + {% for tool in objectactions %} + {% if tool.form %} + {% url tools_view_name tool=tool.name as action_url %} +
    +
    + {% csrf_token %} + +
    +
    + {% endif %} + {% endfor %} +
    +{% endblock %} diff --git a/django_object_actions/tests/test_admin.py b/django_object_actions/tests/test_admin.py index 8fba3b4..ecbda4b 100644 --- a/django_object_actions/tests/test_admin.py +++ b/django_object_actions/tests/test_admin.py @@ -8,6 +8,7 @@ from .tests import LoggedInTestCase from example_project.polls.factories import ( + ChoiceFactory, CommentFactory, PollFactory, RelatedDataFactory, @@ -134,3 +135,36 @@ def test_redirect_back_from_secondary_admin(self): response = self.client.post(action_url) self.assertRedirects(response, admin_change_url) + + +class FormTests(LoggedInTestCase): + def test_form_is_rendered_in_change_view(self): + choice = ChoiceFactory() + admin_change_url = reverse("admin:polls_choice_change", args=(choice.pk,)) + + response = self.client.get(admin_change_url) + + # form is in the admin + action_url_lookup = 'action="/admin/polls/choice/1/actions/change_votes/"' + self.assertIn(action_url_lookup, response.rendered_content) + form_lookup = '
    Date: Wed, 28 Dec 2022 14:41:28 +0100 Subject: [PATCH 3/3] Add support for intermediate page with "modal" form --- .../django_object_actions/change_form.html | 4 ++-- .../django_object_actions/change_list.html | 4 ++-- .../django_object_actions/modal_form.html | 15 ++++++++++++ django_object_actions/utils.py | 24 ++++++++++++++++--- example_project/polls/admin.py | 8 +++++++ 5 files changed, 48 insertions(+), 7 deletions(-) create mode 100644 django_object_actions/templates/django_object_actions/modal_form.html diff --git a/django_object_actions/templates/django_object_actions/change_form.html b/django_object_actions/templates/django_object_actions/change_form.html index b33d787..60b1783 100644 --- a/django_object_actions/templates/django_object_actions/change_form.html +++ b/django_object_actions/templates/django_object_actions/change_form.html @@ -9,7 +9,7 @@ {% block object-tools-items %} {% for tool in objectactions %} - {% if not tool.form %} + {% if not tool.has_inline_form %}
  • {% url tools_view_name pk=object_id tool=tool.name as action_url %} @@ -31,7 +31,7 @@ {% block object-tools %} {{ block.super }} {% for tool in objectactions %} - {% if tool.form %} + {% if tool.has_inline_form %} {% url tools_view_name pk=object_id tool=tool.name as action_url %}
    diff --git a/django_object_actions/templates/django_object_actions/change_list.html b/django_object_actions/templates/django_object_actions/change_list.html index 4e6fba6..906ef0c 100644 --- a/django_object_actions/templates/django_object_actions/change_list.html +++ b/django_object_actions/templates/django_object_actions/change_list.html @@ -9,7 +9,7 @@ {% block object-tools-items %} {% for tool in objectactions %} - {% if not tool.form %} + {% if not tool.has_inline_form %}
  • {% url tools_view_name tool=tool.name as action_url %} @@ -31,7 +31,7 @@ {% block object-tools %} {{ block.super }} {% for tool in objectactions %} - {% if tool.form %} + {% if tool.has_inline_form %} {% url tools_view_name tool=tool.name as action_url %}
    diff --git a/django_object_actions/templates/django_object_actions/modal_form.html b/django_object_actions/templates/django_object_actions/modal_form.html new file mode 100644 index 0000000..cbb87be --- /dev/null +++ b/django_object_actions/templates/django_object_actions/modal_form.html @@ -0,0 +1,15 @@ +{% extends "admin/base_site.html" %} +{% load add_preserved_filters from admin_urls %} + +{% block content %} +
    + + {% csrf_token %} + {{ form.as_table }} +
    + + +
    + +
    +{% endblock %} diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py index 4a40698..a2c92e0 100644 --- a/django_object_actions/utils.py +++ b/django_object_actions/utils.py @@ -11,6 +11,7 @@ from django.views.generic.detail import SingleObjectMixin from django.views.generic.list import MultipleObjectMixin from django.urls import re_path, reverse +from django.shortcuts import render class BaseDjangoObjectActions(object): @@ -155,12 +156,14 @@ def _get_tool_dict(self, tool_name): """Represents the tool as a dict with extra meta.""" tool = getattr(self, tool_name) standard_attrs, custom_attrs = self._get_button_attrs(tool) + form, is_inline = self._get_form(tool) return dict( name=tool_name, label=getattr(tool, "label", tool_name.replace("_", " ").capitalize()), standard_attrs=standard_attrs, custom_attrs=custom_attrs, - form=self._get_form(tool), + form=form, + has_inline_form=is_inline, ) def _get_button_attrs(self, tool): @@ -198,7 +201,8 @@ def _get_form(self, tool): form = getattr(tool, "form", None) if callable(form) and not isinstance(form, Form): form = form() - return form + is_inline = form and not getattr(tool, "form_in_modal", False) + return form, is_inline class DjangoObjectActions(BaseDjangoObjectActions): @@ -256,6 +260,17 @@ def post(self, request, tool, **kwargs): except KeyError: raise Http404("Action does not exist") + form = getattr(view, "form", None) + show_modal = form and getattr(view, "form_in_modal", False) + + if show_modal and "modal_cancel" in request.POST: + return HttpResponseRedirect(self.back_url) + + if show_modal and "modal_submit" not in request.POST: + return render( + request, "django_object_actions/modal_form.html", {"form": form} + ) + ret = view(request, *self.view_args) if isinstance(ret, HttpResponseBase): return ret @@ -325,7 +340,8 @@ def action( description=None, label=None, attrs=None, - form=None + form=None, + form_in_modal=None, ): """ Conveniently add attributes to an action function:: @@ -363,6 +379,8 @@ def decorator(func): func.attrs = attrs if form is not None: func.form = form + if form_in_modal is not None: + func.form_in_modal = form_in_modal return func if function is None: diff --git a/example_project/polls/admin.py b/example_project/polls/admin.py index 2ce55ef..f77e21a 100644 --- a/example_project/polls/admin.py +++ b/example_project/polls/admin.py @@ -34,6 +34,13 @@ def change_votes(self, request, obj): obj.votes += change_by obj.save() + @action(form=ChangeVotesForm, form_in_modal=True) + def change_votes_modal(self, request, obj): + change_by = int(request.POST["change_by"]) + obj.votes += change_by + obj.save() + self.message_user(request, f"Number of votes changed by {change_by}") + @action( description="+1", label="vote++", @@ -82,6 +89,7 @@ def raise_key_error(self, request, obj): "increment_vote", "decrement_vote", "change_votes", + "change_votes_modal", "reset_vote", "edit_poll", "raise_key_error",