[PATCH v4 5/5] patch-list: add inline dropdown for delegate and state one-off changes
Raxel Gutierrez
raxelgutierrez09 at gmail.com
Tue Aug 31 14:23:44 AEST 2021
> On Aug 26, 2021, at 10:20 AM, Daniel Axtens <dja at axtens.net> wrote:
>
>> 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.
I think this makes sense on a per-project basis, so I believe a Project Settings page would be appropriate for guardrails. However, I’m wondering why maintainers would be surprised given that the patch form is set to check patch edit permissions [1]. I understand if maybe the structure within their project is that they choose the delegate and nobody else messes with it, but if normal users include the patch submitter and delegate, then it’s still possible for normal users to make these changes in Patchwork’s current state. Thoughts on the Project Settings page option?
> 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
[1] https://github.com/getpatchwork/patchwork/blob/65547c8701004f1a2a9ed9d16f1a372f4bd856e4/patchwork/views/__init__.py#L310
More information about the Patchwork
mailing list