[PATCH 2/4] models, templates: Add submission relations
Daniel Axtens
dja at axtens.net
Sun Jan 12 00:08:05 AEDT 2020
Stephen Finucane <stephen at that.guru> writes:
> 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'.
>
What's odd about a cover-letter relationship? The CL of v1 is related to
the CL of v2, surely?
If we moved CLs into series rather than making them their own thing,
we'd need to just find a way to relate series as whole things. I wonder
if that's the way to go and I'd be interested in seeing what that looked
like. You can make a very good logical argument that a CL is much more
conceptually linked to a series than to a patch... yeah, I'd be really
interested in seeing that.
I still think I'm right wrt the code as it stands now, but I'm far more
interested in seeing at least the first step of this in 2.2 because I
think otherwise it will get lost in the push to 3.0, so I'll respin with
this reverted back to the orignal version. (Sorry Mete!!)
>> @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.
Fixed.
>
>> + 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).
Fixed. Btw I notice we're using on_delete=CASCADE for event fields:
shouldn't we be using SET_NULL or better yet creating some blah_invalid
entries and referring to them? Events showing that something happened
even if we don't know all of what it is any more is better than no
events at all.
I'm going to leave it here for now because I'm really trying to not stay
up late hacking on stuff so much in 2020, but I hope to carve out time
next week to polish these two patches off.
Regards,
Daniel
>
>> +
>> + 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
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list