Reduce usage of edit handlers for workflow management (#6172)

* Use plain old Django forms for tasks

* Revert "Add ExcludeFieldsOnEditMixin for edit handlers, disabling specific fields when bound to an existing instance rather than creating a new one"

This reverts commit 62961b74bb.

* Move Workflow edit handler creation into Workflow forms

Only advantage of having it the way it was before is that allows
overriding the edit handler in subclasses. But nobody will be doing this
with workflows.

I've added a note into the code pointing out that we might want to
rethink using edit handlers if:

 - It gets easier to style formsets without using InlinePanel
 - We want to allow customisation of the form (the use of edit handlers
   should be considered an internal implementation detail at the moment)

* Make task name readonly but groups editable

* Update wagtail/admin/templates/wagtailadmin/workflows/task_chooser/includes/create_form.html

Co-authored-by: Dan Braghis <dan@zerolab.org>

* Update wagtail/admin/forms/workflows.py

Co-authored-by: Dan Braghis <dan@zerolab.org>

Co-authored-by: Dan Braghis <dan@zerolab.org>
pull/6257/head
Karl Hobley 2020-06-25 14:35:05 +01:00 zatwierdzone przez Matt Westcott
rodzic a80c34983b
commit edfd17a1f1
9 zmienionych plików z 147 dodań i 135 usunięć

Wyświetl plik

@ -26,7 +26,7 @@ All custom tasks must be models inheriting from ``wagtailcore.Task``. In this se
Subclassed Tasks follow the same approach as Pages: they are concrete models, with the specific subclass instance accessible by calling ``Task.specific()``.
You can now add any custom fields. To make these editable in the admin, they must also be added as panels.
You can now add any custom fields. To make these editable in the admin, add the names of the fields into the `admin_form_fields` attribute:
For example:
@ -36,18 +36,17 @@ For example:
from django.conf import settings
from django.db import models
from wagtail.admin.edit_handlers import FieldPanel
from wagtail.core.models import Task
class UserApprovalTask(Task):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True, blank=False)
panels = Task.panels + [FieldPanel('user')]
admin_form_fields = Task.admin_form_fields + ['user']
Any fields that shouldn't be edited after task creation - for example, anything that would fundamentally change the meaning of the task in any history logs -
can be added to ``exclude_on_edit``. For example:
Any fields that shouldn't be edited after task creation - for example, anything that would fundamentally change the meaning of the task in any history logs -
can be added to ``admin_form_readonly_on_edit_fields``. For example:
.. code-block:: python
@ -55,24 +54,48 @@ can be added to ``exclude_on_edit``. For example:
from django.conf import settings
from django.db import models
from wagtail.admin.edit_handlers import FieldPanel
from wagtail.core.models import Task
class UserApprovalTask(Task):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True, blank=False)
panels = Task.panels + [FieldPanel('user')]
admin_form_fields = Task.admin_form_fields + ['user']
# prevent editing of ``user`` after the task is created
exclude_on_edit = {'user'}
# by default, this attribute contains the 'name' field to prevent tasks from being renamed
admin_form_readonly_on_edit_fields = Task.admin_form_readonly_on_edit_fields + ['user']
Wagtail will choose a default form widget to use based on the field type. But you can override the form widget using the `admin_form_widgets` attribute:
.. code-block:: python
# <project>/models.py
from django.conf import settings
from django.db import models
from wagtail.core.models import Task
from .widgets import CustomUserChooserWidget
class UserApprovalTask(Task):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True, blank=False)
admin_form_fields = Task.admin_form_fields + ['user']
admin_form_widgets = {
'user': CustomUserChooserWidget,
}
Custom TaskState models
~~~~~~~~~~~~~~~~~~~~~~~
You might also need to store custom state information for the task: for example, a comment left by an approving user.
Normally, this is done on an instance of ``TaskState``, which is created when a page starts the task. However, this can
Normally, this is done on an instance of ``TaskState``, which is created when a page starts the task. However, this can
also be subclassed equivalently to ``Task``:
.. code-block:: python
@ -93,7 +116,6 @@ Your custom task must then be instructed to generate an instance of your custom
from django.conf import settings
from django.db import models
from wagtail.admin.edit_handlers import FieldPanel
from wagtail.core.models import Task, TaskState
@ -104,13 +126,10 @@ Your custom task must then be instructed to generate an instance of your custom
class UserApprovalTask(Task):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True, blank=False)
panels = Task.panels + [FieldPanel('user')]
admin_form_fields = Task.admin_form_fields + ['user']
task_state_class = UserApprovalTaskState
# prevent editing of ``user`` after the task is created
exclude_on_edit = {'user'}
Customising behaviour
~~~~~~~~~~~~~~~~~~~~~
@ -148,7 +167,7 @@ For example:
``Task.on_action(task_state, user, action_name)``:
This performs the actions specified in ``Task.get_actions(page, user)``: it is passed an action name, eg ``approve``, and the relevant task state. By default,
it calls ``approve`` and ``reject`` methods on the task state when the corresponding action names are passed through.
it calls ``approve`` and ``reject`` methods on the task state when the corresponding action names are passed through.
For example, let's say we wanted to add an additional option: cancelling the entire workflow:
@ -251,7 +270,7 @@ Next, you need to instantiate the notifier, and connect it to the ``task_submitt
def register_signal_handlers():
task_submitted.connect(user_approval_task_submission_email_notifier, dispatch_uid='user_approval_task_submitted_email_notification')
``register_signal_handlers()`` should then be run on loading the app: for example, by adding it to the ``ready()`` method in your ``AppConfig``
``register_signal_handlers()`` should then be run on loading the app: for example, by adding it to the ``ready()`` method in your ``AppConfig``
(and making sure this config is set as ``default_app_config`` in ``<project>/__init__.py``).
.. code-block:: python
@ -267,4 +286,4 @@ Next, you need to instantiate the notifier, and connect it to the ``task_submitt
def ready(self):
from .signal_handlers import register_signal_handlers
register_signal_handlers()
register_signal_handlers()

