[PATCH v3 2/5] patch-list: clean up patch-list page and refactor patch forms
Daniel Axtens
dja at axtens.net
Tue Aug 3 11:56:46 AEST 2021
Hi Raxel,
I think this patch may do a bit much, and I am struggling to follow it.
If I understand correctly,
> Clean up the patch-list page by moving forms to a new template file
> patch-forms.html and move them to the top of the page, add ids to
this bit makes a user visible change, but
> No user-visible change should be noticed since this change does not
> make stye changes. Refactor forms.py, __init__.py, patch.py, and
this other change doesn't.
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!)
> Clean up the patch-list page by moving forms to a new template file
> patch-forms.html and move them to the top of the page, add ids to
> table cells, and rename selectors using hyphen delimited strings where
> the relevant changes were made. Also, create a partial template for
> errors that render with form submission. These changes improve the
> discoverability of the patch-list forms and make the code healthier,
> ready for change, and overall more readable.
Is there a reason to prefer hyphen-delimited selectors? It looks like in
the css file we have a mix of hyphen-delimited ones from bootstrap, ones
without delimiters at all, and even underscores. I wonder if this would
be something we should try to change all at once, rather than just
changing the odd selector here and there? (Not saying you have to do
that now! Just trying to contain the churn and review size.)
Apart from that, these non-user-visible changes seem like good ideas to me.
> 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.
Keen on this. I'd be happy to apply a patch that does just this.
> No user-visible change should be noticed since this change does not
> make stye changes. 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 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.
>
I like this in theory but some of the python is a bit scary and I'm
struggling to figure out which parts of this patch belong to this change
and which parts of the patch belong to the other changes.
Some smaller comments follow.
> Signed-off-by: Raxel Gutierrez <raxel at google.com>
> ---
> htdocs/README.rst | 7 +
> htdocs/css/style.css | 10 +-
> htdocs/js/patch-list.js | 12 ++
> patchwork/forms.py | 22 ++-
> patchwork/templates/patchwork/list.html | 11 +-
> .../templates/patchwork/partials/errors.html | 9 ++
> .../patchwork/partials/patch-forms.html | 45 ++++++
> .../patchwork/partials/patch-list.html | 137 +++---------------
> patchwork/templates/patchwork/submission.html | 91 +-----------
> patchwork/tests/views/test_bundles.py | 44 +++---
> patchwork/tests/views/test_patch.py | 4 +-
> patchwork/views/__init__.py | 73 +++++-----
> patchwork/views/patch.py | 35 ++---
> 13 files changed, 197 insertions(+), 303 deletions(-)
> create mode 100644 htdocs/js/patch-list.js
> create mode 100644 patchwork/templates/patchwork/partials/errors.html
> create mode 100644 patchwork/templates/patchwork/partials/patch-forms.html
>
> diff --git a/htdocs/README.rst b/htdocs/README.rst
> index 128dc7c..404356a 100644
> --- a/htdocs/README.rst
> +++ b/htdocs/README.rst
> @@ -131,6 +131,13 @@ js
> :GitHub: https://github.com/js-cookie/js-cookie/
> :Version: 3.0.0
>
> +``patch-list.js.``
> +
> + Event helpers and other application logic for patch-list.html. These
> + support patch list manipulation (e.g. inline dropdown changes).
> +
(Technically you haven't added inline dropdown changes yet! Maybe just
drop the e.g. part.)
> + Part of Patchwork.
> +
> ``selectize.min.js``
>
> Selectize is the hybrid of a ``textbox`` and ``<select>`` box. It's jQuery
> diff --git a/htdocs/css/style.css b/htdocs/css/style.css
> index 243caa0..1bcc93e 100644
> --- a/htdocs/css/style.css
> +++ b/htdocs/css/style.css
> @@ -122,7 +122,7 @@ a.colinactive:hover {
> div.filters {
> }
>
> -div.patchforms {
> +div.patch-forms {
> margin-top: 1em;
> }
>
> @@ -204,7 +204,7 @@ table.patchmeta tr th, table.patchmeta tr td {
> }
>
> /* checks forms */
> -/* TODO(stephenfin): Merge this with 'div.patchform' rules */
> +/* TODO(stephenfin): Merge this with 'div.patch-form' rules */
> .checks {
> border: 1px solid gray;
> margin: 0.5em 1em;
> @@ -347,7 +347,7 @@ table.bundlelist td
> }
>
> /* forms that appear for a patch */
> -div.patchform {
> +div.patch-form {
> border: thin solid #080808;
> padding-left: 0.6em;
> padding-right: 0.6em;
> @@ -355,7 +355,7 @@ div.patchform {
> margin: 0.5em 5em 0.5em 10px;
> }
>
> -div.patchform h3 {
> +div.patch-form h3 {
> margin-top: 0em;
> margin-left: -0.6em;
> margin-right: -0.6em;
> @@ -365,7 +365,7 @@ div.patchform h3 {
> font-size: 100%;
> }
>
> -div.patchform ul {
> +div.patch-form ul {
> list-style-type: none;
> padding-left: 0.2em;
> margin-top: 0em;
> diff --git a/htdocs/js/patch-list.js b/htdocs/js/patch-list.js
> new file mode 100644
> index 0000000..5c94e66
> --- /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();
> + });
> +});
> 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.
> 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.)
> 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?
> count = Bundle.objects.filter(owner=self.instance.owner,
> name=name).count()
> if count > 0:
> - raise forms.ValidationError('A bundle called %s already exists'
> - % name)
> + raise ValidationError('A bundle called %(name)s already exists',
> + code="invalid",
> + params={'name': name})
> return name
>
>
> diff --git a/patchwork/templates/patchwork/list.html b/patchwork/templates/patchwork/list.html
> index 5d3d82a..d4e81bc 100644
> --- 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 %}
> 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">
> +{% if patchform %}
> + <div id="patch-form-properties" class="patch-form">
> + <div id="patch-form-state">
> + {{ patchform.state.errors }}
> + {{ patchform.state }}
> + </div>
> + <div id="patch-form-delegate">
> + {{ patchform.delegate.errors }}
> + {{ patchform.delegate }}
> + </div>
> + <div id="patch-form-archive">
> + {{ patchform.archived.errors }}
> + {{ patchform.archived.label_tag }} {{ patchform.archived }}
> + </div>
> + <button class="patch-form-submit btn btn-primary" name="action" value="update">Update</button>
> + </div>
> +{% endif %}
> +{% if user.is_authenticated %}
> + <div id="patch-form-bundle" class="patch-form">
> + <div id="create-bundle">
> + {{ createbundleform.name.errors }}
> + {{ createbundleform.name }}
> + <input class="patch-form-submit btn btn-primary" name="action" value="Create" type="submit"/>
> + </div>
> + {% if bundles %}
> + <div id="add-to-bundle">
> + <select class="add-bundle" name="bundle_id">
> + <option value="" selected>Add to bundle</option>
> + {% for bundle in bundles %}
> + <option value="{{bundle.id}}">{{bundle.name}}</option>
> + {% endfor %}
> + </select>
> + <input class="patch-form-submit btn btn-primary" name="action" value="Add" type="submit"/>
> + </div>
> + {% endif %}
> + {% if bundle %}
> + <div id="remove-bundle">
> + <input type="hidden" name="removed_bundle_id" value="{{bundle.id}}"/>
> + <button class="patch-form-submit btn btn-primary" name="action" value="Remove">Remove from bundle</button>
> + </div>
> + {% endif %}
> + </div>
> +{% endif %}
> +</div>
> diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html
> index 02d6dff..5e2f0dd 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 %}
>
> -{% include "patchwork/partials/filters.html" %}
> +{% block headers %}
> + <script src="{% static "js/patch-list.js" %}"></script>
> +{% endblock %}
>
> -{% include "patchwork/partials/pagination.html" %}
> +{% include "patchwork/partials/filters.html" %}
>
> {% if order.editable %}
> <table class="patchlist">
> @@ -27,33 +29,14 @@
> </table>
> {% endif %}
>
> -{% if page.paginator.long_page and user.is_authenticated %}
> -<div class="floaty">
> - <a title="jump to form" href="#patchforms">
> - <span style="font-size: 120%">▾</span>
> - </a>
> -</div>
> -{% endif %}
> -
> -<script type="text/javascript">
> -$(document).ready(function() {
> - $('#patchlist').stickyTableHeaders();
> -
> - $('#check-all').change(function(e) {
> - if(this.checked) {
> - $('#patchlist > tbody').checkboxes('check');
> - } else {
> - $('#patchlist > tbody').checkboxes('uncheck');
> - }
> - e.preventDefault();
> - });
> -});
> -</script>
> -
> -<form method="post">
> +<form id="patch-list-form" method="post">
> {% 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" %}
> +</div>
> <table id="patchlist" class="table table-hover table-extra-condensed table-striped pw-list"
> data-toggle="checkboxes" data-range="true">
> <thead>
> @@ -174,9 +157,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:{{patch.id}}" style="text-align: center;">
> <input type="checkbox" name="patch_id:{{patch.id}}"/>
> </td>
> {% endif %}
> @@ -188,24 +171,24 @@ $(document).ready(function() {
> </button>
> </td>
> {% endif %}
> - <td>
> + <td id="patch-name:{{patch.id}}">
> <a href="{% url 'patch-detail' project_id=project.linkname msgid=patch.url_msgid %}">
> {{ patch.name|default:"[no subject]"|truncatechars:100 }}
> </a>
> </td>
> - <td>
> + <td id="patch-series:{{patch.id}}">
> {% if patch.series %}
> <a href="?series={{patch.series.id}}">
> {{ patch.series|truncatechars:100 }}
> </a>
> {% endif %}
> </td>
> - <td class="text-nowrap">{{ patch|patch_tags }}</td>
> - <td class="text-nowrap">{{ patch|patch_checks }}</td>
> - <td class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
> - <td>{{ patch.submitter|personify:project }}</td>
> - <td>{{ patch.delegate.username }}</td>
> - <td>{{ patch.state }}</td>
> + <td id="patch-tags:{{patch.id}}" class="text-nowrap">{{ patch|patch_tags }}</td>
> + <td id="patch-checks:{{patch.id}}" class="text-nowrap">{{ patch|patch_checks }}</td>
> + <td id="patch-date:{{patch.id}}" class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
> + <td id="patch-submitter:{{patch.id}}">{{ patch.submitter|personify:project }}</td>
> + <td id="patch-delegate:{{patch.id}}">{{ patch.delegate.username }}</td>
> + <td id="patch-state:{{patch.id}}">{{ patch.state }}</td>
> </tr>
> {% empty %}
> <tr>
> @@ -217,86 +200,6 @@ $(document).ready(function() {
>
> {% if page.paginator.count %}
> {% include "patchwork/partials/pagination.html" %}
> -
> -<div class="patchforms" id="patchforms">
> -
> -{% if patchform %}
> - <div class="patchform patchform-properties">
> - <h3>Properties</h3>
> - <table class="form">
> - <tr>
> - <th>Change state:</th>
> - <td>
> - {{ patchform.state }}
> - {{ patchform.state.errors }}
> - </td>
> - </tr>
> - <tr>
> - <th>Delegate to:</th>
> - <td>
> - {{ patchform.delegate }}
> - {{ patchform.delegate.errors }}
> - </td>
> - </tr>
> - <tr>
> - <th>Archive:</th>
> - <td>
> - {{ patchform.archived }}
> - {{ patchform.archived.errors }}
> - </td>
> - </tr>
> - <tr>
> - <td></td>
> - <td>
> - <input type="submit" name="action" value="{{patchform.action}}"/>
> - </td>
> - </tr>
> - </table>
> - </div>
> -
> -{% endif %}
> -
> -{% if user.is_authenticated %}
> - <div class="patchform patchform-bundle">
> - <h3>Bundling</h3>
> - <table class="form">
> - <tr>
> - <td>Create bundle:</td>
> - <td>
> - <input type="text" name="bundle_name"/>
> - <input name="action" value="Create" type="submit"/>
> - </td>
> - </tr>
> - {% if bundles %}
> - <tr>
> - <td>Add to bundle:</td>
> - <td>
> - <select name="bundle_id">
> - {% for bundle in bundles %}
> - <option value="{{bundle.id}}">{{bundle.name}}</option>
> - {% endfor %}
> - </select>
> - <input name="action" value="Add" type="submit"/>
> - </td>
> - </tr>
> - {% endif %}
> - {% if bundle %}
> - <tr>
> - <td>Remove from bundle:</td>
> - <td>
> - <input type="hidden" name="removed_bundle_id" value="{{bundle.id}}"/>
> - <input name="action" value="Remove" type="submit"/>
> - </td>
> - </tr>
> - {% endif %}
> - </table>
> - </div>
> -{% endif %}
> -
> - <div style="clear: both;">
> - </div>
> -</div>
> -
> {% endif %}
>
> </form>
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index 978559b..17255ee 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -27,6 +27,8 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> }
> </script>
>
> +{% include "patchwork/partials/errors.html" %}
> +
> <div>
> {% include "patchwork/partials/download-buttons.html" %}
> <h1>{{ submission.name }}</h1>
> @@ -149,91 +151,10 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> {% endif %}
> </table>
>
> -<div class="patchforms">
> -{% if patchform %}
> - <div class="patchform patchform-properties">
> - <h3>Patch Properties</h3>
> - <form method="post">
> - {% csrf_token %}
> - <table class="form">
> - <tr>
> - <th>Change state:</th>
> - <td>
> - {{ patchform.state }}
> - {{ patchform.state.errors }}
> - </td>
> - </tr>
> - <tr>
> - <th>Delegate to:</th>
> - <td>
> - {{ patchform.delegate }}
> - {{ patchform.delegate.errors }}
> - </td>
> - </tr>
> - <tr>
> - <th>Archived:</th>
> - <td>
> - {{ patchform.archived }}
> - {{ patchform.archived.errors }}
> - </td>
> - </tr>
> - <tr>
> - <td></td>
> - <td>
> - <input type="submit" value="Update">
> - </td>
> - </tr>
> - </table>
> - </form>
> - </div>
> -{% endif %}
> -
> -{% if createbundleform %}
> - <div class="patchform patchform-bundle">
> - <h3>Bundling</h3>
> - <table class="form">
> - <tr>
> - <td>Create bundle:</td>
> - <td>
> - {% if createbundleform.non_field_errors %}
> - <dd class="errors">{{createbundleform.non_field_errors}}</dd>
> - {% endif %}
> - <form method="post">
> - {% csrf_token %}
> - <input type="hidden" name="action" value="createbundle"/>
> - {% if createbundleform.name.errors %}
> - <dd class="errors">{{createbundleform.name.errors}}</dd>
> - {% endif %}
> - {{ createbundleform.name }}
> - <input value="Create" type="submit"/>
> - </form>
> - </td>
> - </tr>
> -{% if bundles %}
> - <tr>
> - <td>Add to bundle:</td>
> - <td>
> - <form method="post">
> - {% csrf_token %}
> - <input type="hidden" name="action" value="addtobundle"/>
> - <select name="bundle_id"/>
> - {% for bundle in bundles %}
> - <option value="{{bundle.id}}">{{bundle.name}}</option>
> - {% endfor %}
> - </select>
> - <input value="Add" type="submit"/>
> - </form>
> - </td>
> - </tr>
> -{% endif %}
> - </table>
> -
> - </div>
> -{% endif %}
> -
> - <div style="clear: both;">
> - </div>
> -</div>
> +<form id="patch-list-form" method="POST">
> + {% csrf_token %}
> + {% include "patchwork/partials/patch-forms.html" %}
> +</form>
>
> {% if submission.pull_url %}
> <h2>Pull-request</h2>
> diff --git a/patchwork/tests/views/test_bundles.py b/patchwork/tests/views/test_bundles.py
> index 6a74409..2233c21 100644
> --- a/patchwork/tests/views/test_bundles.py
> +++ b/patchwork/tests/views/test_bundles.py
> @@ -146,7 +146,7 @@ class BundleUpdateTest(BundleTestBase):
> data = {
> 'form': 'bundle',
> 'action': 'update',
> - 'name': newname,
> + 'bundle_name': newname,
> 'public': '',
> }
> response = self.client.post(bundle_url(self.bundle), data)
> @@ -159,7 +159,7 @@ class BundleUpdateTest(BundleTestBase):
> data = {
> 'form': 'bundle',
> 'action': 'update',
> - 'name': self.bundle.name,
> + 'bundle_name': self.bundle.name,
> 'public': 'on',
> }
> response = self.client.post(bundle_url(self.bundle), data)
> @@ -243,7 +243,7 @@ class BundlePublicModifyTest(BundleTestBase):
> data = {
> 'form': 'bundle',
> 'action': 'update',
> - 'name': newname,
> + 'bundle_name': newname,
> }
> self.bundle.name = oldname
> self.bundle.save()
> @@ -353,7 +353,7 @@ class BundleCreateFromListTest(BundleTestBase):
>
> def test_create_empty_bundle(self):
> newbundlename = 'testbundle-new'
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'bundle_name': newbundlename,
> 'action': 'Create',
> 'project': self.project.id}
> @@ -369,7 +369,7 @@ class BundleCreateFromListTest(BundleTestBase):
> newbundlename = 'testbundle-new'
> patch = self.patches[0]
>
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'bundle_name': newbundlename,
> 'action': 'Create',
> 'project': self.project.id,
> @@ -393,7 +393,7 @@ class BundleCreateFromListTest(BundleTestBase):
>
> n_bundles = Bundle.objects.count()
>
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'bundle_name': '',
> 'action': 'Create',
> 'project': self.project.id,
> @@ -414,7 +414,7 @@ class BundleCreateFromListTest(BundleTestBase):
> newbundlename = 'testbundle-dup'
> patch = self.patches[0]
>
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'bundle_name': newbundlename,
> 'action': 'Create',
> 'project': self.project.id,
> @@ -440,7 +440,9 @@ class BundleCreateFromListTest(BundleTestBase):
> params)
>
> 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)
>
> @@ -451,8 +453,8 @@ class BundleCreateFromPatchTest(BundleTestBase):
> newbundlename = 'testbundle-new'
> patch = self.patches[0]
>
> - params = {'name': newbundlename,
> - 'action': 'createbundle'}
> + params = {'bundle_name': newbundlename,
> + 'action': 'Create'}
>
> response = self.client.post(
> reverse('patch-detail',
> @@ -470,8 +472,8 @@ class BundleCreateFromPatchTest(BundleTestBase):
> newbundlename = self.bundle.name
> patch = self.patches[0]
>
> - params = {'name': newbundlename,
> - 'action': 'createbundle'}
> + params = {'bundle_name': newbundlename,
> + 'action': 'Create'}
>
> response = self.client.post(
> reverse('patch-detail',
> @@ -489,7 +491,7 @@ class BundleAddFromListTest(BundleTestBase):
>
> def test_add_to_empty_bundle(self):
> patch = self.patches[0]
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'action': 'Add',
> 'project': self.project.id,
> 'bundle_id': self.bundle.id,
> @@ -509,7 +511,7 @@ class BundleAddFromListTest(BundleTestBase):
> def test_add_to_non_empty_bundle(self):
> self.bundle.append_patch(self.patches[0])
> patch = self.patches[1]
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'action': 'Add',
> 'project': self.project.id,
> 'bundle_id': self.bundle.id,
> @@ -538,7 +540,7 @@ class BundleAddFromListTest(BundleTestBase):
> count = self.bundle.patches.count()
> patch = self.patches[0]
>
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'action': 'Add',
> 'project': self.project.id,
> 'bundle_id': self.bundle.id,
> @@ -559,7 +561,7 @@ class BundleAddFromListTest(BundleTestBase):
> count = self.bundle.patches.count()
> patch = self.patches[0]
>
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'action': 'Add',
> 'project': self.project.id,
> 'bundle_id': self.bundle.id,
> @@ -584,7 +586,7 @@ class BundleAddFromPatchTest(BundleTestBase):
>
> def test_add_to_empty_bundle(self):
> patch = self.patches[0]
> - params = {'action': 'addtobundle',
> + params = {'action': 'Add',
> 'bundle_id': self.bundle.id}
>
> response = self.client.post(
> @@ -594,7 +596,7 @@ class BundleAddFromPatchTest(BundleTestBase):
>
> self.assertContains(
> response,
> - 'added to bundle "%s"' % self.bundle.name,
> + 'added to bundle %s' % self.bundle.name,
> count=1)
>
> self.assertEqual(self.bundle.patches.count(), 1)
> @@ -603,7 +605,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',
> + params = {'action': 'Add',
> 'bundle_id': self.bundle.id}
>
> response = self.client.post(
> @@ -613,7 +615,7 @@ class BundleAddFromPatchTest(BundleTestBase):
>
> self.assertContains(
> response,
> - 'added to bundle "%s"' % self.bundle.name,
> + 'added to bundle %s' % self.bundle.name,
> count=1)
>
> self.assertEqual(self.bundle.patches.count(), 2)
> @@ -650,7 +652,7 @@ class BundleInitialOrderTest(BundleTestBase):
> newbundlename = 'testbundle-new'
>
> # need to define our querystring explicity to enforce ordering
> - params = {'form': 'patchlistform',
> + params = {'form': 'patch-list-form',
> 'bundle_name': newbundlename,
> 'action': 'Create',
> 'project': self.project.id,
> diff --git a/patchwork/tests/views/test_patch.py b/patchwork/tests/views/test_patch.py
> index 1a1243c..483ab99 100644
> --- a/patchwork/tests/views/test_patch.py
> +++ b/patchwork/tests/views/test_patch.py
> @@ -304,7 +304,7 @@ class PatchViewTest(TestCase):
>
> class PatchUpdateTest(TestCase):
>
> - properties_form_id = 'patchform-properties'
> + properties_form_id = 'patch-form-properties'
>
> def setUp(self):
> self.project = create_project()
> @@ -318,7 +318,7 @@ class PatchUpdateTest(TestCase):
> self.base_data = {
> 'action': 'Update',
> 'project': str(self.project.id),
> - 'form': 'patchlistform',
> + 'form': 'patch-list-form',
> 'archived': '*',
> 'delegate': '*',
> 'state': '*'
> diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py
> index 3efe90c..3ea2af4 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,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?
> + 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'])
> + 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']:
> - 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()
> @@ -158,10 +150,21 @@ def set_bundle(request, project, action, data, patches, context):
> request,
> "Patch '%s' removed from bundle %s\n" % (patch.name,
> bundle.name))
> + return []
>
> - bundle.save()
>
> - return []
> +def addBundlePatches(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(request, project, view, view_args=None, filter_settings=None,
> @@ -216,17 +219,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()
>
> # special case: the user may have hit enter in the 'create bundle'
> @@ -237,7 +243,7 @@ def generic_list(request, project, view, view_args=None, filter_settings=None,
> 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(request, properties_form,
> @@ -288,6 +294,7 @@ def generic_list(request, project, view, view_args=None, filter_settings=None,
> context.update({
> 'page': paginator.current_page,
> 'patchform': properties_form,
> + 'createbundleform': create_bundle_form,
> 'project': project,
> 'order': order,
> })
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index 3e6874a..229e1dd 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
>
> @@ -60,6 +60,7 @@ def patch_detail(request, project_id, msgid):
>
> form = None
> createbundleform = None
> + errors = None
>
> if editable:
> form = PatchForm(instance=patch)
> @@ -71,34 +72,14 @@ 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':
> - 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()
>
> - elif action is None:
> + elif action == 'update':
> form = PatchForm(data=request.POST, instance=patch)
> if form.is_valid():
> form.save()
> @@ -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
>
> return render(request, 'patchwork/submission.html', context)
>
> --
> 2.32.0.554.ge1b32706d8-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list