[PATCH 04/11] Fetch all series for patch/cover viewing

[Stephen Finucane]

On Fri, 2018-08-31 at 15:09 +0100, Stephen Finucane wrote:
> On Fri, 2018-08-10 at 18:00 +1000, Stewart Smith wrote:
> > e.g. a 10 comment patch goes from 26 queries in 17-20ms down to 20
> > queries in 12ms.
> > 
> > A 67 comment cover letter goes from 14 queries in 16ms down to 8 queries
> > in 8ms.
> > 
> > So, effectively, a near 2x perf improvement.
> > 
> > Previously, at several points we were asking for the latest series and
> > then asking for all the series. Since there just usually aren't *that*
> > many series, fetch them all and take the first one if we need to.
> > 
> > Signed-off-by: Stewart Smith <stewart at linux.ibm.com>
> 
> Another "don't do stuff like this in templates" example. This one also
> makes sense to me and definitely improves performance.
> 
> Reviewed-by: Stephen Finucane <stephen at that.guru>

There's a small issue with this one also. As with the other patch, I
can fix at merge time.

> Stephen
> 
> > ---
> >  patchwork/templates/patchwork/submission.html | 10 +++++-----
> >  patchwork/views/cover.py                      |  2 +-
> >  patchwork/views/patch.py                      |  1 +
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> > index 2f69735d6925..3b6f9fbe909e 100644
> > --- a/patchwork/templates/patchwork/submission.html
> > +++ b/patchwork/templates/patchwork/submission.html
> > @@ -64,15 +64,15 @@ function toggle_div(link_id, headers_id)
> >     </div>
> >    </td>
> >   </tr>
> > -{% if submission.latest_series %}
> > +{% if submission.all_series %}

This should presumably read 'all_series' - not 'submission.all_series'.

> >   <tr>
> >    <th>Series</th>
> >    <td>
> >     <div class="patchrelations">
> >      <ul>
> > -     {% for series in submission.series.all %}
> > +     {% for series in all_series %}
> >       <li>
> > -      {% if series == submission.latest_series %}
> > +      {% if forloop.first %}
> >         {{ series }}
> >        {% else %}
> >         <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ series.id }}">
> > @@ -93,7 +93,7 @@ function toggle_div(link_id, headers_id)
> >     >show</a>
> >     <div id="patchrelations" class="patchrelations" style="display:none;">
> >      <ul>
> > -    {% with submission.latest_series.cover_letter as cover %}
> > +    {% with all_series.cover_letter as cover %}

'all_series' is a queryset so it doesn't have a 'cover_letter'
attribute. What we want is something like this.

   	<div id="patchrelations" class="patchrelations" style="display:none;">
  +	 {% for series in all_series %}
     	 <ul>
  +	 {% with series.cover_letter as cover %}

This will actually fix a small theoretical issue we have, whereby
dependencies for other series than the first weren't listed. I say
theoretical as we never actually assign more than one series to a patch
and I am actually looking at making the series-patch relationship a 1:N
relationship shortly.

Note that I didn't spot either of these initially as the templates
don't error out on missing attributes. I should check to see if that
behaviour is configurable.

Stephen

> >       <li>
> >       {% if cover %}
> >        {% if cover == submission %}
> > @@ -106,7 +106,7 @@ function toggle_div(link_id, headers_id)
> >       {% endif %}
> >       </li>
> >      {% endwith %}
> > -    {% for sibling in submission.latest_series.patches.all %}
> > +    {% for sibling in all_series.patches.all %}
> >       <li>
> >        {% if sibling == submission %}
> >         {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
> > diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
> > index edad90bc694d..1ee2b3f988fa 100644
> > --- a/patchwork/views/cover.py
> > +++ b/patchwork/views/cover.py
> > @@ -49,7 +49,7 @@ def cover_detail(request, cover_id):
> >      comments = comments.select_related('submitter')
> >      comments = comments.only('submitter','date','id','content','submission')
> >      context['comments'] = comments
> > -
> > +    context['all_series'] = cover.series.all().order_by('-date')
> >      return render_to_response('patchwork/submission.html', context)
> >  
> >  
> > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> > index f43fbecd9a4d..e1d0cdcfcf39 100644
> > --- a/patchwork/views/patch.py
> > +++ b/patchwork/views/patch.py
> > @@ -118,6 +118,7 @@ def patch_detail(request, patch_id):
> >      comments = comments.select_related('submitter')
> >      comments = comments.only('submitter','date','id','content','submission')
> >  
> > +    context['all_series'] = patch.series.all().order_by('-date')
> >      context['comments'] = comments
> >      context['submission'] = patch
> >      context['patchform'] = form
> 
> 
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork