[ Issue #509 + #510] Series views #589

andrepapoti · 2024-04-23T19:19:26Z

Description

This PR adds a new series-list and series-detail view.
Some projects can have hundreds of patches actives at once, by giving the user the ability to have an overview of all of the series makes knowing what's the current state of a project much more simple.
Series-list also allows the user to apply changes to all of the patches of a determined series, making the management of them easier
Added navigation within a series for the patch-detail view

I've taken a look at this locally but have yet to go into depth on it. There are two obvious issues starting out though. Firstly, you need to do some serious work on the queries: I loaded up the patchwork archives (following the guide in our development docs and loading the series-list page took over 7 seconds and ~3050 queries (:smile:). Secondly, the current iteration of the UI doesn't work and it scrolls past the edge of the screen. I would suggest the following changes:

Series Name should be the first column and called Series, as we do for patch list and Patch
Rather than showing cover letter text (expensive and, as things stand, rather ugly), show a unicode tick if there's a cover letter present.
Shorten Number of Patches to Patches
Get rid of the bulk update widget: it's too complicated to represent in a list view (IMO) and users can either click into the series to do that or use the API

Other changes:

/series/{series} should be /project/{project}/series/{series} to map with patches
/project/patchwork/series-list/ should be /project/patchwork/series/, and we should rename /project/patchwork/list/ to /project/patchwork/patches/ (with an alias in place).
We need a different icon for the series view link in the top toolbar (tbh, we also need a different icon for the patch view but that's not your issue)

This is rather beefy PR so if you have the ability to do, I'd suggest submitting v2 via the mailing list if you have the ability/know-how to do so. That'll be easier for me to review. Feel free to stick to GitHub if you can't/aren't comfortable using the mailing list though 🙂

This view is meant to give a quickoverview on a project since some of them can have hundreds of patches actives. This view also allows the user to apply changes on all of the series patches at once Signed-off-by: andrepapoti <[email protected]>

Signed-off-by: andrepapoti <[email protected]>

andrepapoti · 2024-08-01T14:21:28Z

@stephenfin I've completed the adjustments you specified and also added pagination to the series list, the page is displaying 100 series at once and It's making 7 requests per page.
I've also made the adjustments for the table display and endpoint urls.
I can send this patch via email as well but it may be interesting to keep the discussion on github since the PR is tracking issues that are registered on this repository.

hero24 · 2024-10-23T14:42:44Z

@andrepapoti I gave this a run. Main issue I have noticed is the default sorting, patches in Series view should be sorted from newest to the oldest, like they are in patches. It doesnt really make sense to have it sorted the other way around.

Should a navigation bar within a patch - next/previous be visible If there is a single patch in a series,?

edit: Finally on the sorting bit, patches view allows for sorting by different tables (submitter, state, date etc.) and it allows to reverse sorting as well, so that if something is sorted by date (oldest -> newest), if one clicks date again it changes to newest->oldest, or if one clicks on patch, then it sorts it alphabetically by patch name. I think it would be nice to have that in series view as well, to keep it constant.

hero24 · 2024-10-30T19:47:18Z

patchwork/tests/views/test_series.py

+# Patchwork - automated patch tracking system
+# Copyright (C) 2012 Jeremy Kerr <[email protected]>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+


Is this correct? For example is the year in copyright correct?

Missed this header, I'll change it to the correct one

hero24 · 2024-10-30T20:05:21Z

patchwork/views/series.py

+            'id',
+        )
+        .select_related('project')
+        .order_by('date')


Should this not be reversed then to sort it from newest to oldest?
Also are we fetching all of them or are we skipping the archived ones?

Agreed. Also, the query needs more work. I'm seeing ~300 queries for listing series (once I remove the lookup for series-detail from above and am able to render the page).

This is a problem. Fortunately it should be easy join. If you click on that SQL tab, you'll see a description of the (many) queries:

Clicking on one of these, we can see an example of the kind of issue:

That indicates that you need to join on submitter also, which you can do with select_related. You also need to join on patches and a few others. You should be able to get this down to < 30 queries once you resolve these.

andrepapoti · 2024-11-01T04:53:30Z

Should a navigation bar within a patch - next/previous be visible If there is a single patch in a series,?

There is no planned design for the task, I kept each button disabled if there are no next/previous values, but if you believe it's best to remove them I don't have any issues with it

Regarding the sorting I'll add a new commit to this MR to allow the user to change it by clicking on the table

stephenfin

The queries still need work, and I have a few other comments on top of @hero24's comments, but this is getting there.

stephenfin · 2024-11-01T11:03:18Z

patchwork/templates/patchwork/series-list.html

+        <a href="{% url 'series-detail' project_id=project.linkname series_id=series.id %}">
+          {{ series.name|default:"[no subject]"|truncatechars:100 }}
+        </a>


This shouldn't be present in the first commit since the corresponding view doesn't exist yet.

Can you drop the <a> tags and just print the name here. You can re-add the <a> tags in the commit that adds the series-detail view.

stephenfin · 2024-11-01T11:04:58Z

patchwork/templates/patchwork/series-list.html

+{% endif %}
+
+      <th>
+        <span class="colinactive">Series</span>


You shouldn't need the colinactive stuff since this is only used for bundles, which don't apply to series.

Suggested change

<span class="colinactive">Series</span>

<span>Series</span>

Below also.

stephenfin · 2024-11-01T11:06:30Z

patchwork/urls.py

    path(
-        'project/<project_id>/list/',
+        'project/<project_id>/patches/',
        patch_views.patch_list,
        name='patch-list',
    ),


If you rename this you need to provide a redirect. There are existing examples of redirects here. Don't forget to preserve query string arguments though since those matter here but didn't matter for fetching e.g. patch mboxes.

This should also be a separate commit, ideally placed before this one in the series.

stephenfin · 2024-11-01T11:11:15Z

patchwork/views/series.py

+            'id',
+        )
+        .select_related('project')
+        .order_by('date')


Agreed. Also, the query needs more work. I'm seeing ~300 queries for listing series (once I remove the lookup for series-detail from above and am able to render the page).

This is a problem. Fortunately it should be easy join. If you click on that SQL tab, you'll see a description of the (many) queries:

Clicking on one of these, we can see an example of the kind of issue:

That indicates that you need to join on submitter also, which you can do with select_related. You also need to join on patches and a few others. You should be able to get this down to < 30 queries once you resolve these.

stephenfin · 2024-11-01T11:11:41Z

patchwork/tests/views/test_series.py

Please fold this into the previous patch 🙏

stephenfin · 2024-11-01T11:13:55Z

patchwork/templates/patchwork/series-detail.html

We need tests for this also.

stephenfin · 2024-11-01T11:14:58Z

releasenotes/notes/add_series_list_view-bf219022216fea6a.yaml

+prelude: >
+    Some projects can have hundreds of patches actives at once, by giving the user
+    the ability to have an overview of all of the series makes knowing what's the
+    current state of a project much more simple.


The prelude section is used to provide a high level overview of a release. Only a maintainer should create one of these. You can drop this.

stephenfin · 2024-11-01T11:16:36Z

releasenotes/notes/add_series_list_view-bf219022216fea6a.yaml

+    Add series-list view, a view containing all of the series for a project and that also
+    implements the SeriesBulkUpdatePatchesForm, allowing the user to edit all of the patches
+    within a series using the web ui.
+    Add series-detail view, a view containing details about a series and all of it's patches


Can you be a little more generic here since series-list and series-detail mean nothing to a user of Patchwork that hasn't read the code. How about:

A series view is now available, allowing users to list available series and view details of individual series.

stephenfin · 2024-11-01T11:17:07Z

patchwork/templates/patchwork/submission.html

+  <a
+    class="btn btn-default {% if not previous_submission %} disabled {% endif %}"


nit: can you put this on one line like above

stephenfin · 2024-11-01T11:17:15Z

patchwork/templates/patchwork/submission.html

+    {% if previous_submission %} href="{% url 'patch-detail' project_id=project.linkname msgid=previous_submission.encoded_msgid %}" {% endif %}
+  >


hero24 · 2024-11-01T12:07:32Z

Just to make this clear. As it is the series view aren't exactly appealing, they show every patch series that has been ever submitted. Series should have states - open/closed, so they can be filtered on the web interface.
As mentioned before, series should be sorted by date descending by default, and possibly only series that have state == open should be displayed by default. Because this sounds like a design decision it possibly should be discussed/clarified on mailing list if two states (is_open = True/False) is ok, or if there is a need for lets say new/open/closed/archived set of states like there is for patch. (Personally I think two are fine, but it's no harm asking the community what they think before making a decision).

mchehab · 2024-12-09T15:35:31Z

Sorry for the absurd delay getting to this: my free time for Patchwork is pretty much zero recently... 😅

I've taken a look at this locally but have yet to go into depth on it. There are two obvious issues starting out though. Firstly, you need to do some serious work on the queries: I loaded up the patchwork archives (following the guide in our development docs and loading the series-list page took over 7 seconds and ~3050 queries (:smile:). Secondly, the current iteration of the UI doesn't work and it scrolls past the edge of the screen. I would suggest the following changes:
* `Series Name` should be the first column and called `Series`, as we do for patch list and `Patch`

* Rather than showing cover letter text (expensive and, as things stand, rather ugly), show a unicode tick if there's a cover letter present.

* Shorten `Number of Patches` to `Patches`

* Get rid of the bulk update widget: it's too complicated to represent in a list view (IMO) and users can either click into the series to do that or use the API
Other changes:
* `/series/{series}` should be `/project/{project}/series/{series}` to map with patches

* `/project/patchwork/series-list/` should be `/project/patchwork/series/`, and we should rename `/project/patchwork/list/` to `/project/patchwork/patches/` (with an alias in place).

* We need a different icon for the series view link in the top toolbar (tbh, we also need a different icon for the patch view but that's not your issue)
This is rather beefy PR so if you have the ability to do, I'd suggest submitting v2 via the mailing list if you have the ability/know-how to do so. That'll be easier for me to review. Feel free to stick to GitHub if you can't/aren't comfortable using the mailing list though 🙂

Did some tests on it today, as we're very interested on having a series view for patchwork.linuxtv.org.

My comments (didn't look at the code, just at the results), after testing on a development instance we use to check for patchwork issues before migrations:

Patch series require a filter. In the case of media, we have 66 pages of patch series since 2019-05 (most already applied);
The patch series should be reversed, with new series at the top;
by default, it should show only series with action required;
Cover letter needs to be shown at the series detail pages, together with any e-mail replies. The rationale is that it is common for people to reply to the cover letter - either with acked-by/reviewed-by or due to other issues that affects the entire series;
It should also display at the view a summary of CI test results (S/W/F fields) from patches view, as, for projects using CI integration, it is important to know if the series was already tested. See for instance, our usage of S/W/F at:
https://patchwork.linuxtv.org/project/linux-media/list/

mchehab · 2024-12-09T15:44:09Z

Btw, some patch series are shown as without any patches because patches aren't at "Action required" states:

Changing the filter makes them appear again:

IMO, it should use keep using the same filter that Series would have (se my previous comment).

stephenfin added enhancement awaiting-feedback labels Jul 12, 2024

andrepapoti force-pushed the i509 branch from ede319f to 35b4121 Compare July 31, 2024 04:40

andrepapoti added 4 commits July 31, 2024 01:57

views: add series-list view

1799337

This view is meant to give a quickoverview on a project since some of them can have hundreds of patches actives. This view also allows the user to apply changes on all of the series patches at once Signed-off-by: andrepapoti <[email protected]>

tests: Add tests for series list view

5878433

Signed-off-by: andrepapoti <[email protected]>

views: add series-detail view

5cfa0dd

Signed-off-by: andrepapoti <[email protected]>

views: add shortcut to series-list view on base.html

55f0276

Signed-off-by: andrepapoti <[email protected]>

andrepapoti force-pushed the i509 branch from 35b4121 to 98bf401 Compare July 31, 2024 04:57

andrepapoti added 2 commits August 1, 2024 11:08

docs: add release notes for series-list and series-detail

ebe0397

Signed-off-by: andrepapoti <[email protected]>

views: add series navigation buttons for patch-detail view

edff002

Signed-off-by: andrepapoti <[email protected]>

andrepapoti force-pushed the i509 branch from 98bf401 to edff002 Compare August 1, 2024 14:09

hero24 suggested changes Oct 30, 2024

View reviewed changes

stephenfin requested changes Nov 1, 2024

View reviewed changes

andrepapoti mentioned this pull request Nov 4, 2024

[Issue #506] Allow linking between series #587

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ Issue #509 + #510] Series views #589

[ Issue #509 + #510] Series views #589

andrepapoti commented Apr 23, 2024 •

edited

Loading

stephenfin commented Jul 12, 2024

andrepapoti commented Aug 1, 2024 •

edited

Loading

hero24 commented Oct 23, 2024 •

edited

Loading

hero24 Oct 30, 2024

andrepapoti Nov 1, 2024

hero24 Oct 30, 2024

stephenfin Nov 1, 2024

andrepapoti commented Nov 1, 2024

stephenfin left a comment

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

stephenfin Nov 1, 2024

hero24 commented Nov 1, 2024

mchehab commented Dec 9, 2024

mchehab commented Dec 9, 2024

		<a
		class="btn btn-default {% if not previous_submission %} disabled {% endif %}"

		{% if previous_submission %} href="{% url 'patch-detail' project_id=project.linkname msgid=previous_submission.encoded_msgid %}" {% endif %}
		>

[ Issue #509 + #510] Series views #589

Are you sure you want to change the base?

[ Issue #509 + #510] Series views #589

Conversation

andrepapoti commented Apr 23, 2024 • edited Loading

Description

Related

stephenfin commented Jul 12, 2024

andrepapoti commented Aug 1, 2024 • edited Loading

hero24 commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrepapoti commented Nov 1, 2024

stephenfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hero24 commented Nov 1, 2024

mchehab commented Dec 9, 2024

mchehab commented Dec 9, 2024

andrepapoti commented Apr 23, 2024 •

edited

Loading

andrepapoti commented Aug 1, 2024 •

edited

Loading

hero24 commented Oct 23, 2024 •

edited

Loading