[PATCH v2 5/5] patch-list: add inline dropdown for delegate and state one-off changes

Daniel Axtens dja at axtens.net
Tue Jul 27 10:14:19 AEST 2021


Raxel Gutierrez <raxel at google.com> writes:

> 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.
>
> Signed-off-by: Raxel Gutierrez <raxel at google.com>
> ---
>  htdocs/js/patch-list.js                       | 34 +++++++++++++++++++
>  htdocs/js/rest.js                             |  2 +-
>  .../patchwork/partials/patch-list.html        | 31 +++++++++++++++--
>  patchwork/views/__init__.py                   |  3 ++
>  templates/base.html                           |  2 +-
>  5 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/htdocs/js/patch-list.js b/htdocs/js/patch-list.js
> index 8c7640f..75d9b38 100644
> --- a/htdocs/js/patch-list.js
> +++ b/htdocs/js/patch-list.js
> @@ -1,4 +1,17 @@
> +import { updateProperty } from "./rest.js";
> +
>  $( document ).ready(function() {
> +    // Change listener for dropdowns that change an individual patch's delegate and state properties
> +    $("select[class^='change-property-']").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);
> +    });
> +
>      $("#patchlist").stickyTableHeaders();
>  
>      $("#check-all").change(function(e) {
> @@ -9,4 +22,25 @@ $( document ).ready(function() {
>          }
>          e.preventDefault();
>      });
> +
> +    /**
> +     * Returns the data to make property changes to a patch through fetch request.
> +     * @param {Element} propertySelect Property select element modified.
> +     * @param {string} property Patch property modified (e.g. "state", "delegate")
> +     * @return {{property: string, value: string}}
> +     *     property: Property field to be modified in request.
> +     *     value: New value for property to be modified to in request.
> +     */
> +    function getPatchProperties(propertySelect, property) {
> +        const selectedOption = propertySelect.options[propertySelect.selectedIndex];
> +        const patchId = propertySelect.parentElement.parentElement.dataset.patchId;
> +        const propertyValue = (property === "state") ? selectedOption.text
> +                            : (selectedOption.value === "*") ? null : selectedOption.value
> +        const data = {};
> +        data[property] = propertyValue;
> +        return {
> +            "url": "/api/patches/" + patchId + "/",
> +            "data": data,
> +        };
> +    }
>  });
> \ No newline at end of file
> diff --git a/htdocs/js/rest.js b/htdocs/js/rest.js
> index 34d41f5..254b5ae 100644
> --- a/htdocs/js/rest.js
> +++ b/htdocs/js/rest.js
> @@ -68,4 +68,4 @@ function handleErrorMessages(errorMessage) {
>      container.prepend(errorHeader);
>  }
>  
> -export { updateProperty, handleUpdateMessages, handleUpdateMessages};
> \ No newline at end of file
> +export { updateProperty };
> \ No newline at end of file
> diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html
> index 339cf42..8967cdd 100644
> --- a/patchwork/templates/patchwork/partials/patch-list.html
> +++ b/patchwork/templates/patchwork/partials/patch-list.html
> @@ -5,7 +5,7 @@
>  {% load static %}
>  
>  {% block headers %}
> -  <script src="{% static "js/patch-list.js" %}"></script>
> +  <script type="module" src="{% static "js/patch-list.js" %}"></script>
>  {% endblock %}
>  
>  {% include "patchwork/partials/filters.html" %}
> @@ -195,8 +195,33 @@
>     <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>
> +          {% else %}
> +            <option value="{{ maintainer.id }}">{{ maintainer.name }}</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>

This is pretty cool. My initial thoughts are as follows:

Can we make these only appear editable if you are either:
 - logged in,
 - or ideally, if you have the rights to change them

Currently I see a bunch of drop downs when I access the page as a
non-logged-in user where I couldn't possibly change the state or
delegate.

I'm umming and ahing about whether I want to gate this feature entirely
behind a user-specific setting: check out commit 6e32965b04cc ("'mpe
mode': click to copy patch IDs") for an example. Or whether it should be
on by default but disable-able via a setting. I'm a bit worried that it
makes rows wider and our power users have tended to get a bit irritated
when we do things that break their muscle memory.

Also, if there's an error setting a value (e.g. I'm not logged in) it
would be nice if the value reset so that I only see what the actual
state in the db.

I'll try logging in and poking around next but I wanted to get these
thoughts out sooner rather than later!

Kind regards,
Daniel

>    </tr>
>   {% empty %}
>    <tr>
> diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py
> index 178d185..2223202 100644
> --- a/patchwork/views/__init__.py
> +++ b/patchwork/views/__init__.py
> @@ -15,6 +15,7 @@ from patchwork.models import Bundle
>  from patchwork.models import BundlePatch
>  from patchwork.models import Patch
>  from patchwork.models import Project
> +from patchwork.models import State
>  from patchwork.models import Check
>  from patchwork.paginator import Paginator
>  
> @@ -176,6 +177,8 @@ def generic_list(request, project, view, view_args=None, filter_settings=None,
>          'project': project,
>          'projects': Project.objects.all(),
>          'filters': filters,
> +        'maintainers': project.maintainer_project.all(),
> +        'states': State.objects.all(),
>      }
>  
>      # pagination
> diff --git a/templates/base.html b/templates/base.html
> index 8700602..e57e2d5 100644
> --- a/templates/base.html
> +++ b/templates/base.html
> @@ -114,7 +114,7 @@
>    {% endfor %}
>    </div>
>  {% endif %}
> -  <div class="container-fluid">
> +  <div id="main-content" class="container-fluid">
>  {% block body %}
>  {% endblock %}
>    </div>
> -- 
> 2.32.0.432.gabb21c7263-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork


More information about the Patchwork mailing list