487610 - Messages marked manually as Junk are never moved to Junk folder (if user doesn't check "Enable adaptive junk mail controls for this account", even though UnJunk moves back to Inbox regardless of user's choice of the option)

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[TGOS]

Let me start with my configuration:

In the global preferences I have checked

[X] When I mark a message as junk:
    (O) Move them to the account's "Junk" folder
    ( ) Delete them

In the account junk settings I have checked:

[ ] Enable adaptive junk mail controls for this account
:
[X] Move new junk messages to:
    ( ) "Junk folder on: [...]
    (O) Other:           [Junk on XXX]


I don't want it to mark mails as junk automatically, thus I have not checked the first option; I'll do this myself manually. XXX is the name of my server account and Junk is a folder that exists on this server (IMAP folder, it's an IMAP server).

When I select a message in my inbox and mark it as junk (no matter how, there are at least three ways how to do this, same result on all), the junk marker is set on this message, but the message is moved nowhere.

I saw a lot of bug reports regarding this for Thunderbird 2, so I don't know if this is just the same old bug as before or a new one related only to Thunderbird 3 (thus I have filed a new bug - if you are sure it is exactly the same old bug, mark this one as a duplicate). Even though it's just a time saver to get this working, it's annoying if this is not working as the user expects.

Comment 1

[WADA:World Anti-bad-Duping Agency]

Oh, you've opened this bug before I post Bug 392714 Comment #17.

Comment 2

[TGOS]

I still haven't found another bug that describes the issue any better, but I have something to add that makes the whole thing even more strange:

When I have a message in my IMAP Junk folder, that I also have manually flagged as junk and I remove the junk flag, it is moved back to my Inbox. LOL. So moving it to the Junk folder by tagging it does not work, but moving it back to my Inbox by untagging it does work... funny thing is, the other way round would be exactly what I need. As untagging should not move them to Inbox, because I have different folders for different kind of mails and when I untag a mail, I'd rather move it to the correct folder then and not to the catch-all-Inbox. Maybe I should mention that I have no filters in Thunderbird that would move it elsewhere, as I filter online directly on my IMAP server.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #2)
> (A) So moving it to the Junk folder by tagging it does not work,
> (B) but moving it back to my Inbox by untagging it does work...

(A) is current implementation, as I wrote in Bug 392714 Comment #17.
(B) is implemented by Bug 208197. See also Bug 466041.
"move back to Inbox when manual mark as Not Junk" looks to be independent from "automatic Junk filtering".

Comment 4

[Mark Banner (:standard8)]

This works for me on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090508 Shredder/3.0b3pre.

TGOS: Please can you get a protocol log of marking a message as junk and it not being moved. Details on how to do that are here:

https://wiki.mozilla.org/MailNews:Logging

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #4)

This bug is for next case.
> In the account junk settings I have checked:
> [ ] Enable adaptive junk mail controls for this account
> :
> [X] Move new junk messages to:
> (snip)
AFAIR, There were bugs for next problem.
(A) "[X] When I mark a message as junk: / (O) Move them to the account's Junk
    folder" doesn't work, if "Enable adaptive junk mail controls for this
    account" is not checked.

Mark, is problem (A) already resolved?

Because of "Move *NEW* junk messages to:", UI can't be said "wrong", but very confusing. So UI change was requested.
(B) Reflect (A) to UI.
    - Change "Move new junk messages to:" to sub-option of "Enable adaptive...".
And, AFAIR, there is bug requests next. 
(C) Remove dependency to "Enable adaptive junk mail controls..."
    from "(O) Move them to the account's Junk folder"".
    (removal of "new" from "Move *NEW* junk messages to:" is required too?)
But I couldn't reach bugs again...

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> (In reply to comment #4)
> 
> This bug is for next case.
> > In the account junk settings I have checked:
> > [ ] Enable adaptive junk mail controls for this account
> > :
> > [X] Move new junk messages to:
> > (snip)
> AFAIR, There were bugs for next problem.
> (A) "[X] When I mark a message as junk: / (O) Move them to the account's Junk
>     folder" doesn't work, if "Enable adaptive junk mail controls for this
>     account" is not checked.
> 
> Mark, is problem (A) already resolved?

Ok, now I read it again, this doesn't work.

Looks like an error in the logic around nsMsgDBView::DetermineActionsForJunkChange specifically the GetLevel check.

Comment 7

[WADA:World Anti-bad-Duping Agency]

Adding description of related setting to bug summary, for ease of search.

Comment 8

[Mark Banner (:standard8)]

Is this actually a regression from TB 2 or is this the same behaviour as TB 2?

Comment 9

[WADA:World Anti-bad-Duping Agency]

Same behaviour since initial of Junk filtering support.

Comment 10

[Wayne Mery (:wsmwk)]

sounds easy to fix

Comment 11

[Kent James (:rkent)]

I'll look at this if it's "easy to fix" :)

Comment 12

[Kent James (:rkent)]

Attachment: Rev a: allow junk actions, and create missing junk folder

Here's the fix, though I'm not certain this will generate enough interest to get past review and approval for TB3 at this late date. I suppose there is a question about whether allowing these actions (move or delete) is the desired behavior, though it seems correct to me.

I should also note that the junk view action on messages are actually requests to train the messages with the bayesian filter, as well as do any additional actions, which seems odd if junk analysis is disabled. I did not change that, that is junk training is performed with or without this patch, but perhaps it should be changed. There is a lot of confusion on the difference between mark a message as junk, and train as junk. But that is another bug.

Comment 13

[John Andersen]

(In reply to comment #5)

> (C) Remove dependency to "Enable adaptive junk mail controls..."
>     from "(O) Move them to the account's Junk folder"".
>     (removal of "new" from "Move *NEW* junk messages to:" is required too?)
> But I couldn't reach bugs again...

I think you might refer to https://bugzilla.mozilla.org/show_bug.cgi?id=470114

With Gmail or any corporate spamassassin setup, requiring adaptive filtering client side to be turned on automatically MOVE missed spam to the designated spam folder seems clearly un-desirable.

Comment 14

[Kent James (:rkent)]

Let me also point out, as a workaround, in TB3 it is possible to disable junk processing selectively using an inherited folder properly. So you can accomplish the same thing that this bug does by enabling junk processing using the server's normal method, but overriding it completely using the inherited folder property.

That is, if you set the preference:

"[e-mail]#junk"

to string value "false", then no bayes processing will be done on any folder in the system.

Though I still think the patch is valid.

Comment 15

[Jack Daniels]

I have the same issue - the feature doesn't work as described and the menu options are misleading:

http://forums.mozillazine.org/viewtopic.php?f=39&t=1691185

I've wrote my experiences in the above thread.

Comment 18

[Kent James (:rkent)]

I would like to get this in, but for a slightly different reason than most people who are asking for it.

Right now, you need to turn on junk processing to do any statistical training of messages. But at first, before training is done, the junk filter will work really badly. This causes all sorts of issues for people who try to start using the junk filter.

Instead, what I would like to recommend to people is that they manually train a number of messages before they enable junk processing. This patch would allow that.

For the main body of people interested in this bug, who don't want to use the core bayesian processor, this has the disadvantage that any training they do will result in time and disk space wasted storing training information on those messages. I don't think most people would even notice that. I can't think of a simple way to separate these two requirements, so I would like to see this landed as is.

Adding bienvenu to cc as the pingee.

Comment 19

[Charles]

No objections from me... disk space is cheap... :)

Comment 20

[Jack Daniels]

None from me either - I'm not sure what you mean by time, but disk space wouldn't even get noticed.

Plus If I ever did want to enable the filter in the future, the data is there. In fact, it seems like a sensible thing.

The most important thing is you can manually move junk to and from the Junk folder using the normal buttons, but with the adaptive filter disabled so nothing gets moved automatically.

If it still records and trains in the background, fine.

Comment 21

[David :Bienvenu]

Comment on attachment 408137 [details] [diff] [review]
Rev a: allow junk actions, and create missing junk folder

The first part makes sense - my question would be, what happens with IMAP here:

+        //           the correct folder.
+        rv = GetOrCreateFolder(spamFolderURI, nsnull /* aListener */);
+        if (NS_SUCCEEDED(rv))
+          rv = GetExistingFolder(spamFolderURI, targetFolder);


Creating imap folders is async...

We should probably have mozmill tests for some of the junk stuff, part of which could test this change...

Comment 22

[Kent James (:rkent)]

Thanks for pointing out the IMAP issue. 

This code was largely copied from http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#8627  How is that case any different?

It seems to me that the simple thing to do is to start the folder creation, but if that creation is delayed because of async issues, just skip the junk processing the first time. That would show up as a junk message that did not move the first time that a user tried to mark an IMAP message as junk. Or is that too tacky? It's hard to see the value in a whole callback system to manage the async at this point, for a one-time event in an edge case.

Comment 23

[David :Bienvenu]

(In reply to comment #22)
> Thanks for pointing out the IMAP issue. 
> 
> This code was largely copied from
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#8627
>  How is that case any different?

It's somewhat different in that we we stand a chance of actually having the imap folder created by the time we finish the junk processing because we start trying to create it after the first junk message is found...whereas in the patch, we have no chance. Neither case is good, but we can probably live with it, because we try to create the Junk folder whenever the junk prefs are changed to make junk messages get moved to the junk folder, so we shouldn't get here unless the user has actually removed the junk folder somehow.

Comment 24

[Kent James (:rkent)]

So according to my tests, with the existing patch on a new profile, after I have disabled automatic junk processing but enabled move of manually marked junk to the Junk folder, manually marking a message fails the first time (but the Junk folder appears in the folder tree), then succeeds on subsequent trials. This seems to be independent of whether the Junk folder actually exists at the server or not.

In the automatic case, we create the junk folder in the account manager when someone enables the option to move the messages. In the manual case, this is trickier, because this is not a per-account option, but a global option. I'm not sure that we want to precreate junk folders on all accounts just because someone enabled this.

The issue being addressed here, by the way, is also a problem in the existing core code. That is, if you do not enable junk moves to a folder when junk is marked automatically, but do enable it when you mark it manually, then the moves to the junk folder will fail in the manual case because the junk folder does not exist. The only reason I have included it here is that this scenario is much more likely in the case where people are not using the automatic junk marking.

So the issue is, is the existing patch behavior (failing the first time only for IMAP accounts) acceptable, or is that too tacky?

Comment 25

[David :Bienvenu]

Is it possible to write an xpcshell test for this, instead of mozmill?

Comment 26

[David :Bienvenu]

>So the issue is, is the existing patch behavior (failing the first time only
>for IMAP accounts) acceptable, or is that too tacky?

It's a bit tacky, but fixing it is hard...

Comment 27

[Kent James (:rkent)]

Yes I think this is approachable from xpcshell. I'll give it a try.

Comment 29

[Kent James (:rkent)]

Attachment: add unit test

This patch leaves the C++ unchanged from the previous patch, but adds a unit test.

Comment 30

[David :Bienvenu]

Comment on attachment 425398 [details] [diff] [review]
add unit test

Thx for writing the test. kudos on getting the async stuff working.

can you lose this dump() statement? The rest are fine...

+  dump("in actually_run_test\n");

Unless this test is going to get extended, I don't think we need gTestFolder, or the extra param to setup_view. It's always gLocalInboxFolder, from what I can tell. But I'll leave it up to you...

Comment 31

[Kent James (:rkent)]

Attachment: tweak unit test per bienvenu's suggestions

Carrying over reviews.

Comment 32

[Kent James (:rkent)]

Comment on attachment 425582 [details] [diff] [review]
tweak unit test per bienvenu's suggestions

Checked in http://hg.mozilla.org/comm-central/rev/7f00e1ae066d

Comment 33

[Kent James (:rkent)]

I'm adding the flag about tb 3.0 to keep it on the radar screen, because this is at least a candidate to land on the branch, though one might argue this is a feature enhancement. But let's give it a few days on trunk before I ask for the approval for the patch.

Comment 34

[Sander Marechal]

Any idea when this will land?

Comment 35

[Kent James (:rkent)]

It will be in 3.1, so you might try the 3.1 beta. I think its release is soon.

As a long-standing issue that is more of a design decision than a bug, the case is not very good to try to land in 3.0

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 425582 [details] [diff] [review]
tweak unit test per bienvenu's suggestions

Having thought about this a bit, it is a change of behaviour for how things are working, which we normally try and avoid for minor updates.

Given that 3.1 is quite close now, I think it is better for this behaviour change to occur on a major update, rather than a minor one.

Comment 38

[Charles]

I was pleasantly surprised to see this is now working, and working well! Also marks it as read, which is logical, and marks it as unread if/when you unjunk it...

Good work guys!

Comment 39

[Mark Jones]

In reply to comment 38, for me, marking email as read when you mark it as junk is not logical, and a little irritating. This was not the original TB behaviour but was changed at some point.

It is not logical because mail marked junk is not necessarily read so it should not be marked read until it is actually read. It is irritating because I have set TB to move mail to the junk folder when I mark it as junk and I like to see how many junk mail are in the junk folder without opening it so I can decide to send all to Spamcop.net when it reaches a certain level. I cannot see this when mail marked as junk is also marked as read.

Perhaps TB needs another option setting - "mark mail as read when marking as junk"? (I should open a new feature request for this, I know.)

Anyway, this issue is resolved and I am grateful for that and what is done is done now.

Thanks.

Comment 40

[Richard Mlynarik]

Automatically marking as read is wrong.

Note that moving to trash doesn't do so, and is correct not to do so.

Comment 41

[Charles]

You're kidding me???

Who in their right mind would mark a message as Junk (or Delete it for that matter) without reading it???

Comment 42

[Richard Mlynarik]

"Who in their right mind would mark a message as Junk (or Delete it for that
matter) without reading it???'

Ummmmmmmmm ... how about anybody who has ever received a message with the string "viagra" in the subject line?

My trash file contains 12100 unread messages.

Comment 43

[Charles]

I can only assume you mean the message was moved by a mail filter, which has no bearing on this bug, which has to do with what happens when you MANUALLY mark a message as junk with the junk button (or the J key)...

Comment 44

[Richard Mlynarik]

You assume incorrectly in Comment 43.  I select multiple messages from the subject pane, read the message bodies of none of them, and move them to the trash folder with a single delete-of-riffic keystroke.

(I don't use Thunderbird's junkmail stuff in any fashion and wish I could just turn *all* of it off without bits of stray residual UI to trip over, but the same principle applies.)

Comment 45

[John Andersen]

Dear Charles Marcus:

Your email has won the British Lottery. My name is price Gazebo.

But more seriously.... 

Richard is correct, with something in excess of 90 percent of all email being spam it doesn't require that you read each spam to detect them, and to Ctrl-Click several in the message list and mark them as junk, which tn my case puts them where my iMap server will train Spamassassin with them, or report them as junk to Gmail.

So I suspect that Nobody in their right mind reads all the spam, and if you do, I admire your free time, but I assure you this is not normal behavior.

I'd just as soon have it not marked read, unless it was displayed in the message text pane for at least a second.

Comment 46

[Wayne Mery (:wsmwk)]

gentleman, bugzilla is not a discussion forum, nor is "mark as read" especially relevant to this bug, much less a fixed bug. please take the subject and follow up postings somewhere else. thanks.

Comment 47

[Charles]

(In reply to comment #44)
> You assume incorrectly in Comment 43.  I select multiple messages from the
> subject pane, read the message bodies of none of them, and move them to the
> trash folder with a single delete-of-riffic keystroke.

I was talking more to the Junk button (the reference to the Trash was in parenthesis)...

So, you delete messages without reading them... do you also use your Trash folder for permanent storage? Sorry, that is not what it is for.

I understand 'different strokes for different folks', and don't have a problem with people who do things their way, but that doesn't mean that everyone shoudl be forced to this least common denominator.

> (I don't use Thunderbird's junkmail stuff in any fashion and wish I could just
> turn *all* of it off without bits of stray residual UI to trip over, but the
> same principle applies.)

I do turn it all off, except that now, since this bug is fixed, I can happily mark messages as Junk and have them auto moved to my Junk folder and marked as read...

Comment 48

[Charles]

Sorry Wayne, posted without reading all comments...