From: Raxel Gutierrez Date: Mon, 23 Aug 2021 18:28:31 +0000 (+0000) Subject: views: Move and refactor patch-forms X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=17726d72f701dd09b65ffef6a306ef63f863a534;p=thirdparty%2Fpatchwork.git views: Move and refactor patch-forms Move patch forms in patch-list and detail page to a new template file patch-forms.html and move them to the top of the patch-list page to improve their discoverability. 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. In particular, the changes normalize the behavior of the error and update messages of the patch forms and updates tests to reflect the changes. Overall, these changes make patch forms ready for change and more synchronized in their behavior. More specifically: - Previously patch forms changes were separated between the patch-detail and patch-list pages. Thus, desired changes to the patch forms required changes to patch-list.html, submission.html, and forms.py. So, the most important benefit to this change is that forms.py and patch-forms.html become the two places to adjust the forms to handle form validation and functionality as well as UI changes. - Previously the patch forms in patch-list.html handled error and update messages through views in patch.py, whereas the patch forms in submission.html handled the messages with forms.py. Now, with a single patch forms component in patch-forms.html, forms.py is set to handle the messages and handle form validation for both pages. Signed-off-by: Raxel Gutierrez Signed-off-by: Stephen Finucane [stephenfin: Address merge conflicts] --- 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 BundleForm(forms.ModelForm): 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 @@ {% 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 @@ class BundleCreateFromListTest(BundleTestBase): newbundlename = 'testbundle-new' params = { 'form': 'patch-list-form', - 'bundle_name': newbundlename, + 'name': newbundlename, 'action': 'Create', 'project': self.project.id, } @@ -389,7 +389,7 @@ class BundleCreateFromListTest(BundleTestBase): 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 @@ class BundleCreateFromListTest(BundleTestBase): params = { 'form': 'patch-list-form', - 'bundle_name': '', + 'name': '', 'action': 'Create', 'project': self.project.id, 'patch_id:%d' % patch.id: 'checked', @@ -444,7 +444,7 @@ class BundleCreateFromListTest(BundleTestBase): 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 @@ class BundleCreateFromListTest(BundleTestBase): ) 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 @@ class BundleCreateFromPatchTest(BundleTestBase): 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 @@ class BundleCreateFromPatchTest(BundleTestBase): 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 @@ class BundleAddFromListTest(BundleTestBase): 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 @@ class BundleAddFromPatchTest(BundleTestBase): self.assertContains( response, - 'added to bundle "%s"' % self.bundle.name, + 'added to bundle %s' % self.bundle.name, count=1, ) @@ -669,7 +671,7 @@ class BundleAddFromPatchTest(BundleTestBase): 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 @@ class BundleAddFromPatchTest(BundleTestBase): self.assertContains( response, - 'added to bundle "%s"' % self.bundle.name, + 'added to bundle %s' % self.bundle.name, count=1, ) @@ -724,7 +726,7 @@ class BundleInitialOrderTest(BundleTestBase): # 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 @@ 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_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 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 @@ -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)