This series is a revision to the previous version of the patch series. Following comments, I changed the commit messages to use the conventional imperative mood and added more details to better express the reasons behind the changes.
For the first patch, I removed the version number to make the use of the JS cookie library cleaner. There is still debate around where and how the file should be stored [1]. For the second patch, I did a lot more cleanup of the patch-list page. The error messages from Django form validation is now it’s own partial template so that it can be easily reused in various templates. The most important and major changes are that the patch forms also become a partial template so that it can be used for a list of patches in the patch-list page and for individual patches in the patch-details page. The patch forms in each respective page had their own quirks, so some refactoring had to be done on related files to preserve the functionality in both use cases. The effort was worth it as now the patch forms tool is more ready for change. For the third patch, the commit message features before and after images for better reference of the style changes to an action bar UI. As noted by jrnieder’s comments, I fixed the potential risk of unadapted callers by using keyword-only arguments as better practice. Also, the responsibility of setting the CSS class selector for the patch form’s fields is moved from the form to the fields’ constructors which adds more flexibility for naming within each field. For the fourth patch, the utils.js file is renamed to rest.js to initiate the convention of different groupings of utility files and functions. For the fifth patch, the commit message changes to better explain the changes and accounts for the change to the rest.js file it uses. [1] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006966.html Raxel Gutierrez (5): static: add JS Cookie Library to get csrftoken for fetch requests patch-list: clean up patch-list page and refactor patch forms patch-list: style modification forms as an action bar static: add rest.js to handle requests & respective messages patch-list: add inline dropdown for delegate and state one-off changes htdocs/README.rst | 23 +++ htdocs/css/style.css | 85 ++++++++-- htdocs/js/js.cookie.min.js | 3 + htdocs/js/patch-list.js | 46 ++++++ htdocs/js/rest.js | 71 ++++++++ patchwork/forms.py | 64 +++++-- patchwork/templates/patchwork/list.html | 11 +- .../templates/patchwork/partials/errors.html | 9 + .../patchwork/partials/patch-forms.html | 45 +++++ .../patchwork/partials/patch-list.html | 156 ++++++------------ patchwork/templates/patchwork/submission.html | 91 +--------- patchwork/tests/views/test_bundles.py | 44 ++--- patchwork/tests/views/test_patch.py | 4 +- patchwork/views/__init__.py | 76 +++++---- patchwork/views/patch.py | 33 +--- templates/base.html | 3 +- 16 files changed, 446 insertions(+), 318 deletions(-) create mode 100644 htdocs/js/js.cookie.min.js create mode 100644 htdocs/js/patch-list.js create mode 100644 htdocs/js/rest.js create mode 100644 patchwork/templates/patchwork/partials/errors.html create mode 100644 patchwork/templates/patchwork/partials/patch-forms.html Interdiff: diff --git a/htdocs/README.rst b/htdocs/README.rst index 2bae34c..12174de 100644 --- a/htdocs/README.rst +++ b/htdocs/README.rst @@ -126,7 +126,7 @@ js Library used to handle cookies. - This is used to get the ``csrftoken`` cookie for AJAX POST requests. + This is used to get the ``csrftoken`` cookie for AJAX requests in JavaScript. :GitHub: https://github.com/js-cookie/js-cookie/ :Version: 2.2.1 @@ -138,6 +138,13 @@ js Part of Patchwork. +``rest.js.`` + + Utility module for REST API requests to be used by other Patchwork js files + (fetch requests, handling update & error messages). + + Part of Patchwork. + ``selectize.min.js`` Selectize is the hybrid of a ``textbox`` and ``<select>`` box. It's jQuery @@ -147,10 +154,3 @@ js :Website: https://selectize.github.io/selectize.js/ :GitHub: https://github.com/selectize/selectize.js :Version: 0.11.2 - -``utils.js.`` - - General utility module for functions used throughout other static Patchwork - js files (fetch requests, handling update & error messages). - - Part of Patchwork. diff --git a/htdocs/css/style.css b/htdocs/css/style.css index 2a45ec7..9982f92 100644 --- a/htdocs/css/style.css +++ b/htdocs/css/style.css @@ -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; @@ -354,19 +354,19 @@ table.bundlelist td } /* forms that appear for a patch */ -.patchforms { +.patch-forms { display: inline-flex; flex-wrap: wrap; margin: 16px 0px; } -div.patchform { +div.patch-form { display: flex; flex-wrap: wrap; align-items: center; } -.change-property, select[class^="change-property-"], .archive-patch, .add-bundle { +select[class^=change-property-], .archive-patch-select, .add-bundle { padding: 4px; margin-right: 8px; box-sizing: border-box; @@ -374,12 +374,26 @@ div.patchform { background-color: var(--light-color); } -.patchform-submit { +#patch-form-archive { + display: flex; + align-items: center; + margin-right: 4px; +} + +#patch-form-archive > label { + margin: 0px; +} + +#patch-form-archive > select, #patch-form-archive > input { + margin: 0px 4px 0px 4px; +} + +.patch-form-submit { font-weight: bold; padding: 4px; } -#patchform-bundle, #add-to-bundle, #remove-bundle { +#patch-form-bundle, #add-to-bundle, #remove-bundle { margin-left: 16px; } @@ -390,6 +404,29 @@ div.patchform { border-radius: 4px; } +div.patch-form h3 { + margin-top: 0em; + margin-left: -0.6em; + margin-right: -0.6em; + padding: 0.3em 0.3em 0.3em 0.6em; + background-color: #222; + color: #999; + font-size: 100%; +} + +div.patch-form ul { + list-style-type: none; + padding-left: 0.2em; + margin-top: 0em; +} + +.create-bundle { + padding: 4px; + margin-right: 8px; + box-sizing: border-box; + border-radius: 4px; +} + span.help_text { font-size: 80%; } diff --git a/htdocs/js/js.cookie-2.2.1.min.js b/htdocs/js/js.cookie.min.js similarity index 60% rename from htdocs/js/js.cookie-2.2.1.min.js rename to htdocs/js/js.cookie.min.js index 72f312a..f5f4c36 100644 --- a/htdocs/js/js.cookie-2.2.1.min.js +++ b/htdocs/js/js.cookie.min.js @@ -1,3 +1,3 @@ /*! js-cookie v2.2.1 | MIT */ -!function(a){var b;if("function"==typeof define&&define.amd&&(define(a),b=!0),"object"==typeof exports&&(module.exports=a(),b=!0),!b){var c=window.Cookies,d=window.Cookies=a();d.noConflict=function(){return window.Cookies=c,d}}}(function(){function a(){for(var a=0,b={};a<arguments.length;a++){var c=arguments[a];for(var d in c)b[d]=c[d]}return b}function b(a){return a.replace(/(%[0-9A-Z]{2})+/g,decodeURIComponent)}function c(d){function e(){}function f(b,c,f){if("undefined"!=typeof document){f=a({path:"/"},e.defaults,f),"number"==typeof f.expires&&(f.expires=new Date(1*new Date+864e5*f.expires)),f.expires=f.expires?f.expires.toUTCString():"";try{var g=JSON.stringify(c);/^[\{\[]/.test(g)&&(c=g)}catch(j){}c=d.write?d.write(c,b):encodeURIComponent(c+"").replace(/%(23|24|26|2B|3A|3C|3E|3D|2F|3F|40|5B|5D|5E|60|7B|7D|7C)/g,decodeURIComponent),b=encodeURIComponent(b+"").replace(/%(23|24|26|2B|5E|60|7C)/g,decodeURIComponent).replace(/[\(\)]/g,escape);var h="";for(var i in f)f[i]&&(h+="; "+i, +!function(a){var b;if("function"==typeof define&&define.amd&&(define(a),b=!0),"object"==typeof exports&&(module.exports=a(),b=!0),!b){var c=window.Cookies,d=window.Cookies=a();d.noConflict=function(){return window.Cookies=c,d}}}(function(){function a(){for(var a=0,b={};a<arguments.length;a++){var c=arguments[a];for(var d in c)b[d]=c[d]}return b}function b(a){return a.replace(/(%[0-9A-Z]{2})+/g,decodeURIComponent)}function c(d){function e(){}function f(b,c,f){if("undefined"!=typeof document){f=a({path:"/"},e.defaults,f),"number"==typeof f.expires&&(f.expires=new Date(1*new Date+864e5*f.expires)),f.expires=f.expires?f.expires.toUTCString():"";try{var g=JSON.stringify(c);/^[\{\[]/.test(g)&&(c=g)}catch(j){}c=d.write?d.write(c,b):encodeURIComponent(c+"").replace(/%(23|24|26|2B|3A|3C|3E|3D|2F|3F|40|5B|5D|5E|60|7B|7D|7C)/g,decodeURIComponent),b=encodeURIComponent(b+"").replace(/%(23|24|26|2B|5E|60|7C)/g,decodeURIComponent).replace(/[\(\)]/g,escape);var h="";for(var i in f)f[i]&&(h+="; "+i,!0!==f[i]&&(h+="="+f[i].split(";")[0]));return document.cookie=b+"="+c+h}}function g(a,c){if("undefined"!=typeof document){for(var e={},f=document.cookie?document.cookie.split("; "):[],g=0;g<f.length;g++){var h=f[g].split("="),i=h.slice(1).join("=");c||'"'!==i.charAt(0)||(i=i.slice(1,-1));try{var j=b(h[0]);if(i=(d.read||d)(i,j)||b(i),c)try{i=JSON.parse(i)}catch(k){}if(e[j]=i,a===j)break}catch(k){}}return a?e[a]:e}}return e.set=f,e.get=function(a){return g(a,!1)},e.getJSON=function(a){return g(a,!0)},e.remove=function(b,c){f(b,"",a(c,{expires:-1}))},e.defaults={},e.withConverter=c,e}return c(function(){})}); \ No newline at end of file diff --git a/htdocs/js/patch-list.js b/htdocs/js/patch-list.js index b88bad5..75d9b38 100644 --- a/htdocs/js/patch-list.js +++ b/htdocs/js/patch-list.js @@ -1,4 +1,4 @@ -import { updateProperty } from "./utils.js"; +import { updateProperty } from "./rest.js"; $( document ).ready(function() { // Change listener for dropdowns that change an individual patch's delegate and state properties diff --git a/htdocs/js/utils.js b/htdocs/js/rest.js similarity index 100% rename from htdocs/js/utils.js rename to htdocs/js/rest.js diff --git a/patchwork/forms.py b/patchwork/forms.py index b684026..9727932 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 - 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,9 +50,19 @@ class EmailForm(forms.Form): class BundleForm(forms.ModelForm): + # Map 'name' field to 'bundle_name' attr + field_mapping = {'name': 'bundle_name'} name = forms.RegexField( - regex=r'^[^/]+$', min_length=1, max_length=50, label=u'Name', - error_messages={'invalid': 'Bundle names can\'t contain slashes'}) + regex=r'^[^/]+$', min_length=1, max_length=50, required=False, + error_messages={'invalid': 'Bundle names can\'t contain slashes'}, + widget=forms.TextInput( + attrs={'class': 'create-bundle', + 'placeholder': 'Bundle name'})) + + # Maps form fields 'name' attr rendered in HTML element + def add_prefix(self, field_name): + field_name = self.field_mapping.get(field_name, field_name) + return super(BundleForm, self).add_prefix(field_name) class Meta: model = Bundle @@ -70,11 +80,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") + 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 @@ -114,19 +129,28 @@ class PatchForm(forms.ModelForm): def __init__(self, instance=None, project=None, *args, **kwargs): super(PatchForm, self).__init__(instance=instance, *args, **kwargs) self.fields['delegate'] = forms.ModelChoiceField( - queryset=_get_delegate_qs(project, instance), required=False) + queryset=_get_delegate_qs(project, instance), + widget=forms.Select(attrs={'class': 'change-property-delegate'}), + required=False) class Meta: model = Patch fields = ['state', 'archived', 'delegate'] + widgets = { + 'state': forms.Select( + attrs={'class': 'change-property-state'}), + 'archived': forms.CheckboxInput( + attrs={'class': 'archive-patch-check'}), + } class OptionalModelChoiceField(forms.ModelChoiceField): no_change_choice = ('*', 'No change') to_field_name = None - def __init__(self, placeholder, *args, **kwargs): + def __init__(self, *args, placeholder, className, **kwargs): self.no_change_choice = ('*', placeholder) + self.widget = forms.Select(attrs={'class': className}) super(OptionalModelChoiceField, self).__init__( initial=self.no_change_choice[0], *args, **kwargs) @@ -155,6 +179,10 @@ class OptionalModelChoiceField(forms.ModelChoiceField): class OptionalBooleanField(forms.TypedChoiceField): + def __init__(self, className, *args, **kwargs): + self.widget = forms.Select(attrs={'class': className}) + super(OptionalBooleanField, self).__init__(*args, **kwargs) + def is_no_change(self, value): return value == self.empty_value @@ -162,6 +190,7 @@ class OptionalBooleanField(forms.TypedChoiceField): class MultiplePatchForm(forms.Form): action = 'update' archived = OptionalBooleanField( + className="archive-patch-select", choices=[('*', 'No change'), ('True', 'Archive'), ('False', 'Unarchive')], coerce=lambda x: x == 'True', @@ -173,18 +202,14 @@ class MultiplePatchForm(forms.Form): self.fields['delegate'] = OptionalModelChoiceField( queryset=_get_delegate_qs(project=project), placeholder="Delegate to", + className="change-property-delegate", label="Delegate to", required=False) self.fields['state'] = OptionalModelChoiceField( queryset=State.objects.all(), placeholder="Change state", + className="change-property-state", label="Change state") - self.fields['state'].widget.attrs.update( - {'class': 'change-property'}) - self.fields['delegate'].widget.attrs.update( - {'class': 'change-property'}) - self.fields['archived'].widget.attrs.update( - {'class': 'archive-patch'}) def save(self, instance, commit=True): opts = instance.__class__._meta 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 48b47db..8967cdd 100644 --- a/patchwork/templates/patchwork/partials/patch-list.html +++ b/patchwork/templates/patchwork/partials/patch-list.html @@ -31,7 +31,7 @@ {% if page.paginator.long_page and user.is_authenticated %} <div class="floaty"> - <a title="jump to form" href="#patchforms"> + <a title="jump to form" href="#patch-forms"> <span style="font-size: 120%">▾</span> </a> </div> @@ -39,53 +39,10 @@ <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"> - <div class="patchforms" id="patchforms"> - {% if patchform %} - <div id="patchform-properties" class="patchform"> - <div id="patchform-state"> - {{ patchform.state.errors }} - {{ patchform.state }} - </div> - <div id="patchform-delegate"> - {{ patchform.delegate.errors }} - {{ patchform.delegate }} - </div> - <div id="patchform-archive"> - {{ patchform.archived.errors }} - {{ patchform.archived.label_tag }} {{ patchform.archived }} - </div> - <button class="patchform-submit btn btn-primary" name="action" value="{{patchform.action}}">Update</button> - </div> - {% endif %} - {% if user.is_authenticated %} - <div id="patchform-bundle" class="patchform"> - <div id="create-bundle"> - <input class="create-bundle" type="text" name="bundle_name" placeholder="Bundle name"/> - <input class="patchform-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="patchform-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="patchform-submit btn btn-primary" name="action" value="Remove">Remove from bundle</button> - </div> - {% endif %} - </div> - {% endif %} - </div> + {% 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" 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 cdd6f55..2223202 100644 --- a/patchwork/views/__init__.py +++ b/patchwork/views/__init__.py @@ -2,6 +2,7 @@ # Copyright (C) 2008 Jeremy Kerr <j...@ozlabs.org> # # SPDX-License-Identifier: GPL-2.0-or-later +import json from django.contrib import messages from django.shortcuts import get_object_or_404 @@ -9,6 +10,7 @@ from django.db.models import Prefetch from patchwork.filters import Filters from patchwork.forms import MultiplePatchForm +from patchwork.forms import CreateBundleForm from patchwork.models import Bundle from patchwork.models import BundlePatch from patchwork.models import Patch @@ -17,7 +19,6 @@ from patchwork.models import State from patchwork.models import Check from patchwork.paginator import Paginator - bundle_actions = ['create', 'add', 'remove'] @@ -109,48 +110,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()) + errors = [formErrors['name'][0]['message']] + 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() @@ -161,10 +149,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, @@ -221,17 +220,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' @@ -242,7 +244,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, @@ -293,6 +295,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..5ef6916 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,30 +72,10 @@ 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() @@ -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) diff --git a/templates/base.html b/templates/base.html index 9bdb35f..e57e2d5 100644 --- a/templates/base.html +++ b/templates/base.html @@ -21,7 +21,7 @@ <script src="{% static "js/bootstrap.min.js" %}"></script> <script src="{% static "js/selectize.min.js" %}"></script> <script src="{% static "js/clipboard.min.js" %}"></script> - <script src="{% static "js/js.cookie-2.2.1.min.js" %}"></script> + <script src="{% static "js/js.cookie.min.js" %}"></script> <script> $(document).ready(function() { new Clipboard(document.querySelectorAll('button.btn-copy')); -- 2.32.0.432.gabb21c7263-goog _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork