Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1799337
views: add series-list view
andrepapoti Apr 22, 2024
5878433
tests: Add tests for series list view
andrepapoti Apr 22, 2024
5cfa0dd
views: add series-detail view
andrepapoti Apr 23, 2024
55f0276
views: add shortcut to series-list view on base.html
andrepapoti Apr 22, 2024
ebe0397
docs: add release notes for series-list and series-detail
andrepapoti Apr 22, 2024
edff002
views: add series navigation buttons for patch-detail view
andrepapoti Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions patchwork/templates/patchwork/series-list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
{% extends "base.html" %}

{% load person %}
{% load static %}

{% block title %}{{project.name}}{% endblock %}
{% block series_active %}active{% endblock %}

{% block body %}

{% load person %}
{% load listurl %}
{% load patch %}
{% load project %}
{% load static %}

{% include "patchwork/partials/pagination.html" %}

<input type="hidden" name="form" value="serieslistform"/>
<input type="hidden" name="project" value="{{project.id}}"/>

<table id="serieslist" class="table table-hover table-extra-condensed table-striped pw-list" data-toggle="checkboxes" data-range="true">
<thead>
<tr>
{% if user.is_authenticated and user.profile.show_ids %}
<th>
ID
</th>
{% endif %}

<th>
<span class="colinactive">Series</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</th>

<th>
Version
</th>

<th>
<span class="colinactive">Cover Letter</span>
</th>

<th>
<span class="colinactive">Patches</span>
</th>

<th>
<span class="colinactive">Date</span>
</th>

<th>
<span class="colinactive">Submitter</span>
</th>
</tr>
</thead>

<tbody>
{% for series in page %}
<tr id="series_row:{{series.id}}">
{% if user.is_authenticated and user.profile.show_ids %}
<td>
<button type="button" class="btn btn-xs btn-copy" data-clipboard-text="{{ series.id }}" title="Copy to Clipboard">
{{ series.id }}
</button>
</td>
{% endif %}
<td>
<a href="{% url 'series-detail' project_id=project.linkname series_id=series.id %}">
{{ series.name|default:"[no subject]"|truncatechars:100 }}
</a>
Comment on lines +68 to +70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screen Shot 2024-11-01 at 11 03 09

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.

</td>
<td>
{{ series.version|default:"-"}}
</td>

<td>
{% if series.cover_letter.content %}
<b>&check;</b>
{% else %}
-
{% endif %}
</td>
<td>{{ series.received_total}}</td>
<td class="text-nowrap">{{ series.date|date:"Y-m-d" }}</td>
<td>{{ series.submitter|personify:project }}</td>
</tr>
{% empty %}
<tr>
<td colspan="8">No series to display</td>
</tr>
{% endfor %}
</tbody>
</table>

{% if page.paginator.count %}
{% include "patchwork/partials/pagination.html" %}
{% endif %}
{% endblock %}
7 changes: 6 additions & 1 deletion patchwork/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,15 @@
path('admin/', admin.site.urls),
path('', project_views.project_list, name='project-list'),
path(
'project/<project_id>/list/',
'project/<project_id>/patches/',
patch_views.patch_list,
name='patch-list',
),
Comment on lines 34 to 38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

path(
'project/<project_id>/series/',
series_views.series_list,
name='series-list',
),
path(
'project/<project_id>/bundles/',
bundle_views.bundle_list,
Expand Down
39 changes: 39 additions & 0 deletions patchwork/views/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@
# Copyright (C) 2017 Stephen Finucane <[email protected]>
#
# SPDX-License-Identifier: GPL-2.0-or-later
import collections

from django.http import HttpResponse
from django.shortcuts import get_object_or_404
from django.shortcuts import render

from patchwork.models import Series
from patchwork.models import Project
from patchwork.views.utils import series_to_mbox
from patchwork.paginator import Paginator


def series_mbox(request, series_id):
Expand All @@ -20,3 +24,38 @@ def series_mbox(request, series_id):
)

return response


def series_list(request, project_id):
project = get_object_or_404(Project, linkname=project_id)
context = {}
series_list = (
Series.objects.filter(project=project)
.only(
'submitter',
'project',
'version',
'name',
'date',
'id',
)
.select_related('project')
.order_by('date')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Screen Shot 2024-11-01 at 11 08 32

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:

Screen Shot 2024-11-01 at 11 08 58

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

Screen Shot 2024-11-01 at 11 09 09

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.

)

paginator = Paginator(request, series_list)
context.update(
{
'project': project,
'projects': Project.objects.all(),
'series_list': series_list,
'page': paginator.current_page,
'list_view': {
'view': 'series-list',
'view_params': {'project_id': project.linkname},
'params': collections.OrderedDict(),
},
}
)

return render(request, 'patchwork/series-list.html', context)