Renaming a message filter doesn't check whether the new name already exists

VERIFIED FIXED

Status

MailNews Core
Filters
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: Sven Grull, Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

Trunk
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030513
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030513

Renaming a message filter doesn't check whether the new name already exists.

Reproducible: Always

Steps to Reproduce:
1. Take a mail account with at least two filter rules.
2. Rename one filter to match the name of the other filter.
Actual Results:  
One of the filters disappears from the list but is still stored in
msgFilterRules.dat.

Expected Results:  
An alert should pop up like when I try to create a new filter using an existing
filter name.

This is a similar problem as in bug 60528.
This bug can be seen on Linux (Mandrake) as well, under the following cvs build:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040821

Can't confirm as I don't have permissions.
Created attachment 157273 [details] [diff] [review]
Filter List Dialog Patch

This patch causes the Filter List Dialog to pass the current filter name AND
list to the filter editor when editing a filter. When the filter editor
attempts to save the filter, it will have the filter list present to check
against.

Also modified window.alert and window.confirm to use the newer prompt service
code.
Comment on attachment 157273 [details] [diff] [review]
Filter List Dialog Patch

Sorry, this patch doesn't work. It doesn't cater for the case where the user
puts the name back to the original before the edit. Will be able to provide a
revised fix in a week or so (I'm on holiday from tomorrow).
Attachment #157273 - Attachment is obsolete: true
Created attachment 157937 [details] [diff] [review]
Filter Names Patch v2

This patch is based around the first one (passes filter list to the edit
dialog). 

Now when we try and save the filter, if a duplicate exists, we check to see if
the original name (only set for the edit function, not the new) matches the new
name, if it does it's not a duplicate, so we allow it, otherwise we stop it.

Tested against latest truck with different combinations of names for new and
edit, and also create by example.
Attachment #157937 - Flags: review?(neil.parkwaycc.co.uk)

Comment 5

13 years ago
Comment on attachment 157937 [details] [diff] [review]
Filter Names Patch v2

>     if ("arguments" in window && window.arguments[0]) {
>         var args = window.arguments[0];
> 
>         if ("filter" in args) {
>           // editing a filter
>           gFilter = window.arguments[0].filter;
>           initializeDialog(gFilter);
>+          // Only set this when loading from an existing filter.
>+          gOriginalFilterName = gFilter.filterName;
>         } 
>         else {
>           gFilterList = args.filterList;
Unfortunately gFilterList doesn't get set when editing an existing filter.

> function onAccept()
> {
>-    if (duplicateFilterNameExists(gFilterNameElement.value))
>-    {
This seems to be a slightly odd place for this as all the other checks are done
in saveFilter.

>+    // If we think have a duplicate, then we need to check that if we
>+    // have an original filter name (i.e. we are editing a filter), then
>+    // we must check that the original is not the current as that is what
>+    // the duplicateFilterNameExists function will have picked up.
>+    if (duplicateFilterNameExists(gFilterNameElement.value) &&
>+        (!gOriginalFilterName || gOriginalFilterName != gFilterNameElement.value)) {
At this point the original filter, if it exists, will still have its original
name, so it should be possible to tell if the name is unchanged without using
an extra global variable.

>-
>+    
You accidentally added some whitespace here.

The changes to FilterListDialog.js look OK.
Attachment #157937 - Flags: review?(neil.parkwaycc.co.uk) → review-
Created attachment 157956 [details] [diff] [review]
Filter Names Patch v3

Updated patch as per Neil's comments to tidy the code.
Attachment #157937 - Attachment is obsolete: true
Attachment #157956 - Flags: review?(neil.parkwaycc.co.uk)

Comment 7

13 years ago
The existing file has a mixture of two-space and four-space indentation :-/

Ideally all the new code would have two-space indentation internally
although it may still have to line up with existing badly indented code.

Hopefully I can test this soon so don't update the patch just yet.

Comment 8

13 years ago
Comment on attachment 157956 [details] [diff] [review]
Filter Names Patch v3

Patch is basically fine, except some whitespace and minor nits to clean up:

>+        if ("filterList" in args) {
>+            gFilterList = args.filterList;
>+        }
I don't think this test is strictly necessary... but you should fix the
whitespace anyway, so you might as well remove it ;-)

> function onAccept()
> {
>-    if (duplicateFilterNameExists(gFilterNameElement.value))
>-    {
>-        if (gPromptService)
>-          gPromptService.alert(window,
>-            gFilterBundle.getString("cannotHaveDuplicateFilterTitle"),
>-            gFilterBundle.getString("cannotHaveDuplicateFilterMessage")
>-          );
>-        return false;
>-    }
>-
>     if (!saveFilter()) return false;
> 
>     // parent should refresh filter list..
>     // this should REALLY only happen when some criteria changes that
>     // are displayed in the filter dialog, like the filter name
>     window.arguments[0].refresh = true;
>     return true;
Heh, this method is getting small. By the time the comment is fixed, I expect
we'll be doing all the work in saveFilter ;-)

>+  if (duplicateFilterNameExists(filterName) &&
>+      (!gFilter || gFilter.filterName != filterName)) {
It's probably better to check that the name hasn't changed before checking to
see if it's a duplicate.
Attachment #157956 - Flags: review?(neil.parkwaycc.co.uk) → review+
Created attachment 158474 [details] [diff] [review]
Patch for Check in.

Modified to fix whitespace and Neil's other comments.
Attachment #158474 - Flags: review+
Comment on attachment 158474 [details] [diff] [review]
Patch for Check in.

Review flag copied from Neil's on Patch v3. Requesting superreview.

Anychance someone can confirm this bug as I don't have the relevant
permissions.
Attachment #158474 - Flags: superreview?(bienvenu)
Attachment #157956 - Attachment is obsolete: true

Comment 11

13 years ago
who says you can't have two filters with the same name? I can't think of
anything bad that happens - the name is just for the user to describe the
filter. I can sr this, but it's a little misleading to say that you can't have
two filters with the same name...

Updated

13 years ago
Attachment #158474 - Flags: superreview?(bienvenu) → superreview+
Just to reply to beinvenu's note: mozilla doesn't cope with duplicate names, if
you do have two duplicates saved in the list, only one of them shows up.

Comment 13

13 years ago
Fix checked in.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Comment 14

13 years ago
Verfified with Windows XP - tinderbox build 2004091421.
Thanks Mark!
Product: MailNews → Core
Verified per comment 14
Status: RESOLVED → VERIFIED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.