384853 - Junk mail loses classification when moving to other account

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[MichelV]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.12) Gecko/20070508 Firefox/1.5.0.12
Build Identifier: 20060604

When junk mail is moved to the junk folder of another account, or to local, it looses the junk classification and the junk filter needs to be run again.

Reproducible: Always

Steps to Reproduce:
1. Filter message setup to move to junk of other account
2. View junk message being moved
3. See classification being lost


Expected Results:  
Keep junk classification once set

This did not occur in 1.x series, just since the 2.x released. Reproducible on more than one computer.

Comment 1

[Adam Guthrie]

Are you using POP or IMAP when this happens?

Comment 2

[MichelV]

This happens on IMAP, since I've upgraded to 2.x, so didn't happen before.

Comment 3

[Magnus Melin [:mkmelin] (away, back June 30)]

Confirmed on linux/trunk. Set my junk mail folder to be the junk mail folder of another imap account -> junk moved there do not have the junk mail status.

Comment 4

[Wayne Mery (:wsmwk)]

manual moves as well.  junk message on local junk folder moved to imap account, junk status of message is now default, "unknown"

backend?
(I didn't look for any dupes)

Comment 5

[Kent James (:rkent)]

I traced this out while responding to bug

Comment 6

[Kent James (:rkent)]

(... continuing after premature post)

I traced this out while responding to bug 360463. The fix for that bug relies on copying the "keywords" property from message headers of the old message. But that field is primarily for tags, and is not a reliable indicator of junk status.

There's two levels of fixes needed here. One level, which we can represent in this bug, is to properly form the IMAP custom keywords field when moving messages, so that the custom keyword is properly set on the new IMAP location based on the "junkscore" header field.

The second level is to maintain all metadata locally for message moves by copying the entire db row as part of the move. That will be a separate bug.

I'm also making this bug for trunk, not branch as previously reported. I have traced it on trunk, but I believe it also occurs on TB 2.0

Comment 7

[Alan Hart]

Confirmed as still a problem on TB 2.0.0.17 (20080914).

Thanks Kent!

Comment 8

[Sven Giermann]

(In reply to comment #6)
> The second level is to maintain all metadata locally for message moves by
> copying the entire db row as part of the move. That will be a separate bug.

Yes, it's bug 254589.

Comment 9

[Kent James (:rkent)]

Attachment: Add Junk/NonJunk to IMAP keywords

This fixes the issue of maintaining junk status when copying a message from a local folder to an IMAP folder, for a server that supports custom flags.

Comment 10

[David :Bienvenu]

Comment on attachment 370364 [details] [diff] [review]
Add Junk/NonJunk to IMAP keywords

in general, looks good, thx.

One style nit - I much prefer this:

        mailCopyState->m_message->GetStringProperty("keywords",
                                                    getter_Copies(aKeywords));
        mailCopyState->m_message->GetStringProperty("junkscore",
                                                    getter_Copies(junkscore));

I'll fix that before I land.

I'm a little worried that in some situations, Junk/NonJunk might make its way into the keywords already somehow, in which case we'd be doubling them with this code. For example, if you copy a message from imap to local, and then back from local up to imap. I'll try a quick check here, but it would be good if you could try it as well. If that could happen, we should check for the existence of the junk/nonjunk keyword in keywords before adding it. It's possible that we always strip it out when going from imap to local...

Comment 11

[David :Bienvenu]

Comment on attachment 370364 [details] [diff] [review]
Add Junk/NonJunk to IMAP keywords

I did find a nonjunk keyword in a local message, after copying it from an imap folder, so I think this code does need to protect against duplicates. The keywords are lower-cased, it looks like, so you either need to do a case-insensitive find, or look for the lower-case string.

Comment 12

[Kent James (:rkent)]

Attachment: Remove any leftover junk, nonjunk keywords

I'm amazed that there is not an easier way to do this case-insensitive find and strip, but here is what I came up with.

Comment 13

[Kent James (:rkent)]

Attachment: Trimmed spaces

In some cases, the previous patch left leading or trailing spaces. I'm not sure if that is bad or not, but I trimmed them to be sure.

Comment 14

[David :Bienvenu]

Neil might be able to think of an easier way to strip the strings out, w/ leading spaces.

Comment 15

[[e-mail]]

See attachment 359747 [details] [diff] [review] with particular reference to the use of MsgFindKeyword.

Comment 16

[David :Bienvenu]

Why wouldn't you simply not add the junk/non junk keywords if they're already in the keywords string? Is it to make the case look right on the server?

The other thought is that you could use MsgFindKeyword - if you really want to fix the case, you could copy the keywords and use MsgFindKeyword and fix the case yourself.

Comment 17

[Kent James (:rkent)]

I wanted to match the existing case, since I didn't really know the impact of using lower case. Can you assure me that 1) the junk and nonjunk in the existing keywords will always be lower case, and 2) the IMAP protocol string doesn't care whether they are upper or lower case?

Comment 18

[David :Bienvenu]

(In reply to comment #17)
> 1) the junk and nonjunk in the existing keywords will always be lower case.

We lowercase the keywords because imap servers don't guarantee to maintain the case of keywords. We can store the "Junk" keyword and get "junk" back. Literally.

> and 2) the IMAP protocol string doesn't care whether they are upper or lower case?

since imap servers don't maintain the case, we have to handle that.

Comment 19

[Kent James (:rkent)]

I *think* what you said is that I don't need to be concerned about the case that I return to the imap protocol code, and that I can assume that the string I get back from hdr.GetStringProperty("keywords") has already been lower-cased, so I can just use all lower case in my code and not worry about it.

Comment 20

[David :Bienvenu]

I think you should use MsgFindKeyword so you don't have to worry about the case of the string you get back from GetStringProperty. Most likely if the local msg has junk or nonjunk in the keywords property, that's because it came from an imap message first, and for imap messages, the keywords will be lower-cased. I don't know of any other way junk/non junk would end up in a local msg's keywords, but using MsgFindKeyword means you don't have to worry about it.

You don't have to worry about the case you give to the imap server. I'd prefer that if you are adding the keyword, you use the normal Junk/NonJunk keywords that we use, just so the protocol logs look nice, but I would not worry about changing an existing "junk" or "nonjunk" keyword to Junk/NonJunk.

Comment 21

[Kent James (:rkent)]

How about extra spaces, like "<space>NonJunk" - is that OK as the string to send to nsImapProtocol?

Comment 22

[David :Bienvenu]

(In reply to comment #21)
> How about extra spaces, like "<space>NonJunk" - is that OK as the string to
> send to nsImapProtocol?

I'm not sure - the protocol is +Flags (flag1 flag2 flag3) so I wouldn't want to risk encountering a server that was unhappy with +Flags ( flag1)

Comment 23

[Kent James (:rkent)]

An interesting consequence of the existing leftover junk/nonjunk strings in keywords with the existing trunk, is if I copy a message from IMAP to a local folder, change its junk status in the local folder, then copy it back to an IMAP folder, the changed junk status is not seen in the IMAP folder copy.

That will be fixed when I am through with this bug.

Comment 24

[Kent James (:rkent)]

Attachment: Use MsgFindKeyword [Checkin: Comment 31]

Comment 25

[David :Bienvenu]

Comment on attachment 370702 [details] [diff] [review]
Use MsgFindKeyword
[Checkin: Comment 31]

the code that adds Junk/NonJunk seems more complicated than it needs to be. I'm not sure why we're trusting junkscore over the keywords either, though perhaps there's a good reason for that.

I guess it feels wrong that you have to write all this code - this probably means that the code that does the exact same thing in nsMsgDBFolder::Add/RemoveKeywords should be made so that you could use it instead of duplicating it. But I don't want to hold this patch hostage to that, so I'll r/sr this and file a follow-on bug.

Comment 26

[Kent James (:rkent)]

(In reply to comment #25)

> I'm
> not sure why we're trusting junkscore over the keywords either, though perhaps
> there's a good reason for that.

Huh? There is no expectation that junk status is reflected in the keywords, and the fact that it is there at all is simply a bug from the code that copies custom IMAP flags. Or maybe I don't understand your question. "junkscore" is the official tristate indicator of junkstatus, which we decided to just accept when we defined the junkpercent property.

Comment 27

[Kent James (:rkent)]

(In reply to comment #25)
> 
> I guess it feels wrong that you have to write all this code - this probably
> means that the code that does the exact same thing in
> nsMsgDBFolder::Add/RemoveKeywords should be made so that you could use it
> instead of duplicating it.

I looked over the code, trying to understand the length. I'm not sure that you could solve this easily in the add/remove keywords code. We have several issues that are specific to junk code problems: 1) the tristate junkscore as mentioned in my previous comment, 2) the fact that we sort of support NotJunk as well as NonJunk as a possible indicator, and 3) the fact that a false junk/nonjunk keyword value has drifted into the keywords, which must both be removed and cannot be trusted.

That being said, it would be useful to have a lower level string manipulation for sets of space-separated keywords, that dealt with 1) possible case independence, 2) searches for keywords that can be space, EOL or beginning of line delimited, 3) cleans up any extra spaces, or spaces at the front or end, and 4) uses nsACstring instead of nsCString. I would not tie it to the message keywords(tags) string specifically though. And of course it needs to deal with frozen linkage as well as internal.

