[PATCH v2 2/5] patch-list: clean up patch-list page and refactor patch forms
Raxel Gutierrez
raxel at google.com
Wed Jul 28 05:08:58 AEST 2021
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 at 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 at ozlabs.org>
> > #
> > # SPDX-License-Identifier: GPL-2.0-or-later
> > -
> > from django.contrib.auth.models import User
> > from django import forms
> > from django.db.models import Q
> > +from django.core.exceptions import ValidationError
> > from django.db.utils import ProgrammingError
> >
> > from patchwork.models import Bundle
> > @@ -50,10 +50,17 @@ class EmailForm(forms.Form):
> >
> >
> > class BundleForm(forms.ModelForm):
> > + # Map 'name' field to 'bundle_name' attr
> > + field_mapping = {'name': 'bundle_name'}
>
> This comment restates what the code does. Instead, is there a
> description of _why_ we are performing this mapping that the comment
> could include? In other words, focusing in comments on intent and
> context instead of mechanism should make this easier to understand.
>
> > name = forms.RegexField(
> > - regex=r'^[^/]+$', min_length=1, max_length=50, label=u'Name',
> > + regex=r'^[^/]+$', min_length=1, max_length=50, required=False,
> > error_messages={'invalid': 'Bundle names can\'t contain slashes'})
> >
> > + # Maps form fields 'name' attr rendered in HTML element
> > + def add_prefix(self, field_name):
>
> I don't understand this comment. Can you elaborate a little? E.g.,
> what is a caller meaning to accomplish when they call this method?
>
> > + field_name = self.field_mapping.get(field_name, field_name)
> > + return super(BundleForm, self).add_prefix(field_name)
> > +
> > class Meta:
> > model = Bundle
> > fields = ['name', 'public']
> > @@ -70,11 +77,16 @@ class CreateBundleForm(BundleForm):
> >
> > def clean_name(self):
> > name = self.cleaned_data['name']
> > + if not name:
> > + raise ValidationError('No bundle name was specified',
> > + code="invalid")
> > +
>
> Makes sense.
>
> [...]
> > --- a/patchwork/templates/patchwork/list.html
> > +++ b/patchwork/templates/patchwork/list.html
> > @@ -8,16 +8,7 @@
> >
> > {% block body %}
> >
> > -{% if errors %}
> > -<p>The following error{{ errors|length|pluralize:" was,s were" }} encountered
> > -while updating patches:</p>
> > -<ul class="errorlist">
> > -{% for error in errors %}
> > - <li>{{ error }}</li>
> > -{% endfor %}
> > -</ul>
> > -{% endif %}
> > -
> > +{% include "patchwork/partials/errors.html" %}
> > {% include "patchwork/partials/patch-list.html" %}
> >
> > {% endblock %}
> > diff --git a/patchwork/templates/patchwork/partials/errors.html b/patchwork/templates/patchwork/partials/errors.html
> > new file mode 100644
> > index 0000000..9e15009
> > --- /dev/null
> > +++ b/patchwork/templates/patchwork/partials/errors.html
> > @@ -0,0 +1,9 @@
> > +{% if errors %}
> > +<p>The following error{{ errors|length|pluralize:" was,s were" }} encountered
> > +while updating patches:</p>
> > +<ul class="errorlist">
> > +{% for error in errors %}
> > + <li>{{ error }}</li>
> > +{% endfor %}
> > +</ul>
> > +{% endif %}
>
> This allows the same error list to be shared by submission.html.
> Makes sense.
>
> [...]
> > diff --git a/patchwork/templates/patchwork/partials/patch-forms.html b/patchwork/templates/patchwork/partials/patch-forms.html
> > new file mode 100644
> > index 0000000..5a824aa
> > --- /dev/null
> > +++ b/patchwork/templates/patchwork/partials/patch-forms.html
> > @@ -0,0 +1,45 @@
> > +<div class="patch-forms" id="patch-forms">
> [etc]
>
> Makes sense.
>
> > diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html
> > index 02d6dff..339cf42 100644
> > --- a/patchwork/templates/patchwork/partials/patch-list.html
> > +++ b/patchwork/templates/patchwork/partials/patch-list.html
> > @@ -4,9 +4,11 @@
> > {% load project %}
> > {% load static %}
> >
> > +{% block headers %}
> > + <script src="{% static "js/patch-list.js" %}"></script>
> > +{% endblock %}
> > +
> > {% include "patchwork/partials/filters.html" %}
> > -
> > -{% include "patchwork/partials/pagination.html" %}
>
> What happens to the pagination widget?
>
> [...]
> > -<form method="post">
> > +<form id="patch-list-form" method="post">
>
> Reasonable.
>
> > {% csrf_token %}
> > -<input type="hidden" name="form" value="patchlistform"/>
> > +<input type="hidden" name="form" value="patch-list-form"/>
> > <input type="hidden" name="project" value="{{project.id}}"/>
> > +<div class="patch-list-actions">
> > + {% include "patchwork/partials/patch-forms.html" %}
> > + {% include "patchwork/partials/pagination.html" %}
>
> ... ah, the pagination widget moves here. That's to allow it to go
> below the action bar, so makes sense.
>
> > +</div>
> > <table id="patchlist" class="table table-hover table-extra-condensed table-striped pw-list"
> > data-toggle="checkboxes" data-range="true">
> > <thead>
> > @@ -174,9 +165,9 @@ $(document).ready(function() {
> >
> > <tbody>
> > {% for patch in page.object_list %}
> > - <tr id="patch_row:{{patch.id}}">
> > + <tr id="patch_row:{{patch.id}}" data-patch-id="{{patch.id}}">
> > {% if user.is_authenticated %}
> > - <td style="text-align: center;">
> > + <td id="select-patch" style="text-align: center;">
>
> Wouldn't this produce the same id for every patch? Should it be
> id="select-patch:{{patch.id}}" instead?
>
> > <input type="checkbox" name="patch_id:{{patch.id}}"/>
> > </td>
> > {% endif %}
> > @@ -188,24 +179,24 @@ $(document).ready(function() {
> > </button>
> > </td>
> > {% endif %}
> > - <td>
> > + <td id="patch-name">
>
> Likewise.
>
> [...]
> > + <td id="patch-series">
>
> Likewise.
>
> [...]
> > + <td id="patch-tags" class="text-nowrap">{{ patch|patch_tags }}</td>
> > + <td id="patch-checks" class="text-nowrap">{{ patch|patch_checks }}</td>
> > + <td id="patch-date" class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
> > + <td id="patch-submitter">{{ patch.submitter|personify:project }}</td>
> > + <td id="patch-delegate">{{ patch.delegate.username }}</td>
> > + <td id="patch-state">{{ patch.state }}</td>
>
> Likewise for these as well.
>
> [..]
> > --- a/patchwork/views/__init__.py
> > +++ b/patchwork/views/__init__.py
> > @@ -2,6 +2,7 @@
> > # Copyright (C) 2008 Jeremy Kerr <jk at ozlabs.org>
> > #
> > # SPDX-License-Identifier: GPL-2.0-or-later
> > +import json
>
> Usual style seems to be to have a blank line after the license block.
>
> >
> > from django.contrib import messages
> > from django.shortcuts import get_object_or_404
> > @@ -9,6 +10,7 @@ from django.db.models import Prefetch
> >
> > from patchwork.filters import Filters
> > from patchwork.forms import MultiplePatchForm
> > +from patchwork.forms import CreateBundleForm
>
> nit: I think this belongs one line up, to maintain alphabetical order.
>
> > from patchwork.models import Bundle
> > from patchwork.models import BundlePatch
> > from patchwork.models import Patch
> > @@ -16,7 +18,6 @@ from patchwork.models import Project
> > from patchwork.models import Check
> > from patchwork.paginator import Paginator
> >
> > -
>
> Unintended removed line?
>
> > bundle_actions = ['create', 'add', 'remove']
> >
> >
> > @@ -108,46 +109,35 @@ class Order(object):
> >
> >
> > # TODO(stephenfin): Refactor this to break it into multiple, testable functions
> > -def set_bundle(request, project, action, data, patches, context):
> > +def set_bundle(request, project, action, data, patches):
> > # set up the bundle
> > bundle = None
> > user = request.user
> >
> > if action == 'create':
> > bundle_name = data['bundle_name'].strip()
> > - if '/' in bundle_name:
> > - return ['Bundle names can\'t contain slashes']
> > -
> > - if not bundle_name:
> > - return ['No bundle name was specified']
> > -
> > - if Bundle.objects.filter(owner=user, name=bundle_name).count() > 0:
> > - return ['You already have a bundle called "%s"' % bundle_name]
> > -
> > bundle = Bundle(owner=user, project=project,
> > name=bundle_name)
> > - bundle.save()
> > - messages.success(request, "Bundle %s created" % bundle.name)
> > + create_bundle_form = CreateBundleForm(instance=bundle,
> > + data=request.POST)
> > + if create_bundle_form.is_valid():
>
> Nice.
>
> > + create_bundle_form.save()
> > + addBundlePatches(request, patches, bundle)
> > + bundle.save()
> > + create_bundle_form = CreateBundleForm()
> > + messages.success(request, 'Bundle %s created' % bundle.name)
> > + else:
> > + formErrors = json.loads(create_bundle_form.errors.as_json())
> > + errors = [formErrors['name'][0]['message']]
> > + return errors
>
> Hm, does this extract only the first error? I wonder whether something
> like
>
> return [e['message'] for e in formErrors['name']]
>
> would extract them all robustly.
>
> > elif action == 'add':
> > + if not data['bundle_id']:
> > + return ['No bundle was selected']
> > bundle = get_object_or_404(Bundle, id=data['bundle_id'])
> > + addBundlePatches(request, patches, bundle)
> > elif action == 'remove':
> > bundle = get_object_or_404(Bundle, id=data['removed_bundle_id'])
> > -
> > - if not bundle:
> > - return ['no such bundle']
> > -
> > - for patch in patches:
> > - if action in ['create', 'add']:
>
> What happens to this check against action?
>
> > - bundlepatch_count = BundlePatch.objects.filter(bundle=bundle,
> > - patch=patch).count()
> > - if bundlepatch_count == 0:
> > - bundle.append_patch(patch)
> > - messages.success(request, "Patch '%s' added to bundle %s" %
> > - (patch.name, bundle.name))
> > - else:
> > - messages.warning(request, "Patch '%s' already in bundle %s" %
> > - (patch.name, bundle.name))
> > - elif action == 'remove':
> > + for patch in patches:
> > try:
> > bp = BundlePatch.objects.get(bundle=bundle, patch=patch)
> > bp.delete()
> [...]
> > @@ -216,17 +217,20 @@ def generic_list(request, project, view, view_args=None, filter_settings=None,
> > data = None
> > user = request.user
> > properties_form = None
> > + create_bundle_form = None
> >
> > if user.is_authenticated:
> > # we only pass the post data to the MultiplePatchForm if that was
> > # the actual form submitted
> > data_tmp = None
> > - if data and data.get('form', '') == 'patchlistform':
> > + if data and data.get('form', '') == 'patch-list-form':
> > data_tmp = data
> >
> > properties_form = MultiplePatchForm(project, data=data_tmp)
> > + if request.user.is_authenticated:
> > + create_bundle_form = CreateBundleForm()
> >
> > - if request.method == 'POST' and data.get('form') == 'patchlistform':
> > + if request.method == 'POST' and data.get('form') == 'patch-list-form':
> > action = data.get('action', '').lower()
>
> There are a lot of parts related to patchlistform -> patch-list-form; I
> wonder whether this patch would be easier to review if that change were
> its own patch.
>
> [...]
> > --- a/patchwork/views/patch.py
> > +++ b/patchwork/views/patch.py
> > @@ -14,11 +14,11 @@ from django.urls import reverse
> >
> > from patchwork.forms import CreateBundleForm
> > from patchwork.forms import PatchForm
> > -from patchwork.models import Bundle
> > from patchwork.models import Cover
> > from patchwork.models import Patch
> > from patchwork.models import Project
> > from patchwork.views import generic_list
> > +from patchwork.views import set_bundle
> > from patchwork.views.utils import patch_to_mbox
> > from patchwork.views.utils import series_patch_to_mbox
> >
> > @@ -60,6 +60,7 @@ def patch_detail(request, project_id, msgid):
> >
> > form = None
> > createbundleform = None
> > + errors = None
> >
> > if editable:
> > form = PatchForm(instance=patch)
> > @@ -71,30 +72,10 @@ def patch_detail(request, project_id, msgid):
> > if action:
> > action = action.lower()
> >
> > - if action == 'createbundle':
> > - bundle = Bundle(owner=request.user, project=project)
> > - createbundleform = CreateBundleForm(instance=bundle,
> > - data=request.POST)
> > - if createbundleform.is_valid():
> > - createbundleform.save()
> > - bundle.append_patch(patch)
> > - bundle.save()
> > - createbundleform = CreateBundleForm()
> > - messages.success(request, 'Bundle %s created' % bundle.name)
> > - elif action == 'addtobundle':
>
> Ah, are the actions renamed here? I assume that's fine because any
> caller other than the patchwork UI would use the REST API instead?
>
> > - bundle = get_object_or_404(
> > - Bundle, id=request.POST.get('bundle_id'))
> > - if bundle.append_patch(patch):
> > - messages.success(request,
> > - 'Patch "%s" added to bundle "%s"' % (
> > - patch.name, bundle.name))
> > - else:
> > - messages.error(request,
> > - 'Failed to add patch "%s" to bundle "%s": '
> > - 'patch is already in bundle' % (
> > - patch.name, bundle.name))
> > -
> > - # all other actions require edit privs
> > + if action in ['create', 'add']:
> > + errors = set_bundle(request, project, action,
> > + request.POST, [patch])
> > +
> > elif not editable:
> > return HttpResponseForbidden()
> >
> > @@ -133,6 +114,8 @@ def patch_detail(request, project_id, msgid):
> > context['project'] = patch.project
> > context['related_same_project'] = related_same_project
> > context['related_different_project'] = related_different_project
> > + if errors:
> > + context['errors'] = errors
>
> What does this part do?
>
> Thanks,
> Jonathan
More information about the Patchwork
mailing list