| Message ID | 20210722171251.2554142-3-raxel@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/5] static: add JS Cookie Library to get csrftoken for fetch requests | expand |
| Related | show |
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@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@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
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 <jrnieder@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 <jk@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@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
diff --git a/htdocs/README.rst b/htdocs/README.rst index fa1616c..a018f31 100644 --- 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. + ``selectize.min.js`` Selectize is the hybrid of a ``textbox`` and ``<select>`` box. It's jQuery diff --git a/htdocs/css/style.css b/htdocs/css/style.css index 243caa0..1bcc93e 100644 --- a/htdocs/css/style.css +++ b/htdocs/css/style.css @@ -122,7 +122,7 @@ a.colinactive:hover { div.filters { } -div.patchforms { +div.patch-forms { margin-top: 1em; } @@ -204,7 +204,7 @@ table.patchmeta tr th, table.patchmeta tr td { } /* checks forms */ -/* TODO(stephenfin): Merge this with 'div.patchform' rules */ +/* TODO(stephenfin): Merge this with 'div.patch-form' rules */ .checks { border: 1px solid gray; margin: 0.5em 1em; @@ -347,7 +347,7 @@ table.bundlelist td } /* forms that appear for a patch */ -div.patchform { +div.patch-form { border: thin solid #080808; padding-left: 0.6em; padding-right: 0.6em; @@ -355,7 +355,7 @@ div.patchform { margin: 0.5em 5em 0.5em 10px; } -div.patchform h3 { +div.patch-form h3 { margin-top: 0em; margin-left: -0.6em; margin-right: -0.6em; @@ -365,7 +365,7 @@ div.patchform h3 { font-size: 100%; } -div.patchform ul { +div.patch-form ul { list-style-type: none; padding-left: 0.2em; margin-top: 0em; diff --git a/htdocs/js/patch-list.js b/htdocs/js/patch-list.js new file mode 100644 index 0000000..8c7640f --- /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 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@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'} 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): + 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") + count = Bundle.objects.filter(owner=self.instance.owner, name=name).count() if count > 0: - raise forms.ValidationError('A bundle called %s already exists' - % name) + raise ValidationError('A bundle called %(name)s already exists', + code="invalid", + params={'name': name}) return name diff --git a/patchwork/templates/patchwork/list.html b/patchwork/templates/patchwork/list.html index 5d3d82a..d4e81bc 100644 --- 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 %} 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"> +{% if patchform %} + <div id="patch-form-properties" class="patch-form"> + <div id="patch-form-state"> + {{ patchform.state.errors }} + {{ patchform.state }} + </div> + <div id="patch-form-delegate"> + {{ patchform.delegate.errors }} + {{ patchform.delegate }} + </div> + <div id="patch-form-archive"> + {{ patchform.archived.errors }} + {{ patchform.archived.label_tag }} {{ patchform.archived }} + </div> + <button class="patch-form-submit btn btn-primary" name="action" value="update">Update</button> + </div> +{% endif %} +{% if user.is_authenticated %} + <div id="patch-form-bundle" class="patch-form"> + <div id="create-bundle"> + {{ createbundleform.name.errors }} + {{ createbundleform.name }} + <input class="patch-form-submit btn btn-primary" name="action" value="Create" type="submit"/> + </div> + {% if bundles %} + <div id="add-to-bundle"> + <select class="add-bundle" name="bundle_id"> + <option value="" selected>Add to bundle</option> + {% for bundle in bundles %} + <option value="{{bundle.id}}">{{bundle.name}}</option> + {% endfor %} + </select> + <input class="patch-form-submit btn btn-primary" name="action" value="Add" type="submit"/> + </div> + {% endif %} + {% if bundle %} + <div id="remove-bundle"> + <input type="hidden" name="removed_bundle_id" value="{{bundle.id}}"/> + <button class="patch-form-submit btn btn-primary" name="action" value="Remove">Remove from bundle</button> + </div> + {% endif %} + </div> +{% endif %} +</div> 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 %} -{% include "patchwork/partials/filters.html" %} +{% block headers %} + <script src="{% static "js/patch-list.js" %}"></script> +{% endblock %} -{% include "patchwork/partials/pagination.html" %} +{% include "patchwork/partials/filters.html" %} {% if order.editable %} <table class="patchlist"> @@ -29,31 +31,20 @@ {% if page.paginator.long_page and user.is_authenticated %} <div class="floaty"> - <a title="jump to form" href="#patchforms"> + <a title="jump to form" href="#patch-forms"> <span style="font-size: 120%">▾</span> </a> </div> {% endif %} -<script type="text/javascript"> -$(document).ready(function() { - $('#patchlist').stickyTableHeaders(); - - $('#check-all').change(function(e) { - if(this.checked) { - $('#patchlist > tbody').checkboxes('check'); - } else { - $('#patchlist > tbody').checkboxes('uncheck'); - } - e.preventDefault(); - }); -}); -</script> - -<form method="post"> +<form id="patch-list-form" method="post"> {% 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" %} +</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;"> <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"> <a href="{% url 'patch-detail' project_id=project.linkname msgid=patch.url_msgid %}"> {{ patch.name|default:"[no subject]"|truncatechars:100 }} </a> </td> - <td> + <td id="patch-series"> {% if patch.series %} <a href="?series={{patch.series.id}}"> {{ patch.series|truncatechars:100 }} </a> {% endif %} </td> - <td class="text-nowrap">{{ patch|patch_tags }}</td> - <td class="text-nowrap">{{ patch|patch_checks }}</td> - <td class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td> - <td>{{ patch.submitter|personify:project }}</td> - <td>{{ patch.delegate.username }}</td> - <td>{{ patch.state }}</td> + <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> </tr> {% empty %} <tr> @@ -217,86 +208,6 @@ $(document).ready(function() { {% if page.paginator.count %} {% include "patchwork/partials/pagination.html" %} - -<div class="patchforms" id="patchforms"> - -{% if patchform %} - <div class="patchform patchform-properties"> - <h3>Properties</h3> - <table class="form"> - <tr> - <th>Change state:</th> - <td> - {{ patchform.state }} - {{ patchform.state.errors }} - </td> - </tr> - <tr> - <th>Delegate to:</th> - <td> - {{ patchform.delegate }} - {{ patchform.delegate.errors }} - </td> - </tr> - <tr> - <th>Archive:</th> - <td> - {{ patchform.archived }} - {{ patchform.archived.errors }} - </td> - </tr> - <tr> - <td></td> - <td> - <input type="submit" name="action" value="{{patchform.action}}"/> - </td> - </tr> - </table> - </div> - -{% endif %} - -{% if user.is_authenticated %} - <div class="patchform patchform-bundle"> - <h3>Bundling</h3> - <table class="form"> - <tr> - <td>Create bundle:</td> - <td> - <input type="text" name="bundle_name"/> - <input name="action" value="Create" type="submit"/> - </td> - </tr> - {% if bundles %} - <tr> - <td>Add to bundle:</td> - <td> - <select name="bundle_id"> - {% for bundle in bundles %} - <option value="{{bundle.id}}">{{bundle.name}}</option> - {% endfor %} - </select> - <input name="action" value="Add" type="submit"/> - </td> - </tr> - {% endif %} - {% if bundle %} - <tr> - <td>Remove from bundle:</td> - <td> - <input type="hidden" name="removed_bundle_id" value="{{bundle.id}}"/> - <input name="action" value="Remove" type="submit"/> - </td> - </tr> - {% endif %} - </table> - </div> -{% endif %} - - <div style="clear: both;"> - </div> -</div> - {% endif %} </form> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html index 978559b..17255ee 100644 --- a/patchwork/templates/patchwork/submission.html +++ b/patchwork/templates/patchwork/submission.html @@ -27,6 +27,8 @@ function toggle_div(link_id, headers_id, label_show, label_hide) } </script> +{% include "patchwork/partials/errors.html" %} + <div> {% include "patchwork/partials/download-buttons.html" %} <h1>{{ submission.name }}</h1> @@ -149,91 +151,10 @@ function toggle_div(link_id, headers_id, label_show, label_hide) {% endif %} </table> -<div class="patchforms"> -{% if patchform %} - <div class="patchform patchform-properties"> - <h3>Patch Properties</h3> - <form method="post"> - {% csrf_token %} - <table class="form"> - <tr> - <th>Change state:</th> - <td> - {{ patchform.state }} - {{ patchform.state.errors }} - </td> - </tr> - <tr> - <th>Delegate to:</th> - <td> - {{ patchform.delegate }} - {{ patchform.delegate.errors }} - </td> - </tr> - <tr> - <th>Archived:</th> - <td> - {{ patchform.archived }} - {{ patchform.archived.errors }} - </td> - </tr> - <tr> - <td></td> - <td> - <input type="submit" value="Update"> - </td> - </tr> - </table> - </form> - </div> -{% endif %} - -{% if createbundleform %} - <div class="patchform patchform-bundle"> - <h3>Bundling</h3> - <table class="form"> - <tr> - <td>Create bundle:</td> - <td> - {% if createbundleform.non_field_errors %} - <dd class="errors">{{createbundleform.non_field_errors}}</dd> - {% endif %} - <form method="post"> - {% csrf_token %} - <input type="hidden" name="action" value="createbundle"/> - {% if createbundleform.name.errors %} - <dd class="errors">{{createbundleform.name.errors}}</dd> - {% endif %} - {{ createbundleform.name }} - <input value="Create" type="submit"/> - </form> - </td> - </tr> -{% if bundles %} - <tr> - <td>Add to bundle:</td> - <td> - <form method="post"> - {% csrf_token %} - <input type="hidden" name="action" value="addtobundle"/> - <select name="bundle_id"/> - {% for bundle in bundles %} - <option value="{{bundle.id}}">{{bundle.name}}</option> - {% endfor %} - </select> - <input value="Add" type="submit"/> - </form> - </td> - </tr> -{% endif %} - </table> - - </div> -{% endif %} - - <div style="clear: both;"> - </div> -</div> +<form id="patch-list-form" method="POST"> + {% csrf_token %} + {% include "patchwork/partials/patch-forms.html" %} +</form> {% if submission.pull_url %} <h2>Pull-request</h2> diff --git a/patchwork/tests/views/test_bundles.py b/patchwork/tests/views/test_bundles.py index 6a74409..2233c21 100644 --- a/patchwork/tests/views/test_bundles.py +++ b/patchwork/tests/views/test_bundles.py @@ -146,7 +146,7 @@ class BundleUpdateTest(BundleTestBase): data = { 'form': 'bundle', 'action': 'update', - 'name': newname, + 'bundle_name': newname, 'public': '', } response = self.client.post(bundle_url(self.bundle), data) @@ -159,7 +159,7 @@ class BundleUpdateTest(BundleTestBase): data = { 'form': 'bundle', 'action': 'update', - 'name': self.bundle.name, + 'bundle_name': self.bundle.name, 'public': 'on', } response = self.client.post(bundle_url(self.bundle), data) @@ -243,7 +243,7 @@ class BundlePublicModifyTest(BundleTestBase): data = { 'form': 'bundle', 'action': 'update', - 'name': newname, + 'bundle_name': newname, } self.bundle.name = oldname self.bundle.save() @@ -353,7 +353,7 @@ class BundleCreateFromListTest(BundleTestBase): def test_create_empty_bundle(self): newbundlename = 'testbundle-new' - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'bundle_name': newbundlename, 'action': 'Create', 'project': self.project.id} @@ -369,7 +369,7 @@ class BundleCreateFromListTest(BundleTestBase): newbundlename = 'testbundle-new' patch = self.patches[0] - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'bundle_name': newbundlename, 'action': 'Create', 'project': self.project.id, @@ -393,7 +393,7 @@ class BundleCreateFromListTest(BundleTestBase): n_bundles = Bundle.objects.count() - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'bundle_name': '', 'action': 'Create', 'project': self.project.id, @@ -414,7 +414,7 @@ class BundleCreateFromListTest(BundleTestBase): newbundlename = 'testbundle-dup' patch = self.patches[0] - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'bundle_name': newbundlename, 'action': 'Create', 'project': self.project.id, @@ -440,7 +440,9 @@ class BundleCreateFromListTest(BundleTestBase): params) self.assertNotContains(response, 'Bundle %s created' % newbundlename) - self.assertContains(response, 'You already have a bundle called') + self.assertContains( + response, + 'A bundle called %s already exists' % newbundlename) self.assertEqual(Bundle.objects.count(), n_bundles) self.assertEqual(bundle.patches.count(), 1) @@ -451,8 +453,8 @@ class BundleCreateFromPatchTest(BundleTestBase): newbundlename = 'testbundle-new' patch = self.patches[0] - params = {'name': newbundlename, - 'action': 'createbundle'} + params = {'bundle_name': newbundlename, + 'action': 'Create'} response = self.client.post( reverse('patch-detail', @@ -470,8 +472,8 @@ class BundleCreateFromPatchTest(BundleTestBase): newbundlename = self.bundle.name patch = self.patches[0] - params = {'name': newbundlename, - 'action': 'createbundle'} + params = {'bundle_name': newbundlename, + 'action': 'Create'} response = self.client.post( reverse('patch-detail', @@ -489,7 +491,7 @@ class BundleAddFromListTest(BundleTestBase): def test_add_to_empty_bundle(self): patch = self.patches[0] - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'action': 'Add', 'project': self.project.id, 'bundle_id': self.bundle.id, @@ -509,7 +511,7 @@ class BundleAddFromListTest(BundleTestBase): def test_add_to_non_empty_bundle(self): self.bundle.append_patch(self.patches[0]) patch = self.patches[1] - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'action': 'Add', 'project': self.project.id, 'bundle_id': self.bundle.id, @@ -538,7 +540,7 @@ class BundleAddFromListTest(BundleTestBase): count = self.bundle.patches.count() patch = self.patches[0] - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'action': 'Add', 'project': self.project.id, 'bundle_id': self.bundle.id, @@ -559,7 +561,7 @@ class BundleAddFromListTest(BundleTestBase): count = self.bundle.patches.count() patch = self.patches[0] - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'action': 'Add', 'project': self.project.id, 'bundle_id': self.bundle.id, @@ -584,7 +586,7 @@ class BundleAddFromPatchTest(BundleTestBase): def test_add_to_empty_bundle(self): patch = self.patches[0] - params = {'action': 'addtobundle', + params = {'action': 'Add', 'bundle_id': self.bundle.id} response = self.client.post( @@ -594,7 +596,7 @@ class BundleAddFromPatchTest(BundleTestBase): self.assertContains( response, - 'added to bundle "%s"' % self.bundle.name, + 'added to bundle %s' % self.bundle.name, count=1) self.assertEqual(self.bundle.patches.count(), 1) @@ -603,7 +605,7 @@ class BundleAddFromPatchTest(BundleTestBase): def test_add_to_non_empty_bundle(self): self.bundle.append_patch(self.patches[0]) patch = self.patches[1] - params = {'action': 'addtobundle', + params = {'action': 'Add', 'bundle_id': self.bundle.id} response = self.client.post( @@ -613,7 +615,7 @@ class BundleAddFromPatchTest(BundleTestBase): self.assertContains( response, - 'added to bundle "%s"' % self.bundle.name, + 'added to bundle %s' % self.bundle.name, count=1) self.assertEqual(self.bundle.patches.count(), 2) @@ -650,7 +652,7 @@ class BundleInitialOrderTest(BundleTestBase): newbundlename = 'testbundle-new' # need to define our querystring explicity to enforce ordering - params = {'form': 'patchlistform', + params = {'form': 'patch-list-form', 'bundle_name': newbundlename, 'action': 'Create', 'project': self.project.id, diff --git a/patchwork/tests/views/test_patch.py b/patchwork/tests/views/test_patch.py index 1a1243c..483ab99 100644 --- a/patchwork/tests/views/test_patch.py +++ b/patchwork/tests/views/test_patch.py @@ -304,7 +304,7 @@ class PatchViewTest(TestCase): class PatchUpdateTest(TestCase): - properties_form_id = 'patchform-properties' + properties_form_id = 'patch-form-properties' def setUp(self): self.project = create_project() @@ -318,7 +318,7 @@ class PatchUpdateTest(TestCase): self.base_data = { 'action': 'Update', 'project': str(self.project.id), - 'form': 'patchlistform', + 'form': 'patch-list-form', 'archived': '*', 'delegate': '*', 'state': '*' diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py index 3efe90c..178d185 100644 --- a/patchwork/views/__init__.py +++ b/patchwork/views/__init__.py @@ -2,6 +2,7 @@ # Copyright (C) 2008 Jeremy Kerr <jk@ozlabs.org> # # SPDX-License-Identifier: GPL-2.0-or-later +import json 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 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 - 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(): + 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 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']: - 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() @@ -158,10 +148,21 @@ def set_bundle(request, project, action, data, patches, context): request, "Patch '%s' removed from bundle %s\n" % (patch.name, bundle.name)) + return [] - bundle.save() - return [] +def addBundlePatches(request, patches, bundle): + for patch in patches: + bundlepatch_count = BundlePatch.objects.filter(bundle=bundle, + patch=patch).count() + if bundlepatch_count == 0: + bundle.append_patch(patch) + bundle.save() + 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)) def generic_list(request, project, view, view_args=None, filter_settings=None, @@ -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() # special case: the user may have hit enter in the 'create bundle' @@ -237,7 +241,7 @@ def generic_list(request, project, view, view_args=None, filter_settings=None, ps = Patch.objects.filter(id__in=get_patch_ids(data)) if action in bundle_actions: - errors = set_bundle(request, project, action, data, ps, context) + errors = set_bundle(request, project, action, data, ps) elif properties_form and action == properties_form.action: errors = process_multiplepatch_form(request, properties_form, @@ -288,6 +292,7 @@ def generic_list(request, project, view, view_args=None, filter_settings=None, context.update({ 'page': paginator.current_page, 'patchform': properties_form, + 'createbundleform': create_bundle_form, 'project': project, 'order': order, }) diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 3e6874a..5ef6916 100644 --- 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': - 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 return render(request, 'patchwork/submission.html', context)
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 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. 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. Signed-off-by: Raxel Gutierrez <raxel@google.com> --- htdocs/README.rst | 7 + htdocs/css/style.css | 10 +- htdocs/js/patch-list.js | 12 ++ patchwork/forms.py | 20 ++- patchwork/templates/patchwork/list.html | 11 +- .../templates/patchwork/partials/errors.html | 9 ++ .../patchwork/partials/patch-forms.html | 45 ++++++ .../patchwork/partials/patch-list.html | 131 +++--------------- patchwork/templates/patchwork/submission.html | 91 +----------- patchwork/tests/views/test_bundles.py | 44 +++--- patchwork/tests/views/test_patch.py | 4 +- patchwork/views/__init__.py | 73 +++++----- patchwork/views/patch.py | 33 ++--- 13 files changed, 194 insertions(+), 296 deletions(-) create mode 100644 htdocs/js/patch-list.js create mode 100644 patchwork/templates/patchwork/partials/errors.html create mode 100644 patchwork/templates/patchwork/partials/patch-forms.html