| Message ID | 1360972641-4086-1-git-send-email-dianders@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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
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
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
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...
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
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.
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 >
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:
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(-)