Last Comment Bug 742155 - If both "Copy to folder" and "Move to folder" is requested in single filter rule, folder path of "Move to folder" action is not updated when folder location is changed by move/rename of mail folder
: If both "Copy to folder" and "Move to folder" is requested in single filter r...
Status: RESOLVED FIXED
: dataloss
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- major (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 541416
Blocks: 755658
  Show dependency treegraph
 
Reported: 2012-04-03 20:07 PDT by WADA
Modified: 2012-05-16 02:21 PDT (History)
17 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-


Attachments
patch (999 bytes, patch)
2012-04-10 14:06 PDT, :aceman
standard8: review+
Details | Diff | Splinter Review

Description WADA 2012-04-03 20:07:33 PDT
Build ID:
> Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120403 Thunderbird/14.0a1

If both "Copy to folder" and "Move to folder" is requested in single filter rule, folder path of secondly specified action is not updated when folder location is changed by move/rename of mail folder.

[Steps to reproduce]
(0) Folder structure, filter rules 
      ABC
        F1, F2, F3
    Rule-1 : Copy to ABC/F1, Move to ABC/F2 
    Rule-2 : Copy to ABC/F2, Move to ABC/F3 
    Rule-3 : Copy to ABC/F3, Move to ABC/F1 
    Rule-4 : Move to ABC/F1, Copy to ABC/F3 
(1) Rename ABC to XYZ
    Rule-1 : Copy to XYZ/F1 (Good), Move to ABC/F2 (BAD!, not updated)
    Rule-2 : Copy to XYZ/F2 (Good), Move to ABC/F3 (BAD!, not updated)
    Rule-3 : Copy to XYZ/F3 (Good), Move to ABC/F1 (BAD!, not updated)
    Rule-4 : Move to XYZ/F1 (Good), Copy to ABC/F3 (BAD!, not updated)
Note: Bug 735940 is already fixed, so first action is corrrectly updated.

Fortunately, only one Copy action is currently permitted in message filter UI, so "loss of folder path setting in a single filter rule of Copy&Move" occurs on one action only. However, if multiple Copy actions will be permitted, user will loose many folder path settings in a filter rule at same time.
Comment 1 :aceman 2012-04-04 00:10:12 PDT
Nice flag and dependencies games WADA!:)

Seriously, when you write:
> Rule-1 : Copy to XYZ/F1 (Good), Move to ABC/F2 (BAD!, not updated)

what is the resulting value of "Move to" ?

When I had Copy to ABC/F1 and Move to ABC/F1/F2, then moving ABC to other place caused Move to become empty. What value did you get?

Anyway, confirming there is a problem and taking.
Comment 2 :aceman 2012-04-04 02:43:57 PDT
standard8, it is not yet sure if this is in TB12. WADA revoked all the dependencies and flags.

