[PATCH v3 2/5] patch-list: clean up patch-list page and refactor patch forms

[Raxel Gutierrez]

Hi Daniel,

> Could you please split user-visible vs non-user-visible changes into
> separate patches? I'm pretty keen on being able to apply refactorings
> and cleanups while we debate the UI and I can't do that if with they're
> intermingled. (In general, as with the kernel, patches that do one thing
> at a time are generally preferred --- although I'm not dogmatic about
> this and I am aware that refactoring is often hard to neatly split!)

Yes, I agree the patch is doing too much. The upcoming v4 will split
it between user-visible and non-user-visible changes


> (Technically you haven't added inline dropdown changes yet! Maybe just
> drop the e.g. part.)

Done.

> > diff --git a/patchwork/forms.py b/patchwork/forms.py
> > index 24322c7..3670142 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
> > -
>
> We could probably keep that blank line.

Done.

> >  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,19 @@ class EmailForm(forms.Form):
> >
> >
> >  class BundleForm(forms.ModelForm):
> > +    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'})
> >
> > +    # 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.
> > +    def add_prefix(self, field_name):
> > +        field_name = self.field_mapping.get(field_name, field_name)
> > +        return super(BundleForm, self).add_prefix(field_name)
> > +
> Hmm. Can we keep 'name' and change views/__init__.py to use data['name']
> instead of data['bundle_name']? (Maybe we can't, I haven't checked
> in much detail.)

Changed it back to 'name' and changed the appropriate files and tests
including views/__init.py. I originally decided to change to
'bundle_name' to remain consistent.

> >      class Meta:
> >          model = Bundle
> >          fields = ['name', 'public']
> > @@ -70,11 +79,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")
> > +
>
> Why do we add required=False and then verify it here?

The `required=False` is needed for the patch forms to submit in the
case where no bundle is being created but there are changes to patch
properties (e.g. delegate, state, archived). With required=False, the
form will not make the POST request.

The different submissions patch properties and bundle actions can be
separated into their own separate form elements which would mean that
required=True would validate empty input. However, I decided against
having separate forms because a lot of fields/information is shared
between the two submissions. For example, in the patch-list page the
**selected** patches are passed through a wrapper/container form.

If the forms are separated, then that information will require a lot
more effort to collect for each individual form submission which is
required for the POST requests. So, I think simply sending an error
after a CreateBundleForm submission with an empty bundle name is a
simpler solution. In order to have these nicely separated forms, I
believe that a JS solution would be ideal but more complicated (I
implemented a JS version before).

> >  from django.contrib import messages
> >  from django.shortcuts import get_object_or_404
> >  from django.db.models import Prefetch
> >
> >  from patchwork.filters import Filters
> > +from patchwork.forms import CreateBundleForm
> >  from patchwork.forms import MultiplePatchForm
> >  from patchwork.models import Bundle
> >  from patchwork.models import BundlePatch
> > @@ -108,46 +111,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())
>
> You're trying to perform a deep copy? Why is this needed?

I'm not sure where the deep copy is made. My assumption is that you
mean, this line `create_bundle_form = CreateBundleForm()`? If so, I
removed it (passes all tests cases) for the upcoming v4 series. I had
it there because the code in views/patch.py that handled the patch
forms submission in the patch detail page had the line [1] and I
transferred it to the views/__init__.py.

[1] https://github.com/getpatchwork/patchwork/blob/master/patchwork/views/patch.py#L82