219122 - Forgetting to select a target folder causes new filter not to be created.

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Mark Banner (:standard8)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030827
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030827

If you forget to select a target folder whilst creating a new filter, the filter
does not get created once the target folder has been selected.

Reproducible: Always

Steps to Reproduce:
1. Select Tools|Message Filters
2. Select New
3. Enter name "Test" (for example)
4. After "Subject" "contains" enter "test" (or any other string)
5. Check "Move to Folder"
6. Select "OK"
7. A dialog is presented that says "You must select a target folder"
8. Select "OK"
9. Select a target folder.
10. Select "OK"

Actual Results:  
The "filter rules" dialog disappears, no filter is created. msgFilterRules.dat
is not updated. Restarting mozilla (or message filter dialog) does not solve the
problem either

Expected Results:  
A new filter should have been created with the required details.

Comment 1

[Joe Infla]

*** This bug has been marked as a duplicate of 206627 ***

Comment 2

[Joe Infla]

confirmed in 1.6

Comment 3

[Mark Banner (:standard8)]

FYI: Just upgraded to 1.6 and it still exists for me.

Comment 4

[Mark Banner (:standard8)]

I have a potential bug fix for this, but need to test it and I'm away for the
next few days. Will upload it when I have time and have read all the "hacking"
information.

Comment 5

[Mark Banner (:standard8)]

Attachment: FilterEditor.js fix

Note: this attachment requires the v3 patch from bug 233091 to be applied
before the patch.

Bug fix for test case above, also for the following test case:

1. Ensure a filter already exists.
1. Select Tools|Message Filters
2. Select Edit
3. Deselect all actions
4. Select OK
5. Acknowledge Failure
6. Select Cancel
7. Select Filter just edited
8. Select Edit

Failure Result: no actions are selected but they should have been.

Result with fix: actions are selected as they were previously.

Comment 6

[Mark Banner (:standard8)]

Ok, not quite a dependency, but adding link to bug 233091 to show it needs to be
checked in before this one. Feel free to remove it if I've got dependencies wrong.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 143226 [details] [diff] [review]
FilterEditor.js fix

Requesting review - have checked test cases and pre-checkin tests against
latest trunk.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 143226 [details] [diff] [review]
FilterEditor.js fix

>+  if (!gMoveToFolderCheckbox.checked &&
>+      !gChangePriorityCheckbox.checked &&
>+      !gLabelCheckbox.checked &&
>+      !gMarkReadCheckbox.checked &&
>+      !gMarkFlaggedCheckbox.checked &&
>+      !gDeleteCheckbox.checked &&
>+      !gWatchCheckbox.checked &&
>+      !gKillCheckbox.checked &&
>+      !gDeleteFromServerCheckbox.checked)

My personal taste would be |if (!(... || ...))|.

Comment 9

[Mark Banner (:standard8)]

Attachment: FilterEditor.js fix v2

Incorporated comments from comment 8 - I agree it looks better.

Comment 10

[Mike Cowperthwaite]

Mark, you might get quicker results on your patch if you ask someone besides 
Seth for review.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 143304 [details] [diff] [review]
FilterEditor.js fix v2

No review from sspitzer since 2004-03-08

Comment 12

[Mark Banner (:standard8)]

Attachment: FilterEditor.js fix v3

Updated patch to accomodate latest changes in code. Retested to same as before.

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 149716 [details] [diff] [review]
FilterEditor.js fix v3

