Patchwork models: Don't munge the 'From' field of patches

login
register
mail settings
Submitter Doug Anderson
Date Feb. 15, 2013, 11:57 p.m.
Message ID <1360972641-4086-1-git-send-email-dianders@chromium.org>
Download mbox | patch
Permalink /patch/220899/
State Changes Requested
Headers show

Comments

Doug Anderson - Feb. 15, 2013, 11:57 p.m.
At the moment patchwork always uses the official submitter name (as
patchwork understands it) as the "From" for patches that you receive.
This isn't quite what users expect and has some unfortunate
consequences.

The biggest problem is that patchwork saves the "official" name for an
email address the first time it sees an email from them.  If that name
is wrong (or was missing) patchwork will be confused even if future
emails from this person are fixed.  There are similar problems if a
user changes his/her name (get married?).

It seems better to just have each patch report the actual "From" that
was used to send that patch.  We'll still return the submitter in
'X-Patchwork-Submitter' just in case someone wants it.

Reported-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 apps/patchwork/models.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Jeremy Kerr - Feb. 16, 2013, 5:05 a.m.
Hi Doug,

> At the moment patchwork always uses the official submitter name (as
> patchwork understands it) as the "From" for patches that you receive.
> This isn't quite what users expect and has some unfortunate
> consequences.

Agreed.

> The biggest problem is that patchwork saves the "official" name for an
> email address the first time it sees an email from them.  If that name
> is wrong (or was missing) patchwork will be confused even if future
> emails from this person are fixed.  There are similar problems if a
> user changes his/her name (get married?).
>
> It seems better to just have each patch report the actual "From" that
> was used to send that patch.  We'll still return the submitter in
> 'X-Patchwork-Submitter' just in case someone wants it.

This change will cause the mbox download to show a different name than 
the web UI, which may also be confusing. How about we store the name in 
the Patch model (if it differs from the Person object) from the From header?

Cheers,


Jeremy
Doug Anderson - Feb. 18, 2013, 11:03 p.m.
Jeremy,

On Fri, Feb 15, 2013 at 9:05 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
>> The biggest problem is that patchwork saves the "official" name for an
>> email address the first time it sees an email from them.  If that name
>> is wrong (or was missing) patchwork will be confused even if future
>> emails from this person are fixed.  There are similar problems if a
>> user changes his/her name (get married?).
>>
>> It seems better to just have each patch report the actual "From" that
>> was used to send that patch.  We'll still return the submitter in
>> 'X-Patchwork-Submitter' just in case someone wants it.
>
> This change will cause the mbox download to show a different name than the
> web UI, which may also be confusing. How about we store the name in the
> Patch model (if it differs from the Person object) from the From header?

In which part of the web UI?  I played with this and the "Headers"
still shows the proper (un-munged) From address.  Certainly the
"Submitter" at the top still shows the official patchwork idea of what
the person's name is but I'm not convinced that's a bad thing.
Overall patchwork has the concept of what the official name of a
submitter in several places (replies, the "Filters" UI, etc).  Having
the UI show a different name than the "From" mail header is not so
different than seeing a "reply" have a different name than the
"Acked-by" like that the reply contains...

Anyway, I'm happy to give your proposal a shot if you want.  Let me know...

-Doug
Wolfram Sang - Sept. 26, 2013, 8:45 a.m.
On Mon, Feb 18, 2013 at 03:03:33PM -0800, Doug Anderson wrote:
> Jeremy,
> 
> On Fri, Feb 15, 2013 at 9:05 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> >> The biggest problem is that patchwork saves the "official" name for an
> >> email address the first time it sees an email from them.  If that name
> >> is wrong (or was missing) patchwork will be confused even if future
> >> emails from this person are fixed.  There are similar problems if a
> >> user changes his/her name (get married?).
> >>
> >> It seems better to just have each patch report the actual "From" that
> >> was used to send that patch.  We'll still return the submitter in
> >> 'X-Patchwork-Submitter' just in case someone wants it.
> >
> > This change will cause the mbox download to show a different name than the
> > web UI, which may also be confusing. How about we store the name in the
> > Patch model (if it differs from the Person object) from the From header?
> 
> In which part of the web UI?  I played with this and the "Headers"
> still shows the proper (un-munged) From address.  Certainly the
> "Submitter" at the top still shows the official patchwork idea of what
> the person's name is but I'm not convinced that's a bad thing.
> Overall patchwork has the concept of what the official name of a
> submitter in several places (replies, the "Filters" UI, etc).  Having
> the UI show a different name than the "From" mail header is not so
> different than seeing a "reply" have a different name than the
> "Acked-by" like that the reply contains...
> 
> Anyway, I'm happy to give your proposal a shot if you want.  Let me know...

