<div><div><div><br></div><div><br><div class="gmail_quote"></div></div></div><div><div><div class="gmail_quote"><div dir="auto">On Sa., 11. Jan. 2020 at 13:08, Daniel Axtens <<a href="mailto:dja@axtens.net" target="_blank">dja@axtens.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Stephen Finucane <stephen@that.guru> writes:<br>
<br>
> On Sat, 2019-12-07 at 17:46 +0100, Mete Polat wrote:<br>
>> Introduces the ability to add relations between submissions. Relations<br>
>> are displayed in the details page of a submission under 'Related'.<br>
>> Related submissions located in another projects can be viewed as well.<br>
><br>
> Apologies for the delay /o\ I finally had a chance to look at this and<br>
> went ahead and rebased this this evening. I've a lot of notes below.<br>
><br>
>> Signed-off-by: Mete Polat <<a href="mailto:metepolat2000@gmail.com" target="_blank">metepolat2000@gmail.com</a>><br>
>> ---<br>
>> .../migrations/0038_submission_relations.py | 30 +++++++++++++++<br>
>> patchwork/models.py | 10 +++++<br>
>> patchwork/templates/patchwork/submission.html | 37 +++++++++++++++++++<br>
>> patchwork/views/patch.py | 7 ++++<br>
>> 4 files changed, 84 insertions(+)<br>
>> create mode 100644 patchwork/migrations/0038_submission_relations.py<br>
>> <br>
>> diff --git a/patchwork/migrations/0038_submission_relations.py b/patchwork/migrations/0038_submission_relations.py<br>
>> new file mode 100644<br>
>> index 000000000000..4c5274c64c09<br>
>> --- /dev/null<br>
>> +++ b/patchwork/migrations/0038_submission_relations.py<br>
>> @@ -0,0 +1,30 @@<br>
>> +# Generated by Django 2.2.6 on 2019-12-08 03:00<br>
>> +<br>
>> +import datetime<br>
>> +from django.conf import settings<br>
>> +from django.db import migrations, models<br>
>> +import django.db.models.deletion<br>
>> +import patchwork.models<br>
>> +<br>
>> +<br>
>> +class Migration(migrations.Migration):<br>
>> +<br>
>> + dependencies = [<br>
>> + migrations.swappable_dependency(settings.AUTH_USER_MODEL),<br>
>> + ('patchwork', '0037_event_actor'),<br>
>> + ]<br>
>> +<br>
>> + operations = [<br>
>> + migrations.CreateModel(<br>
>> + name='SubmissionRelation',<br>
>> + fields=[<br>
>> + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),<br>
>> + ('by', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),<br>
>> + ],<br>
>> + ),<br>
>> + migrations.AddField(<br>
>> + model_name='submission',<br>
>> + name='related',<br>
>> + 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'),<br>
>> + ),<br>
>> + ]<br>
>> diff --git a/patchwork/models.py b/patchwork/models.py<br>
>> index 7f0efd489ae3..a92203b24ff2 100644<br>
>> --- a/patchwork/models.py<br>
>> +++ b/patchwork/models.py<br>
>> @@ -374,6 +374,9 @@ class Submission(FilenameMixin, EmailMixin, models.Model):<br>
>> # submission metadata<br>
>> <br>
>> name = models.CharField(max_length=255)<br>
>> + related = models.ForeignKey(<br>
>> + 'SubmissionRelation', null=True, blank=True, on_delete=models.SET_NULL,<br>
>> + related_name='submissions', related_query_name='submission')<br>
><br>
> So I know Daniel suggested moving this out of Patch into Submission,<br>
> but I don't think that's the right thing to do. I've a few alternative<br>
> designs for resolving the performance issues caused by the Submission<br>
> base class and merging everything back into one is only one of them<br>
> (the others being a complete separation, and merging the CoverLetter<br>
> class into the Series class). Given how you plan to use this, and the<br>
> oddness of a cover letter relationship, I'd be in favour of reverting<br>
> to version 1 of the series and keeping this as a field on 'Patch'.<br>
><br>
<br>
What's odd about a cover-letter relationship? The CL of v1 is related to<br>
the CL of v2, surely?<br>
<br>
If we moved CLs into series rather than making them their own thing,<br>
we'd need to just find a way to relate series as whole things. I wonder<br>
if that's the way to go and I'd be interested in seeing what that looked<br>
like. You can make a very good logical argument that a CL is much more<br>
conceptually linked to a series than to a patch... yeah, I'd be really<br>
interested in seeing that.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
I still think I'm right wrt the code as it stands now, but I'm far more<br>
interested in seeing at least the first step of this in 2.2 because I<br>
think otherwise it will get lost in the push to 3.0, so I'll respin with<br>
this reverted back to the orignal version. (Sorry Mete!!)<br>
</blockquote><div dir="auto"><br></div></div></div></div></div><div><div class="gmail_quote" dir="auto"><div dir="auto">In the end version, we will probably need to have both concepts in patchwork, relations among patches and relations among series. Just consider the evolution of the series and patches for this feature:</div><div dir="auto"><br></div><div dir="auto">Patches appear at a certain version and integrated into git after another version.</div><div dir="auto">The series lives on while some patches stabilize, some are integrated and some simply disappear.</div><div dir="auto"><br></div><div dir="auto">Also the algorithms to find relations between series and between patches are different; of course, these algorithms for patches and series can mutually use the computed relationships of the other to compute its own relation.</div><div dir="auto"><br></div><div dir="auto">Let us get the relationship on patches into 2.2, and continue to look into this topic for 3.0.</div></div></div><div><div><div><div class="gmail_quote"><div dir="auto"><br></div></div></div></div><div><div><div class="gmail_quote"><div dir="auto">Lukas</div><div dir="auto"><br></div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
>> @property<br>
>> def list_archive_url(self):<br>
>> @@ -863,6 +866,13 @@ class BundlePatch(models.Model):<br>
>> ordering = ['order']<br>
>> <br>
>> <br>
>> +class SubmissionRelation(models.Model):<br>
><br>
> You'd need the '@python_2_unicode_compatible' decorator for this if it<br>
> were to be included in 2.2. If it ends up in 3.0, that shouldn't be the<br>
> case.<br>
<br>
Fixed.<br>
<br>
><br>
>> + by = models.ForeignKey(User, on_delete=models.CASCADE)<br>
><br>
> Again, I'm not a fan of this /o\ We already have a mechanism for<br>
> tracking who does stuff - 'Events'. Instead of doing this, could we<br>
> create a new event type and create these instead? That would provide a<br>
> far more detailed audit trail to boot, since we could track removals as<br>
> well as additions (assuming the former is allowed).<br>
<br>
Fixed. Btw I notice we're using on_delete=CASCADE for event fields:<br>
shouldn't we be using SET_NULL or better yet creating some blah_invalid<br>
entries and referring to them? Events showing that something happened<br>
even if we don't know all of what it is any more is better than no<br>
events at all.<br>
<br>
I'm going to leave it here for now because I'm really trying to not stay<br>
up late hacking on stuff so much in 2020, but I hope to carve out time<br>
next week to polish these two patches off.<br>
<br>
Regards,<br>
Daniel<br>
<br>
><br>
>> +<br>
>> + def __str__(self):<br>
>> + return ', '.join(<a href="http://s.name" rel="noreferrer" target="_blank">s.name</a> for s in self.submissions.all()) or '<Empty>'<br>
>> +<br>
>> +<br>
>> @python_2_unicode_compatible<br>
>> class Check(models.Model):<br>
>> <br>
>> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html<br>
>> index 77a2711ab5b4..bb0391f98ff4 100644<br>
>> --- a/patchwork/templates/patchwork/submission.html<br>
>> +++ b/patchwork/templates/patchwork/submission.html<br>
>> @@ -110,6 +110,43 @@ function toggle_div(link_id, headers_id, label_show, label_hide)<br>
>> </td><br>
>> </tr><br>
>> {% endif %}<br>
>> +{% if submission.related %}<br>
>> + <tr><br>
>> + <th>Related</th><br>
>> + <td><br>
>> + <a id="togglerelated"<br>
>> + href="javascript:toggle_div('togglerelated', 'related')"<br>
>> + >show</a><br>
>> + <div id="related" class="submissionlist" style="display:none;"><br>
>> + <ul><br>
>> + {% for sibling in submission.related.submissions.all %}<br>
>> + <li><br>
>> + {% if <a href="http://sibling.id" rel="noreferrer" target="_blank">sibling.id</a> != <a href="http://submission.id" rel="noreferrer" target="_blank">submission.id</a> and sibling.project_id == <a href="http://project.id" rel="noreferrer" target="_blank">project.id</a> %}<br>
>> + <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}"><br>
>> + {{ <a href="http://sibling.name" rel="noreferrer" target="_blank">sibling.name</a>|default:"[no subject]"|truncatechars:100 }}<br>
>> + </a><br>
>> + {% endif %}<br>
><br>
> You need to preload these here too, like you do in the REST API, to<br>
> prevent the database getting hammered.<br>
><br>
>> + </li><br>
>> + {% endfor %}<br>
>> + {% if related_outside %}<br>
>> + <a id="togglerelatedoutside"<br>
>> + href="javascript:toggle_div('togglerelatedoutside', 'relatedoutside', 'show from other projects')"<br>
>> + >show from other projects</a><br>
>> + <div id="relatedoutside" class="submissionlist" style="display:none;"><br>
>> + {% for sibling in related_outside %}<br>
>> + <li><br>
>> + <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}"><br>
>> + {{ <a href="http://sibling.name" rel="noreferrer" target="_blank">sibling.name</a>|default:"[no subject]"|truncatechars:100 }}<br>
>> + </a> (in {{ sibling.project }})<br>
>> + </li><br>
>> + {% endfor %}<br>
>> + </div><br>
>> + {% endif %}<br>
>> + </ul><br>
>> + </div><br>
>> + </td><br>
>> + </tr><br>
>> +{% endif %}<br>
>> </table><br>
>> <br>
>> <div class="patchforms"><br>
>> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py<br>
>> index f34053ce57da..0480614614ad 100644<br>
>> --- a/patchwork/views/patch.py<br>
>> +++ b/patchwork/views/patch.py<br>
>> @@ -110,12 +110,19 @@ def patch_detail(request, project_id, msgid):<br>
>> comments = comments.only('submitter', 'date', 'id', 'content',<br>
>> 'submission')<br>
>> <br>
>> + if patch.related:<br>
>> + related_outside = patch.related.submissions \<br>
>> + .exclude(project=patch.project)<br>
>> + else:<br>
>> + related_outside = []<br>
>> +<br>
><br>
> I'm not sure why this exists. Could you add some additional context to<br>
> the commit message? In any case, I think it would be more performant to<br>
> just prefetch the related patches and do this filtering in the view.<br>
><br>
>> context['comments'] = comments<br>
>> context['checks'] = patch.check_set.all().select_related('user')<br>
>> context['submission'] = patch<br>
>> context['patchform'] = form<br>
>> context['createbundleform'] = createbundleform<br>
>> context['project'] = patch.project<br>
>> + context['related_outside'] = related_outside<br>
>> <br>
>> return render(request, 'patchwork/submission.html', context)<br>
>> <br>
><br>
> I was going to ask why you did patch relations rather than series<br>
> relations, but I'm guessing PaStA works on a patch level? No issues<br>
> with that, but it would be nice to reuse the relationship information<br>
> to link series somehow. I've no idea how that would happen though, so<br>
> we can probably think about that later.<br>
><br>
> Stephen<br>
><br>
> _______________________________________________<br>
> Patchwork mailing list<br>
> <a href="mailto:Patchwork@lists.ozlabs.org" target="_blank">Patchwork@lists.ozlabs.org</a><br>
> <a href="https://lists.ozlabs.org/listinfo/patchwork" rel="noreferrer" target="_blank">https://lists.ozlabs.org/listinfo/patchwork</a><br>
_______________________________________________<br>
Patchwork mailing list<br>
<a href="mailto:Patchwork@lists.ozlabs.org" target="_blank">Patchwork@lists.ozlabs.org</a><br>
<a href="https://lists.ozlabs.org/listinfo/patchwork" rel="noreferrer" target="_blank">https://lists.ozlabs.org/listinfo/patchwork</a><br>
</blockquote></div></div>
</div>
</div>