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.

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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 41 additions & 0 deletions patchwork/templates/patchwork/series-detail.html
Copy link
Member

Choose a reason for hiding this comment

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

We need tests for this also.

Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{% extends "base.html" %}

{% load humanize %}
{% load syntax %}
{% load person %}
{% load patch %}
{% load static %}
{% load utils %}

{% block title %}{{series.name}}{% endblock %}

{% block body %}

<div>
<h1>{{ series.name }}</h1>
</div>

<table class="patch-meta">
<tr>
<th>Cover Letter</th>
<td>{{ series.cover_letter.content|default:"No cover letter available" }}</td>
</tr>
<tr>
<th>Date</th>
<td>{{ series.date }}</td>
</tr>
<tr>
<th>Submitter</th>
<td>{{ series.submitter }}</td>
</tr>
<tr>
<th>Total</th>
<td>{{ series.patches }}</td>
</tr>
</table>
<br>
<h2>Patches:</h2>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<h2>Patches:</h2>
<h2>Patches</h2>

<br>
{% include "patchwork/partials/patch-list.html" %}

{% endblock %}
98 changes: 98 additions & 0 deletions patchwork/templates/patchwork/series-list.html
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 %}
16 changes: 16 additions & 0 deletions patchwork/templates/patchwork/submission.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,22 @@
<h1>{{ submission.name }}</h1>
</div>

<div class="btn-group pull-right">
<a class="btn btn-default {% if not next_submission %} disabled {% endif %}"
{% if next_submission %} href="{% url 'patch-detail' project_id=project.linkname msgid=next_submission.encoded_msgid %}" {% endif %}>
next
</a>
</div>

<div class="btn-group pull-right">
<a
class="btn btn-default {% if not previous_submission %} disabled {% endif %}"
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you put this on one line like above

{% if previous_submission %} href="{% url 'patch-detail' project_id=project.linkname msgid=previous_submission.encoded_msgid %}" {% endif %}
>
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

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

ditto

previous
</a>
</div>

<table id="patch-meta" class="patch-meta" data-submission-type={{submission|verbose_name_plural|lower}} data-submission-id={{submission.id}}>
<tr>
<th>Message ID</th>
Expand Down
51 changes: 51 additions & 0 deletions patchwork/tests/views/test_series.py
Copy link
Member

Choose a reason for hiding this comment

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

Please fold this into the previous patch 🙏

Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Patchwork - automated patch tracking system
# Copyright (C) 2012 Jeremy Kerr <[email protected]>
#
# SPDX-License-Identifier: GPL-2.0-or-later

Comment on lines +1 to +5
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

from datetime import datetime as dt

from django.test import TestCase
from django.urls import reverse

from patchwork.models import Person
from patchwork.tests.utils import create_patch
from patchwork.tests.utils import create_cover
from patchwork.tests.utils import create_person
from patchwork.tests.utils import create_project
from patchwork.tests.utils import create_series
from patchwork.tests.utils import create_user


class SeriesList(TestCase):
def setUp(self):
self.project = create_project()
self.user = create_user()
self.person_1 = Person.objects.get(user=self.user)
self.person_2 = create_person()
self.series_1 = create_series(project=self.project)
self.series_2 = create_series(project=self.project)
create_cover(project=self.project, series=self.series_1)

for i in range(5):
create_patch(
submitter=self.person_1,
project=self.project,
series=self.series_1,
date=dt(2014, 3, 16, 13, 4, 50, 155643),
)
create_patch(
submitter=self.person_2,
project=self.project,
series=self.series_2,
date=dt(2014, 3, 16, 13, 4, 50, 155643),
)

def test_series_list(self):
requested_url = reverse(
'series-list',
kwargs={'project_id': self.project.linkname},
)
response = self.client.get(requested_url)

self.assertEqual(response.status_code, 200)
12 changes: 11 additions & 1 deletion patchwork/urls.py
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 Expand Up @@ -110,6 +115,11 @@
name='comment-redirect',
),
# series views
path(
'project/<project_id>/series/<int:series_id>/',
series_views.series_detail,
name='series-detail',
),
path(
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have a proper series view, could you add a new view for project/<project_id>/series/<int:series_id>/mbox/ to keep things consistent? That should be a separate patch early in the series and a redirect from the current URL should be provided.

'series/<int:series_id>/mbox/',
series_views.series_mbox,
Expand Down
14 changes: 14 additions & 0 deletions patchwork/views/patch.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,20 @@ def patch_detail(request, project_id, msgid):
context['related_same_project'] = related_same_project
context['related_different_project'] = related_different_project

try:
context['previous_submission'] = Patch.objects.get(
series=patch.series, number=patch.number - 1
)
except Patch.DoesNotExist:
context['previous_submission'] = None

try:
context['next_submission'] = Patch.objects.get(
series=patch.series, number=patch.number + 1
)
except Patch.DoesNotExist:
context['next_submission'] = None

return render(request, 'patchwork/submission.html', context)


Expand Down
62 changes: 62 additions & 0 deletions patchwork/views/series.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@
# 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 Patch
from patchwork.models import Project
from patchwork.views import generic_list
from patchwork.views.utils import series_to_mbox
from patchwork.paginator import Paginator


def series_mbox(request, series_id):
Expand All @@ -20,3 +26,59 @@ def series_mbox(request, series_id):
)

return response


def series_detail(request, project_id, series_id):
series = get_object_or_404(Series, id=series_id)

patches = Patch.objects.filter(series=series)

context = generic_list(
request,
series.project,
'series-detail',
view_args={
'project_id': project_id,
'series_id': series_id,
},
patches=patches,
)

context.update({'series': series})

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


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)
11 changes: 11 additions & 0 deletions releasenotes/notes/add_series_list_view-bf219022216fea6a.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
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.
Comment on lines +2 to +5
Copy link
Member

Choose a reason for hiding this comment

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

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.

features:
- |
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
Comment on lines +8 to +11
Copy link
Member

Choose a reason for hiding this comment

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

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.

6 changes: 6 additions & 0 deletions templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@
{% block navbarmenu %}
{% if project %}
<ul class="nav navbar-nav">
<li class="{% block series_active %}{% endblock %}">
<a href="{% url 'series-list' project_id=project.linkname %}">
<span class="glyphicon glyphicon-book"></span>
Series
</a>
</li>
<li class="{% block patch_active %}{% endblock %}">
<a href="{% url 'patch-list' project_id=project.linkname %}">
<span class="glyphicon glyphicon-file"></span>
Expand Down
Loading