Ping. Still an issue: http://patchwork.ozlabs.org/patch/277644/

Thanks,

   Wolfram
Jeremy Kerr - Sept. 26, 2013, 9:05 a.m.
Hi all,

>> In which part of the web UI?  I played with this and the "Headers"
>> still shows the proper (un-munged) From address.  Certainly the
>> "Submitter" at the top still shows the official patchwork idea of what
>> the person's name is but I'm not convinced that's a bad thing.
>> Overall patchwork has the concept of what the official name of a
>> submitter in several places (replies, the "Filters" UI, etc).  Having
>> the UI show a different name than the "From" mail header is not so
>> different than seeing a "reply" have a different name than the
>> "Acked-by" like that the reply contains...
>>
>> Anyway, I'm happy to give your proposal a shot if you want.  Let me know...
> 
> Ping. Still an issue: http://patchwork.ozlabs.org/patch/277644/

Sorry, been a bit distracted lately. Doug: do you want to have a go at this?

One thing we'll need to work out is how to process these
potentially-different names in the lists of patches.

Cheers,


Jeremy
Doug Anderson - Sept. 26, 2013, 8:16 p.m.
Jeremy,


On Thu, Sep 26, 2013 at 2:05 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi all,
>
>>> In which part of the web UI?  I played with this and the "Headers"
>>> still shows the proper (un-munged) From address.  Certainly the
>>> "Submitter" at the top still shows the official patchwork idea of what
>>> the person's name is but I'm not convinced that's a bad thing.
>>> Overall patchwork has the concept of what the official name of a
>>> submitter in several places (replies, the "Filters" UI, etc).  Having
>>> the UI show a different name than the "From" mail header is not so
>>> different than seeing a "reply" have a different name than the
>>> "Acked-by" like that the reply contains...
>>>
>>> Anyway, I'm happy to give your proposal a shot if you want.  Let me know...
>>
>> Ping. Still an issue: http://patchwork.ozlabs.org/patch/277644/
>
> Sorry, been a bit distracted lately. Doug: do you want to have a go at this?
>
> One thing we'll need to work out is how to process these
> potentially-different names in the lists of patches.

I can put it on my list of things and try to get to it over the next
few weeks.  If we can come up with a simple solution then it will be
easier to find time.  ;)


So I think that the two "simple" solutions I can think of are one of these two:

1. Just add a "From" field in the Web UI in the case that the
"Submitter" and "From" are different.  That would also match the code
I already wrote.

2. Assign a unique submitter ID every time an email comes in with a
different Real Name (but the same email address).  You could auto-lump
these into someone's account if they've logged in to patchwork.

Basically there are lots of places in the UI (the filter UI, search
results view, etc) that assume that a single submitter ID matches to a
single real name, so showing anything other than the official
"Submitter" name as the Submitter seems wrong.

--

Let's work out an example.  Say we have "Jenny Jones" who changed her
name to "Jenny Smith" when she got married.  Her email address didn't
change and was "jenny@example.com" the whole time.  She sent patches
both before and after her name change.  Patchwork has assigned her a
unique submitter ID as 1234.  Jenny has never logged into patchwork.

Right now, patchwork treats all patches from the same email address as
a single Submitter, ignoring the "name" part of the address.  So all
of Jenny's patches are submitter ID 1234 and patchwork thinks her
official name is the first one it saw: "Jenny Jones".  That means that
if you search for patches from Jenny and you open the Filters page,
the popup will contain "Jenny Jones" and not "Jenny Smith", right?
...but searching for "Jenny Jones" will still show patches that showed
up after the name change (so you'll see patches from Jenny Jones and
Jenny Smith)

