[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