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 <j...@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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
