[PATCH v3 0/5] patch-list: improve usability of list action bar
Raxel Gutierrez
raxel at google.com
Thu Jul 29 01:24:14 AEST 2021
This series is a revision to the previous version of the patch series.
The series mainly addresses the review comments from the v2 series [1].
For the first patch, the recently released v3.0.0 of the JS cookie
library replaces the previous version.
For the second patch, following jrnieder’s comments [2], the commit
message is updated to better explain the benefits of the change and its
details. Also, an unclear comment in forms.py for BundleForm is cleaned
up to better explain what the changes do. The id for <td> table cells in
patch-list.html are correctly differentiated by patch id. I noticed a
bug where property changes in the patch detail page weren’t working
because the respective “update” action was not accounted for.
For the third patch, the “jump to form” arrow is removed [3].
For the fourth patch, the `updateProperty` function now returns whether
the update request was successful or not with `response.ok` so that
callers can use that information and respond accordingly. The next patch
adds a use case for the return value.
For the fifth patch, the inline dropdowns are changed so that they are
only visible to logged in users. Also, upon any unsuccessful update
requests, the dropdown selection reverts to its previous selection.
Since update requests to a patch’s state and delegate fields fail for
unauthorized users, this behavior covers the case where unauthorized
users don't see the current state of the db after they change the
dropdown selection. This behavior is a useful example of patch four’s
change of returning whether an update request is successful.
[1] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006968.html
[2] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006988.html
[3] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006991.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 | 2 +
htdocs/js/patch-list.js | 60 ++++++
htdocs/js/rest.js | 72 ++++++++
patchwork/forms.py | 66 +++++--
patchwork/templates/patchwork/list.html | 11 +-
.../templates/patchwork/partials/errors.html | 9 +
.../patchwork/partials/patch-forms.html | 45 +++++
.../patchwork/partials/patch-list.html | 171 ++++++------------
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 | 35 +---
templates/base.html | 3 +-
16 files changed, 472 insertions(+), 325 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
Range-diff:
1: 2a34394 ! 1: 859789e static: add JS Cookie Library to get csrftoken for fetch requests
@@ htdocs/README.rst: js
+ 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
++ :Version: 3.0.0
+
``selectize.min.js``
@@ htdocs/README.rst: js
## htdocs/js/js.cookie.min.js (new) ##
@@
-+/*! 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,!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
++/*! js-cookie v3.0.0 | MIT */
++!function(e,t){"object"==typeof exports&&"undefined"!=typeof module?module.exports=t():"function"==typeof define&&define.amd?define(t):(e=e||self,function(){var n=e.Cookies,r=e.Cookies=t();r.noConflict=function(){return e.Cookies=n,r}}())}(this,(function(){"use strict";function e(e){for(var t=1;t<arguments.length;t++){var n=arguments[t];for(var r in n)e[r]=n[r]}return e}var t={read:function(e){return e.replace(/(%[\dA-F]{2})+/gi,decodeURIComponent)},write:function(e){return encodeURIComponent(e).replace(/%(2[346BF]|3[AC-F]|40|5[BDE]|60|7[BCD])/g,decodeURIComponent)}};return function n(r,o){function i(t,n,i){if("undefined"!=typeof document){"number"==typeof(i=e({},o,i)).expires&&(i.expires=new Date(Date.now()+864e5*i.expires)),i.expires&&(i.expires=i.expires.toUTCString()),t=encodeURIComponent(t).replace(/%(2[346B]|5E|60|7C)/g,decodeURIComponent).replace(/[()]/g,escape),n=r.write(n,t);var c="";for(var u in i)i[u]&&(c+="; "+u,!0!==i[u]&&(c+="="+i[u].split(";")[0]));return document.cookie=t+"="+n+c}}return Object.create({set:i,get:function(e){if("undefined"!=typeof document&&(!arguments.length||e)){for(var n=document.cookie?document.cookie.split("; "):[],o={},i=0;i<n.length;i++){var c=n[i].split("="),u=c.slice(1).join("=");'"'===u[0]&&(u=u.slice(1,-1));try{var f=t.read(c[0]);if(o[f]=r.read(u,f),e===f)break}catch(e){}}return e?o[e]:o}},remove:function(t,n){i(t,"",e({},n,{expires:-1}))},withAttributes:function(t){return n(this.converter,e({},this.attributes,t))},withConverter:function(t){return n(e({},this.converter,t),this.attributes)}},{attributes:{value:Object.freeze(o)},converter:{value:Object.freeze(r)}})}(t,{path:"/"})}));
## templates/base.html ##
@@
2: b30ba35 ! 2: 2ac10c8 patch-list: clean up patch-list page and refactor patch forms
@@ Commit message
patch-list: clean up patch-list page and refactor patch forms
Clean up the patch-list page by moving forms to a new template file
- patch-forms.html and moving them to the top of the page, adding ids to
- table cells, and renaming selectors using hyphen delimited strings where
- the relevant changes were made. Also, created a partial template for
+ 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 makes the code healthier,
+ discoverability of the patch-list forms and make the code healthier,
ready for change, and overall more readable.
Also, move patch-list related js code to a new patch-list.js file, to
@@ Commit message
simplifies a possible future migration to TypeScript if the project
chooses to go in that direction.
- 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. Error messages and success/warning
- messages are now normalized throughout these files for testing and
- functionality. Also, important to these change is that CreateBundleForm
- in forms.py handles the validation of the bundle form input so that now
- the patch-list bundle form also makes use of the Django forms API.
+ 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.
## htdocs/README.rst ##
@@ htdocs/README.rst: js
:GitHub: https://github.com/js-cookie/js-cookie/
- :Version: 2.2.1
+ :Version: 3.0.0
+``patch-list.js.``
+
-+ Utility functions for patch list manipulation (inline dropdown changes,
-+ etc.)
++ Event helpers and other application logic for patch-list.html. These
++ support patch list manipulation (e.g. inline dropdown changes).
+
+ Part of Patchwork.
+
@@ htdocs/js/patch-list.js (new)
+ e.preventDefault();
+ });
+});
- \ No newline at end of file
## patchwork/forms.py ##
@@
@@ patchwork/forms.py: 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',
+ regex=r'^[^/]+$', min_length=1, max_length=50, required=False,
error_messages={'invalid': 'Bundle names can\'t contain slashes'})
-+ # Maps form fields 'name' attr rendered in HTML element
++ # 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)
@@ patchwork/templates/patchwork/partials/patch-list.html
{% if order.editable %}
<table class="patchlist">
@@
-
- {% 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>
+ </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();
@@ patchwork/templates/patchwork/partials/patch-list.html: $(document).ready(functi
+ <tr id="patch_row:{{patch.id}}" data-patch-id="{{patch.id}}">
{% if user.is_authenticated %}
- <td style="text-align: center;">
-+ <td id="select-patch" style="text-align: center;">
++ <td id="select-patch:{{patch.id}}" style="text-align: center;">
<input type="checkbox" name="patch_id:{{patch.id}}"/>
</td>
{% endif %}
@@ patchwork/templates/patchwork/partials/patch-list.html: $(document).ready(functi
</td>
{% endif %}
- <td>
-+ <td id="patch-name">
++ <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">
++ <td id="patch-series:{{patch.id}}">
{% if patch.series %}
<a href="?series={{patch.series.id}}">
{{ patch.series|truncatechars:100 }}
@@ patchwork/templates/patchwork/partials/patch-list.html: $(document).ready(functi
- <td>{{ patch.submitter|personify:project }}</td>
- <td>{{ patch.delegate.username }}</td>
- <td>{{ patch.state }}</td>
-+ <td id="patch-tags" class="text-nowrap">{{ patch|patch_tags }}</td>
-+ <td id="patch-checks" class="text-nowrap">{{ patch|patch_checks }}</td>
-+ <td id="patch-date" class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
-+ <td id="patch-submitter">{{ patch.submitter|personify:project }}</td>
-+ <td id="patch-delegate">{{ patch.delegate.username }}</td>
-+ <td id="patch-state">{{ 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>
@@ patchwork/tests/views/test_patch.py: class PatchUpdateTest(TestCase):
## patchwork/views/__init__.py ##
@@
- # Copyright (C) 2008 Jeremy Kerr <jk at ozlabs.org>
#
# SPDX-License-Identifier: GPL-2.0-or-later
-+import json
++import json
++
from django.contrib import messages
from django.shortcuts import get_object_or_404
-@@ patchwork/views/__init__.py: from django.db.models import Prefetch
+ from django.db.models import Prefetch
from patchwork.filters import Filters
- from patchwork.forms import MultiplePatchForm
+from patchwork.forms import CreateBundleForm
+ from patchwork.forms import MultiplePatchForm
from patchwork.models import Bundle
from patchwork.models import BundlePatch
- from patchwork.models import Patch
-@@ patchwork/views/__init__.py: from patchwork.models import Project
- from patchwork.models import Check
- from patchwork.paginator import Paginator
-
--
- bundle_actions = ['create', 'add', 'remove']
-
-
@@ patchwork/views/__init__.py: class Order(object):
@@ patchwork/views/__init__.py: class Order(object):
+ messages.success(request, 'Bundle %s created' % bundle.name)
+ else:
+ formErrors = json.loads(create_bundle_form.errors.as_json())
-+ errors = [formErrors['name'][0]['message']]
++ errors = [e['message'] for e in formErrors['name']]
+ return errors
elif action == 'add':
+ if not data['bundle_id']:
@@ patchwork/views/patch.py: def patch_detail(request, project_id, msgid):
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()
@@ patchwork/views/patch.py: def patch_detail(request, project_id, msgid):
context['project'] = patch.project
context['related_same_project'] = related_same_project
3: 3571476 ! 3: 5d5eedc patch-list: style modification forms as an action bar
@@ patchwork/forms.py: class BundleForm(forms.ModelForm):
+ attrs={'class': 'create-bundle',
+ 'placeholder': 'Bundle name'}))
- # Maps form fields 'name' attr rendered in HTML element
- def add_prefix(self, field_name):
+ # Changes the HTML 'name' attr of the input element from "name"
+ # (inherited from the model field 'name' of the Bundle object)
@@ patchwork/forms.py: class PatchForm(forms.ModelForm):
def __init__(self, instance=None, project=None, *args, **kwargs):
super(PatchForm, self).__init__(instance=instance, *args, **kwargs)
4: c3f2a17 ! 4: d77051d static: add rest.js to handle requests & respective messages
@@ htdocs/js/rest.js (new)
+ body: JSON.stringify(data),
+ });
+
-+ await fetch(request)
++ return await fetch(request)
+ .then(response => {
+ if (!response.ok) {
+ response.text().then(text => {
@@ htdocs/js/rest.js (new)
+ // Update message for successful changes
+ handleUpdateMessages(updateMessage.some);
+ }
++ return response.ok
+ });
+}
+
5: 62e3434 ! 5: e4e0674 patch-list: add inline dropdown for delegate and state one-off changes
@@ Commit message
patch-list: add inline dropdown for delegate and state one-off changes
Add dropdown for the cell values of the Delegate and State columns for
- each individual patch to make one-off changes to patches. Change the
- generic_list method to pass the list of states and maintainers to the
- patch list view context to populate the dropdown options. The static
- patch-list.js file now uses the modularity of the fetch request and
- update/error messages handling of rest.js.
+ each individual patch to make one-off changes to patches. The dropdowns
+ are only viewable when logged in and revert selections made by users
+ that don't have permission to change a given patch's state or delegate.
+ Change the generic_list method to pass the list of states and
+ maintainers to the patch list view context to populate the dropdown
+ options. The static patch-list.js file now uses the modularity of the
+ fetch request and update/error messages handling of rest.js.
## htdocs/js/patch-list.js ##
@@
+import { updateProperty } from "./rest.js";
+
$( document ).ready(function() {
++ let inlinePropertyDropdowns = $("td > select[class^='change-property-']");
++ $(inlinePropertyDropdowns).each(function() {
++ // Store previous dropdown selection
++ $(this).data("prevProperty", $(this).val());
++ });
++
+ // Change listener for dropdowns that change an individual patch's delegate and state properties
-+ $("select[class^='change-property-']").change((event) => {
++ $(inlinePropertyDropdowns).change((event) => {
+ const property = event.target.getAttribute("value");
+ const { url, data } = getPatchProperties(event.target, property);
+ const updateMessage = {
+ 'none': "No patches updated",
+ 'some': "1 patch updated",
+ };
-+ updateProperty(url, data, updateMessage);
++ updateProperty(url, data, updateMessage).then(is_success => {
++ if (!is_success) {
++ // Revert to previous selection
++ $(event.target).val($(event.target).data("prevProperty"));
++ } else {
++ // Update to new previous selection
++ $(event.target).data("prevProperty", $(event.target).val());
++ }
++ });
+ });
+
$("#patchlist").stickyTableHeaders();
@@ htdocs/js/patch-list.js: $( document ).ready(function() {
+ };
+ }
});
- \ No newline at end of file
## htdocs/js/rest.js ##
@@ htdocs/js/rest.js: function handleErrorMessages(errorMessage) {
@@ patchwork/templates/patchwork/partials/patch-list.html
{% include "patchwork/partials/filters.html" %}
@@
- <td id="patch-checks" class="text-nowrap">{{ patch|patch_checks }}</td>
- <td id="patch-date" class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
- <td id="patch-submitter">{{ patch.submitter|personify:project }}</td>
-- <td id="patch-delegate">{{ patch.delegate.username }}</td>
-- <td id="patch-state">{{ patch.state }}</td>
-+ <td id="patch-delegate">
-+ <select class="change-property-delegate" value="delegate">
-+ {% if not patch.delegate.username %}
-+ <option value="*" selected>No delegate</option>
-+ {% else %}
-+ <option value="*">No delegate</option>
-+ {% endif %}
-+ {% for maintainer in maintainers %}
-+ {% if maintainer.name == patch.delegate.username %}
-+ <option value="{{ patch.delegate.username.id }}" selected>{{ patch.delegate.username }}</option>
+ <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>
++ <td id="patch-delegate:{{patch.id}}">
++ <td id="patch-delegate:{{patch.id}}">
++ {% if user.is_authenticated %}
++ <select class="change-property-delegate" value="delegate">
++ {% if not patch.delegate.username %}
++ <option value="*" selected>No delegate</option>
+ {% else %}
-+ <option value="{{ maintainer.id }}">{{ maintainer.name }}</option>
++ <option value="*">No delegate</option>
+ {% endif %}
-+ {% endfor %}
-+ </select>
-+ </td>
-+ <td id="patch-state">
-+ <select class="change-property-state" value="state">
-+ {% for state in states %}
-+ {% if state.name == patch.state.name %}
-+ <option value="{{ patch.state.ordering }}" selected>{{ patch.state }}</option>
-+ {% else %}
-+ <option value="{{ state.ordering }}">{{ state.name }}</option>
-+ {% endif %}
-+ {% endfor %}
-+ </select>
-+ </td>
++ {% for maintainer in maintainers %}
++ {% if maintainer.name == patch.delegate.username %}
++ <option value="{{ patch.delegate.username.id }}" selected>{{ patch.delegate.username }}</option>
++ {% else %}
++ <option value="{{ maintainer.id }}">{{ maintainer.name }}</option>
++ {% endif %}
++ {% endfor %}
++ </select>
++ {% else %}
++ {{ patch.delegate.username }}
++ {% endif %}
++ </td>
++ <td id="patch-state:{{patch.id}}">
++ {% if user.is_authenticated %}
++ <select class="change-property-state" value="state">
++ {% for state in states %}
++ {% if state.name == patch.state.name %}
++ <option value="{{ patch.state.ordering }}" selected>{{ patch.state }}</option>
++ {% else %}
++ <option value="{{ state.ordering }}">{{ state.name }}</option>
++ {% endif %}
++ {% endfor %}
++ </select>
++ {% else %}
++ {{ patch.state }}
++ {% endif %}
++ </td>
</tr>
{% empty %}
<tr>
--
2.32.0.554.ge1b32706d8-goog
More information about the Patchwork
mailing list