[PATCH v2 2/5] patch-list: clean up patch-list page and refactor patch forms
Jonathan Nieder
jrnieder at gmail.com
Tue Jul 27 10:19:20 AEST 2021
Raxel Gutierrez wrote:
> Clean up the patch-list page by moving forms to a new template file
> patch-forms.html and moving them to the top of the page, adding ids to
> table cells, and renaming selectors using hyphen delimited strings where
> the relevant changes were made. Also, created a partial template for
nit: s/created/create/
> errors that render with form submission. These changes improve the
> discoverability of the patch-list forms and makes the code healthier,
> ready for change, and overall more readable.
nit: s/makes/make/
> Also, move patch-list related js code to a new patch-list.js file, to
> make the JavaScript easy to read and change in one place. This makes
> automatic code formatting easier, makes it more straightforward to
> measure test coverage and discover opportunities for refactoring, and
> simplifies a possible future migration to TypeScript if the project
> chooses to go in that direction.
>
> Refactor forms.py, __init__.py, patch.py, and test_bundles.py files
> so that the shared bundle form in patch-forms.html works for both the
> patch-list and patch-detail pages. Error messages and success/warning
> messages are now normalized throughout these files for testing and
> functionality. Also, important to these change is that CreateBundleForm
> in forms.py handles the validation of the bundle form input so that now
> the patch-list bundle form also makes use of the Django forms API.
I wonder if there's a way to explain this change that is easier to scan
through, focusing on the overall effect. That is, I wonder
- what user-visible change should I expect from this change? The commit
message may want to lead with that.
- what was difficult to do in this code that is easier to do now? That
seems like a second useful thing to point out
- only then, once those are out of the way, does it seem like a good
moment to list what stylistic changes occured to accomplish this, for
example as a bulleted list
[...]
> --- a/htdocs/README.rst
> +++ b/htdocs/README.rst
> @@ -131,6 +131,13 @@ js
> :GitHub: https://github.com/js-cookie/js-cookie/
> :Version: 2.2.1
>
> +``patch-list.js.``
> +
> + Utility functions for patch list manipulation (inline dropdown changes,
> + etc.)
> +
> + Part of Patchwork.
> +
Makes sense.
Does patch-list.js contain event handlers and other helpers for
patch-list.html specifically? In other words, is it only used by that
one file? If so, it might make sense to say so --- e.g.
``patch-list.js.``
Event helpers and other application logic for patch-list.html. These
support patch list manipulation (for example, inline dropdown changes).
Part of Patchwork.
[...]
> --- a/htdocs/css/style.css
> +++ b/htdocs/css/style.css
> @@ -122,7 +122,7 @@ a.colinactive:hover {
> div.filters {
> }
>
> -div.patchforms {
> +div.patch-forms {
Makes sense.
[...]
> --- /dev/null
> +++ b/htdocs/js/patch-list.js
> @@ -0,0 +1,12 @@
> +$( document ).ready(function() {
> + $("#patchlist").stickyTableHeaders();
> +
> + $("#check-all").change(function(e) {
> + if(this.checked) {
> + $("#patchlist > tbody").checkboxes("check");
> + } else {
> + $("#patchlist > tbody").checkboxes("uncheck");
> + }
> + e.preventDefault();
> + });
> +});
> \ No newline at end of file
nit: you'll want to include a newline at the end of the file (in other
words, to press <enter> at the end to make the last line a complete
line)
> diff --git a/patchwork/forms.py b/patchwork/forms.py
> index 24322c7..7f1fd31 100644
> --- a/patchwork/forms.py
> +++ b/patchwork/forms.py
> @@ -2,10 +2,10 @@
> # Copyright (C) 2008 Jeremy Kerr <jk at ozlabs.org>
> #
> # SPDX-License-Identifier: GPL-2.0-or-later
> -
> from django.contrib.auth.models import User
> from django import forms
> from django.db.models import Q
> +from django.core.exceptions import ValidationError
> from django.db.utils import ProgrammingError
>
> from patchwork.models import Bundle
> @@ -50,10 +50,17 @@ class EmailForm(forms.Form):
>
>
> class BundleForm(forms.ModelForm):
> + # Map 'name' field to 'bundle_name' attr
> + field_mapping = {'name': 'bundle_name'}
This comment restates what the code does. Instead, is there a
description of _why_ we are performing this mapping that the comment
could include? In other words, focusing in comments on intent and
context instead of mechanism should make this easier to understand.
> name = forms.RegexField(
> - regex=r'^[^/]+$', min_length=1, max_length=50, label=u'Name',
> + regex=r'^[^/]+$', min_length=1, max_length=50, required=False,
> error_messages={'invalid': 'Bundle names can\'t contain slashes'})
>
> + # Maps form fields 'name' attr rendered in HTML element
> + def add_prefix(self, field_name):
I don't understand this comment. Can you elaborate a little? E.g.,
what is a caller meaning to accomplish when they call this method?
> + field_name = self.field_mapping.get(field_name, field_name)
> + return super(BundleForm, self).add_prefix(field_name)
> +
> class Meta:
> model = Bundle
> fields = ['name', 'public']
> @@ -70,11 +77,16 @@ class CreateBundleForm(BundleForm):
>
> def clean_name(self):
> name = self.cleaned_data['name']
> + if not name:
> + raise ValidationError('No bundle name was specified',
> + code="invalid")
> +
Makes sense.
[...]
> --- a/patchwork/templates/patchwork/list.html
> +++ b/patchwork/templates/patchwork/list.html
> @@ -8,16 +8,7 @@
>
> {% block body %}
>
> -{% if errors %}
> -<p>The following error{{ errors|length|pluralize:" was,s were" }} encountered
> -while updating patches:</p>
> -<ul class="errorlist">
> -{% for error in errors %}
> - <li>{{ error }}</li>
> -{% endfor %}
> -</ul>
> -{% endif %}
> -
> +{% include "patchwork/partials/errors.html" %}
> {% include "patchwork/partials/patch-list.html" %}
>
> {% endblock %}
> diff --git a/patchwork/templates/patchwork/partials/errors.html b/patchwork/templates/patchwork/partials/errors.html
> new file mode 100644
> index 0000000..9e15009
> --- /dev/null
> +++ b/patchwork/templates/patchwork/partials/errors.html
> @@ -0,0 +1,9 @@
> +{% if errors %}
> +<p>The following error{{ errors|length|pluralize:" was,s were" }} encountered
> +while updating patches:</p>
> +<ul class="errorlist">
> +{% for error in errors %}
> + <li>{{ error }}</li>
> +{% endfor %}
> +</ul>
> +{% endif %}
This allows the same error list to be shared by submission.html.
Makes sense.
[...]
> diff --git a/patchwork/templates/patchwork/partials/patch-forms.html b/patchwork/templates/patchwork/partials/patch-forms.html
> new file mode 100644
> index 0000000..5a824aa
> --- /dev/null
> +++ b/patchwork/templates/patchwork/partials/patch-forms.html
> @@ -0,0 +1,45 @@
> +<div class="patch-forms" id="patch-forms">
[etc]
Makes sense.
> diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html
> index 02d6dff..339cf42 100644
> --- a/patchwork/templates/patchwork/partials/patch-list.html
> +++ b/patchwork/templates/patchwork/partials/patch-list.html
> @@ -4,9 +4,11 @@
> {% load project %}
> {% load static %}
>
> +{% block headers %}
> + <script src="{% static "js/patch-list.js" %}"></script>
> +{% endblock %}
> +
> {% include "patchwork/partials/filters.html" %}
> -
> -{% include "patchwork/partials/pagination.html" %}
What happens to the pagination widget?
[...]
> -<form method="post">
> +<form id="patch-list-form" method="post">
Reasonable.
> {% csrf_token %}
> -<input type="hidden" name="form" value="patchlistform"/>
> +<input type="hidden" name="form" value="patch-list-form"/>
> <input type="hidden" name="project" value="{{project.id}}"/>
> +<div class="patch-list-actions">
> + {% include "patchwork/partials/patch-forms.html" %}
> + {% include "patchwork/partials/pagination.html" %}
... ah, the pagination widget moves here. That's to allow it to go
below the action bar, so makes sense.
> +</div>
> <table id="patchlist" class="table table-hover table-extra-condensed table-striped pw-list"
> data-toggle="checkboxes" data-range="true">
> <thead>
> @@ -174,9 +165,9 @@ $(document).ready(function() {
>
> <tbody>
> {% for patch in page.object_list %}
> - <tr id="patch_row:{{patch.id}}">
> + <tr id="patch_row:{{patch.id}}" data-patch-id="{{patch.id}}">
> {% if user.is_authenticated %}
> - <td style="text-align: center;">
> + <td id="select-patch" style="text-align: center;">
Wouldn't this produce the same id for every patch? Should it be
id="select-patch:{{patch.id}}" instead?
> <input type="checkbox" name="patch_id:{{patch.id}}"/>
> </td>
> {% endif %}
> @@ -188,24 +179,24 @@ $(document).ready(function() {
> </button>
> </td>
> {% endif %}
> - <td>
> + <td id="patch-name">
Likewise.
[...]
> + <td id="patch-series">
Likewise.
[...]
> + <td id="patch-tags" class="text-nowrap">{{ patch|patch_tags }}</td>
> + <td id="patch-checks" class="text-nowrap">{{ patch|patch_checks }}</td>
> + <td id="patch-date" class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
> + <td id="patch-submitter">{{ patch.submitter|personify:project }}</td>
> + <td id="patch-delegate">{{ patch.delegate.username }}</td>
> + <td id="patch-state">{{ patch.state }}</td>
Likewise for these as well.
[..]
> --- a/patchwork/views/__init__.py
> +++ b/patchwork/views/__init__.py
> @@ -2,6 +2,7 @@
> # Copyright (C) 2008 Jeremy Kerr <jk at ozlabs.org>
> #
> # SPDX-License-Identifier: GPL-2.0-or-later
> +import json
Usual style seems to be to have a blank line after the license block.
>
> from django.contrib import messages
> from django.shortcuts import get_object_or_404
> @@ -9,6 +10,7 @@ from django.db.models import Prefetch
>
> from patchwork.filters import Filters
> from patchwork.forms import MultiplePatchForm
> +from patchwork.forms import CreateBundleForm
nit: I think this belongs one line up, to maintain alphabetical order.
> from patchwork.models import Bundle
> from patchwork.models import BundlePatch
> from patchwork.models import Patch
> @@ -16,7 +18,6 @@ from patchwork.models import Project
> from patchwork.models import Check
> from patchwork.paginator import Paginator
>
> -
Unintended removed line?
> bundle_actions = ['create', 'add', 'remove']
>
>
> @@ -108,46 +109,35 @@ class Order(object):
>
>
> # TODO(stephenfin): Refactor this to break it into multiple, testable functions
> -def set_bundle(request, project, action, data, patches, context):
> +def set_bundle(request, project, action, data, patches):
> # set up the bundle
> bundle = None
> user = request.user
>
> if action == 'create':
> bundle_name = data['bundle_name'].strip()
> - if '/' in bundle_name:
> - return ['Bundle names can\'t contain slashes']
> -
> - if not bundle_name:
> - return ['No bundle name was specified']
> -
> - if Bundle.objects.filter(owner=user, name=bundle_name).count() > 0:
> - return ['You already have a bundle called "%s"' % bundle_name]
> -
> bundle = Bundle(owner=user, project=project,
> name=bundle_name)
> - bundle.save()
> - messages.success(request, "Bundle %s created" % bundle.name)
> + create_bundle_form = CreateBundleForm(instance=bundle,
> + data=request.POST)
> + if create_bundle_form.is_valid():
Nice.
> + create_bundle_form.save()
> + addBundlePatches(request, patches, bundle)
> + bundle.save()
> + create_bundle_form = CreateBundleForm()
> + messages.success(request, 'Bundle %s created' % bundle.name)
> + else:
> + formErrors = json.loads(create_bundle_form.errors.as_json())
> + errors = [formErrors['name'][0]['message']]
> + return errors
Hm, does this extract only the first error? I wonder whether something
like
return [e['message'] for e in formErrors['name']]
would extract them all robustly.
> elif action == 'add':
> + if not data['bundle_id']:
> + return ['No bundle was selected']
> bundle = get_object_or_404(Bundle, id=data['bundle_id'])
> + addBundlePatches(request, patches, bundle)
> elif action == 'remove':
> bundle = get_object_or_404(Bundle, id=data['removed_bundle_id'])
> -
> - if not bundle:
> - return ['no such bundle']
> -
> - for patch in patches:
> - if action in ['create', 'add']:
What happens to this check against action?
> - bundlepatch_count = BundlePatch.objects.filter(bundle=bundle,
> - patch=patch).count()
> - if bundlepatch_count == 0:
> - bundle.append_patch(patch)
> - messages.success(request, "Patch '%s' added to bundle %s" %
> - (patch.name, bundle.name))
> - else:
> - messages.warning(request, "Patch '%s' already in bundle %s" %
> - (patch.name, bundle.name))
> - elif action == 'remove':
> + for patch in patches:
> try:
> bp = BundlePatch.objects.get(bundle=bundle, patch=patch)
> bp.delete()
[...]
> @@ -216,17 +217,20 @@ def generic_list(request, project, view, view_args=None, filter_settings=None,
> data = None
> user = request.user
> properties_form = None
> + create_bundle_form = None
>
> if user.is_authenticated:
> # we only pass the post data to the MultiplePatchForm if that was
> # the actual form submitted
> data_tmp = None
> - if data and data.get('form', '') == 'patchlistform':
> + if data and data.get('form', '') == 'patch-list-form':
> data_tmp = data
>
> properties_form = MultiplePatchForm(project, data=data_tmp)
> + if request.user.is_authenticated:
> + create_bundle_form = CreateBundleForm()
>
> - if request.method == 'POST' and data.get('form') == 'patchlistform':
> + if request.method == 'POST' and data.get('form') == 'patch-list-form':
> action = data.get('action', '').lower()
There are a lot of parts related to patchlistform -> patch-list-form; I
wonder whether this patch would be easier to review if that change were
its own patch.
[...]
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -14,11 +14,11 @@ from django.urls import reverse
>
> from patchwork.forms import CreateBundleForm
> from patchwork.forms import PatchForm
> -from patchwork.models import Bundle
> from patchwork.models import Cover
> from patchwork.models import Patch
> from patchwork.models import Project
> from patchwork.views import generic_list
> +from patchwork.views import set_bundle
> from patchwork.views.utils import patch_to_mbox
> from patchwork.views.utils import series_patch_to_mbox
>
> @@ -60,6 +60,7 @@ def patch_detail(request, project_id, msgid):
>
> form = None
> createbundleform = None
> + errors = None
>
> if editable:
> form = PatchForm(instance=patch)
> @@ -71,30 +72,10 @@ def patch_detail(request, project_id, msgid):
> if action:
> action = action.lower()
>
> - if action == 'createbundle':
> - bundle = Bundle(owner=request.user, project=project)
> - createbundleform = CreateBundleForm(instance=bundle,
> - data=request.POST)
> - if createbundleform.is_valid():
> - createbundleform.save()
> - bundle.append_patch(patch)
> - bundle.save()
> - createbundleform = CreateBundleForm()
> - messages.success(request, 'Bundle %s created' % bundle.name)
> - elif action == 'addtobundle':
Ah, are the actions renamed here? I assume that's fine because any
caller other than the patchwork UI would use the REST API instead?
> - bundle = get_object_or_404(
> - Bundle, id=request.POST.get('bundle_id'))
> - if bundle.append_patch(patch):
> - messages.success(request,
> - 'Patch "%s" added to bundle "%s"' % (
> - patch.name, bundle.name))
> - else:
> - messages.error(request,
> - 'Failed to add patch "%s" to bundle "%s": '
> - 'patch is already in bundle' % (
> - patch.name, bundle.name))
> -
> - # all other actions require edit privs
> + if action in ['create', 'add']:
> + errors = set_bundle(request, project, action,
> + request.POST, [patch])
> +
> elif not editable:
> return HttpResponseForbidden()
>
> @@ -133,6 +114,8 @@ def patch_detail(request, project_id, msgid):
> context['project'] = patch.project
> context['related_same_project'] = related_same_project
> context['related_different_project'] = related_different_project
> + if errors:
> + context['errors'] = errors
What does this part do?
Thanks,
Jonathan
More information about the Patchwork
mailing list