[PATCH 2/4] models, templates: Add submission relations
Mete Polat
metepolat2000 at gmail.com
Mon Dec 30 23:09:44 AEDT 2019
Hi Stephen,
On 27.12.19 19:05, Stephen Finucane wrote:
> 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.
>
Unfortunately I won't be able to make bigger changes on this patch
anymore as my employment contract just ended (and exams coming up).
Nevertheless I think most of the work is done and both Patchwork and
PaStA could really benefit from this. Feel free to base you work on this
if you are interested to further work on it.
>> 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.
>
Ahh I always forget the python2 compatibility /.\
>> + 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.
>
I had some problems while trying to do the filtering in django's
template engine but to be honest I can't remember anymore what exactly
the problem was.
>> 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
>
Best regards and a happy new year full of patches!
Mete
More information about the Patchwork
mailing list