Now you open one of those patches.  What would you expect it to say?
Should it say that the Submitter is "Jenny Jones" or "Jenny Smith"?
Right now it will say "Jenny Jones".  If it showed "Jenny Smith" it
would be confusing since it doesn't match the Filter you searched for.

--

So IMHO either a submitter ID maps to single Real Name (and we add the
"From" field to the UI) or start creating more unique submitter IDs.


Let me know what you think.  I think that creating more unique
submitter IDs might be beyond what I have time for, but I think it
might be a better long term solution.


-Doug
Jeremy Kerr - Oct. 1, 2013, 1:18 a.m.
Hi Doug,

> I can put it on my list of things and try to get to it over the next
> few weeks.  If we can come up with a simple solution then it will be
> easier to find time.  ;)
> 
> 
> So I think that the two "simple" solutions I can think of are one of these two:
> 
> 1. Just add a "From" field in the Web UI in the case that the
> "Submitter" and "From" are different.  That would also match the code
> I already wrote.
> 
> 2. Assign a unique submitter ID every time an email comes in with a
> different Real Name (but the same email address).  You could auto-lump
> these into someone's account if they've logged in to patchwork.
> 
> Basically there are lots of places in the UI (the filter UI, search
> results view, etc) that assume that a single submitter ID matches to a
> single real name, so showing anything other than the official
> "Submitter" name as the Submitter seems wrong.

Been thinking about this a little, and this is what I think would be best:

When a patch or comment comes in, we still lookup a submitter id by
case-insensitive search on the email address field. This sets the
submitter FK on the Patch/Comment object.

However, the web view and mbox downloads use the actual From: header as
the display & download. We could either extract this from Patch.headers
in the view function (check out the usage of HeaderParser in
patch_to_mbox), or save the full From: value as an additional field in
the Patch object. Comments can continue to use Person.name.

List views should still use the Person object to show the submitter, and
we still use the person FK in filter-by-submitter.

However, we should keep the Person.name field current, so that the list
views reflect reality. When a mail comes in, we should update
Person.name to what's in the From field, provided it's non-empty (and
doesn't already match, to avoid an extra DB write). This way, patchwork
doesn't persist with the first mail's From header.

How does that sound?

Cheers,


Jeremy
Doug Anderson - Oct. 1, 2013, 2:01 a.m.
Jeremy,

On Mon, Sep 30, 2013 at 3:18 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Doug,
>
>> I can put it on my list of things and try to get to it over the next
>> few weeks.  If we can come up with a simple solution then it will be
>> easier to find time.  ;)
>>
>>
>> So I think that the two "simple" solutions I can think of are one of these two:
>>
>> 1. Just add a "From" field in the Web UI in the case that the
>> "Submitter" and "From" are different.  That would also match the code
>> I already wrote.
>>
>> 2. Assign a unique submitter ID every time an email comes in with a
>> different Real Name (but the same email address).  You could auto-lump
>> these into someone's account if they've logged in to patchwork.
>>
>> Basically there are lots of places in the UI (the filter UI, search
>> results view, etc) that assume that a single submitter ID matches to a
>> single real name, so showing anything other than the official
>> "Submitter" name as the Submitter seems wrong.
>
> Been thinking about this a little, and this is what I think would be best:
>
> When a patch or comment comes in, we still lookup a submitter id by
> case-insensitive search on the email address field. This sets the
> submitter FK on the Patch/Comment object.
>
> However, the web view and mbox downloads use the actual From: header as
> the display & download. We could either extract this from Patch.headers
> in the view function (check out the usage of HeaderParser in
> patch_to_mbox), or save the full From: value as an additional field in
> the Patch object. Comments can continue to use Person.name.
>
> List views should still use the Person object to show the submitter, and
> we still use the person FK in filter-by-submitter.
>
> However, we should keep the Person.name field current, so that the list
> views reflect reality. When a mail comes in, we should update
> Person.name to what's in the From field, provided it's non-empty (and
> doesn't already match, to avoid an extra DB write). This way, patchwork
> doesn't persist with the first mail's From header.
>
> How does that sound?

Sounds reasonable to me, except I'd modify it to say that we don't
update the Person.name field for anyone who has actually ever logged
into patchwork.  This allows people who care to prevent abuse.  I can
just imagine someone sending some sort of email with my email address
and the real name "Captain Dumbohead".  It would update things
everywhere!  ;)

