diff --git a/patchwork/forms.py b/patchwork/forms.py index ed06d0d1..59e82ba0 100644 --- a/patchwork/forms.py +++ b/patchwork/forms.py @@ -64,7 +64,7 @@ class BundleForm(forms.ModelForm): regex=r'^[^/]+$', min_length=1, max_length=50, - label='Name', + required=False, error_messages={'invalid': "Bundle names can't contain slashes"}, ) @@ -76,12 +76,19 @@ class Meta: class CreateBundleForm(BundleForm): def clean_name(self): name = self.cleaned_data['name'] + if not name: + raise forms.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 + 'A bundle called %(name)s already exists', + code='invalid', + params={'name': name}, ) return name diff --git a/patchwork/templates/patchwork/partials/patch-forms.html b/patchwork/templates/patchwork/partials/patch-forms.html new file mode 100644 index 00000000..80f82815 --- /dev/null +++ b/patchwork/templates/patchwork/partials/patch-forms.html @@ -0,0 +1,49 @@ +
+{% if patch_form %} +
+
+ {{ patch_form.state.errors }} + {{ patch_form.state }} +
+
+ {{ patch_form.delegate.errors }} + {{ patch_form.delegate }} +
+
+ {{ patch_form.archived.errors }} + {{ patch_form.archived.label_tag }} {{ patch_form.archived }} +
+ +
+{% endif %} +{% if user.is_authenticated %} +
+
+ {{ create_bundle_form.name.errors }} + {{ create_bundle_form.name }} + +
+ {% if bundles %} +
+ + +
+ {% endif %} + {% if bundle %} +
+ + +
+ {% endif %} +
+{% endif %} +
diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html index 1bdef36d..981ceee5 100644 --- a/patchwork/templates/patchwork/partials/patch-list.html +++ b/patchwork/templates/patchwork/partials/patch-list.html @@ -9,7 +9,6 @@ {% endblock %} {% include "patchwork/partials/filters.html" %} -{% include "patchwork/partials/pagination.html" %} {% if order.editable %} @@ -28,18 +27,15 @@
{% endif %} -{% if page.paginator.long_page and user.is_authenticated %} -
- - - -
-{% endif %} - -
+ {% csrf_token %} + +
+{% include "patchwork/partials/patch-forms.html" %} +{% include "patchwork/partials/pagination.html" %} +
@@ -159,7 +155,7 @@ {% for patch in page.object_list %} - + {% if user.is_authenticated %}
@@ -201,82 +197,5 @@ {% if page.paginator.count %} {% include "patchwork/partials/pagination.html" %} - -
- -{% if patch_form %} -
-

Properties

- - - - - - - - - - - - - - - - - -
Change state: - {{ patch_form.state }} - {{ patch_form.state.errors }} -
Delegate to: - {{ patch_form.delegate }} - {{ patch_form.delegate.errors }} -
Archive: - {{ patch_form.archived }} - {{ patch_form.archived.errors }} -
- -
-
-{% endif %} - -{% if user.is_authenticated %} -
-

Bundling

- - - - - -{% if bundles %} - - - - -{% endif %} -{% if bundle %} - - - - -{% endif %} -
Create bundle: - - -
Add to bundle: - - -
Remove from bundle: - - -
-
-{% endif %} -
-
-
{% endif %} diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html index aa4e5553..cd74491c 100644 --- a/patchwork/templates/patchwork/submission.html +++ b/patchwork/templates/patchwork/submission.html @@ -132,89 +132,10 @@

{{ submission.name }}

{% endif %}
-
-{% if patch_form %} -
-

Patch Properties

-
- {% csrf_token %} - - - - - - - - - - - - - - - - - -
Change state: - {{ patch_form.state }} - {{ patch_form.state.errors }} -
Delegate to: - {{ patch_form.delegate }} - {{ patch_form.delegate.errors }} -
Archived: - {{ patch_form.archived }} - {{ patch_form.archived.errors }} -
- -
-
-
-{% endif %} - -{% if create_bundle_form %} -
-

Bundling

- - - - - -{% if bundles %} - - - - -{% endif %} -
Create bundle: -{% if create_bundle_form.non_field_errors %} -
{{create_bundle_form.non_field_errors}}
-{% endif %} -
- {% csrf_token %} - -{% if create_bundle_form.name.errors %} -
{{create_bundle_form.name.errors}}
-{% endif %} - {{ create_bundle_form.name }} - -
-
Add to bundle: -
-{% csrf_token %} - - - -
-
-
-{% endif %} -
-
-
+
+ {% csrf_token %} + {% include "patchwork/partials/patch-forms.html" %} +
{% if submission.pull_url %}

Pull-request

diff --git a/patchwork/tests/views/test_bundles.py b/patchwork/tests/views/test_bundles.py index 24ebe612..1d317c30 100644 --- a/patchwork/tests/views/test_bundles.py +++ b/patchwork/tests/views/test_bundles.py @@ -369,7 +369,7 @@ def test_create_empty_bundle(self): newbundlename = 'testbundle-new' params = { 'form': 'patch-list-form', - 'bundle_name': newbundlename, + 'name': newbundlename, 'action': 'Create', 'project': self.project.id, } @@ -389,7 +389,7 @@ def test_create_non_empty_bundle(self): params = { 'form': 'patch-list-form', - 'bundle_name': newbundlename, + 'name': newbundlename, 'action': 'Create', 'project': self.project.id, 'patch_id:%d' % patch.id: 'checked', @@ -418,7 +418,7 @@ def test_create_non_empty_bundle_empty_name(self): params = { 'form': 'patch-list-form', - 'bundle_name': '', + 'name': '', 'action': 'Create', 'project': self.project.id, 'patch_id:%d' % patch.id: 'checked', @@ -444,7 +444,7 @@ def test_create_duplicate_name(self): params = { 'form': 'patch-list-form', - 'bundle_name': newbundlename, + 'name': newbundlename, 'action': 'Create', 'project': self.project.id, 'patch_id:%d' % patch.id: 'checked', @@ -475,7 +475,9 @@ def test_create_duplicate_name(self): ) 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) @@ -485,7 +487,7 @@ def test_create_non_empty_bundle(self): newbundlename = 'testbundle-new' patch = self.patches[0] - params = {'name': newbundlename, 'action': 'createbundle'} + params = {'name': newbundlename, 'action': 'Create'} response = self.client.post( reverse( @@ -508,7 +510,7 @@ def test_create_with_existing_name(self): newbundlename = self.bundle.name patch = self.patches[0] - params = {'name': newbundlename, 'action': 'createbundle'} + params = {'name': newbundlename, 'action': 'Create'} response = self.client.post( reverse( @@ -644,7 +646,7 @@ def test_add_new_and_duplicate(self): class BundleAddFromPatchTest(BundleTestBase): def test_add_to_empty_bundle(self): patch = self.patches[0] - params = {'action': 'addtobundle', 'bundle_id': self.bundle.id} + params = {'action': 'Add', 'bundle_id': self.bundle.id} response = self.client.post( reverse( @@ -659,7 +661,7 @@ def test_add_to_empty_bundle(self): self.assertContains( response, - 'added to bundle "%s"' % self.bundle.name, + 'added to bundle %s' % self.bundle.name, count=1, ) @@ -669,7 +671,7 @@ def test_add_to_empty_bundle(self): def test_add_to_non_empty_bundle(self): self.bundle.append_patch(self.patches[0]) patch = self.patches[1] - params = {'action': 'addtobundle', 'bundle_id': self.bundle.id} + params = {'action': 'Add', 'bundle_id': self.bundle.id} response = self.client.post( reverse( @@ -684,7 +686,7 @@ def test_add_to_non_empty_bundle(self): self.assertContains( response, - 'added to bundle "%s"' % self.bundle.name, + 'added to bundle %s' % self.bundle.name, count=1, ) @@ -724,7 +726,7 @@ def _test_order(self, ids, expected_order): # need to define our querystring explicity to enforce ordering params = { 'form': 'patch-list-form', - 'bundle_name': newbundlename, + 'name': newbundlename, 'action': 'Create', 'project': self.project.id, } diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py index 51649488..db484c79 100644 --- a/patchwork/views/__init__.py +++ b/patchwork/views/__init__.py @@ -3,11 +3,14 @@ # # SPDX-License-Identifier: GPL-2.0-or-later +import json + 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,52 +111,34 @@ def apply(self, qs): # 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_name = data['name'].strip() 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() + add_bundle_patches(request, patches, bundle) + bundle.save() + messages.success(request, 'Bundle %s created' % bundle.name) + else: + formErrors = json.loads(create_bundle_form.errors.as_json()) + errors = [e['message'] for e in formErrors['name']] + 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']) + add_bundle_patches(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() @@ -165,10 +150,26 @@ def set_bundle(request, project, action, data, patches, context): "Patch '%s' removed from bundle %s\n" % (patch.name, bundle.name), ) + return [] - bundle.save() - return [] +def add_bundle_patches(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( @@ -232,6 +233,7 @@ def generic_list( 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 @@ -241,19 +243,20 @@ def generic_list( data_tmp = data properties_form = MultiplePatchForm(project, data=data_tmp) + create_bundle_form = CreateBundleForm() 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' # text field, so if non-empty, assume the create action: - if data.get('bundle_name', False): + if data.get('name', False): action = 'create' 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( @@ -320,6 +323,7 @@ def generic_list( { 'page': paginator.current_page, 'patch_form': properties_form, + 'create_bundle_form': create_bundle_form, 'project': project, 'order': order, } diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 83839163..efe94f17 100644 --- a/patchwork/views/patch.py +++ b/patchwork/views/patch.py @@ -14,11 +14,11 @@ 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 @@ -64,6 +64,7 @@ def patch_detail(request, project_id, msgid): form = None create_bundle_form = None + errors = None if editable: form = PatchForm(instance=patch) @@ -75,39 +76,15 @@ def patch_detail(request, project_id, msgid): if action: action = action.lower() - if action == 'createbundle': - bundle = Bundle(owner=request.user, project=project) - create_bundle_form = CreateBundleForm( - instance=bundle, data=request.POST + if action in ['create', 'add']: + errors = set_bundle( + request, project, action, request.POST, [patch] ) - if create_bundle_form.is_valid(): - create_bundle_form.save() - bundle.append_patch(patch) - bundle.save() - create_bundle_form = 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 elif not editable: return HttpResponseForbidden() - elif action is None: + elif action == 'update': form = PatchForm(data=request.POST, instance=patch) if form.is_valid(): form.save() @@ -147,6 +124,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)