diff mbox series

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

Message ID 20210823182833.3976100-6-raxel@google.com
State Rejected
Headers show
Series patch-list: improve usability of list action bar | expand
Related show

Commit Message

Raxel Gutierrez Aug. 23, 2021, 6:28 p.m. UTC 
Add dropdown for the cell values of the Delegate and State columns for
each individual patch to make one-off changes to patches. The dropdowns
are only viewable if the user has patch edit permissions.

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.

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.

Signed-off-by: Raxel Gutierrez <raxel@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(-)

Comments

Daniel Axtens Aug. 26, 2021, 2:20 p.m. UTC | #1 
> 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@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Daniel Axtens Aug. 26, 2021, 2:29 p.m. UTC | #2 
Ah, that is but one of the issues - I forgot to check what happened if
you were logged in as a normal user rather than a maintainer. Add this too:

diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py
index d41c4609fe6e..f1acfb3a599e 100644
--- a/patchwork/views/__init__.py
+++ b/patchwork/views/__init__.py
@@ -280,7 +280,7 @@ def generic_list(request, project, view, view_args=None, filter_settings=None,
     # but we will need to follow the state and submitter relations for
     # rendering the list template
     patches = patches.select_related('state', 'submitter', 'delegate',
-                                     'series')
+                                     'series', 'submitter__user')
 
     patches = patches.only('state', 'submitter', 'delegate', 'project',
                            'series__name', 'name', 'date', 'msgid')


Daniel Axtens <dja@axtens.net> writes:

>> 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@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@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/patchwork
Raxel Gutierrez Aug. 31, 2021, 4:23 a.m. UTC | #3 
> 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
Stephen Finucane Nov. 1, 2024, 4:03 p.m. UTC | #4 
On Mon, 2021-08-23 at 18:28 +0000, Raxel Gutierrez wrote:
> Add dropdown for the cell values of the Delegate and State columns for
> each individual patch to make one-off changes to patches. The dropdowns
> are only viewable if the user has patch edit permissions.
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Raxel Gutierrez <raxel@google.com>

I didn't apply this patch. I'm not overly keen on adding yet another column to
that already-huge table and would like to re-evaluate how we could do this. The
rest is merged now though.

Stephen
diff mbox series

Patch

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({