41720 - Filters: Need to handle rename or delete folder used in filter.

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect, P2)

Description

[laurel]

Using jun05 commercial build

We need to implement handling for when a user renames or deletes a folder which
is specified as a destination in a filter rule.

I suppose this becomes more complicated in the multiple account world.

Basic 4.x behavior (I'm using 4.7 win32 IMAP as my basis, I'm sure there are
more branches in the behavior):
1.  When user renamed a folder, alert dialog appeared asking user "Change rule
to reflect new folder location? (OK/Cancel)".   If user chose OK, the filter
rule was changed to show the new/renamed folder as destination.  If user chose
Cancel, rule was changed to Inbox as the filter action destination.
2.  When user deleted a folder, alert dialog appeared asking user "Disable
filter rule for this folder? (OK/Cancel)".  If user chose OK, filter rule was
disabled and the rule was changed to Inbox as filter action destination. If user
chose Cancel, folder was not deleted and rule was not changed at all.

I remember from 4.5 days that there were varying opinions as to the best way to
handle rename/delete folder in conjunction with filters.

I think we need some kind of error handling. Whatever you decide, please spell
out the rules.

Comment 1

[Alec Flett]

oh man. I forgot about this. Sure would be nice, but it's alot of work - for now
I'll just make it M18...

Comment 2

[lchiang]

It would be good to have a design for this in-place.  cc: jennifer

Or, have some way for users (as a workaround) to address this or at least a 
warning that their filter won't work and let them know that they have to 
manually change the filter.

Comment 3

[selmer (gone)]

At a minimum, this case should "do no harm." If the message is left in the Inbox 
then we've met that criteria and this becomes an enhancement request to do 
something better.  I was tempted to just mark this enhancement, but I'd prefer 
one of you to make that call.

Comment 4

[Alec Flett]

this is definately not an enhancement. If we don't fix the filter URIs, we run
the risk that the folder may be recreated, or that it will loose the message
entirely (i.e. dataloss!)

Comment 5

[lchiang]

mark nsbeta3 then.

Comment 6

[sol]

If we lose the message - then I'd mark this nsbeta3+.

If the filter fires and then recreates the renamed/deleted folder, then I'd 
mark this nsbeta3-/future. (It's annoying, but you didn't lose your mail.)

Comment 7

[laurel]

Current behavior (tried POP, assume IMAP same) is message lost.  Move
destination gets set to invalid/blank in the ui, rules.dat retains original path
to the now deleted folder. Message is not found in destination folder (even if
folder is still in trash) and is not returned to inbox either.  Results align
with bug #46876.

Comment 8

[lchiang]

+ per mail triage

Comment 9

[robinf]

Adding myself to CC List.

Comment 10

[Alec Flett]

ugh. This is looking really hard to fix. I think it would be best if we latered
this. I thought it would be critical, but now that I look at it I don't think
it's TERRIBLE.. as long as we don't crash and there is no dataloss. Can someone
see what happens when we rename a folder and then try to filter into it?

Comment 11

[laurel]

Well, using aug15 commercial build, the message still gets lost. I haven't yet
gone through testing for the fixes bienvenu made (#46876 and there's another one
or two,I think) involving moving to an invalid destination. 

Anyway, after renaming a folder which is a destination for a filter move, when
the filter fires you get an alert that the command failed because mailbox
doesn't exist (I'm using IMAP) and the filtered message is in neither the INBOX
or the destination/renamed folder. The message is lost.

Comment 12

[Alec Flett]

david, did you just fix the above error - where the message appears to be lost? 
If so then this bug should be safe to nsbeta3-

Comment 13

[David :Bienvenu]

yes, I did.

Comment 14

[Alec Flett]

renominating for nsbeta3- because the message should no longer be lost (and in 
fact the filter should now be disabled automatically)

Comment 15

[lchiang]

- per mail triage.  Laurel - pls confirm alec's items above.  If message is 
still lost, we will have to reassess this.

Thanks

Comment 16

[scottputterman]

Sorry for the extra mail. Removing mail4 keyword.

Comment 17

[Alec Flett]

reassigning my filter bugs to gayatrib..

Comment 18

[laurel]

nominating for consideration in next release

Comment 19

[scottputterman]

marking nsbeta1-

Comment 20

[laurel]

*** Bug 66127 has been marked as a duplicate of this bug. ***

Comment 21

[Matthew Tuck [:CodeMachine]]

Wow, this really needs to be fixed.  For me, this is one thing that prevents me
from using Mozilla MailNews.

But please, don't include that ridiculous 4.x dialog.

To start with, it says "Cancel", when it will not cancel anything.

To end with, I've never wanted to press "Cancel" in my life, so please nuke the
dialog entirely and just auto-update the filters.  Why would you not want to? 
If you wanted to move a bunch of messages out of the way and create another
folder with the same name, you can just create another folder and move the
messages!

Comment 22

[Matthew Tuck [:CodeMachine]]

The above refers to moving & renaming.

Regarding deleting, I see possible problems.  The following seem to be the two
options to me:

You could move with the folder, but mail that matches that filter will
temporarily go to the trash.

Or you could pop up a dialog and ask the user for a new folder (with the parent
of the trashed folder being the default), but then you would have to fix the
folder if you restored the folder from the trash.

AFAICS it's difficult to fix this properly without a really advanced undo
architecture, the sort of which I've never seen in an app before.

If you choose the latter option above, then it would be nice to include a cancel
action, but make it a real one please.

mpt, comments on any of this bug's UI?

Comment 23

[Matthew T (active 1999-2002)]

Re-adding `dataloss'. Scenario:
*   Have a `Comments' filter, which takes all messages from
    [e-mail] with `Additional Comments From' in the body, and
    places them in your `Bguzilla' folder.
*   Have a `Bugzilla Spam' filter, applying after the `Comments' filter, which
    takes any remaining messages from [e-mail] and deletes
    them.
*   Realize your mistake, delete your `Bguzilla' folder, and create a `Bugzilla'
    folder.
*   The `Comments' filter gets disabled, and *all* Bugzilla notifications you
    receive (even those with comments you wanted to read) begin to be deleted by
    the `Bugzilla spam' filter. Oops.

Ok, on to the UI.

For renaming a folder: There shouldn't be any UI at all. Filters (and other 
folder-specific things, such as where to keep a copy of sent messages) should not 
reference folders just by name. They should primarily reference folders by a 
unique id which is stored with the folder, and which does not change when you 
move/rename the folder. That way, when you rename the folder, even if it is an 
IMAP folder which you rename using a copy of Mozilla on someone else's computer, 
your Mozilla filters (and other settings) on your own computer will still work.

If a folder with the appropriate id does not exist, only *then* should Mozilla 
look for another folder with the same name -- to cater for the case where a 
misbehaving folder was deleted and a new one with the same name (but a different 
id) was created in its place.

For deleting a folder: use this alert.

+-----------------------------------------------------+
|:::::::::::::::::::::::::::::::::::::::::::::::::::::|
+-----------------------------------------------------+
|  .   The mail filter "Bugzilla spam" cannot move    | \ large
| /!\  messages to the "Bugzilla" folder, because the | > system
| """  folder cannot be found.                        | / font
|                                                     |
|      Until you delete or fix the filter, messages   | \ small
|      which it matches will be left in your Inbox.   | / system font
|                                                     |
|         (Delete Filter) (Fix Filter ...) ((  OK  )) |
+-----------------------------------------------------+

This alert should be presented as soon as a message is downloaded which would be 
processed by the filter, and no further messages should be downloaded until the 
alert is dismissed. That way, if the user wishes to fix the filter, they can do 
so and have the repaired filter immediately apply to the problem message and to 
any subsequent downloaded messages.

Clicking `OK' should not disable the filter; it should change the filter action 
to `do nothing'. This prevents (or should prevent) dataloss from filters further 
down the food chain -- like the case I described above -- since those filters 
will still not activate on the messages which this filter used to move.

Comment 24

[timeless]

Not OK. s/OK/Ignore/.

Comment 25

[jglick]

>Comments From Matthew Thomas (mpt) 2001-02-17 23:21

Sounds good!

Comment 26

[Matthew T (active 1999-2002)]

> Not OK. s/OK/Ignore/.

Sorry, that was my mistake. I drew a `caution'-type alert (with a `!' icon), as 
in `you have to make a choice right now'. But it should actually be a `stop'-type 
alert (with a `X' icon), as in `something just went wrong'. The user does not 
have to make a choice right now, they can just click `OK'; the `Delete Filter' 
and `Fix Filter ...' buttons are just added bonuses, since the user can do both 
those things elsewhere in the UI later on.

For a `caution'-type alert, Timeless is correct -- buttons should have 
descriptive names, to help the user make a quick decision (though `Disable' would 
perhaps be a better choice than `Ignore', since `Ignore' implies ignoring the 
alert itself). But for `note'- and `stop'-type alerts, the button text should 
always be `OK' for consistency -- even if the alert text is `Your house just 
burnt down'.

Comment 27

[scottputterman]

reassigning to naving

Comment 28

[laurel]

Update Comments for the triage team for dataloss keyword:
The potential for dataloss has been greatly diminished since the fix for bug
66992.  Last case for dataloss (mpt 2-17-01) is fixed by 66992 (verified 05-03-01).

In simple tests may30 when the target folder is gone we do still retain message
in Inbox, filter is disabled, and in the case of the recreated folder (66992)
the filter continues to fly.

Comment 29

[Håkan Waara]

*** Bug 85717 has been marked as a duplicate of this bug. ***

Comment 30

[Håkan Waara]

*** Bug 61732 has been marked as a duplicate of this bug. ***

Comment 31

[Håkan Waara]

Taking

Comment 32

[Navin Gupta]

hwaara, There are a lot of issues that will need to be taken care of in this
bug. Are you planning to address all of them ?

Comment 33

[Håkan Waara]

This is my plan:

First fix. (I will do this)
Instead of disabling the filter if a target folder is not found, display the
dialog mpt suggests with the buttons "Delete Filter" and "Ignore Filter".

Second fix. (Hand over bug to naving)
When renaming/deleting/moving a folder, check if any filters is watching this
folder and pop up a dialog.

This way we'll prevent dataloss in two cases!  Does that sound good to you?

Comment 34

[Håkan Waara]

Ignore my last comment and plan.

Naving: yes, I can fix all cases - assuming you mean all of those that mpt
describes.  I think I can fix "Fix Filter...", "Delete Filter" and "Disable
Filter" buttons.

Naving, is this OK?

Comment 35

[Navin Gupta]

I think mpt suggestion for Delete will be a lot of work especially Fix 
Filters... A better and more easier way to deal with deleting of folders 
is to warn the user (popup alert) that he has deleted a filter folder and he 
should fix the filter otherwise messages will go to inbox. 
This alert should pop-up at the time of deletion of folder.

hwaara, are you just talking about front end.  

Comment 36

[Matthew T (active 1999-2002)]

There are four parts to this bug, all of which should be fixed.
(1) Add the alert which I drew, but with only the `OK' button. Clicking `OK'
    sets the filter's action to `do nothing' (which also needs to be reflected
    in the Edit Filter dialog). It does *not* disable the filter.
(2) Add the `Delete Filter' button to the alert. Clicking it deletes the
    filter.
(3) Add the `Fix Filter ...' button to the alert. Clicking it closes the alert,
    and opens the `Edit Filter' dialog (with `do nothing' selected), so the
    user can choose a new folder to move the messages to, or do whatever else
    they want to fix the filter.
(4) Make the mail/news back-end use a primary key for folders which is not the
    name of the folder, such that I can rename an IMAP folder using Mozilla on
    computer A, and my filters in Mozilla on computer B will still work.

Håkan could probably fix (1), and maybe even (2) and (3), but (4) will require 
some heavy lifting on the back-end side.

Comment 37

[timeless]

what happens if 3 filters reference one folder?

Comment 38

[Navin Gupta]

taking, I am working on a fix. 

Comment 39

[Håkan Waara]

Navin: What will your fix include?  There are many different aspects of this bug...

Comment 40

[Navin Gupta]

The majority of the work is in the back-end area. I may do the FE part also
otherwise i will give it back to you. 

Comment 41

[Navin Gupta]

Attachment: proposed fix, v1

Comment 42

[Navin Gupta]

I have added a boolean on nsIMsgFolder that helps us to keep track on what
folder the filter is set. When the user does a rename, then we just change 
the filter destination to the renamed folder. Upon deleting a folder 
under trash i.e the folder will be no longer there, the user is alerted
that the filter will be disabled. 

There is also some code that passes msgWindow in the mailparsing and filtering
code. I have left it in there because msgWindow always helps us in 
telling user about the state of app. It could be used in future. 

Note, this implementation will bring the current behavior close to 4x behavior.
We can spin off another bug if someone wants to do what mpt has suggested
for deleted folders. 

Comment 43

[Håkan Waara]

Cool. I'll test this patch some...

Comment 44

[David :Bienvenu]

Overall, this looks good. However, I would rather not add another boolean to
nsIMsgFolder - it already has way too many. I would prefer we did it the way 4.x
did it, which was basically to ask the filter list if it had a filter that
pointed to any of the folders being deleted before deleting the folders. This
might also make it possible not to have to change FindSubFolder the way you have.

Comment 45

[Håkan Waara]

One problem with this is that if you have the "ask me before deleting a folder"
pref on, and you delete a folder that a filter points to, you will have TWO
dialog boxes popping up on you.  Not very nice.

Comment 46

[Navin Gupta]

jatin, this it the alert I was talking about 

All the filters having destination 'folderName' will be disabled.

Comment 47

[Navin Gupta]

There is a small change

All the filters having destination 'folderName' are disabled.

Comment 48

[Jatin Billimoria]

My suggested wording:
"Filters that are affected by deleting this folder are now disabled."

Comment 49

[Navin Gupta]

fix checked in. 

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

a=dbaron for trunk checkin during 0.9.3 closure for checking in the file you
missed yesterday.

(Hmmm.  Why are the review and super-review not noted on the bug?)

Comment 51

[laurel]

*** Bug 93831 has been marked as a duplicate of this bug. ***

Comment 52

[laurel]

OK using aug29 commercial trunk builds: win98, mac OS 9.0, linux rh6.2
General scenarios tested, work ok for IMAP and POP.  Any specific cases found to
have a problem with the rename/delete of filter involved folders will be logged
separately.

Comment 53

[Simon Spiegel]

I don't know if this is supposed to be a feature or if the fix is not yet
complete, but using Build 2001082703 on Win98 the filter just gets fixed without
informing the user at all. No dialog or anything is appearing. Although it is
nice that the filter gets fixed this is *very* bad behaviour IMO.

Comment 54

[robinf]

I agree with the previous comment. If I rename or move a folder that's used by a
filter, the filter automatically updates to use the renamed or moved folder.
This is the desired behavior, but we should inform the user what's happening. 

"Filters that are affected by renaming this folder will be updated."
"Filters that are affected by moving this folder will be updated."

Comment 55

[Håkan Waara]

No, we shouldn't inform the user - that is redundant. When the user makes a
filter point to a folder, it doesn't really matter what the folder's name is.

If the user chooses to rename "Work" to "Work email", this would have the exact
same function and a alert to notify would be plain annoying.

Comment 56

[laurel]

FYI:  I logged a bug about when the alert happens for deleting a folder -- see
bug 93968.  I think deleting a folder should alert the user when they del/move
to trash.  

As for the general rename folder (not to trash) scenario I did not log any bug,
so if someone feels strongly that we should have an alert on rename please log a
separate bug. As you can see this bug report is essentially closed, so a better
place to make sure the issue gets attention is in a new report.