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

Reply via email to