Both of us confirmed it only on TB14 so far.
Comment 3 WADA 2012-04-04 03:01:47 PDT
(In reply to :aceman from comment #1)
> Seriously, when you write:
> > Rule-1 : Copy to XYZ/F1 (Good), Move to ABC/F2 (BAD!, not updated)
> what is the resulting value of "Move to" ?
> When I had Copy to ABC/F1 and Move to ABC/F1/F2, then moving ABC to other
> place caused Move to become empty. What value did you get?

What's wong in following filter rules? 
    Rule-1 : If From=from1, Copy to ABC/F1, Move to ABC/F2 
    Rule-2 : If From=from2, Copy to ABC/F2, Move to ABC/F3 
    Rule-3 : If From=from3, Copy to ABC/F3, Move to ABC/F1 
    Rule-4 : If From=from4, Move to ABC/F1, Copy to ABC/F3 
If mail1, mail2, mail3, mail4 has from1, from2, from3, from4 respectively, mail1 is copied to F1 and moved to F2, mail2 is copied to F2 and moved to F3, mail3 is copied to F3 and moved to F1, mail4 is copied to F3 and moved to F1.

Even when move action appears first and copy action appears second in msgFilterRules.dat(user sets in this order at UI, and Tb shows in this order at UI), Copy is executed first and Move is second. 
And, "Move to" implies "Stop Filter Execution" on a mail as "Delete" does, because mail doesn't exist in Inbox any more by "Move" or "Delete".
"Move", "Delete", "Stop Filter Execution" is final action in application of filter rules on a mail, so only one of three actions should be permitted at UI to avoid confusion. And, bug 541416 exists for it.

Please note that who changed
  from  Rule-1 : If From=from1, Copy to ABC/F1, Move to ABC/F2 
  to    Rule-1 : If From=from1, Copy to XYZ/F1, Move to ABC/F2(BAD!,not updated)
  when ABC/F1 is renamed to XYZ/F1 and ABC/F2 is renamed to XYZ/F2 at once,
is Tb, not me.
I merely renamed folder ABC/F1, ABC/F2, ABC/F3 to XYZ/F1, XYZ/F2, XYZ/F3 respectively, at same time, by single "rename ABC XYZ" operation via Tb's UI.

(In reply to :aceman from comment #1)
> Nice flag and dependencies games WADA!:)
(In reply to :aceman from comment #2)
> WADA revoked all the dependencies and flags.

It was never "revoke by me", although "games by me" may be right.

I created this bug by "Clone This Bug" of bug 735940, then Tb automatically copied them, unexpectedly. Because I confirmed this bug in Tb trunk only and I didn't check with Tb 11, 12, 13, I don't know this bug occurs in Tb 11, 12, 13 or not. So I had to remove them.
I should have created this bug as purely new bug.
Comment 4 :aceman 2012-04-04 07:45:39 PDT
> to    Rule-1 : If From=from1, Copy to XYZ/F1, Move to ABC/F2(BAD!,not updated)

I see what you mean now. "Move to" value of ABC/F2 is only seen in the msgfilterrules.dat file. The UI filter editor shows the target folder as blank. Because ABC/F2 no longer exists, ABC was renamed to XYZ.
Comment 5 :aceman 2012-04-04 12:18:14 PDT
Could you try if it appear in TB 11,12,13?
Comment 6 WADA 2012-04-07 17:58:58 PDT
This bug was observed with Tb 11.0.1 on Win-XP too.
Comment 7 :aceman 2012-04-10 14:06:34 PDT
Created attachment 613755 [details] [diff] [review]
patch

The code seems to wrongly assume there can only be one of "copy to" or "move to" actions but not both and it shortcuts out of the loop when the first one is found. So it does not update the other action even if it exists.
So just removing the shortcut (break) solves the problem for me.
WADA, are you able to test the patch?

The shortcut code (break) seems to be very old, from before HG repository. So maybe it hasn't worked for ages. Maybe in the past it wasn't possible to use both copy and move in one filter so the assumption was correct.

Dbienvenu, should I just remove the 'break' as in the patch, or is it worth to keep it but count to two (copy+move) before break? But that may essentially amount to no break at all because most filters do not have both so the code will loop through all actions before finding out there is no second copy/move. Is there any other optimization possible (but a safe one)?
Comment 8 :aceman 2012-04-10 14:13:37 PDT
standard8, see comment 7. If I understand hg blame correctly (on mailnews/base/search/src/nsMsgFilterList.cpp), this is not a recent regression. It surely would be nice to get into branches due to dataloss, but not because of a recent regression status. (Mentioning this due to your setting of tracking-thunderbird12: ?)
Let's see what David Bienvenu thinks.
Comment 9 Magnus Melin 2012-04-10 22:12:38 PDT
I guess you can copy several times, but what would several moves even mean?
Comment 10 :aceman 2012-04-11 00:52:06 PDT
Currently the filter editor UI does not allow multiple Copy actions. Only one of each action (with exception of Tag and Forward). I am currently mucking around this in bug 541416, you can comment there and propose allowing multiple Copies. But I am not the one to see if the backend would cope with that.
Comment 11 Magnus Melin 2012-04-11 01:18:48 PDT
I meant you can't remove the break at least for moves, since that can't really do anything reasonable - if the message is moved it's not conceptually possible to move it (from the same place) again.
Comment 12 :aceman 2012-04-11 03:35:10 PDT
Why? I understand the break is just an optimization, assuming there is only one move/copy (as it treats them the same). And that is not true. You can have both. Now I do not know if it is OK to have both, but the UI allows it so far.

The patch just removes the break so that the loop iterates over all the actions and updates the path in all of them (all copy/move rules) regardless of how many it finds and whether it is allowed to have so many. What is wrong with that?
Comment 13 Magnus Melin 2012-04-11 04:20:46 PDT
I'm just saying a message can't be moved twice from the same destination. It's obviously not there for the second pass. (Not a problem for copy.) I don't know much about the actual code there :)
Comment 14 :aceman 2012-04-11 05:48:23 PDT
It is true that there can only be one Move. But the code wants to find all Move AND Copy rules and therefore there can be 0, 1 or 2 of them.
Comment 15 Magnus Melin 2012-04-11 06:05:01 PDT
Exactly, you want to count copy-rules separately and moves separately, and for moves there can be only one.
Comment 16 :aceman 2012-04-11 08:03:29 PDT
It looks like there can also only be one copy too.
But why would I want to count the Moves? I still need to find the Copies, so no break shortcut is possible.

I could only break if I found 1 Copy AND 1 Move, which is exactly what I ask in comment 7. And this break would almost never be hit as most filters do not have both actions. So just looping through all actions without counting anything is almost the same, just easier.
Comment 17 :aceman 2012-04-11 23:47:28 PDT
So if the patch in bug 541416 passes and allows unlimited Copy actions then there is nothing we can count here. We must loop through all actions in the filter and fix any Copy/Move found. They can be in any order and mixed with other actions.
Comment 18 Mark Banner (:standard8, limited time in Dec) 2012-04-17 02:32:39 PDT
(In reply to :aceman from comment #8)
> standard8, see comment 7. If I understand hg blame correctly (on
> mailnews/base/search/src/nsMsgFilterList.cpp), this is not a recent
> regression. It surely would be nice to get into branches due to dataloss,
> but not because of a recent regression status. (Mentioning this due to your
> setting of tracking-thunderbird12: ?)

Thanks for the info, that's useful to know.
Comment 19 :aceman 2012-04-19 11:02:19 PDT
Comment on attachment 613755 [details] [diff] [review]
patch

But can we get it at least into TB14? It is not a regression but still corrupts users' filters. And will do more so after bug 541416 has landed.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-04-23 16:10:05 PDT
http://hg.mozilla.org/comm-central/rev/9aace1fa76ff
Comment 21 :aceman 2012-04-24 05:11:28 PDT
WADA, the patch was checked in. Can you please verify the fix in tomorrows build of TB14/15? Thanks.

Note You need to log in before you can comment on or make changes to this bug.