[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