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 <j...@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 <j...@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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
