Closed Bug 1071754 Opened 10 years ago Closed 10 years ago

Filter to move messages to IMAP folder on getting new mail doesn't work unless Filter after Junk Classification is selected

Categories

(MailNews Core :: Filters, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alta88, Assigned: alta88)

Details

Attachments

(1 file)

This was discovered by creating a filter rule in a feed account to match all messages and Move to an imap folder.  The filter log shows seemingly successful moves to imap, but they neither appear there nor leave the original feed folder.  Moves to pop account or local account folders work fine.

Copy works if Filter Before is selected, so it's odd.  This may explain some related Move to imap filter issues, though for feeds there isn't a logical concept of 'junk' (in the spam sense, heh).

A simple lame fix could be to autoselect Filter After Junk and disable the menulist.  But it seems for feeds the bayesian spam filter should be skipped altogether (if that's what does junk classification).

rkent, thoughts?
"Filter after" uses entirely different code (the FilterAfterTheFact code) than "filter before", so it is common that bugs in one do not appear in the other. Although that was added to allow the bayes filter to be used, there are other special cases where works, such as this one. The bayes filter will not actually occur unless it is selected for the account. Not sure if RSS allows that or not.
It turns out this is the result of basically a typo in bug 402392; OnStopRequest has to run before EndMsgDownload (and only the imap movecoalescer is affected), which is the sequence in pop3sink and why it works for pop accounts.

Also for pop accounts, if an imap folder is not authed, an attempt to move/copy will prompt, which is the correct thing to do.  This has been fixed here as well.

(There is no auth prompt for Filter After, either for pop or local (feed) accounts, apparently intentionally[1] in bug 198100.  I think this is rather incorrect..)

[1] http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2350
Assignee: nobody → alta88
Attachment #8496575 - Flags: review?(kent)
(In reply to alta88 from comment #2)
> 
> (There is no auth prompt for Filter After, either for pop or local (feed)
> accounts, apparently intentionally[1] in bug 198100.  I think this is rather
> incorrect..)
> 
> [1]
> http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.
> cpp#2350

There have been different philosophies of this, and it is probably worth a discussion.
(In reply to Kent James (:rkent) from comment #4)
> (In reply to alta88 from comment #2)
> > 
> > (There is no auth prompt for Filter After, either for pop or local (feed)
> > accounts, apparently intentionally[1] in bug 198100.  I think this is rather
> > incorrect..)
> > 
> > [1]
> > http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.
> > cpp#2350
> 
> There have been different philosophies of this, and it is probably worth a
> discussion.

Well, I guess my position is evident ;)

However, I'll say that there isn't a defensible philosophy for non prompt of a required input (in a non batch context) whose lack leads to failure in completing a user expected task.  What's extra worse is the bizarre logging philosophy, where a message phrased as success is printed ("Applied" means success) even before the action is taken, and certainly not considering its async failure possibility, which is guaranteed for non auth, and whose failure is silent.

On top of that sort of lying to the user, we're actually regressing (progressing in lying) in the strange consensus/patch in bug 935934, where the filter log message is now always (not just for the imap move case) to be printed before the actual attempt of the action.

Honestly, we've lost a sense of best practices around here.
Comment on attachment 8496575 [details] [diff] [review]
imapMoveFilter.patch

Review of attachment 8496575 [details] [diff] [review]:
-----------------------------------------------------------------

As far as I can tell, this method seems to be used either in test, or in RSS. I don't know what the correct choice is for setting a msgWindow or not in automatic filters, but I won't oppose it in RSS. A caution though. I've had issues with topmostMsgWindow in mac builds, and had to switch to ww.activeWindow, otherwise the window gets blocked and is invisible. I did not test this use of the window here, it would be worth testing on a Mac build to make sure that the window does indeed appear as you expect.

The whole ending sequence of nsMsgMailboxParser is pretty poorly documented, and I see your point that the order was changed in another bug.
Attachment #8496575 - Flags: review?(kent) → review+
(In reply to alta88 from comment #5)
> 
> On top of that sort of lying to the user, we're actually regressing
> (progressing in lying) in the strange consensus/patch in bug 935934, where
> the filter log message is now always (not just for the imap move case) to be
> printed before the actual attempt of the action.
> 
> Honestly, we've lost a sense of best practices around here.

I think that you can make the case that logging before the attempted filter action has value, so that you know what is attempted, but it is really bad that the errors are not logged or reported to the user. See my comments today https://bugzilla.mozilla.org/show_bug.cgi?id=697522#c5 on a proposal to also log or report errors. There is so much that I dislike about logging, error handling, and async handling in filters that it is hard for me to make consistent arguments. The handling of async operations in filters is pretty random, and that makes it very difficult to reliably add logging after the async operation is completed.
Thanks, I'll note to watch for the mac case, although there are a number of GetTopmostMsgWindow calls in generic code so the assumption would lean to safe..

As for logging: a "before the action" attempt is debug level logging, while info level (for users) should be "upon final success" or "upon final failure".  The big problem is changing the logging to print "before the action" and not changing the phrasing to "About to.." or such.  This is very harmful to diagnosis and user confidence once they realize log messages are not trustworthy.  And having just an "About to" without final result is only of moderate diagnostic utility.  So I hope we reconsider bug 935934 some more.

Otherwise, bug 697522 is a significant renovation to get right and what to log a treatise in itself.  In js async isn't that hard as long as a callback is hauled around, or promises now; not as easy in cpp.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/359f6c0c65d1 -> FIXED
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.