537012 - "Create filter from message" fails from Smart Folders

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Kent James (:rkent)]

STR:

1) In Smart Folders inbox, include a number of different accounts
2) Select a message in an IMAP folder, while Smart Folder Inbox is selected in folder pane
3) Select Message/Create Filter From Message
4) Give the filter a unique name, like "8979323846"
5) Press OK

Expected results: Filter list dialog appears, with filter "8979323846" appearing
Actual results: Filter list dialog appears, but filter does not show

Variation 1: One user in #thunderbird reports that the filter list dialog does not appear. Manually editing filters, the filter does not appear. Trying to recreate the filter with the same name following the STR, gives error that filter already exists. (I get the "filter already exists" error as well on second try, though I always get the filter list dialog)

Additional observation: The "filter already exists" error does not appear when the STR is repeated after a restart, until the second application of the STR.

Comment 1

[Kent James (:rkent)]

This is an oft reported issue in GS. After creating the filter, the Filter List dialog should show the list for where the message was created, not something that is dependent on the selected folder.

Comment 2

[Marcus Münch]

I can confirm this for Thunderbird 3.0.x on Mac, too. It's not only an Windows issue. The only way to create new filters is currently to switch to the old "all folders" view and create it there.

Comment 3

[Kristof Elst]

Marcus, you can also create a filter by switching to the appropiate account's inbox and creating the filter from there.

Comment 4

[Marcus Münch]

Ok. This workaround sounds a bit better than mine for one does not have to switch the view.

Comment 5

[Kent James (:rkent)]

Here is my analysis of this.

The issue only appears with IMAP messages, not POP3 messages.

The folder used for storing these filters is set here http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1914. In particular, it relies on the accountKey property from the msgHdr to locate the original receiving account. That is set in POP3 from the X-Account-Key header. But in IMAP we do not set the X-Account-Key header, so the accountKey property is not set, and the initial folder selection in mailWindowOverlay fails. When that happens, the folder reverts to the first selected folder - which is the Smart Folders virtual folder in the Smart Folders case. That is the wrong folder, and can't store filters in any case, so the filter create fails.

A very easy partial solution would be, when the detection of the folder fails due to the missing accountKey, we could instead use the root folder of the folder where the message currently resides. This would however generate the wrong folder in cases where the message to be filtered has been moved to a different folder on a different server.

The more difficult correct solution is to communicate the original account somehow to the new message header created in nsParseMailbox.cpp  I need to look at ways to do that.

Another "solution" which is probably the correct thing to do given the current backend, is to disable the Create Filter From Message function for virtual folders (such as the Smart/Unified folders). This however would still leave the filter as created on the wrong server in the IMAP non-virtual case when the message has been moved. (There may be a previous bug somewhere that shows that when messages are moved by filters to a different server, then IMAP does not create the filter on the correct server for Create Filter From Message). So I don't see that this would be any better than the proposed partial solution.

Comment 6

[Kent James (:rkent)]

Here's a possible IMAP solution. The parser that sets the accountKey knows the database that the new message belongs in, and therefore should know the folder and server that the message is in. Maybe I can get the accountKey from that, and use it in cases where the X-Account-Key header is missing.

Comment 7

[Kent James (:rkent)]

Attachment: add accountKey to IMAP messages

This patch adds accountKey to the message header for all messages that don't have them, which means IMAP messages. It solves the issues of this bug for new messages, but not for existing messages. To help existing messages or after loss of the database, perhaps we should also change the fallback in mailWindowOverlay.js, so that if accountKey is missing, it uses the root folder for the message where it is currently located, rather than the first selected folder. But I would rather do that in a separate patch, as the issues are slightly different (plus that file is forked between TB and SM).

Comment 8

[Kent James (:rkent)]

I'm also requesting blocking-thunderbird3.1 for this bug. What that means here is "Even though I realize that you would not block the release on this bug, it is a very important bug that needs to get review attention. Now that you have a patch, we should really do whatever is needed to get this into 3.1"

Comment 9

[David :Bienvenu]

so why not just get the folder directly from the hdr if there is no account key?