>@@ -387,7 +388,11 @@
> {
>   var isNewFilter;
>   var filterAction; 
>+  var targetUri;
> 
>+  // All checks for correct filter details must be performed before
>+  // creating/editing the new gFilter. Otherwise problems occur with
>+  // the filter. See bug #219122

|Otherwise problems occur with
+  // the filter. See bug #219122|
might not be needed.

>   var filterName= gFilterNameElement.value;
>   if (!filterName || filterName == "") 
>   {
>@@ -398,6 +403,50 @@
>     return false;
>   }
> 
>+  if (!(gMoveToFolderCheckbox.checked ||
>+        gChangePriorityCheckbox.checked ||
>+        gLabelCheckbox.checked ||
>+        gJunkScoreCheckbox.checked ||
>+        gMarkReadCheckbox.checked ||
>+        gMarkFlaggedCheckbox.checked ||
>+        gDeleteCheckbox.checked ||
>+        gWatchCheckbox.checked ||
>+        gKillCheckbox.checked ||
>+        gDeleteFromServerCheckbox.checked))

It bitrotted again, due to r1.85:
|gFetchBodyFromServerCheckbox.checked| addition :-|

>+  {
>+    if (gPromptService)
>+      gPromptService.alert(window, null,
>+                           gFilterBundle.getString("mustSelectAction"));
>+    return false;
>+  }
>+
>+  if (gActionTargetElement)
>+    targetUri = gActionTargetElement.getAttribute("uri");
>+  if (gMoveToFolderCheckbox.checked &&
>+      (!targetUri || targetUri == ""))

|if (gMoveToFolderCheckbox.checked| should stay the first test, before |if
(gActionTargetElement)|:
|targetUri| won't be used if |gMoveToFolderCheckbox| is not checked.

>+  {
>+    if (gPromptService)
>+      gPromptService.alert(window, null,
>+                           gFilterBundle.getString("mustSelectFolder"));
>+    return false;
>+  }
>+
>+  if (!gActionPriority.selectedItem) 
>+  {
>+    if (gPromptService)
>+      gPromptService.alert(window, null,
>+                           gFilterBundle.getString("mustSelectPriority"));
>+    return false;
>+  }
>+
>+  if (!gActionLabel.selectedItem) 
>+  {
>+    if (gPromptService)
>+      gPromptService.alert(window, null,
>+                           gFilterBundle.getString("mustSelectLabel"));
>+    return false;
>+  }

Same: these two should be |if (xyz.checked && !yyy.selectedItem)|.

Then, Label and Priority are lists with default values:
You should be able to remove these tests altogether ! (as the
|gJunkScoreCheckbox.checked| case is)
I don't know if this applies to Folder too ??

>+
>   if (!gFilter) 
>   {
>     gFilter = gFilterList.createFilter(gFilterNameElement.value);

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)

> Then, Label and Priority are lists with default values:
> You should be able to remove these tests altogether ! (as the
> |gJunkScoreCheckbox.checked| case is)
> I don't know if this applies to Folder too ??

Answers are: remove L and P; keep F :->


[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040608] (<-- 1.7rc3 !)
(W98SE)

Confirming bug on Windows...

Comment 15

[Mark Banner (:standard8)]

Assigning to myself (after conversation with Asa) as I'm working on this.

Comment 16

[Mark Banner (:standard8)]

Attachment: FilterEditor.js fix v4

Updated to incorporate latest changes and gautheri's review comments.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 156298 [details] [diff] [review]
FilterEditor.js fix v4

Then, you should remove
{{
/mailnews/base/search/resources/locale/en-US/filter.properties, line 2 --
mustSelectPriority=You must select a priority.
/mailnews/base/search/resources/locale/en-US/filter.properties, line 3 --
mustSelectLabel=You must select a label
}}
too ;-)

Previously, you used to remove the
{{
535   if (gFilter.actionList.Count() <= 0)
}}
block.
Does it have to be kept now ??

*****

Note that I'm not an actual reviewer; I jumped in to "r-" ... but I can't give
a r+ when the time comes. (see <http://www.mozilla.org/owners.html>)

Comment 18

[Mark Banner (:standard8)]

Attachment: Filter fix v5 (SM Trunk part) [Checked in: Comment 22]

Updated as per Gautherie's comments - had forgotten about if
(gFilter.actionList.Count() <= 0) & didn't know about the other.

Hopefully good for review now. ;-)

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> Created an attachment (id=156373)
> Filter fix v5
> 
> Hopefully good for review now. ;-)

It looks good to me...

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 156373 [details] [diff] [review]
Filter fix v5 (SM Trunk part)
[Checked in: Comment 22]

I guess:
*it (should) applies to v1.7.x branch !
*it will then be automatically included in aviary branch ? (if needed)

Mark:
Do you have cvs checkin rights, or do you need Neil(!?) to do it for you ?

Comment 21

[Mark Banner (:standard8)]

I have no check-in rights, so if someone can do that for me it'll be great.
Thanks in advance.

Comment 22

[[e-mail]]

Fix checked in to the trunk, but I can't help with branches, try bienvenu.

Comment 23

[David :Bienvenu]

the patch doesn't cleanly apply to the aviary branch...

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> the patch doesn't cleanly apply to the aviary branch...

I don't know much about Aviary (could you post any hint on what is out of sync ?);
then 'gautheri: approval1.7.3?' moved on "needed" patch from 233091 first...

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <FilterEditor.js> ++ (SM v1.7.x branch part)

Filter fix v5 (SM Trunk part), "back-ported" to v1.7(.3) branch.

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 156823 [details] [diff] [review]
(Bv1) <FilterEditor.js> ++ (SM v1.7.x branch part)

Keeping
{{
Filter fix v5 (SM Trunk part) [Checked in: Comment 22]		 patch	       
2004-08-17 14:28 PDT	neil.parkwaycc.co.uk: review+
bienvenu: superreview+
}}

Comment 27

[Mike Kaply [:mkaply]]

Comment on attachment 156823 [details] [diff] [review]
(Bv1) <FilterEditor.js> ++ (SM v1.7.x branch part)

For this to go in 1.7, it can't change a DTD file. Obviously this change
doesn't have a negative impact, but I don't want the DTD file checked into 1.7
at all.

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) <FilterEditor.js> ++ (SM v1.7.x branch part) [Checked in: Comment 33]

(Bv1) <FilterEditor.js> ++ (SM v1.7.x branch part), without the not-mandatory
.properties ("dtd") file change.

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 156841 [details] [diff] [review]
(Bv1a) <FilterEditor.js> ++ (SM v1.7.x branch part)
[Checked in: Comment 33]

Keeping
{{
(Bv1) <FilterEditor.js> ++ (SM v1.7.x branch part)	 patch	       
2004-08-23 13:06 PDT	mkaply: approval1.7.3+
gautheri: review+
gautheri: superreview+
}}

I think I remember that Aviary branch is synchronized with v1.7 branch
"automatically",
or should I request approval for explicit checkin ?

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 156841 [details] [diff] [review]
(Bv1a) <FilterEditor.js> ++ (SM v1.7.x branch part)
[Checked in: Comment 33]

(In reply to comment #23)
> the patch doesn't cleanly apply to the aviary branch...

David:
{{
I think I remember that Aviary branch is synchronized with v1.7 branch
"automatically",
or should I request approval for explicit checkin ?
}}
Well, asking for 'approval-aviary=?' again , on the new v1.7 patch this time:
dismiss it and/or explain if this is not needed/appropriate.
Thanks.

Comment 31

[David :Bienvenu]

I don't really know if they are synchronized automatically.  Dbaron might be
doing that, or it might have been a one time thing...

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 156841 [details] [diff] [review]
(Bv1a) <FilterEditor.js> ++ (SM v1.7.x branch part)
[Checked in: Comment 33]

not relevant to aviary.

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 156841 [details] [diff] [review]
(Bv1a) <FilterEditor.js> ++ (SM v1.7.x branch part)
[Checked in: Comment 33]


Check in: { 1.80.2.2 <[e-mail]> 2004-08-23 16:30 }

Comment 34

[Bernardo Carvalho]

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