Only mark series as completed after cover letter has arrived
diff mbox series

Message ID 20190114181211.16445-1-vkabatov@redhat.com
State Changes Requested
Headers show
Series
  • Only mark series as completed after cover letter has arrived
Related show

Commit Message

Veronika Kabatova Jan. 14, 2019, 6:12 p.m. UTC
From: Veronika Kabatova <vkabatov@redhat.com>

People often put metadata into the cover letter. These are needed by
various CI systems built upon Patchwork, and they are unable to process
the series if a cover letter is expected to arrive and it didn't arrive
at the time of processing. Email delaying is normal and the CI systems
are currently unable to exclude series based on the `received_all`
attribute.

Add a new DB field (not exposed in the API) that says "a cover letter
should arrive but it didn't yet" and use this field when assessing
whether the series are completed or not. In the end, a cover letter is
a vital part of the series.

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
---
Most of the people needing this change currently have a workaround in
their pipelines.

Besides protecting automation from race conditions in email delivery,
the patch also complements the proposed tagging feature changes -- one
of the changes (issue #113) deals with adding cover letter tags to each
patch in the series. Together with the planned flattening of DB
structure around Submission/Patch/Cover letter, unifying the behavior 
around cover letters and patches just makes sense.
---
 .../0034_add_cover_expected_to_series.py      | 22 +++++
 patchwork/models.py                           | 23 ++++-
 patchwork/signals.py                          | 23 ++++-
 .../series/base-cover-letter-delayed.mbox     | 94 +++++++++++++++++++
 patchwork/tests/test_series.py                | 21 ++++-
 5 files changed, 178 insertions(+), 5 deletions(-)
 create mode 100644 patchwork/migrations/0034_add_cover_expected_to_series.py
 create mode 100644 patchwork/tests/series/base-cover-letter-delayed.mbox

Comments

Daniel Axtens Aug. 21, 2019, 7:07 a.m. UTC | #1
Hi,

I'm going through old patches/mails, apologies for the huuuuge delay.

> People often put metadata into the cover letter. These are needed by
> various CI systems built upon Patchwork, and they are unable to process
> the series if a cover letter is expected to arrive and it didn't arrive
> at the time of processing. Email delaying is normal and the CI systems
> are currently unable to exclude series based on the `received_all`
> attribute.
>
> Add a new DB field (not exposed in the API) that says "a cover letter
> should arrive but it didn't yet" and use this field when assessing
> whether the series are completed or not. In the end, a cover letter is
> a vital part of the series.

Technically it is exposed in that it is added to the test for
received_all, but I guess you mean it isn't exposed by itself.

So initially, I wondered about series that deliberately don't have cover
letters. e.g. https://patchwork.ozlabs.org/patch/1144273/
But it looks like you've already thought of this!

What about series that are posted in reply to an earlier version? There
was one of these just the other day on linuxppc:
https://lore.kernel.org/linuxppc-dev/20190820175801.GA9420@archlinux-threadripper/T/#m85f5c9d80692f6248cdc4f3e0b0e0defb487bf27
https://patchwork.ozlabs.org/patch/1150491/ (v2)
https://patchwork.ozlabs.org/patch/1148897/ (v1)

Indeed, if I import that series, it would appear that cover_expected
does get confused:

>>> Patch.objects.get(id=1).series.cover_expected
False
>>> Patch.objects.get(id=2).series.cover_expected
True

git send-email (and email generally) is really unfortunately designed
here. I don't know if there's a good solution to this, but I suspect we
have a hack for it somewhere already, probably parsing the
git-send-email message-id to detect when there's a new series. If the
message is in reply to old series only, we're not expecting a cover
letter.

Regards,
Daniel

>
> Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
> ---
> Most of the people needing this change currently have a workaround in
> their pipelines.
>
> Besides protecting automation from race conditions in email delivery,
> the patch also complements the proposed tagging feature changes -- one
> of the changes (issue #113) deals with adding cover letter tags to each
> patch in the series. Together with the planned flattening of DB
> structure around Submission/Patch/Cover letter, unifying the behavior 
> around cover letters and patches just makes sense.
> ---
>  .../0034_add_cover_expected_to_series.py      | 22 +++++
>  patchwork/models.py                           | 23 ++++-
>  patchwork/signals.py                          | 23 ++++-
>  .../series/base-cover-letter-delayed.mbox     | 94 +++++++++++++++++++
>  patchwork/tests/test_series.py                | 21 ++++-
>  5 files changed, 178 insertions(+), 5 deletions(-)
>  create mode 100644 patchwork/migrations/0034_add_cover_expected_to_series.py
>  create mode 100644 patchwork/tests/series/base-cover-letter-delayed.mbox
>
> diff --git a/patchwork/migrations/0034_add_cover_expected_to_series.py b/patchwork/migrations/0034_add_cover_expected_to_series.py
> new file mode 100644
> index 00000000..85cf7281
> --- /dev/null
> +++ b/patchwork/migrations/0034_add_cover_expected_to_series.py
> @@ -0,0 +1,22 @@
> +# -*- coding: utf-8 -*-
> +from __future__ import unicode_literals
> +
> +from django.db import migrations, models
> +
> +
> +# Only add the model field. Since the cover letter should arrive within approx.
> +# 10 mins form the patches (delayed emails happen), we actually don't expect to
> +# wait for any old emails. They should be already there.
> +class Migration(migrations.Migration):
> +
> +    dependencies = [
> +        ('patchwork', '0033_remove_patch_series_model'),
> +    ]
> +
> +    operations = [
> +        migrations.AddField(
> +            model_name='series',
> +            name='cover_expected',
> +            field=models.BooleanField(default=False),
> +        ),
> +    ]
> diff --git a/patchwork/models.py b/patchwork/models.py
> index a7eee4db..d23b0d03 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -7,6 +7,7 @@
>  from collections import Counter
>  from collections import OrderedDict
>  import datetime
> +import email.parser
>  import random
>  import re
>  
> @@ -380,6 +381,10 @@ class Submission(FilenameMixin, EmailMixin, models.Model):
>  
>  class CoverLetter(Submission):
>  
> +    def delete(self, *args, **kwargs):
> +        self.series.cover_expected = True
> +        super(CoverLetter, self).delete(**kwargs)
> +
>      def get_absolute_url(self):
>          return reverse('cover-detail', kwargs={'cover_id': self.id})
>  
> @@ -627,6 +632,7 @@ class Series(FilenameMixin, models.Model):
>      cover_letter = models.OneToOneField(CoverLetter, related_name='series',
>                                          null=True,
>                                          on_delete=models.CASCADE)
>e +    cover_expected = models.BooleanField(default=False)
>  
>      # metadata
>      name = models.CharField(max_length=255, blank=True, null=True,
> @@ -658,7 +664,7 @@ class Series(FilenameMixin, models.Model):
>  
>      @property
>      def received_all(self):
> -        return self.total <= self.received_total
> +        return (self.total <= self.received_total) and not self.cover_expected
>  
>      def add_cover_letter(self, cover):
>          """Add a cover letter to the series.
> @@ -699,13 +705,24 @@ class Series(FilenameMixin, models.Model):
>              if self.name == name:
>                  self.name = self._format_name(cover)
>  
> +        self.cover_expected = False
> +
>          self.save()
>  
>      def add_patch(self, patch, number):
>          """Add a patch to the series."""
>          # both user defined names and cover letter-based names take precedence
> -        if not self.name and number == 1:
> -            self.name = patch.name  # keep the prefixes for patch-based names
> +        if number == 1:
> +            if not self.name:  # keep the prefixes for patch-based names
> +                self.name = patch.name
> +
> +            # Find out if we expect a cover letter to arrive later
> +            if not self.cover_letter:
> +                in_reply_to = email.parser.Parser().parsestr(
> +                    patch.headers, True).get('In-Reply-To')
> +                if in_reply_to:
> +                    self.cover_expected = True
> +
>              self.save()
>  
>          patch.series = self
> diff --git a/patchwork/signals.py b/patchwork/signals.py
> index 666199b6..56157ae5 100644
> --- a/patchwork/signals.py
> +++ b/patchwork/signals.py
> @@ -211,7 +211,7 @@ def create_series_created_event(sender, instance, created, raw, **kwargs):
>  
>  
>  @receiver(pre_save, sender=Patch)
> -def create_series_completed_event(sender, instance, raw, **kwargs):
> +def create_series_completed_by_patch_event(sender, instance, raw, **kwargs):
>  
>      # NOTE(stephenfin): It's actually possible for this event to be fired
>      # multiple times for a given series. To trigger this case, you would need
> @@ -240,5 +240,26 @@ def create_series_completed_event(sender, instance, raw, **kwargs):
>  
>      # we can't use "series.received_all" here since we haven't actually saved
>      # the instance yet so we duplicate that logic here but with an offset
> +    if (instance.series.received_total + 1) >= instance.series.total and \
> +            not instance.series.cover_expected:
> +        create_event(instance.series)
> +
> +
> +@receiver(pre_save, sender=CoverLetter)
> +def create_series_completed_by_cover_event(sender, instance, raw, **kwargs):
> +
> +    def create_event(series):
> +        return Event.objects.create(
> +            category=Event.CATEGORY_SERIES_COMPLETED,
> +            project=series.project,
> +            series=series)
> +
> +    # Don't trigger for items loaded from fixtures or new items
> +    if raw or not instance.pk:
> +        return
> +
> +    # We can't use "series.received_all" here since we haven't actually saved
> +    # the instance yet so we duplicate that logic here. No need to check
> +    # series.cover_expected since the cover has just arrived.
>      if (instance.series.received_total + 1) >= instance.series.total:
>          create_event(instance.series)
> diff --git a/patchwork/tests/series/base-cover-letter-delayed.mbox b/patchwork/tests/series/base-cover-letter-delayed.mbox
> new file mode 100644
> index 00000000..e5bb7d1f
> --- /dev/null
> +++ b/patchwork/tests/series/base-cover-letter-delayed.mbox
> @@ -0,0 +1,94 @@
> +From stephenfinucane@gmail.com Sun Sep 11 23:22:13 2016
> +Return-Path: <stephenfinucane@gmail.com>
> +From: Stephen Finucane <stephenfinucane@gmail.com>
> +To: stephenfinucane@hotmail.com
> +Subject: [PATCH 1/2] test: Add some lorem ipsum
> +Date: Sun, 11 Sep 2016 23:22:03 +0100
> +Message-ID: <1473632524-8585-2-git-send-email-stephenfinucane@gmail.com>
> +X-Mailer: git-send-email 2.7.4
> +In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> +References: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> +MIME-Version: 1.0
> +Content-Type: text/plain
> +Content-Length: 670
> +Lines: 22
> +
> +From: Stephen Finucane <stephenfinucane@hotmail.com>
> +
> +---
> + test.txt | 7 +++++++
> + 1 file changed, 7 insertions(+)
> +
> +diff --git a/test.txt b/test.txt
> +index f75ba05..a6c61c0 100644
> +--- a/test.txt
> ++++ b/test.txt
> +@@ -1 +1,8 @@
> + Hello, world.
> ++
> ++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget eleifend
> ++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> ++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus risus
> ++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet maximus
> ++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> ++tellus commodo bibendum ut vel dolor.
> +-- 
> +2.7.4
> +
> +
> +From stephenfinucane@gmail.com Sun Sep 11 23:22:16 2016
> +Return-Path: <stephenfinucane@gmail.com>
> +From: Stephen Finucane <stephenfinucane@gmail.com>
> +To: stephenfinucane@hotmail.com
> +Subject: [PATCH 2/2] test: Convert to Markdown
> +Date: Sun, 11 Sep 2016 23:22:04 +0100
> +Message-ID: <1473632524-8585-3-git-send-email-stephenfinucane@gmail.com>
> +X-Mailer: git-send-email 2.7.4
> +In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> +References: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> +MIME-Version: 1.0
> +Content-Type: text/plain
> +Content-Length: 1345
> +Lines: 40
> +
> +From: Stephen Finucane <stephenfinucane@hotmail.com>
> +
> +---
> + test.md  | 8 ++++++++
> + test.txt | 8 --------
> + 2 files changed, 8 insertions(+), 8 deletions(-)
> + create mode 100644 test.md
> + delete mode 100644 test.txt
> +
> +diff --git a/test.md b/test.md
> +new file mode 100644
> +index 0000000..e5ff90e
> +--- /dev/null
> ++++ b/test.md
> +@@ -0,0 +1,8 @@
> ++# Hello, world
> ++
> ++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget eleifend
> ++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> ++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus risus
> ++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet maximus
> ++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> ++tellus commodo bibendum ut vel dolor.
> +diff --git a/test.txt b/test.txt
> +deleted file mode 100644
> +index a6c61c0..0000000
> +--- a/test.txt
> ++++ /dev/null
> +@@ -1,8 +0,0 @@
> +-Hello, world.
> +-
> +-Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget eleifend
> +-augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> +-Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus risus
> +-id, elementum felis. In hac habitasse platea dictumst. Nam sit amet maximus
> +-eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> +-tellus commodo bibendum ut vel dolor.
> +-- 
> +2.7.4
> +
> +
> diff --git a/patchwork/tests/test_series.py b/patchwork/tests/test_series.py
> index f5e6b5b0..8eda806b 100644
> --- a/patchwork/tests/test_series.py
> +++ b/patchwork/tests/test_series.py
> @@ -531,7 +531,8 @@ class SeriesTotalTest(_BaseTestCase):
>      def test_complete(self):
>          """Series received with expected number of patches.
>  
> -        Parse a series where all patches are received as expected.
> +        Parse a series where all patches and a cover letter are received as
> +        expected.
>  
>          Input:
>  
> @@ -548,6 +549,24 @@ class SeriesTotalTest(_BaseTestCase):
>          series = patches[0].series
>          self.assertTrue(series.received_all)
>  
> +    def test_cover_delayed(self):
> +        """
> +        Check that series are mark as not complete if a cover is expected but
> +        didn't arrive yet.
> +
> +        Input:
> +
> +            - [PATCH 1/2] test: Add some lorem ipsum
> +            - [PATCH 2/2] test: Convert to Markdown
> +        """
> +        covers, patches, _ = self._parse_mbox(
> +            'base-cover-letter-delayed.mbox', [0, 2, 0])
> +
> +        self.assertSerialized(patches, [2])
> +
> +        series = patches[0].series
> +        self.assertFalse(series.received_all)
> +
>      def test_extra_patches(self):
>          """Series received with additional patches.
>  
> -- 
> 2.17.2
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Veronika Kabatova Sept. 2, 2019, 2:32 p.m. UTC | #2
----- Original Message -----
> From: "Daniel Axtens" <dja@axtens.net>
> To: vkabatov@redhat.com, patchwork@lists.ozlabs.org
> Sent: Wednesday, August 21, 2019 9:07:11 AM
> Subject: Re: [PATCH] Only mark series as completed after cover letter has arrived
> 
> Hi,
> 
> I'm going through old patches/mails, apologies for the huuuuge delay.
> 

No worries... I totally forgot all the details in the meanwhile though so
this is going to be exciting. I was also on vacation so obligatory sorry for
the late reply as well :)

> > People often put metadata into the cover letter. These are needed by
> > various CI systems built upon Patchwork, and they are unable to process
> > the series if a cover letter is expected to arrive and it didn't arrive
> > at the time of processing. Email delaying is normal and the CI systems
> > are currently unable to exclude series based on the `received_all`
> > attribute.
> >
> > Add a new DB field (not exposed in the API) that says "a cover letter
> > should arrive but it didn't yet" and use this field when assessing
> > whether the series are completed or not. In the end, a cover letter is
> > a vital part of the series.
> 
> Technically it is exposed in that it is added to the test for
> received_all, but I guess you mean it isn't exposed by itself.
> 

Yes.

> So initially, I wondered about series that deliberately don't have cover
> letters. e.g. https://patchwork.ozlabs.org/patch/1144273/
> But it looks like you've already thought of this!
> 
> What about series that are posted in reply to an earlier version? There
> was one of these just the other day on linuxppc:
> https://lore.kernel.org/linuxppc-dev/20190820175801.GA9420@archlinux-threadripper/T/#m85f5c9d80692f6248cdc4f3e0b0e0defb487bf27
> https://patchwork.ozlabs.org/patch/1150491/ (v2)
> https://patchwork.ozlabs.org/patch/1148897/ (v1)
> 
> Indeed, if I import that series, it would appear that cover_expected
> does get confused:
> 
> >>> Patch.objects.get(id=1).series.cover_expected
> False
> >>> Patch.objects.get(id=2).series.cover_expected
> True
> 

Hm, good catch! Digging into it, it looks like this is only a problem for
series without cover that are sent as replies.

> git send-email (and email generally) is really unfortunately designed
> here. I don't know if there's a good solution to this, but I suspect we
> have a hack for it somewhere already, probably parsing the
> git-send-email message-id to detect when there's a new series. If the
> message is in reply to old series only, we're not expecting a cover
> letter.
> 

I found

https://github.com/getpatchwork/patchwork/blob/master/patchwork/parser.py#L1061

which looks like what you're referring to. I'll use this check and add a test
too (or do something else if it turns out I'm brain dead after the vacation)
and send an updated version. I'm currently busy with Plumbers and CKI hackfest
preparations so it will take me a while till I get to it though.


Thanks,
Veronika

> Regards,
> Daniel
> 
> >
> > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
> > ---
> > Most of the people needing this change currently have a workaround in
> > their pipelines.
> >
> > Besides protecting automation from race conditions in email delivery,
> > the patch also complements the proposed tagging feature changes -- one
> > of the changes (issue #113) deals with adding cover letter tags to each
> > patch in the series. Together with the planned flattening of DB
> > structure around Submission/Patch/Cover letter, unifying the behavior
> > around cover letters and patches just makes sense.
> > ---
> >  .../0034_add_cover_expected_to_series.py      | 22 +++++
> >  patchwork/models.py                           | 23 ++++-
> >  patchwork/signals.py                          | 23 ++++-
> >  .../series/base-cover-letter-delayed.mbox     | 94 +++++++++++++++++++
> >  patchwork/tests/test_series.py                | 21 ++++-
> >  5 files changed, 178 insertions(+), 5 deletions(-)
> >  create mode 100644
> >  patchwork/migrations/0034_add_cover_expected_to_series.py
> >  create mode 100644 patchwork/tests/series/base-cover-letter-delayed.mbox
> >
> > diff --git a/patchwork/migrations/0034_add_cover_expected_to_series.py
> > b/patchwork/migrations/0034_add_cover_expected_to_series.py
> > new file mode 100644
> > index 00000000..85cf7281
> > --- /dev/null
> > +++ b/patchwork/migrations/0034_add_cover_expected_to_series.py
> > @@ -0,0 +1,22 @@
> > +# -*- coding: utf-8 -*-
> > +from __future__ import unicode_literals
> > +
> > +from django.db import migrations, models
> > +
> > +
> > +# Only add the model field. Since the cover letter should arrive within
> > approx.
> > +# 10 mins form the patches (delayed emails happen), we actually don't
> > expect to
> > +# wait for any old emails. They should be already there.
> > +class Migration(migrations.Migration):
> > +
> > +    dependencies = [
> > +        ('patchwork', '0033_remove_patch_series_model'),
> > +    ]
> > +
> > +    operations = [
> > +        migrations.AddField(
> > +            model_name='series',
> > +            name='cover_expected',
> > +            field=models.BooleanField(default=False),
> > +        ),
> > +    ]
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index a7eee4db..d23b0d03 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -7,6 +7,7 @@
> >  from collections import Counter
> >  from collections import OrderedDict
> >  import datetime
> > +import email.parser
> >  import random
> >  import re
> >  
> > @@ -380,6 +381,10 @@ class Submission(FilenameMixin, EmailMixin,
> > models.Model):
> >  
> >  class CoverLetter(Submission):
> >  
> > +    def delete(self, *args, **kwargs):
> > +        self.series.cover_expected = True
> > +        super(CoverLetter, self).delete(**kwargs)
> > +
> >      def get_absolute_url(self):
> >          return reverse('cover-detail', kwargs={'cover_id': self.id})
> >  
> > @@ -627,6 +632,7 @@ class Series(FilenameMixin, models.Model):
> >      cover_letter = models.OneToOneField(CoverLetter,
> >      related_name='series',
> >                                          null=True,
> >                                          on_delete=models.CASCADE)
> >e +    cover_expected = models.BooleanField(default=False)
> >  
> >      # metadata
> >      name = models.CharField(max_length=255, blank=True, null=True,
> > @@ -658,7 +664,7 @@ class Series(FilenameMixin, models.Model):
> >  
> >      @property
> >      def received_all(self):
> > -        return self.total <= self.received_total
> > +        return (self.total <= self.received_total) and not
> > self.cover_expected
> >  
> >      def add_cover_letter(self, cover):
> >          """Add a cover letter to the series.
> > @@ -699,13 +705,24 @@ class Series(FilenameMixin, models.Model):
> >              if self.name == name:
> >                  self.name = self._format_name(cover)
> >  
> > +        self.cover_expected = False
> > +
> >          self.save()
> >  
> >      def add_patch(self, patch, number):
> >          """Add a patch to the series."""
> >          # both user defined names and cover letter-based names take
> >          precedence
> > -        if not self.name and number == 1:
> > -            self.name = patch.name  # keep the prefixes for patch-based
> > names
> > +        if number == 1:
> > +            if not self.name:  # keep the prefixes for patch-based names
> > +                self.name = patch.name
> > +
> > +            # Find out if we expect a cover letter to arrive later
> > +            if not self.cover_letter:
> > +                in_reply_to = email.parser.Parser().parsestr(
> > +                    patch.headers, True).get('In-Reply-To')
> > +                if in_reply_to:
> > +                    self.cover_expected = True
> > +
> >              self.save()
> >  
> >          patch.series = self
> > diff --git a/patchwork/signals.py b/patchwork/signals.py
> > index 666199b6..56157ae5 100644
> > --- a/patchwork/signals.py
> > +++ b/patchwork/signals.py
> > @@ -211,7 +211,7 @@ def create_series_created_event(sender, instance,
> > created, raw, **kwargs):
> >  
> >  
> >  @receiver(pre_save, sender=Patch)
> > -def create_series_completed_event(sender, instance, raw, **kwargs):
> > +def create_series_completed_by_patch_event(sender, instance, raw,
> > **kwargs):
> >  
> >      # NOTE(stephenfin): It's actually possible for this event to be fired
> >      # multiple times for a given series. To trigger this case, you would
> >      need
> > @@ -240,5 +240,26 @@ def create_series_completed_event(sender, instance,
> > raw, **kwargs):
> >  
> >      # we can't use "series.received_all" here since we haven't actually
> >      saved
> >      # the instance yet so we duplicate that logic here but with an offset
> > +    if (instance.series.received_total + 1) >= instance.series.total and \
> > +            not instance.series.cover_expected:
> > +        create_event(instance.series)
> > +
> > +
> > +@receiver(pre_save, sender=CoverLetter)
> > +def create_series_completed_by_cover_event(sender, instance, raw,
> > **kwargs):
> > +
> > +    def create_event(series):
> > +        return Event.objects.create(
> > +            category=Event.CATEGORY_SERIES_COMPLETED,
> > +            project=series.project,
> > +            series=series)
> > +
> > +    # Don't trigger for items loaded from fixtures or new items
> > +    if raw or not instance.pk:
> > +        return
> > +
> > +    # We can't use "series.received_all" here since we haven't actually
> > saved
> > +    # the instance yet so we duplicate that logic here. No need to check
> > +    # series.cover_expected since the cover has just arrived.
> >      if (instance.series.received_total + 1) >= instance.series.total:
> >          create_event(instance.series)
> > diff --git a/patchwork/tests/series/base-cover-letter-delayed.mbox
> > b/patchwork/tests/series/base-cover-letter-delayed.mbox
> > new file mode 100644
> > index 00000000..e5bb7d1f
> > --- /dev/null
> > +++ b/patchwork/tests/series/base-cover-letter-delayed.mbox
> > @@ -0,0 +1,94 @@
> > +From stephenfinucane@gmail.com Sun Sep 11 23:22:13 2016
> > +Return-Path: <stephenfinucane@gmail.com>
> > +From: Stephen Finucane <stephenfinucane@gmail.com>
> > +To: stephenfinucane@hotmail.com
> > +Subject: [PATCH 1/2] test: Add some lorem ipsum
> > +Date: Sun, 11 Sep 2016 23:22:03 +0100
> > +Message-ID: <1473632524-8585-2-git-send-email-stephenfinucane@gmail.com>
> > +X-Mailer: git-send-email 2.7.4
> > +In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> > +References: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> > +MIME-Version: 1.0
> > +Content-Type: text/plain
> > +Content-Length: 670
> > +Lines: 22
> > +
> > +From: Stephen Finucane <stephenfinucane@hotmail.com>
> > +
> > +---
> > + test.txt | 7 +++++++
> > + 1 file changed, 7 insertions(+)
> > +
> > +diff --git a/test.txt b/test.txt
> > +index f75ba05..a6c61c0 100644
> > +--- a/test.txt
> > ++++ b/test.txt
> > +@@ -1 +1,8 @@
> > + Hello, world.
> > ++
> > ++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget
> > eleifend
> > ++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> > ++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus
> > risus
> > ++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet
> > maximus
> > ++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> > ++tellus commodo bibendum ut vel dolor.
> > +--
> > +2.7.4
> > +
> > +
> > +From stephenfinucane@gmail.com Sun Sep 11 23:22:16 2016
> > +Return-Path: <stephenfinucane@gmail.com>
> > +From: Stephen Finucane <stephenfinucane@gmail.com>
> > +To: stephenfinucane@hotmail.com
> > +Subject: [PATCH 2/2] test: Convert to Markdown
> > +Date: Sun, 11 Sep 2016 23:22:04 +0100
> > +Message-ID: <1473632524-8585-3-git-send-email-stephenfinucane@gmail.com>
> > +X-Mailer: git-send-email 2.7.4
> > +In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> > +References: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
> > +MIME-Version: 1.0
> > +Content-Type: text/plain
> > +Content-Length: 1345
> > +Lines: 40
> > +
> > +From: Stephen Finucane <stephenfinucane@hotmail.com>
> > +
> > +---
> > + test.md  | 8 ++++++++
> > + test.txt | 8 --------
> > + 2 files changed, 8 insertions(+), 8 deletions(-)
> > + create mode 100644 test.md
> > + delete mode 100644 test.txt
> > +
> > +diff --git a/test.md b/test.md
> > +new file mode 100644
> > +index 0000000..e5ff90e
> > +--- /dev/null
> > ++++ b/test.md
> > +@@ -0,0 +1,8 @@
> > ++# Hello, world
> > ++
> > ++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget
> > eleifend
> > ++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> > ++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus
> > risus
> > ++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet
> > maximus
> > ++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> > ++tellus commodo bibendum ut vel dolor.
> > +diff --git a/test.txt b/test.txt
> > +deleted file mode 100644
> > +index a6c61c0..0000000
> > +--- a/test.txt
> > ++++ /dev/null
> > +@@ -1,8 +0,0 @@
> > +-Hello, world.
> > +-
> > +-Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget
> > eleifend
> > +-augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> > +-Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus
> > risus
> > +-id, elementum felis. In hac habitasse platea dictumst. Nam sit amet
> > maximus
> > +-eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> > +-tellus commodo bibendum ut vel dolor.
> > +--
> > +2.7.4
> > +
> > +
> > diff --git a/patchwork/tests/test_series.py
> > b/patchwork/tests/test_series.py
> > index f5e6b5b0..8eda806b 100644
> > --- a/patchwork/tests/test_series.py
> > +++ b/patchwork/tests/test_series.py
> > @@ -531,7 +531,8 @@ class SeriesTotalTest(_BaseTestCase):
> >      def test_complete(self):
> >          """Series received with expected number of patches.
> >  
> > -        Parse a series where all patches are received as expected.
> > +        Parse a series where all patches and a cover letter are received
> > as
> > +        expected.
> >  
> >          Input:
> >  
> > @@ -548,6 +549,24 @@ class SeriesTotalTest(_BaseTestCase):
> >          series = patches[0].series
> >          self.assertTrue(series.received_all)
> >  
> > +    def test_cover_delayed(self):
> > +        """
> > +        Check that series are mark as not complete if a cover is expected
> > but
> > +        didn't arrive yet.
> > +
> > +        Input:
> > +
> > +            - [PATCH 1/2] test: Add some lorem ipsum
> > +            - [PATCH 2/2] test: Convert to Markdown
> > +        """
> > +        covers, patches, _ = self._parse_mbox(
> > +            'base-cover-letter-delayed.mbox', [0, 2, 0])
> > +
> > +        self.assertSerialized(patches, [2])
> > +
> > +        series = patches[0].series
> > +        self.assertFalse(series.received_all)
> > +
> >      def test_extra_patches(self):
> >          """Series received with additional patches.
> >  
> > --
> > 2.17.2
> >
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
>

Patch
diff mbox series

diff --git a/patchwork/migrations/0034_add_cover_expected_to_series.py b/patchwork/migrations/0034_add_cover_expected_to_series.py
new file mode 100644
index 00000000..85cf7281
--- /dev/null
+++ b/patchwork/migrations/0034_add_cover_expected_to_series.py
@@ -0,0 +1,22 @@ 
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+# Only add the model field. Since the cover letter should arrive within approx.
+# 10 mins form the patches (delayed emails happen), we actually don't expect to
+# wait for any old emails. They should be already there.
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0033_remove_patch_series_model'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='series',
+            name='cover_expected',
+            field=models.BooleanField(default=False),
+        ),
+    ]
diff --git a/patchwork/models.py b/patchwork/models.py
index a7eee4db..d23b0d03 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -7,6 +7,7 @@ 
 from collections import Counter
 from collections import OrderedDict
 import datetime
+import email.parser
 import random
 import re
 
@@ -380,6 +381,10 @@  class Submission(FilenameMixin, EmailMixin, models.Model):
 
 class CoverLetter(Submission):
 
+    def delete(self, *args, **kwargs):
+        self.series.cover_expected = True
+        super(CoverLetter, self).delete(**kwargs)
+
     def get_absolute_url(self):
         return reverse('cover-detail', kwargs={'cover_id': self.id})
 
@@ -627,6 +632,7 @@  class Series(FilenameMixin, models.Model):
     cover_letter = models.OneToOneField(CoverLetter, related_name='series',
                                         null=True,
                                         on_delete=models.CASCADE)
+    cover_expected = models.BooleanField(default=False)
 
     # metadata
     name = models.CharField(max_length=255, blank=True, null=True,
@@ -658,7 +664,7 @@  class Series(FilenameMixin, models.Model):
 
     @property
     def received_all(self):
-        return self.total <= self.received_total
+        return (self.total <= self.received_total) and not self.cover_expected
 
     def add_cover_letter(self, cover):
         """Add a cover letter to the series.
@@ -699,13 +705,24 @@  class Series(FilenameMixin, models.Model):
             if self.name == name:
                 self.name = self._format_name(cover)
 
+        self.cover_expected = False
+
         self.save()
 
     def add_patch(self, patch, number):
         """Add a patch to the series."""
         # both user defined names and cover letter-based names take precedence
-        if not self.name and number == 1:
-            self.name = patch.name  # keep the prefixes for patch-based names
+        if number == 1:
+            if not self.name:  # keep the prefixes for patch-based names
+                self.name = patch.name
+
+            # Find out if we expect a cover letter to arrive later
+            if not self.cover_letter:
+                in_reply_to = email.parser.Parser().parsestr(
+                    patch.headers, True).get('In-Reply-To')
+                if in_reply_to:
+                    self.cover_expected = True
+
             self.save()
 
         patch.series = self
diff --git a/patchwork/signals.py b/patchwork/signals.py
index 666199b6..56157ae5 100644
--- a/patchwork/signals.py
+++ b/patchwork/signals.py
@@ -211,7 +211,7 @@  def create_series_created_event(sender, instance, created, raw, **kwargs):
 
 
 @receiver(pre_save, sender=Patch)
-def create_series_completed_event(sender, instance, raw, **kwargs):
+def create_series_completed_by_patch_event(sender, instance, raw, **kwargs):
 
     # NOTE(stephenfin): It's actually possible for this event to be fired
     # multiple times for a given series. To trigger this case, you would need
@@ -240,5 +240,26 @@  def create_series_completed_event(sender, instance, raw, **kwargs):
 
     # we can't use "series.received_all" here since we haven't actually saved
     # the instance yet so we duplicate that logic here but with an offset
+    if (instance.series.received_total + 1) >= instance.series.total and \
+            not instance.series.cover_expected:
+        create_event(instance.series)
+
+
+@receiver(pre_save, sender=CoverLetter)
+def create_series_completed_by_cover_event(sender, instance, raw, **kwargs):
+
+    def create_event(series):
+        return Event.objects.create(
+            category=Event.CATEGORY_SERIES_COMPLETED,
+            project=series.project,
+            series=series)
+
+    # Don't trigger for items loaded from fixtures or new items
+    if raw or not instance.pk:
+        return
+
+    # We can't use "series.received_all" here since we haven't actually saved
+    # the instance yet so we duplicate that logic here. No need to check
+    # series.cover_expected since the cover has just arrived.
     if (instance.series.received_total + 1) >= instance.series.total:
         create_event(instance.series)
diff --git a/patchwork/tests/series/base-cover-letter-delayed.mbox b/patchwork/tests/series/base-cover-letter-delayed.mbox
new file mode 100644
index 00000000..e5bb7d1f
--- /dev/null
+++ b/patchwork/tests/series/base-cover-letter-delayed.mbox
@@ -0,0 +1,94 @@ 
+From stephenfinucane@gmail.com Sun Sep 11 23:22:13 2016
+Return-Path: <stephenfinucane@gmail.com>
+From: Stephen Finucane <stephenfinucane@gmail.com>
+To: stephenfinucane@hotmail.com
+Subject: [PATCH 1/2] test: Add some lorem ipsum
+Date: Sun, 11 Sep 2016 23:22:03 +0100
+Message-ID: <1473632524-8585-2-git-send-email-stephenfinucane@gmail.com>
+X-Mailer: git-send-email 2.7.4
+In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
+References: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
+MIME-Version: 1.0
+Content-Type: text/plain
+Content-Length: 670
+Lines: 22
+
+From: Stephen Finucane <stephenfinucane@hotmail.com>
+
+---
+ test.txt | 7 +++++++
+ 1 file changed, 7 insertions(+)
+
+diff --git a/test.txt b/test.txt
+index f75ba05..a6c61c0 100644
+--- a/test.txt
++++ b/test.txt
+@@ -1 +1,8 @@
+ Hello, world.
++
++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget eleifend
++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus risus
++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet maximus
++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
++tellus commodo bibendum ut vel dolor.
+-- 
+2.7.4
+
+
+From stephenfinucane@gmail.com Sun Sep 11 23:22:16 2016
+Return-Path: <stephenfinucane@gmail.com>
+From: Stephen Finucane <stephenfinucane@gmail.com>
+To: stephenfinucane@hotmail.com
+Subject: [PATCH 2/2] test: Convert to Markdown
+Date: Sun, 11 Sep 2016 23:22:04 +0100
+Message-ID: <1473632524-8585-3-git-send-email-stephenfinucane@gmail.com>
+X-Mailer: git-send-email 2.7.4
+In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
+References: <1473632524-8585-1-git-send-email-stephenfinucane@gmail.com>
+MIME-Version: 1.0
+Content-Type: text/plain
+Content-Length: 1345
+Lines: 40
+
+From: Stephen Finucane <stephenfinucane@hotmail.com>
+
+---
+ test.md  | 8 ++++++++
+ test.txt | 8 --------
+ 2 files changed, 8 insertions(+), 8 deletions(-)
+ create mode 100644 test.md
+ delete mode 100644 test.txt
+
+diff --git a/test.md b/test.md
+new file mode 100644
+index 0000000..e5ff90e
+--- /dev/null
++++ b/test.md
+@@ -0,0 +1,8 @@
++# Hello, world
++
++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget eleifend
++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus risus
++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet maximus
++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
++tellus commodo bibendum ut vel dolor.
+diff --git a/test.txt b/test.txt
+deleted file mode 100644
+index a6c61c0..0000000
+--- a/test.txt
++++ /dev/null
+@@ -1,8 +0,0 @@
+-Hello, world.
+-
+-Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget eleifend
+-augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
+-Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus risus
+-id, elementum felis. In hac habitasse platea dictumst. Nam sit amet maximus
+-eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
+-tellus commodo bibendum ut vel dolor.
+-- 
+2.7.4
+
+
diff --git a/patchwork/tests/test_series.py b/patchwork/tests/test_series.py
index f5e6b5b0..8eda806b 100644
--- a/patchwork/tests/test_series.py
+++ b/patchwork/tests/test_series.py
@@ -531,7 +531,8 @@  class SeriesTotalTest(_BaseTestCase):
     def test_complete(self):
         """Series received with expected number of patches.
 
-        Parse a series where all patches are received as expected.
+        Parse a series where all patches and a cover letter are received as
+        expected.
 
         Input:
 
@@ -548,6 +549,24 @@  class SeriesTotalTest(_BaseTestCase):
         series = patches[0].series
         self.assertTrue(series.received_all)
 
+    def test_cover_delayed(self):
+        """
+        Check that series are mark as not complete if a cover is expected but
+        didn't arrive yet.
+
+        Input:
+
+            - [PATCH 1/2] test: Add some lorem ipsum
+            - [PATCH 2/2] test: Convert to Markdown
+        """
+        covers, patches, _ = self._parse_mbox(
+            'base-cover-letter-delayed.mbox', [0, 2, 0])
+
+        self.assertSerialized(patches, [2])
+
+        series = patches[0].series
+        self.assertFalse(series.received_all)
+
     def test_extra_patches(self):
         """Series received with additional patches.