Wyświetl plik

@ -14,7 +14,7 @@ from taggit.managers import TaggableManager
from wagtail.admin import compare, widgets
from wagtail.core.fields import RichTextField
from wagtail.core.models import GroupApprovalTask, Page, Task, Workflow
from wagtail.core.models import Page
from wagtail.core.utils import camelcase_to_underscore, resolve_model_string
from wagtail.utils.decorators import cached_classmethod
@ -786,58 +786,6 @@ Page.settings_panels = [
Page.base_form_class = WagtailAdminPageForm
# Similarly, set up wagtailcore.Workflow to have edit handlers
Workflow.panels = [
FieldPanel("name", heading=gettext_lazy("Give your workflow a name")),
InlinePanel("workflow_tasks", heading=gettext_lazy("Add tasks to your workflow")),
]
Task.panels = [
FieldPanel("name", heading=gettext_lazy("Give your task a name")),
]
GroupApprovalTask.panels = Task.panels + [FieldPanel('groups', heading=gettext_lazy("Choose approval groups"))]
# do not allow editing of group post creation - this could lead to confusing history if a group is changed after tasks
# are started/completed
GroupApprovalTask.exclude_on_edit = {'groups'}
Workflow.base_form_class = WagtailAdminModelForm
Task.base_form_class = WagtailAdminModelForm
class ExcludeFieldsOnEditMixin:
"""A mixin for edit handlers, which disables fields listed in a model's 'exclude_on_edit' attribute when binding
to an existing instance - editing rather than creating"""
def bind_to(self, *args, **kwargs):
new = super(ExcludeFieldsOnEditMixin, self).bind_to(*args, **kwargs)
# when binding to an existing instance with a pk - ie editing - set those fields in the form to disabled
if new.form and new.instance and new.instance.pk is not None and hasattr(new.model, 'exclude_on_edit'):
for field in new.model.exclude_on_edit:
try:
new.form.fields[field].disabled = True
except KeyError:
continue
return new
class VaryOnEditObjectList(ExcludeFieldsOnEditMixin, ObjectList):
pass
@cached_classmethod
def get_simple_edit_handler(cls):
"""
Get the EditHandler to use in the Wagtail admin when editing this class, constructing an ObjectList from the contents of cls.panels.
"""
if hasattr(cls, 'edit_handler'):
edit_handler = cls.edit_handler
else:
edit_handler = VaryOnEditObjectList(cls.panels, base_form_class=cls.base_form_class)
return edit_handler.bind_to(model=cls)
Workflow.get_edit_handler = get_simple_edit_handler
Task.get_edit_handler = get_simple_edit_handler
@cached_classmethod
def get_edit_handler(cls):

Wyświetl plik

@ -1,10 +1,13 @@
from django import forms
from django.core.exceptions import ValidationError
from django.core.exceptions import ImproperlyConfigured, ValidationError
from django.utils.functional import cached_property
from django.utils.translation import ugettext as _
from django.utils.translation import ugettext_lazy as __
from wagtail.admin import widgets
from wagtail.admin.edit_handlers import FieldPanel, InlinePanel, ObjectList
from wagtail.admin.forms import WagtailAdminModelForm
from wagtail.admin.widgets.workflows import AdminTaskChooser
from wagtail.core.models import Page, Task, Workflow, WorkflowPage
from wagtail.core.utils import get_model_string
@ -141,3 +144,57 @@ class BaseWorkflowPagesFormSet(forms.BaseInlineFormSet):
WorkflowPagesFormSet = forms.inlineformset_factory(
Workflow, WorkflowPage, form=WorkflowPageForm, formset=BaseWorkflowPagesFormSet, extra=1, can_delete=True, fields=['page']
)
class BaseTaskForm(forms.ModelForm):
pass
def get_task_form_class(task_model, for_edit=False):
"""
Generates a form class for the given task model.
If the form is to edit an existing task, set for_edit to True. This applies
the readonly restrictions on fields defined in admin_form_readonly_on_edit_fields.
"""
fields = task_model.admin_form_fields
form_class = forms.modelform_factory(
task_model,
form=BaseTaskForm,
fields=fields,
widgets=getattr(task_model, 'admin_form_widgets', {})
)
if for_edit:
for field_name in getattr(task_model, 'admin_form_readonly_on_edit_fields', []):
if field_name not in form_class.base_fields:
raise ImproperlyConfigured(
"`%s.admin_form_readonly_on_edit_fields` contains the field "
"'%s' that doesn't exist. Did you forget to add "
"it to `%s.admin_form_fields`?"
% (task_model.__name__, field_name, task_model.__name__)
)
form_class.base_fields[field_name].disabled = True
return form_class
def get_workflow_edit_handler():
"""
Returns an edit handler which provides the "name" and "tasks" fields for workflow.
"""
# Note. It's a bit of a hack that we use edit handlers here. Ideally, it should be
# made easier to reuse the inline panel templates for any formset.
# Since this form is internal, we're OK with this for now. We might want to revisit
# this decision later if we decide to allow custom fields on Workflows.
panels = [
FieldPanel("name", heading=_("Give your workflow a name")),
InlinePanel("workflow_tasks", [
FieldPanel('task', widget=AdminTaskChooser(show_clear_link=False)),
], heading=_("Add tasks to your workflow")),
]
edit_handler = ObjectList(panels, base_form_class=WagtailAdminModelForm)
return edit_handler.bind_to(model=Workflow)

Wyświetl plik

@ -25,7 +25,15 @@
<form action="{{ view.get_add_url }}" enctype="multipart/form-data" method="POST" novalidate>
{% csrf_token %}
{% block form %}{{ edit_handler.render_form_content }}{% endblock %}
<ul class="fields nice-padding">
{% for field in form %}
{% if not field.is_hidden %}
{% include "wagtailadmin/shared/field_as_li.html" %}
{% else %}
{{ field }}
{% endif %}
{% endfor %}
</ul>
{% block footer %}
<footer role="contentinfo">

Wyświetl plik

@ -25,7 +25,15 @@
<form action="{{ view.edit_url }}" enctype="multipart/form-data" method="POST" novalidate>
{% csrf_token %}
{% block form %}{{ edit_handler.render_form_content }}{% endblock %}
<ul class="fields nice-padding">
{% for field in form %}
{% if not field.is_hidden %}
{% include "wagtailadmin/shared/field_as_li.html" %}
{% else %}
{{ field }}
{% endif %}
{% endfor %}
</ul>
{% block footer %}
<footer role="contentinfo">

Wyświetl plik

@ -8,7 +8,7 @@
{{ form }}
<div>
<button type="submit" class="button">{% trans "Create" %}</button>
<div class="clearfix">
<button type="submit" class="button action-save button-longrunning" data-clicked-text="{% trans 'Creating…' %}">{% trans "Create" %}</button>
</div>
</form>

Wyświetl plik

@ -596,7 +596,7 @@ class TestEditTaskView(TestCase, WagtailTestUtils):
def setUp(self):
delete_existing_workflows()
self.login()
self.task = SimpleTask.objects.create(name="test_task")
self.task = GroupApprovalTask.objects.create(name="test_task")
self.editor = get_user_model().objects.create_user(
username='editor',
@ -627,14 +627,23 @@ class TestEditTaskView(TestCase, WagtailTestUtils):
self.assertTemplateUsed(response, 'wagtailadmin/workflows/edit_task.html')
def test_post(self):
response = self.post({'name': 'test_task_modified', 'active': 'on'})
self.assertEqual(self.task.groups.count(), 0)
editors = Group.objects.get(name='Editors')
response = self.post({'name': 'test_task_modified', 'active': 'on', 'groups': [str(editors.id)]})
# Should redirect back to index
self.assertRedirects(response, reverse('wagtailadmin_workflows:task_index'))
# Check that the task was updated
task = Task.objects.get(id=self.task.id)
self.assertEqual(task.name, "test_task_modified")
task = GroupApprovalTask.objects.get(id=self.task.id)
# The task name cannot be changed
self.assertEqual(task.name, "test_task")
# This request should've added a group to the task
self.assertEqual(task.groups.count(), 1)
self.assertTrue(task.groups.filter(id=editors.id).exists())
def test_permissions(self):
self.login(user=self.editor)

Wyświetl plik

@ -15,11 +15,11 @@ from django.views.decorators.http import require_POST
from wagtail.admin import messages
from wagtail.admin.auth import PermissionPolicyChecker
from wagtail.admin.edit_handlers import Workflow
from wagtail.admin.forms.workflows import TaskChooserSearchForm, WorkflowPagesFormSet
from wagtail.admin.forms.workflows import (
TaskChooserSearchForm, WorkflowPagesFormSet, get_task_form_class, get_workflow_edit_handler)
from wagtail.admin.modal_workflow import render_modal_workflow
from wagtail.admin.views.generic import CreateView, DeleteView, EditView, IndexView
from wagtail.core.models import Page, Task, TaskState, WorkflowState
from wagtail.core.models import Page, Task, TaskState, Workflow, WorkflowState
from wagtail.core.permissions import task_permission_policy, workflow_permission_policy
from wagtail.core.utils import resolve_model_string
from wagtail.core.workflows import get_task_types
@ -65,18 +65,11 @@ class Create(CreateView):
def get_edit_handler(self):
if not self.edit_handler:
self.edit_handler = self.model.get_edit_handler()
self.edit_handler = self.edit_handler.bind_to(request=self.request)
self.edit_handler = get_workflow_edit_handler().bind_to(request=self.request)
return self.edit_handler
def get_form_class(self):
form_class = self.get_edit_handler().get_form_class()
# TODO Unhack
from wagtail.admin.widgets.workflows import AdminTaskChooser
form_class.formsets['workflow_tasks'].form.base_fields['task'].widget = AdminTaskChooser(show_clear_link=False)
return form_class
return self.get_edit_handler().get_form_class()
def get_form(self, form_class=None):
form = super().get_form(form_class)
@ -137,18 +130,11 @@ class Edit(EditView):
def get_edit_handler(self):
if not self.edit_handler:
self.edit_handler = self.model.get_edit_handler()
self.edit_handler = self.edit_handler.bind_to(request=self.request, instance=self.get_object())
self.edit_handler = get_workflow_edit_handler().bind_to(request=self.request)
return self.edit_handler
def get_form_class(self):
form_class = self.get_edit_handler().get_form_class()
# TODO Unhack
from wagtail.admin.widgets.workflows import AdminTaskChooser
form_class.formsets['workflow_tasks'].form.base_fields['task'].widget = AdminTaskChooser(show_clear_link=False)
return form_class
return self.get_edit_handler().get_form_class()
def get_form(self, form_class=None):
form = super().get_form(form_class)
@ -352,7 +338,6 @@ class CreateTask(CreateView):
edit_url_name = 'wagtailadmin_workflows:edit_task'
index_url_name = 'wagtailadmin_workflows:task_index'
header_icon = 'clipboard-list'
edit_handler = None
@cached_property
def model(self):
@ -370,24 +355,8 @@ class CreateTask(CreateView):
return model
def get_edit_handler(self):
if not self.edit_handler:
self.edit_handler = self.model.get_edit_handler()
self.edit_handler = self.edit_handler.bind_to(request=self.request)
return self.edit_handler
def get_form_class(self):
return self.get_edit_handler().get_form_class()
def get_form(self, form_class=None):
form = super().get_form(form_class)
self.edit_handler = self.edit_handler.bind_to(form=form)
return form
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context['edit_handler'] = self.edit_handler
return context
return get_task_form_class(self.model)
def get_add_url(self):
return reverse(self.add_url_name, kwargs={'app_label': self.kwargs.get('app_label'), 'model_name': self.kwargs.get('model_name')})
@ -407,7 +376,6 @@ class EditTask(EditView):
enable_item_label = _('Enable')
enable_url_name = 'wagtailadmin_workflows:enable_task'
header_icon = 'clipboard-list'
edit_handler = None
@cached_property
def model(self):
@ -420,23 +388,11 @@ class EditTask(EditView):
def get_object(self, queryset=None):
return super().get_object().specific
def get_edit_handler(self):
if not self.edit_handler:
self.edit_handler = self.model.get_edit_handler()
self.edit_handler = self.edit_handler.bind_to(request=self.request, instance=self.get_object())
return self.edit_handler
def get_form_class(self):
return self.get_edit_handler().get_form_class()
def get_form(self, form_class=None):
form = super().get_form(form_class)
self.edit_handler = self.edit_handler.bind_to(form=form)
return form
return get_task_form_class(self.model, for_edit=True)
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context['edit_handler'] = self.edit_handler
context['can_disable'] = (self.permission_policy is None or self.permission_policy.user_has_permission(self.request.user, 'delete')) and self.object.active
context['can_enable'] = (self.permission_policy is None or self.permission_policy.user_has_permission(self.request.user, 'create')) and not self.object.active
@ -561,9 +517,7 @@ def task_chooser(request):
task_type_choices.sort(key=lambda task_type: task_type[1].lower())
if create_model:
edit_handler = create_model.get_edit_handler()
edit_handler = edit_handler.bind_to(request=request)
createform_class = edit_handler.get_form_class()
createform_class = get_task_form_class(create_model)
else:
createform_class = None

Wyświetl plik

@ -4,6 +4,7 @@ from collections import defaultdict
from io import StringIO
from urllib.parse import urlparse
from django import forms
from django.conf import settings
from django.contrib.auth.models import Group, Permission
from django.contrib.contenttypes.models import ContentType
@ -2594,6 +2595,9 @@ class Task(models.Model):
"Active tasks can be added to workflows. Deactivating a task does not remove it from existing workflows."))
objects = TaskManager()
admin_form_fields = ['name']
admin_form_readonly_on_edit_fields = ['name']
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
if not self.id:
@ -2761,6 +2765,11 @@ class Workflow(ClusterableModel):
class GroupApprovalTask(Task):
groups = models.ManyToManyField(Group, verbose_name=_('groups'), help_text=_('Pages at this step in a workflow will be moderated or approved by these groups of users'))
admin_form_fields = Task.admin_form_fields + ['groups']
admin_form_widgets = {
'groups': forms.CheckboxSelectMultiple,
}
def start(self, workflow_state, user=None):
if workflow_state.page.locked_by:
# If the person who locked the page isn't in one of the groups, unlock the page