Comment 28

[David :Bienvenu]

(In reply to comment #26)
> 
> Huh? There is no expectation that junk status is reflected in the keywords, and
> the fact that it is there at all is simply a bug from the code that copies
> custom IMAP flags. Or maybe I don't understand your question. "junkscore" is
> the official tristate indicator of junkstatus, which we decided to just accept
> when we defined the junkpercent property.

The only way I know of for junk/non junk to get in local msg keywords is if they first came from imap messages, which means the keywords came from the imap server. When we see junk/nonjunk server keywords on an imap message, we actually set the junk score based on those, so in a sense, junk score comes from the junk/nonjunk keywords, if present.

Comment 29

[Kent James (:rkent)]

(In reply to comment #28)
> When we see junk/nonjunk server keywords on an imap message, we
> actually set the junk score based on those, so in a sense, junk score comes
> from the junk/nonjunk keywords, if present.

Yes, at first. But if you change the junk status, that does not get updated in the keywords hdr property, since it is not supposed to be in the keywords in the first place. All indicators of junk look to junkscore, not keywords. See my comment 23 for a bug that results from this in the existing trunk.

Comment 30

[David :Bienvenu]

(In reply to comment #29)

> 
> Yes, at first. But if you change the junk status, that does not get updated in
> the keywords hdr property, since it is not supposed to be in the keywords in
> the first place. All indicators of junk look to junkscore, not keywords.

good point - that's the "good reason" I alluded to earlier :-)

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 370702 [details] [diff] [review]
Use MsgFindKeyword
[Checkin: Comment 31]


http://hg.mozilla.org/comm-central/rev/f607d0e464e7