350868 - saved search folders show up for 'run now' in messages filters, but doesn't work

Status: REOPENED

(MailNews Core :: Filters, defect)

Description

[JConley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060831 BonEcho/2.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060831 BonEcho/2.0b2

saved search folders show up as potential targets of 'run now' for message filters, however message filters will not work on 'saved search' folders

this is already done in another area: saved search folders are removed in the saved search builder (since saved searches cannot be based on saved searches)


Reproducible: Always

Steps to Reproduce:
1. create a saved search and a message filter
2. attempt to run that message filter manually on a saved search (nothing happens)




Expected Results:  
I wouldn't expect that message filters would work on saved searches, so I would expect that saved searches not show up as an option for manually running a messages filter via the 'run now' button.

Comment 1

[Magnus Melin [:mkmelin]]

Confirmed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b2) Gecko/20060831 Thunderbird/2.0a1 ID:2006083103

Comment 2

[JConley]

might consider with enhancements (bug 350871) and (bug 347190)

Comment 3

[ovidiu]

version 3.0a2 (2008072418) vista

-if select a virtual folder, tools/run filters.. is grayed, as expected ..
-in the filter manager in the folder dropdown it shows saved searches. with no
effect. wrong

so I guess it's still here

Comment 4

[:aceman]

Adding mode=filing to the folder picker seems to filter out virtual folders. But it also hides Outbox folder.
Is this the correct mode to set or do we have to define some new one?

Comment 5

[:aceman]

Attachment: patch

I am not sure if Seamonkey needs this too. It has a different picker I think.

The menu item Run filters on folder is also disabled when Outbox is selected so the patch behaves consistently with that.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 655680 [details] [diff] [review]
patch

Yeah, this seems okay to me, and consistent with the menu item…

Comment 7

[Kent James (:rkent)]

Comment on attachment 655680 [details] [diff] [review]
patch

Seems reasonable to me.

Comment 8

[:aceman]

Comment on attachment 655680 [details] [diff] [review]
patch

Is this relevant for Seamonkey?

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 655680 [details] [diff] [review]
patch

(In reply to :aceman from comment #8)
> Is this relevant for Seamonkey?

The file you are changing is TB-only (all files under mail/ are) so your patch is not affecting SM. The copy of our code doesn't include the line you are changing, so I guess it's best to fix this on our end in a follow-up bug since we need a different strategy of unknown complexity. For now I'll just mark this one TODO by adding it to our to-port meta bug.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 655680 [details] [diff] [review]
patch

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

Good stuff! Thanks aceman!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/8502d1fd3ccc

Comment 12

[:aceman]

Suddenly in TB18 I get the menu item "Run filters on folder" enabled on a Saved search folder. Can anybody see it too?
I have also tested a filter on such a folder and it worked.

Comment 13

[:aceman]

Also, this causes the folder list dropdown in the filter list to be empty for news accounts and throwing an exception. Running filters after the fact seems to work on news accounts/folders so the folder picker should show them.

How do I request backout of this patch?

Comment 14

[Magnus Melin [:mkmelin]]

I suppose you can attach a reverse of the patch, reopen and mark it checkin-needed.

You seriously need to get checkin rights yourself though. Please file a bug about that, cc me and a few more people who's done reviews for your patches so we can vouch for you.

Comment 15

[:aceman]

Checkin rights will not help me, only faster reviews :)

Comment 16

[:aceman]

Attachment: backout patch

Backout patch for this feature, please land.

Comment 17

[Ludovic Hirlimann [:Usul]]

Aceman that's where having test would help you and the reviewers ...

Comment 18

[:aceman]

Test?

Comment 19

[:aceman]

There should have been a test in existence before we started to work on this. So that we could know what is the correct behaviour. We still do not know it now so I can't create one now. E.g. a comment in the code says filters after the fact (manual) do not work on news so it tries to disable the folder picker (but fails to do so). However, I talked to rkent and jcranmer on IRC and we found those filters do work on news. I even found that they work on a Saved search folder and the menu item in main menu is suddenly enabled on such a folder. So I am not sure what the proper behaviour here is. Maybe if we find it out we can make a test.

Can't mozilla arrange for a person creating tests full time? :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b5b960f438ed

Leaving open for a better fix. Please close as WONTFIX if that's not the case.

Comment 21

[:aceman]

Can we add a new attribute on nsIMsgFolder for this? So that we can easily disable filters to run on types of folders if needed in the future.

Comment 22

[Kent James (:rkent)]

(In reply to :aceman from comment #21)
> Can we add a new attribute on nsIMsgFolder for this? So that we can easily
> disable filters to run on types of folders if needed in the future.

In bug 272490 comment 10 I asked whether we should try to avoid creating additional new attributes to nsIMsgFolder and nsIMsgIncomingServer, and instead use existing methods with a text identifier.

A few years back I added the inheritable string properties to the folder. Internally, it is currently only used to represent booleans (for applying junk processing, and applying incoming filters). Personally I would prefer if new attributes are added in that way rather than expanding the definitions of nsIMsgFolder and nsIMsgIncomingServer, but in the past I have been the only proponent of that position. Perhaps it would be a good tb-planning topic?

Comment 23

[:aceman]

Yes, or a GSoC project that I proposed on the wiki page.

Comment 24

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.