Comment 10

[Kent James (:rkent)]

(In reply to comment #9)
> so why not just get the folder directly from the hdr if there is no account
> key?

Because that would be the folder where the message is now, not where the message was originally. If you move the message cross-account, then the filter created from that message would not work.

Thanks to jcranmer for pointing out this issue to me on IRC yesterday.

Comment 11

[Kent James (:rkent)]

But that is my suggested fallback for existing messages in comment 7.

Comment 12

[David :Bienvenu]

(In reply to comment #10)
> (In reply to comment #9)
> > so why not just get the folder directly from the hdr if there is no account
> > key?
> 
> Because that would be the folder where the message is now, not where the
> message was originally. If you move the message cross-account, then the filter
> created from that message would not work.

You're not saying that you would create a filter on the original pop3 account, in this case, and not the imap server that the message is on now, are you? That would be surprising.

Comment 13

[Kent James (:rkent)]

(In reply to comment #12)
> 
> You're not saying that you would create a filter on the original pop3 account,
> in this case, and not the imap server that the message is on now, are you? That
> would be surprising.

That is exactly what I am saying, because if you create the filter on the IMAP account, it would not work for that message. You have to create it on the POP3 account from which the message came.

Actually that is how I thought the existing code would work, since the moved POP3 message has the X-account-key header, but it did not because we included "account" (which is the database field for "accountKey") in the dontPreserveOn... lists for copying the message header. So I can see that in my patch I really need to remove the "account" from the dontPreserveOn... preferences to get this behavior.

But you are questioning whether we even want this behavior. The other use for accountKey that I see is to get the identity for the message in several places. Don't you think that the message identity is determined when it arrives, and shouldn't change because I decide to store or archive the message somewhere else? In the filter case, don't you think that "create filter from message" should create a filter that would actually work for incoming messages?

And now I reverse myself. I guess the counter argument would be that "Create a filter" should work for Manual messages, which would require it created for its current location (which would also be Local Folder for POP3 global-inbox messages, which I have argued in favor of in the past.)

Hmmm, this is getting more complex than I thought. So here is a proposal.

I still think that accountKey, if it has any value at all, needs to be the account that the message originally arrived on. I will leave the current patch to get it working om IMAP, but also remove "account" from the dontPreserveOn... lists, and also document the definition of accountKey in nsIMsgHdr.idl

It might make sense then to move that patch to a new bug, because I will also propose that I fix this bug by changing the behavior of mailWindowOverlay.js so that it gets the root folder from the existing location of the message, rather than the original message. For messages that are moved cross-account, that will result in Manual filters working but Incoming not. But it will also cause both Manual and Incoming filters to work on POP3 with global inbox, while currently those filters are created on the incomingServer and don't work on Manual (see  Bug 527585 -  Disable filter editing of deferred-from POP3 server filters).

Comment 14

[Kent James (:rkent)]

Comment on attachment 435251 [details] [diff] [review]
add accountKey to IMAP messages

I'm removing the review request until we decide on the proper plan.

Comment 15

[Kent James (:rkent)]

Attachment: Add filter to message's current folder

Here's the patch that uses the current message location as the folder.

This represents a change in behavior for POP3 global inbox users. Previously, filters would be created on the incoming server account (and then not work in manual context.) Now, they will be created on Local Folders, work on both incoming and manual context - but also apply to all incoming accounts, not just the original account. I think this is preferred behavior. It also solves the current bug, that is is works even for virtual (smart, unified) folders.

I'm not sure how to test this. Can we land it without a test?

Comment 16

[Kent James (:rkent)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=553786#c12 for a discussion of why I think that we should be pushing Local Folders in the UI, as is done in the current patch, for the Global Inbox case.

Comment 17

[David :Bienvenu]

Comment on attachment 435534 [details] [diff] [review]
Add filter to message's current folder

Sorry for the delay - why not simply fall back to the msg hdr's folder if we can't get the account key?

so then we'd have something like:

     if (!folder)
        folder = gFolderDisplay.selectedMessage.folder;

It does seem like the code that checks the default account seems a bit iffy, but I suppose that was more of an upgrade issue from when before we had account keys.

Also, news has per-newsgroup filters; did we use to create a server level filter before, or is that a change you're proposing?

Comment 18

[Kent James (:rkent)]

I don't understand the comment. The meat of this patch is these lines:

+        folder = gFolderDisplay.selectedMessage.folder;
+        // except for news, we define the filter on the account's root
+        if (!gFolderDisplay.selectedMessageIsNews)
+          folder = folder.rootFolder;

which seems to do exactly what you are requesting.

Comment 19

[David :Bienvenu]

(In reply to comment #18)
> I don't understand the comment. The meat of this patch is these lines:
> 
> +        folder = gFolderDisplay.selectedMessage.folder;
> +        // except for news, we define the filter on the account's root
> +        if (!gFolderDisplay.selectedMessageIsNews)
> +          folder = folder.rootFolder;
> 
> which seems to do exactly what you are requesting.

Sorry, yes, I misread that. You're right. My first comment still stands, however, afaict - "why not simply fall back to the msg hdr's folder if we
can't get the account key?"

Comment 20

[Kent James (:rkent)]

I guess what you are saying is to prefer the account key's folder over the message header's folder if possible, since the patch already does "why not simply fall back to the msg hdr's folder" but not "if we can't get the account key". I thought I addressed this reason in comment 15, but let me try to expand and clarify.

First, account key only exists in POP3 messages. So let me address what I think is proper behavior in POP3.

There is only a difference in behavior in two cases: if the POP3 message is using a global inbox, or if the POP3 message was moved from its original server.

In the first case, I am regularly replying to people who complain "I created a filter but it did not work" that the reason is because they created it on the incoming POP3 account, so it does not work in Manual context. The standard recommendation is to define the filter on Local Folders instead. From there, it works on both incoming and manual context. And that is what this patch is attempting to do for the recommended account. See also Bug 527585 - Discourage filter editing of deferred-from POP3 server filters.

For the second, you could certainly make the argument that, when possible, the user would prefer that filters be created on the incoming account rather than the destination account. The problem is that "when possible" is only for POP3, which creates an inconsistency in behavior. Unless you are going to go all the way and add Account Key support to IMAP (like my earlier patch did) this inconsistency in not desirable, paricularly given the first case. The problem with adding Account Key in imap is that it has ramifications for choosing of identity that goes beyond this bug, and should not be done lightly. Regardless of what we decide to do, the filter will only work in one of Incoming and Manual contexts. Rather than have it sometimes work only in Manual, and sometimes work only in Incoming, I would prefer a consistent response, which is only work in Manual.

Comment 21

[David :Bienvenu]

Comment on attachment 435534 [details] [diff] [review]
Add filter to message's current folder

OK...the manual running of the filters argument is compelling. The consistency between pop3 and imap is not, since the global inbox is really the important case, and that doesn't apply to imap, obviously. So, r+, and thx for the patch and explanation!

Comment 22

[Kent James (:rkent)]

Comment on attachment 435534 [details] [diff] [review]
Add filter to message's current folder

Checked in http://hg.mozilla.org/comm-central/rev/5b9374704924

Comment 23

[Kent James (:rkent)]

As much as I would like to see this annoying issue fixed in 3.0, this bug fix represents a change in behavior, which is probably beyond the scope of a stability patch for 3.0

Comment 24

[Kent James (:rkent)]

Quoting from my posting on tb-planning:

One of the changes that people need to be aware of in TB 3.1 is a change in the default location for filters. Previously, for situations with deferred-to Local Folders accounts (typically POP3 with a global inbox), then when you either ran Create Filter From Message or Tools/Message Filters, you were shown the account for the POP3 incoming server (the deferred-from account), and created filters did not work when run manually. Now, you are shown the Local Folders account, and those filters will work both for incoming and manual.

The problem with changing the default, is that people are going to do Tools/Message Filters and see the Local Folders account filters, and not the previous deferred-from account. They will then believe that all of their existing filters have disappeared.