Hi! Thanks for the valuable review on this pretty long patch :) Much appreciated!
> > 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 There are no user-visible changes as the changes are mostly cleanup and refactoring. Nonetheless, I changed the commit message to better detail the benefits of the change. > [...] > > --- 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. Yes, patch-list.js contains event handlers and other helpers for patch-list.html specifically. I adopted your description, I was following the bundle.js description for consistency. > > diff --git a/patchwork/forms.py b/patchwork/forms.py > > 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? I agree the comment wasn't very useful. I changed the comment to the following: # Changes the HTML 'name' attr of the input element from "name" # (inherited from the model field 'name' of the Bundle object) # to "bundle_name" as the server expects "bundle_name" as a # parameter not "name" to process patch forms 'POST' requests. > > diff --git a/patchwork/templates/patchwork/partials/patch-list.html > > b/patchwork/templates/patchwork/partials/patch-list.html > > {% 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? > > > + <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. I made a mental note to change the ids but then forgot, thanks for catching this :) > 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. Agreed, currently only one error can be returned on any given submission, but better code :) > > @@ -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. Can be done, the change is a pretty large refactoring for sure. If there are no strong preferences though, then I would like it to remain as the same patch. > > --- a/patchwork/views/patch.py > > +++ b/patchwork/views/patch.py > > 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? Yes, I agree any other caller 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? This part uses the `set_bundle` function from `__init__.py` to process the form submission for bundles. This way both the `patch_list` and `patch_detail` views share the same behavior/functionality for its bundle form actions. The `errors` (if any) returned from the `set_bundle` are passed to the `submission.html` template for rendering after the bundle form is submitted. On Mon, Jul 26, 2021 at 8:19 PM Jonathan Nieder <jrnie...@gmail.com> wrote: > > 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