The above also seems like something I could conceivably tackle, so
I'll put it on my list and try to get to it in the next few weeks.

-Captain Dumbohead
Jeremy Kerr - Oct. 1, 2013, 2:04 a.m.
Hi Doug,

> Sounds reasonable to me, except I'd modify it to say that we don't
> update the Person.name field for anyone who has actually ever logged
> into patchwork.  This allows people who care to prevent abuse.  I can
> just imagine someone sending some sort of email with my email address
> and the real name "Captain Dumbohead".  It would update things
> everywhere!  ;)

Yes, good point.

> The above also seems like something I could conceivably tackle, so
> I'll put it on my list and try to get to it in the next few weeks.

Awesome, thanks. Don't forget to use the test infrastructure, I find it
really helpful when developing features like this. Happy to give you a
hand with this if you like.

> -Captain Dumbohead

I can update the patchwork instance on patchwork.ozlabs.org manually, if
you like? :D

Cheers,


Jeremy
Wolfram Sang - Nov. 14, 2013, 5:31 p.m.
On Tue, Oct 01, 2013 at 10:04:07AM +0800, Jeremy Kerr wrote:
> Hi Doug,
> 
> > Sounds reasonable to me, except I'd modify it to say that we don't
> > update the Person.name field for anyone who has actually ever logged
> > into patchwork.  This allows people who care to prevent abuse.  I can
> > just imagine someone sending some sort of email with my email address
> > and the real name "Captain Dumbohead".  It would update things
> > everywhere!  ;)
> 
> Yes, good point.
> 
> > The above also seems like something I could conceivably tackle, so
> > I'll put it on my list and try to get to it in the next few weeks.
> 
> Awesome, thanks. Don't forget to use the test infrastructure, I find it
> really helpful when developing features like this. Happy to give you a
> hand with this if you like.
> 
> > -Captain Dumbohead
> 
> I can update the patchwork instance on patchwork.ozlabs.org manually, if
> you like? :D

Ping. http://patchwork.ozlabs.org/patch/288221/

Yes, I always ping when James gets another patch accepted...
Doug Anderson - Nov. 18, 2013, 6 a.m.
These patches deal with a problem that Wolfram Sang reported where
sometimes the real name associated with a patchwork submitter doesn't
match the actual name that was associated with a patch file.

The first version of this series just returned the actual "From:"
field in the mbox, but that was thought to be confusing.  Now we:
- Make sure we show when the Submitter / Sent From are different in the UI.
- Update Submitter names in some cases.
- Still return the actual "From:" in the mbox.

These patches have only been lightly tested and don't include any
official tests, so may need to be spun one more time before becoming
official if they seem broken or need official tests.  I'd appreciate
any comments, though.


Doug Anderson (5):
  Move email address parsing functions to a separate module
  models: Add sent_from() and need_sent_from() methods to Patch model
  templates: Add "Sent From" to the patch template
  models: Don't munge the 'From' field of patches
  parsemail: Update a Person's name upon new email unless they are
    registered

 apps/patchwork/bin/parsemail.py  | 65 +++++----------------------
 apps/patchwork/emailutils.py     | 94 ++++++++++++++++++++++++++++++++++++++++
 apps/patchwork/models.py         | 24 ++++++++++
 apps/patchwork/views/__init__.py |  4 +-
 templates/patchwork/patch.html   |  8 +++-
 5 files changed, 139 insertions(+), 56 deletions(-)
 create mode 100644 apps/patchwork/emailutils.py
