183614 - Create Filter from Message doesn't handle no specified action (main filter ui does)

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[laurel]

Using dec4 commercial trunk

When you access the filter rules ui via Create Filter from Message (prefilled)
we set the Move To Folder action as enabled by default.  If you disable that
action then forget to enable at least one action and OK the rules dialog, we
don't warn the user to set an action.  This differs from the current handling if
accessing filter rules through the main filter list ui; there we throw an alert
dialog for user to select an action.

Steps:
1.  Select a mail message (not in Local Folders), use Create Filter from Message
from either Message menu or message header popup.
2.  In the rules dialog notice the default action is enabled -- Move to Folder.
 Uncheck/disable this default action. Don't enable any (other) actions.
3.  OK/confirm the rules dialog.  

Result:  No alert, lets you close the dialog.  Examining the stored filter, it
shows it left the default action enabled, but it is then left at the default
destination -- the relative account, i.e. no destination folder is set.

Expected:  An alert telling you to set at least one action.

Comment 1

[jglick]

User should get the same alert they get when creating a filter through the 
main filter ui; "You must select at least one filter action."

Comment 2

[Samir Gehani]

Mail triage team: nsbeta1+/adt3

Comment 3

[Cavin Song]

Reassigning.

Comment 4

[Cavin Song]

It's caused by the default action entry (MoveToFolder) that's added under a 
'prefill' condition which is the scenario for this bug. So the action count is 
at least one in this case even if you disable the (default) MoveToFolder action, 
and the code only alerts you if the action count is 0 or less (which is never 
true in this case). Working on a solution now.

Comment 5

[esther]

Using build 20030228 on winxp, linux and mac osx this is still a problem.  
I don't get an error when disabling the move to option.
The filter is listed and selected when I view my filters.
The filter is showing the move to is enabled and the default location is at the
 server level even though I had changed my location to local folders before I
disabled the option to move.  
Running the filter does nothing because the location is at the server level.

Comment 6

[Cavin Song]

Attachment: Proposed patch, v1.

Remove the default action after the dialog is initialized.

Comment 7

[(not reading, please use [e-mail] instead)]

this doesn't seem like the right fix.

what's going on in saveFilter()?

why isn't this code doing the right thing?

 if (gMoveToFolderCheckbox.checked)
  {
    if (gActionTargetElement)
      targetUri = gActionTargetElement.getAttribute("uri");
    if (!targetUri || targetUri == "") 
    {
      str = gFilterBundle.getString("mustSelectFolder");
      window.alert(str);
      return false;
    }

hmm, note that targetUri is not declared locally.  we should do that.

var targetUri;
if (gActionTargetElement)
      targetUri = gActionTargetElement.getAttribute("uri");

Comment 8

[(not reading, please use [e-mail] instead)]

have you noticed that if you launch the 3 pane, and immediately try to create a 
filter from the message, the "move" checkbox is not checked, and you can't 
check it?

the second time, it works fine.

cavin / shuehan, if you are seeing this? 

Comment 9

[(not reading, please use [e-mail] instead)]

cavin explained what's going on over aim.

he's trying to see if 

+            // Remove the default action added above now that the dialog is 
initialized.
+            gFilter.clearActionList();

will fix it.

Comment 10

[Cavin Song]

Attachment: Proposed patch, v2

gFilter.clearActionList() does the same trick. Thanks to Seth for pointing me
to this handy call.

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 116012 [details] [diff] [review]
Proposed patch, v1.

clearing review request

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 116158 [details] [diff] [review]
Proposed patch, v2

r/sr=sspitzer

nice work, cavin.

Comment 13

[Cavin Song]

Fix checked in.

Comment 14

[esther]

Using trunk build 30030314 on winxp, macosx and linux this is fixed. Verified.