[PATCH v4 5/5] patch-list: add inline dropdown for delegate and state one-off changes
Daniel Axtens
dja at axtens.net
Fri Aug 27 00:20:35 AEST 2021
> TODO: The loading of the patch-list page is very slow now because it
> seems that for each call to check if a patch is editable by a user, the
> db is accessed. Changes in the backend need to be made so this is done
> with only done with only one call to the db.
AFAICT, the issue is that the code does a new db query for every
patch.is_editable() It didn't make things noticable slow for me, but I
do see an explosion in query volume. Anyway, try the following:
diff --git a/patchwork/models.py b/patchwork/models.py
index 58e4c51e9716..c29f9a988fd5 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -93,10 +93,14 @@ class Project(models.Model):
send_notifications = models.BooleanField(default=False)
use_tags = models.BooleanField(default=True)
+ @cached_property
+ def maintainer_users(self):
+ return [x.user for x in self.maintainer_project.all().only('user')]
+
def is_editable(self, user):
if not user.is_authenticated:
return False
- return self in user.profile.maintainer_projects.all()
+ return user in self.maintainer_users
@cached_property
def tags(self):
FWIW, I still think more than a few maintainers would be surprised that
delegates are editable by normal users... I still think we probably want
to make some guardrails available for projects. Not entirely sure what
that should look like just yet but that's where my head is at.
Kind regards,
Daniel
>
> Signed-off-by: Raxel Gutierrez <raxel at google.com>
> ---
> htdocs/README.rst | 6 +++
> htdocs/js/patch-list.js | 52 ++++++++++++++++++-
> .../patchwork/partials/patch-list.html | 35 +++++++++++--
> patchwork/views/__init__.py | 6 +++
> 4 files changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/htdocs/README.rst b/htdocs/README.rst
> index 6c435124..32550119 100644
> --- a/htdocs/README.rst
> +++ b/htdocs/README.rst
> @@ -133,6 +133,12 @@ js
>
> Part of Patchwork.
>
> +``patch-list.js.``
> + Event helpers and other application logic for patch-list.html. These
> + support patch list manipulation (e.g. inline dropdown changes).
> +
> + Part of Patchwork.
> +
> ``selectize.min.js``
> Selectize is the hybrid of a ``textbox`` and ``<select>`` box. It's jQuery
> based and it has autocomplete and native-feeling keyboard navigation; useful
> diff --git a/htdocs/js/patch-list.js b/htdocs/js/patch-list.js
> index 6ae13721..526f5370 100644
> --- a/htdocs/js/patch-list.js
> +++ b/htdocs/js/patch-list.js
> @@ -1,5 +1,32 @@
> +import { updateProperty } from "./rest.js";
> +
> $( document ).ready(function() {
> - $("#patch-list").stickyTableHeaders();
> + 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
> + $(inlinePropertyDropdowns).change((event) => {
> + const property = event.target.getAttribute("value");
> + const { url, data } = getPatchProperties(event.target, property);
> + const updateMessage = {
> + 'error': "No patches updated",
> + 'success': "1 patch updated",
> + };
> + updateProperty(url, data, updateMessage).then(isSuccess => {
> + if (!isSuccess) {
> + // 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();
>
> $("#check-all").change(function(e) {
> if(this.checked) {
> @@ -9,4 +36,25 @@ $( document ).ready(function() {
> }
> e.preventDefault();
> });
> -});
> \ No newline at end of file
> +
> + /**
> + * Returns the data to make property changes to a patch through PATCH 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,
> + };
> + }
> +});
> diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html
> index aeb26aa8..7d2d2cc9 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" %}
> @@ -187,8 +187,37 @@
> <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}}">
> + {% if patch.is_editable %}
> + <select class="change-property-delegate" value="delegate">
> + {% if not patch.delegate.username %}
> + <option value="*" selected>No delegate</option>
> + {% else %}
> + <option value="*" selected>{{ patch.delegate.username }}</option>
> + {% endif %}
> + {% for maintainer in maintainers %}
> + <option value="{{ maintainer.id }}">{{ maintainer.name }}</option>
> + {% endfor %}
> + </select>
> + {% else %}
> + {{ patch.delegate.username }}
> + {% endif %}
> + </td>
> + <td id="patch-state:{{patch.id}}">
> + {% if patch.is_editable %}
> + <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>
> diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py
> index 5da8046d..d41c4609 100644
> --- a/patchwork/views/__init__.py
> +++ b/patchwork/views/__init__.py
> @@ -16,6 +16,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
>
> @@ -177,6 +178,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
> @@ -287,6 +290,9 @@ def generic_list(request, project, view, view_args=None, filter_settings=None,
> Prefetch('check_set', queryset=Check.objects.only(
> 'context', 'user_id', 'patch_id', 'state', 'date')))
>
> + for patch in patches:
> + patch.is_editable = patch.is_editable(user)
> +
> paginator = Paginator(request, patches)
>
> context.update({
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list