Wolfram Sang - June 2, 2014, 5:23 p.m.
On Thu, Nov 14, 2013 at 06:31:28PM +0100, Wolfram Sang wrote:
> On Tue, Oct 01, 2013 at 10:04:07AM +0800, Jeremy Kerr wrote:
> > Hi Doug,
> > 
> > > Sounds reasonable to me, except I'd modify it to say that we don't
> > > update the Person.name field for anyone who has actually ever logged
> > > into patchwork.  This allows people who care to prevent abuse.  I can
> > > just imagine someone sending some sort of email with my email address
> > > and the real name "Captain Dumbohead".  It would update things
> > > everywhere!  ;)
> > 
> > Yes, good point.
> > 
> > > The above also seems like something I could conceivably tackle, so
> > > I'll put it on my list and try to get to it in the next few weeks.
> > 
> > Awesome, thanks. Don't forget to use the test infrastructure, I find it
> > really helpful when developing features like this. Happy to give you a
> > hand with this if you like.
> > 
> > > -Captain Dumbohead
> > 
> > I can update the patchwork instance on patchwork.ozlabs.org manually, if
> > you like? :D
> 
> Ping. http://patchwork.ozlabs.org/patch/288221/
> 
> Yes, I always ping when James gets another patch accepted...

Is this the same issue?

http://patchwork.ozlabs.org/patch/344062/

has "Nimrod Andy" as submitter whilst in my inbox it is "Fugang Duan
<B38611@freescale.com>". The latter name is the correct one and
important since it matches the SoB.
Doug Anderson - June 2, 2014, 10:21 p.m.
I dunno.  The first patch I see from this email address is
"http://patchwork.ozlabs.org/patch/216781/".  ...and the name wasn't
Nimrod Andy then...

On Mon, Jun 2, 2014 at 10:23 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Nov 14, 2013 at 06:31:28PM +0100, Wolfram Sang wrote:
>> On Tue, Oct 01, 2013 at 10:04:07AM +0800, Jeremy Kerr wrote:
>> > Hi Doug,
>> >
>> > > Sounds reasonable to me, except I'd modify it to say that we don't
>> > > update the Person.name field for anyone who has actually ever logged
>> > > into patchwork.  This allows people who care to prevent abuse.  I can
>> > > just imagine someone sending some sort of email with my email address
>> > > and the real name "Captain Dumbohead".  It would update things
>> > > everywhere!  ;)
>> >
>> > Yes, good point.
>> >
>> > > The above also seems like something I could conceivably tackle, so
>> > > I'll put it on my list and try to get to it in the next few weeks.
>> >
>> > Awesome, thanks. Don't forget to use the test infrastructure, I find it
>> > really helpful when developing features like this. Happy to give you a
>> > hand with this if you like.
>> >
>> > > -Captain Dumbohead
>> >
>> > I can update the patchwork instance on patchwork.ozlabs.org manually, if
>> > you like? :D
>>
>> Ping. http://patchwork.ozlabs.org/patch/288221/
>>
>> Yes, I always ping when James gets another patch accepted...
>
> Is this the same issue?
>
> http://patchwork.ozlabs.org/patch/344062/
>
> has "Nimrod Andy" as submitter whilst in my inbox it is "Fugang Duan
> <B38611@freescale.com>". The latter name is the correct one and
> important since it matches the SoB.
>
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
>

Patch

diff --git a/apps/patchwork/models.py b/apps/patchwork/models.py
index 86a5266..6b9baf7 100644
--- a/apps/patchwork/models.py
+++ b/apps/patchwork/models.py
@@ -281,13 +281,13 @@  class Patch(models.Model):
         mail['Subject'] = self.name
         mail['Date'] = email.utils.formatdate(
                         time.mktime(self.date.utctimetuple()))
-        mail['From'] = unicode(self.submitter)
+        mail['X-Patchwork-Submitter'] = unicode(self.submitter)
         mail['X-Patchwork-Id'] = str(self.id)
         mail['Message-Id'] = self.msgid
         mail.set_unixfrom('From patchwork ' + self.date.ctime())
 
 
-        copied_headers = ['To', 'Cc']
+        copied_headers = ['To', 'Cc', 'From']
         orig_headers = HeaderParser().parsestr(str(self.headers))
         for header in copied_headers:
             if header in orig_headers: