[PATCH 2/4] models, templates: Add submission relations

[Stephen Finucane]

On Sat, 2019-12-07 at 17:46 +0100, Mete Polat wrote:
> Introduces the ability to add relations between submissions. Relations
> are displayed in the details page of a submission under 'Related'.
> Related submissions located in another projects can be viewed as well.

Apologies for the delay /o\ I finally had a chance to look at this and
went ahead and rebased this this evening. I've a lot of notes below.

> Signed-off-by: Mete Polat <metepolat2000 at gmail.com>
> ---
>  .../migrations/0038_submission_relations.py   | 30 +++++++++++++++
>  patchwork/models.py                           | 10 +++++
>  patchwork/templates/patchwork/submission.html | 37 +++++++++++++++++++
>  patchwork/views/patch.py                      |  7 ++++
>  4 files changed, 84 insertions(+)
>  create mode 100644 patchwork/migrations/0038_submission_relations.py
> 
> diff --git a/patchwork/migrations/0038_submission_relations.py b/patchwork/migrations/0038_submission_relations.py
> new file mode 100644
> index 000000000000..4c5274c64c09
> --- /dev/null
> +++ b/patchwork/migrations/0038_submission_relations.py
> @@ -0,0 +1,30 @@
> +# Generated by Django 2.2.6 on 2019-12-08 03:00
> +
> +import datetime
> +from django.conf import settings
> +from django.db import migrations, models
> +import django.db.models.deletion
> +import patchwork.models
> +
> +
> +class Migration(migrations.Migration):
> +
> +    dependencies = [
> +        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
> +        ('patchwork', '0037_event_actor'),
> +    ]
> +
> +    operations = [
> +        migrations.CreateModel(
> +            name='SubmissionRelation',
> +            fields=[
> +                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
> +                ('by', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
> +            ],
> +        ),
> +        migrations.AddField(
> +            model_name='submission',
> +            name='related',
> +            field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='submissions', related_query_name='submission', to='patchwork.SubmissionRelation'),
> +        ),
> +    ]
> diff --git a/patchwork/models.py b/patchwork/models.py
> index 7f0efd489ae3..a92203b24ff2 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -374,6 +374,9 @@ class Submission(FilenameMixin, EmailMixin, models.Model):
>      # submission metadata
>  
>      name = models.CharField(max_length=255)
> +    related = models.ForeignKey(
> +        'SubmissionRelation', null=True, blank=True, on_delete=models.SET_NULL,
> +        related_name='submissions', related_query_name='submission')

So I know Daniel suggested moving this out of Patch into Submission,
but I don't think that's the right thing to do. I've a few alternative
designs for resolving the performance issues caused by the Submission
base class and merging everything back into one is only one of them
(the others being a complete separation, and merging the CoverLetter
class into the Series class). Given how you plan to use this, and the
oddness of a cover letter relationship, I'd be in favour of reverting
to version 1 of the series and keeping this as a field on 'Patch'.

>      @property
>      def list_archive_url(self):
> @@ -863,6 +866,13 @@ class BundlePatch(models.Model):
>          ordering = ['order']
>  
>  
> +class SubmissionRelation(models.Model):

You'd need the '@python_2_unicode_compatible' decorator for this if it
were to be included in 2.2. If it ends up in 3.0, that shouldn't be the
case.

> +    by = models.ForeignKey(User, on_delete=models.CASCADE)

Again, I'm not a fan of this /o\ We already have a mechanism for
tracking who does stuff - 'Events'. Instead of doing this, could we
create a new event type and create these instead? That would provide a
far more detailed audit trail to boot, since we could track removals as
well as additions (assuming the former is allowed).

> +
> +    def __str__(self):
> +        return ', '.join(s.name for s in self.submissions.all()) or '<Empty>'
> +
> +
>  @python_2_unicode_compatible
>  class Check(models.Model):
>  
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index 77a2711ab5b4..bb0391f98ff4 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -110,6 +110,43 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
>    </td>
>   </tr>
>  {% endif %}
> +{% if submission.related %}
> + <tr>
> +  <th>Related</th>
> +  <td>
> +   <a id="togglerelated"
> +      href="javascript:toggle_div('togglerelated', 'related')"
> +   >show</a>
> +   <div id="related" class="submissionlist" style="display:none;">
> +    <ul>
> +     {% for sibling in submission.related.submissions.all %}
> +      <li>
> +       {% if sibling.id != submission.id and sibling.project_id == project.id %}
> +        <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}">
> +         {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
> +        </a>
> +       {% endif %}

You need to preload these here too, like you do in the REST API, to
prevent the database getting hammered.

> +      </li>
> +     {% endfor %}
> +     {% if related_outside %}
> +      <a id="togglerelatedoutside"
> +         href="javascript:toggle_div('togglerelatedoutside', 'relatedoutside', 'show from other projects')"
> +      >show from other projects</a>
> +      <div id="relatedoutside" class="submissionlist" style="display:none;">
> +       {% for sibling in related_outside %}
> +        <li>
> +         <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}">
> +          {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
> +         </a> (in {{ sibling.project }})
> +        </li>
> +       {% endfor %}
> +      </div>
> +     {% endif %}
> +    </ul>
> +   </div>
> +  </td>
> + </tr>
> +{% endif %}
>  </table>
>  
>  <div class="patchforms">
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index f34053ce57da..0480614614ad 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -110,12 +110,19 @@ def patch_detail(request, project_id, msgid):
>      comments = comments.only('submitter', 'date', 'id', 'content',
>                               'submission')
>  
> +    if patch.related:
> +        related_outside = patch.related.submissions \
> +            .exclude(project=patch.project)
> +    else:
> +        related_outside = []
> +

I'm not sure why this exists. Could you add some additional context to
the commit message? In any case, I think it would be more performant to
just prefetch the related patches and do this filtering in the view.

>      context['comments'] = comments
>      context['checks'] = patch.check_set.all().select_related('user')
>      context['submission'] = patch
>      context['patchform'] = form
>      context['createbundleform'] = createbundleform
>      context['project'] = patch.project
> +    context['related_outside'] = related_outside
>  
>      return render(request, 'patchwork/submission.html', context)
>  

I was going to ask why you did patch relations rather than series
relations, but I'm guessing PaStA works on a patch level? No issues
with that, but it would be nice to reuse the relationship information
to link series somehow. I've no idea how that would happen though, so
we can probably think about that later.

Stephen