Patchwork [v2] Recognize mail headers for delegate and state

login
register
mail settings
Submitter Dirk Wallenstein
Date Jan. 26, 2011, 3:12 p.m.
Message ID <1296054749-13453-1-git-send-email-halsmit@t-online.de>
Download mbox | patch
Permalink /patch/80515/
State Changes Requested
Headers show

Comments

Dirk Wallenstein - Jan. 26, 2011, 3:12 p.m.
Introduce two new Patchwork mail headers that determine the initial
state and delegate of a patch.  They take a state name as displayed in
Patchwork and the email address of the wanted delegate.  An example:

X-Patchwork-State: Changes Requested
X-Patchwork-Delegate: maintainer@project.tld

Signed-off-by: Dirk Wallenstein <halsmit@t-online.de>
---
The call to strip() was at the wrong place previously.

 apps/patchwork/bin/parsemail.py |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)
Wolfgang Denk - Feb. 3, 2011, 2:08 p.m.
Dear Jeremy,

In message <1296054749-13453-1-git-send-email-halsmit@t-online.de> Dirk Wallenstein wrote:
> Introduce two new Patchwork mail headers that determine the initial
> state and delegate of a patch.  They take a state name as displayed in
> Patchwork and the email address of the wanted delegate.  An example:
> 
> X-Patchwork-State: Changes Requested
> X-Patchwork-Delegate: maintainer@project.tld
> 
> Signed-off-by: Dirk Wallenstein <halsmit@t-online.de>

Any chance that this (or something similar) gets activated on
patchwork.ozlabs.org ?

I'd _really_ like to have such a feature.

Best regards,

Wolfgang Denk
Jeremy Kerr - Feb. 6, 2011, 1 p.m.
Hi Wolfgang,

> Any chance that this (or something similar) gets activated on
> patchwork.ozlabs.org ?
> 
> I'd _really_ like to have such a feature.

Yep, that should be fine. I've got a lot of other (ie, non-patchwork) stuff on 
at the moment, but will take a look at it soon.

Cheers,


Jeremy
Jeremy Kerr - Feb. 11, 2011, 2:16 a.m.
Hi Dirk,

> Introduce two new Patchwork mail headers that determine the initial
> state and delegate of a patch.  They take a state name as displayed in
> Patchwork and the email address of the wanted delegate.  An example:
> 
> X-Patchwork-State: Changes Requested
> X-Patchwork-Delegate: maintainer@project.tld

Looks good, except for:

> diff --git a/apps/patchwork/bin/parsemail.py
> b/apps/patchwork/bin/parsemail.py index 1b73169..2a4df38 100755
> --- a/apps/patchwork/bin/parsemail.py
> +++ b/apps/patchwork/bin/parsemail.py
> @@ -34,8 +34,10 @@ except ImportError:
>      from email.Utils import parsedate_tz, mktime_tz
> 
>  from patchwork.parser import parse_patch
> -from patchwork.models import Patch, Project, Person, Comment
> +from patchwork.models import Patch, Project, Person, Comment, State
> +from django.contrib.auth.models import User
> 
> +default_patch_state = 'New'

We're duplicating the default-state logic provided in Patch.save() here, which 
uses the database for a lookup (there may not be a 'New' state). It would be 
better to leave the state un-set in this case, rather than selecting a 
default, then falling back to the default provided in the save() method.

Also, could you add a testcase for these? Let me know if you'd like any help 
with that.

Cheers,


Jeremy
Dirk Wallenstein - Feb. 15, 2011, 4:29 p.m.
On Fri, Feb 11, 2011 at 10:16:09AM +0800, Jeremy Kerr wrote:
> Hi Dirk,
> 
> > Introduce two new Patchwork mail headers that determine the initial
> > state and delegate of a patch.  They take a state name as displayed in
> > Patchwork and the email address of the wanted delegate.  An example:
> > 
> > X-Patchwork-State: Changes Requested
> > X-Patchwork-Delegate: maintainer@project.tld
> 
> Looks good, except for:
> 
> > diff --git a/apps/patchwork/bin/parsemail.py
> > b/apps/patchwork/bin/parsemail.py index 1b73169..2a4df38 100755
> > --- a/apps/patchwork/bin/parsemail.py
> > +++ b/apps/patchwork/bin/parsemail.py
> > @@ -34,8 +34,10 @@ except ImportError:
> >      from email.Utils import parsedate_tz, mktime_tz
> > 
> >  from patchwork.parser import parse_patch
> > -from patchwork.models import Patch, Project, Person, Comment
> > +from patchwork.models import Patch, Project, Person, Comment, State
> > +from django.contrib.auth.models import User
> > 
> > +default_patch_state = 'New'
> 
> We're duplicating the default-state logic provided in Patch.save() here, which 
> uses the database for a lookup (there may not be a 'New' state). It would be 
> better to leave the state un-set in this case, rather than selecting a 
> default, then falling back to the default provided in the save() method.

Ups, missed that.

> Also, could you add a testcase for these? Let me know if you'd like any help 
> with that.

Just want to say that I'm on it, but I'm having a bit of a cold
currently, so it might still take a bit.
Wolfgang Denk - April 24, 2012, 6:33 a.m.
Dear Dirk Wallenstein,

In message <1296054749-13453-1-git-send-email-halsmit@t-online.de> you wrote:
> Introduce two new Patchwork mail headers that determine the initial
> state and delegate of a patch.  They take a state name as displayed in
> Patchwork and the email address of the wanted delegate.  An example:
...

Upon Jeremy Kerr's comment you replied:
> >
> > > +default_patch_state = 'New'
> > 
> > We're duplicating the default-state logic provided in Patch.save() here, which 
> > uses the database for a lookup (there may not be a 'New' state). It would be 
> > better to leave the state un-set in this case, rather than selecting a 
> > default, then falling back to the default provided in the save() method.
> 
> Ups, missed that.
> 
> > Also, could you add a testcase for these? Let me know if you'd like any help 
> > with that.
> 
> Just want to say that I'm on it, but I'm having a bit of a cold
> currently, so it might still take a bit.

Has anything happened after that?

I'd really appreciate to have such a feature.

Best regards,

Wolfgang Denk
Dirk Wallenstein - April 24, 2012, 11:15 a.m.
On Tue, Apr 24, 2012 at 08:33:29AM +0200, Wolfgang Denk wrote:
> Dear Dirk Wallenstein,
> 
> In message <1296054749-13453-1-git-send-email-halsmit@t-online.de> you wrote:
> > Introduce two new Patchwork mail headers that determine the initial
> > state and delegate of a patch.  They take a state name as displayed in
> > Patchwork and the email address of the wanted delegate.  An example:
> ...
> 
> Upon Jeremy Kerr's comment you replied:
> > >
> > > > +default_patch_state = 'New'
> > > 
> > > We're duplicating the default-state logic provided in Patch.save() here, which 
> > > uses the database for a lookup (there may not be a 'New' state). It would be 
> > > better to leave the state un-set in this case, rather than selecting a 
> > > default, then falling back to the default provided in the save() method.
> > 
> > Ups, missed that.
> > 
> > > Also, could you add a testcase for these? Let me know if you'd like any help 
> > > with that.
> > 
> > Just want to say that I'm on it, but I'm having a bit of a cold
> > currently, so it might still take a bit.
> 
> Has anything happened after that?
> 
> I'd really appreciate to have such a feature.

The feature is present.  I remember, I was trying to take a step back
and create another test base class for the test but a WIP factory by
Guilherme was preferred at that time.  So, AFAICT the feature is present
but a test is missing.
Wolfgang Denk - April 26, 2012, 12:49 p.m.
Dear Dirk,

In message <20120424111515.GA17273@bottich> you wrote:
>
> > I'd really appreciate to have such a feature.
> 
> The feature is present.  I remember, I was trying to take a step back
> and create another test base class for the test but a WIP factory by
> Guilherme was preferred at that time.  So, AFAICT the feature is present
> but a test is missing.

