On Sat, 2020-01-11 at 17:12 +0000, Lukas Bulwahn wrote:
>
>
> On Sa., 11. Jan. 2020 at 13:08, Daniel Axtens <d...@axtens.net> wrote:
> > Stephen Finucane <step...@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 <metepolat2...@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!!)
Let me know if you need any help with this refactor, Daniel. I was
going to attempt this myself this week (just back from PTO) but if
you're doing it, I'll wait and review instead.
>
> 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:
>
> Patches appear at a certain version and integrated into git after
> another version.
> The series lives on while some patches stabilize, some are integrated
> and some simply disappear.
>
> 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.
>
> Let us get the relationship on patches into 2.2, and continue to look
> into this topic for 3.0.
++ this is a sane approach, IMO.
Stephen
> Lukas
>
>
> > >> @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@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
