Patchwork [1/2] Use an explicit initial default patch state

login
register
mail settings
Submitter Dirk Wallenstein
Date April 29, 2012, 1:11 p.m.
Message ID <1335705109-6584-1-git-send-email-halsmit@t-online.de>
Download mbox | patch
Permalink /patch/155737/
State Accepted
Headers show

Comments

Dirk Wallenstein - April 29, 2012, 1:11 p.m.
This will make editing states through the admin UI less error-prone, and
will facilitate parsing patches when relying on a default state, as well as
testing.

Use the explicit default state when parsing mails.

Signed-off-by: Dirk Wallenstein <halsmit@t-online.de>
---
I've found this very informative.  I somehow missed a way to obtain the
default, otherwise.
http://djangodays.com/2009/05/11/django-foreign-key-default-value-example/

 apps/patchwork/bin/parsemail.py |    5 ++---
 apps/patchwork/models.py        |    5 ++++-
 2 files changed, 6 insertions(+), 4 deletions(-)
Jeremy Kerr - April 30, 2012, 6:45 a.m.
Hi Dirk,

> This will make editing states through the admin UI less error-prone, and
> will facilitate parsing patches when relying on a default state, as well as
> testing.
>
> Use the explicit default state when parsing mails.
>
> Signed-off-by: Dirk Wallenstein<halsmit@t-online.de>


Excellent, thanks for your work on these. I've committed and pushed, 
plus a couple of minor changes.

Wolfgang - these changes are now live the patchwork.ozlabs.org site too 
now :)

Cheers,


Jeremy
Wolfgang Denk - April 30, 2012, 7:57 a.m.
Dear Jeremy,

In message <4F9E34EF.5030005@ozlabs.org> you wrote:
> 
> > This will make editing states through the admin UI less error-prone, and
> > will facilitate parsing patches when relying on a default state, as well as
> > testing.
> >
> > Use the explicit default state when parsing mails.
> >
> > Signed-off-by: Dirk Wallenstein<halsmit@t-online.de>
> 
> 
> Excellent, thanks for your work on these. I've committed and pushed, 
> plus a couple of minor changes.
> 
> Wolfgang - these changes are now live the patchwork.ozlabs.org site too 
> now :)

Thanks a lot!

Um... but is it really working?

I added "X-patchwork-state: Accepted" to a reply on a patch; my reply
was correctly added to the respective PW entry
(http://patchwork.ozlabs.org/patch/155545/) but the status did not
change.

Am I doing something wrong?

Best regards,

Wolfgang Denk
Dirk Wallenstein - April 30, 2012, 9:31 a.m.
On Mon, Apr 30, 2012 at 09:57:04AM +0200, Wolfgang Denk wrote:
> Dear Jeremy,
> 
> In message <4F9E34EF.5030005@ozlabs.org> you wrote:
> > 
> > > This will make editing states through the admin UI less error-prone, and
> > > will facilitate parsing patches when relying on a default state, as well as
> > > testing.
> > >
> > > Use the explicit default state when parsing mails.
> > >
> > > Signed-off-by: Dirk Wallenstein<halsmit@t-online.de>
> > 
> > 
> > Excellent, thanks for your work on these. I've committed and pushed, 
> > plus a couple of minor changes.
> > 
> > Wolfgang - these changes are now live the patchwork.ozlabs.org site too 
> > now :)
> 
> Thanks a lot!
> 
> Um... but is it really working?
> 
> I added "X-patchwork-state: Accepted" to a reply on a patch; my reply
> was correctly added to the respective PW entry
> (http://patchwork.ozlabs.org/patch/155545/) but the status did not
> change.
> 
> Am I doing something wrong?

You can only request an initial state (e.g. RFC) when submitting the
patch -- in the same mail as the patch.  Changing the state later on by
mail is problematic due to the lack of authentication, I'd say.  But you
can do that with pwclient.
Jeremy Kerr - April 30, 2012, 9:37 a.m.
Hi Wolfgang,

> Um... but is it really working?


Is is, but:

> I added "X-patchwork-state: Accepted" to a reply on a patch; my reply
> was correctly added to the respective PW entry
> (http://patchwork.ozlabs.org/patch/155545/) but the status did not
> change.


The patches Dirk sent don't do this; they allow the state to be set on 
the *original mail*, when the patch is first parsed.

The issue with allowing the state to be updated is that there isn't a 
method to authenticate the sender, and ensure that they're a maintainer 
of the project. It's possible to look at the From: field, but that's 
easily faked. In effect, I could send mail to 
incoming@patchwork.ozlabs.org, and set all of the buildroot patches to 
Rejected.

If anyone has useful ideas on how patchwork could do proper 
authentication, then I'd be happy to implement this.

It if suits, you could achieve the same thing with a git hook, which 
sets the patchwork state (using pwclient) when it's applied to your tree.

Cheers,


Jeremy
Wolfgang Denk - April 30, 2012, 10:08 a.m.
Dear Dirk,

In message <20120430093133.GA6118@bottich> you wrote:
>
> > Um... but is it really working?
> > 
> > I added "X-patchwork-state: Accepted" to a reply on a patch; my reply
> > was correctly added to the respective PW entry
> > (http://patchwork.ozlabs.org/patch/155545/) but the status did not
> > change.
> > 
> > Am I doing something wrong?
> 
> You can only request an initial state (e.g. RFC) when submitting the
> patch -- in the same mail as the patch.  Changing the state later on by
> mail is problematic due to the lack of authentication, I'd say.  But you
> can do that with pwclient.

Oops.   Then I completely misunderstood the purpose of this oatch.

Assinging an _initial_ state seems bogus to me - the initial state is
always "New" - what else shouldit be?  Normally it is not the Submitter
who decides about the processing of the patch.

OK, this leaves 'X-Patchwork-Delegate' which could still be useful.


Of course I can use pwclient - but this is exactly what I want to get
rid of.  I want to have a single user interface to use patchwork - I
do NOT want to have to switch between MUA to reply to a message _and_
use another tool (browser or pwclient) to change the patch state.

I am looking for a way to do this in a _single_ step, using _one_ tool
only.


Regarding security: would it be acceptable if (for example per
repository, or even per user) we could opt to enable such function for
the mail addresses registered as admins?  Yes, it would be trivial to
fake such an address.  But in my case I would be willing to accept
this risk for the added convenience.would it be acceptable if (for
example per repository, or even per user) we could opt to enable such
function for the mail addresses registered as admins?  Yes, it would
be trivial to fake such an address.  But in my case I would be willing
to accept this risk for the added convenience.

As is, I'm about to give up use of Patchwork because I find the
additional effort to manage the patch states way too high.

Best regards,

Wolfgang Denk
Wolfgang Denk - April 30, 2012, 10:18 a.m.
Dear Jeremy,

In message <4F9E5D3D.5050504@ozlabs.org> you wrote:
> 
> The patches Dirk sent don't do this; they allow the state to be set on 
> the *original mail*, when the patch is first parsed.

I understand thisnow.  Sorry, I was expecting too much :-(

> The issue with allowing the state to be updated is that there isn't a 
> method to authenticate the sender, and ensure that they're a maintainer 
> of the project. It's possible to look at the From: field, but that's 
> easily faked. In effect, I could send mail to 
> incoming@patchwork.ozlabs.org, and set all of the buildroot patches to 
> Rejected.

I would be willing to accept this.  I don't expect much misuse here,
and for me the benefit appears to be greater than the risk.

> If anyone has useful ideas on how patchwork could do proper 
> authentication, then I'd be happy to implement this.

Sorry, but I don't know of an easy _and_ reliable way for email-based
authentication.

> It if suits, you could achieve the same thing with a git hook, which 
> sets the patchwork state (using pwclient) when it's applied to your tree.

I'm using this already, but this is not what I'm looking for.

I want to be able to change the status even without git interaction;
for example, when asking the submitter to rework his patch, I would
like to add a "X-Patchwork-State: Changes Requested" header; when I
see it falls into the bailiwick of a specific custodian, I would also
set the appropriate "X-Patchwork-Delegate" header.

As is, I always have to use a second tool to perform this action -
which means additional efforts, which means patchwork is more of a
pain than a tool that makes my work more efficient.

Best regards,

Wolfgang Denk
Dirk Wallenstein - April 30, 2012, 12:01 p.m.
On Mon, Apr 30, 2012 at 12:18:53PM +0200, Wolfgang Denk wrote:
> Dear Jeremy,
> 
> In message <4F9E5D3D.5050504@ozlabs.org> you wrote:
> > 
> > The patches Dirk sent don't do this; they allow the state to be set on 
> > the *original mail*, when the patch is first parsed.
> 
> I understand thisnow.  Sorry, I was expecting too much :-(
> 
> > The issue with allowing the state to be updated is that there isn't a 
> > method to authenticate the sender, and ensure that they're a maintainer 
> > of the project. It's possible to look at the From: field, but that's 
> > easily faked. In effect, I could send mail to 
> > incoming@patchwork.ozlabs.org, and set all of the buildroot patches to 
> > Rejected.
> 
> I would be willing to accept this.  I don't expect much misuse here,
> and for me the benefit appears to be greater than the risk.
> 
> > If anyone has useful ideas on how patchwork could do proper 
> > authentication, then I'd be happy to implement this.
> 
> Sorry, but I don't know of an easy _and_ reliable way for email-based
> authentication.
> 
> > It if suits, you could achieve the same thing with a git hook, which 
> > sets the patchwork state (using pwclient) when it's applied to your tree.
> 
> I'm using this already, but this is not what I'm looking for.
> 
> I want to be able to change the status even without git interaction;
> for example, when asking the submitter to rework his patch, I would
> like to add a "X-Patchwork-State: Changes Requested" header; when I
> see it falls into the bailiwick of a specific custodian, I would also
> set the appropriate "X-Patchwork-Delegate" header.
> 
> As is, I always have to use a second tool to perform this action -
> which means additional efforts, which means patchwork is more of a
> pain than a tool that makes my work more efficient.

Actually, that might also be a nice thing for occasional contributors
without an account -- in particular revoking patches (although 
a patch sign language would probably be better in this case, like
[PATCH REVOKED] in the subject).
Anyway, Jeremy, I got the nice automated update notifications this
morning.  Maybe those could be considered sufficient for reassurance?

Patch

diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py
index 52c85fe..f156c42 100755
--- a/apps/patchwork/bin/parsemail.py
+++ b/apps/patchwork/bin/parsemail.py
@@ -34,10 +34,9 @@  except ImportError:
     from email.Utils import parsedate_tz, mktime_tz
 
 from patchwork.parser import parse_patch
-from patchwork.models import Patch, Project, Person, Comment, State
+from patchwork.models import Patch, Project, Person, Comment, State, get_default_initial_patch_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+')
@@ -349,7 +348,7 @@  def get_state(state_name):
             return State.objects.get(name__iexact=state_name)
         except State.DoesNotExist:
             pass
-    return State.objects.get(name=default_patch_state)
+    return get_default_initial_patch_state()
 
 def get_delegate(delegate_email):
     """ Return the delegate with the given email or None """
diff --git a/apps/patchwork/models.py b/apps/patchwork/models.py
index bb8d8e7..09e2c34 100644
--- a/apps/patchwork/models.py
+++ b/apps/patchwork/models.py
@@ -191,6 +191,9 @@  class PatchMbox(MIMENonMultipart):
         self.set_payload(_text.encode(self.patch_charset))
         encode_7or8bit(self)
 
+def get_default_initial_patch_state():
+    return State.objects.get(name="New")
+
 class Patch(models.Model):
     project = models.ForeignKey(Project)
     msgid = models.CharField(max_length=255)
@@ -198,7 +201,7 @@  class Patch(models.Model):
     date = models.DateTimeField(default=datetime.datetime.now)
     submitter = models.ForeignKey(Person)
     delegate = models.ForeignKey(User, blank = True, null = True)
-    state = models.ForeignKey(State)
+    state = models.ForeignKey(State, default=get_default_initial_patch_state)
     archived = models.BooleanField(default = False)
     headers = models.TextField(blank = True)
     content = models.TextField(null = True, blank = True)