By "present" I guess you mean present in the git repo?

So the question is when Jeremy finally updates the code on
http://patchwork.ozlabs.org -  Jeremy, is there any hope we finally
can use this great feature?

Best regards,

Wolfgang Denk
Dirk Wallenstein - April 26, 2012, 12:53 p.m.
On Thu, Apr 26, 2012 at 02:49:05PM +0200, Wolfgang Denk wrote:
> Dear Dirk,
> 
> In message <20120424111515.GA17273@bottich> you wrote:
> >
> > > I'd really appreciate to have such a feature.
> > 
> > The feature is present.  I remember, I was trying to take a step back
> > and create another test base class for the test but a WIP factory by
> > Guilherme was preferred at that time.  So, AFAICT the feature is present
> > but a test is missing.
> 
> By "present" I guess you mean present in the git repo?
Yes 
> So the question is when Jeremy finally updates the code on
> http://patchwork.ozlabs.org -  Jeremy, is there any hope we finally
> can use this great feature?
Jeremy Kerr - April 26, 2012, 2:15 p.m.
Hi all,

>>  By "present" I guess you mean present in the git repo?
> Yes


I'm confused; the patchwork.ozlabs.org site is running the latest 
patchwork git.

However, the latest patchwork git doesn't include this functionality. 
Last I heard, Dirk was re-rolling the patch for a minor change and to 
include a test-case.

Or did I miss something?

Cheers,


Jeremy
Dirk Wallenstein - April 26, 2012, 2:43 p.m.
On Thu, Apr 26, 2012 at 10:15:44PM +0800, Jeremy Kerr wrote:
> Hi all,
> 
> >> By "present" I guess you mean present in the git repo?
> >Yes
> 
> 
> I'm confused; the patchwork.ozlabs.org site is running the latest
> patchwork git.
> 
> However, the latest patchwork git doesn't include this
> functionality. Last I heard, Dirk was re-rolling the patch for a
> minor change and to include a test-case.
> 
> Or did I miss something?

Now, I'm confused, too.

I saw it included (back then) without the minor change and thought it
would therefore be okay as is.  It is still present in the checkout I
just did.  
I was on my way back to my project anyway and must have forgotten at
some point (after drafting the test BC).
It doesn't work?  I'm afraid I think I don't have a working setup right
now.

Patch

diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py
index 1b73169..2a4df38 100755
--- a/apps/patchwork/bin/parsemail.py
+++ b/apps/patchwork/bin/parsemail.py
@@ -34,8 +34,10 @@  except ImportError:
     from email.Utils import parsedate_tz, mktime_tz
 
 from patchwork.parser import parse_patch
-from patchwork.models import Patch, Project, Person, Comment
+from patchwork.models import Patch, Project, Person, Comment, State
+from django.contrib.auth.models import User
 
+default_patch_state = 'New'
 list_id_headers = ['List-ID', 'X-Mailing-List', 'X-list']
 
 whitespace_re = re.compile('\s+')
@@ -346,6 +348,24 @@  def clean_content(str):
     str = sig_re.sub('', str)
     return str.strip()
 
+def get_state(state_name):
+    """ Return the state with the given name or the default State """
+    if state_name:
+        try:
+            return State.objects.get(name__iexact=state_name)
+        except State.DoesNotExist:
+            pass
+    return State.objects.get(name=default_patch_state)
+
+def get_delegate(delegate_email):
+    """ Return the delegate with the given email or None """
+    if delegate_email:
+        try:
+            return User.objects.get(email__iexact=delegate_email)
+        except User.DoesNotExist:
+            pass
+    return None
+
 def parse_mail(mail):
 
     # some basic sanity checks
@@ -381,6 +401,9 @@  def parse_mail(mail):
         patch.submitter = author
         patch.msgid = msgid
         patch.project = project
+        patch.state = get_state(mail.get('X-Patchwork-State', '').strip())
+        patch.delegate = get_delegate(
+                mail.get('X-Patchwork-Delegate', '').strip())
         try:
             patch.save()
         except Exception, ex: