Last Comment Bug 561762 - Add additional error information to "This filter cannot be saved"
: Add additional error information to "This filter cannot be saved"
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 551707 775665
  Show dependency treegraph
 
Reported: 2010-04-26 08:18 PDT by Kent James (:rkent)
Modified: 2012-09-30 15:03 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.81 KB, patch)
2012-07-24 13:38 PDT, :aceman
no flags Details | Diff | Review
patch v2 (8.00 KB, patch)
2012-07-24 14:48 PDT, :aceman
mconley: review+
rkent: feedback+
jh: feedback+
Details | Diff | Review
patch v3 (8.00 KB, patch)
2012-09-12 11:19 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Review
patch v4 (8.13 KB, patch)
2012-09-13 12:42 PDT, :aceman
no flags Details | Diff | Review
patch v5 (9.16 KB, patch)
2012-09-15 08:39 PDT, :aceman
no flags Details | Diff | Review
patch v6 (9.28 KB, patch)
2012-09-18 12:11 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Review
patch v7 (9.56 KB, patch)
2012-09-24 14:15 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Review

Description Kent James (:rkent) 2010-04-26 08:18:23 PDT
From m.s.thunderbird:

> When I modify an existing filter TB3 invariably comes up with an error 
> message saying "This filter cannot be saved because some search terms 
> are invalid in the current context".
> 
> There is nothing wrong with my search terms, but after clicking OK a few 
> times the messages usually goes away and the revised filter works as 
> expected.
> 
> This behaviour is new in TB3. Is it some kind of bug?
> 

The message and check were added to help with legitimate cases where the search terms can become invalid, particularly with the new custom search terms feature. But unexpectedly the message is much more common now as a "catch all" for lots of different filter problems. Those other problems may be bugs, but at the moment we really don't understand why this sometimes happens.

So the filter is detecting some sort of problem. It is not clear what would happen if that check were missing - perhaps the filter would get stored incorrectly, which would be worse. I guess what we need to do is to add some additional diagnostics around that message, so that there would at least be an indication in the error console of exactly what is going on.
Comment 1 Wayne Mery (:wsmwk, NI for questions) 2010-04-26 08:28:27 PDT
I had seen this as well, per previous converation with rkent.  I didn't come up clear steps, simply failed some times and others not using the same set of steps
Comment 2 Tim H. 2011-04-07 01:55:01 PDT
The problem is that the filter is NOT detecting a real problem. I can do something as simple as scroll the filter window a few lines and it will magically accept it. I've examined my 'msgFilterRules.dat' in great detail with a binary editor and there are no bogus characters in the file. Just to make absolutely sure, I used:

tr '\000-\011\013-\037\177-\377' | unix2dos

 to strip out any non-printing characters and then compared the result to the original. They matched.

This is with SeaMonkey 2.0.13 on XP_SP3
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.18)
Comment 3 :aceman 2012-07-24 08:37:07 PDT
Is this still valid? In the current state of the FilterEditor.js file I see there are reportError() calls before the message to the user is shown. Is that not enough? What more needs to be added?
Comment 4 Kent James (:rkent) 2012-07-24 10:07:20 PDT
This error is coming from the code at http://hg.mozilla.org/releases/comm-aurora/annotate/78caca3ca3fb/mailnews/base/search/content/FilterEditor.js#l344

When you save the filter, the filter is searching through the validity table to make sure that the searches are all valid for the filter context. So it is complaining about a specific term in the table, but the error message gives no report what that term is.

So the request is to include some sort of information about the failed term in the error message. It might be sufficient to simply report something to the error console about the problematic term.

Another alternative might be to save the filter anyway (or give a choice), then warn about the invalid term - once again perhaps with a message to the error console.
Comment 5 :aceman 2012-07-24 11:23:26 PDT
(In reply to Kent James (:rkent) from comment #4)
> So the request is to include some sort of information about the failed term
> in the error message. It might be sufficient to simply report something to
> the error console about the problematic term.
Yes, and as I said, there are already calls like: Components.utils.reportError("filter not saved because standard search term not available");
when a rule fails a check. So there are messages in the error console.
But none of them seems to indicate which of the rules has the problem. If that is what needs to be done here I can take it.
Comment 6 Kent James (:rkent) 2012-07-24 11:26:19 PDT
(In reply to :aceman from comment #5)

> But none of them seems to indicate which of the rules has the problem. If
> that is what needs to be done here I can take it.

Yes that is what is needed.
Comment 7 :aceman 2012-07-24 13:38:04 PDT
Created attachment 645473 [details] [diff] [review]
patch

Something like this?
Comment 8 Kent James (:rkent) 2012-07-24 14:07:59 PDT
Comment on attachment 645473 [details] [diff] [review]
patch

Could you assemble a string containing the search term name and operator (localized) and include that in the on-screen error message, instead of just a number?
Comment 9 :aceman 2012-07-24 14:48:45 PDT
Created attachment 645517 [details] [diff] [review]
patch v2

Like this?
Comment 10 Kent James (:rkent) 2012-08-17 12:11:59 PDT
Comment on attachment 645517 [details] [diff] [review]
patch v2

OK this looks good to me. Thanks!

BTW to see this error, do this:

1) Create a filter with Apply Filter When manually run
2) Add a search term Junk Status is Junk
3) Change to Apply filter when Checking Mail
4) OK to save the filter.

You will see the error message.
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-08-27 10:19:35 PDT
Comment on attachment 645517 [details] [diff] [review]
patch v2

Review of attachment 645517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me. Thanks aceman!
Comment 12 Jens Hatlak (:InvisibleSmiley) 2012-09-08 15:24:27 PDT
Comment on attachment 645517 [details] [diff] [review]
patch v2

Review of attachment 645517 [details] [diff] [review]:
-----------------------------------------------------------------

Works well, thanks from me, too! Found some very minor nits. You may keep them, unless Neil is opposed.

::: mailnews/base/search/content/FilterEditor.js
@@ +296,5 @@
>    }
>  
>    // Check that all of the search attributes and operators are valid.
> +  let invalidRule = -1;
> +  for (let index = 0; (index < gSearchTerms.length) && (invalidRule == -1); index++)

Technically, the additional parentheses is completely unnecessary, and it's readable without, too. Don't know TB's coding guidelines by heart, though.

@@ +315,5 @@
>        if (!customTerm)
> +      {
> +        invalidRule = index;
> +        Components.utils.reportError("Filter not saved because custom search term ID " +
> +                                     obj.searchattribute.value + " in rule " + index + " not found.");

Here and below: Adding a full stop suggests this is now a sentence, but it isn't. If you really want to keep it short I'd replace the " because" by a colon and remove the full stop.
Comment 13 :aceman 2012-09-12 11:19:55 PDT
Created attachment 660527 [details] [diff] [review]
patch v3

Done.
Comment 14 Blake Winton (:bwinton) (:☕️) 2012-09-13 10:15:17 PDT
Comment on attachment 660527 [details] [diff] [review]
patch v3

(As a side note, this patch didn't apply on the latest trunk, but was easy enough to fix.)

>+++ b/mail/locales/en-US/chrome/messenger/filter.properties
>@@ -18,17 +18,20 @@ invalidCustomHeader=One of your filters 
>-searchTermsInvalidMessage=This filter cannot be saved because some search terms are invalid in the current context.
>+# LOCALIZATION NOTE(searchTermsInvalidRule)
>+# %1$S=search attribute name from the bad rule
>+# %2$S=search operator from the bad rule
>+searchTermsInvalidRule=This filter cannot be saved because the search term "%1$S %2$S" is invalid in the current context.

Maybe say "from the invalid rule" instead of "from the bad rule"?

But other than that nit (and that's really a review-nit, not a ui-review-nit), ui-r=me!

Thanks,
Blake.
Comment 15 :aceman 2012-09-13 12:42:16 PDT
Created attachment 660934 [details] [diff] [review]
patch v4

Sure, thanks.
Comment 16 :aceman 2012-09-15 08:39:48 PDT
Created attachment 661502 [details] [diff] [review]
patch v5
Comment 17 :aceman 2012-09-18 12:11:16 PDT
Created attachment 662263 [details] [diff] [review]
patch v6
Comment 18 Ian Neal 2012-09-24 12:49:24 PDT
Comment on attachment 662263 [details] [diff] [review]
patch v6

>+  let invalidRule = -1;
>+  for (let index = 0; index < gSearchTerms.length && invalidRule == -1; index++)
My preference is to use while rather than for here.

>+        Components.utils.reportError("Filter not saved because custom search term ID " +
>+                                     obj.searchattribute.value + " in rule " + index + " not found");
>+          Components.utils.reportError("Filter not saved because custom search term " +
>+                                       customTerm.name + " in rule " + index + " not available");
>+        Components.utils.reportError("Filter not saved because standard search term " +
>+                                     attribValue + " in rule " + index + " not available");
Could the rule name rather than index be used? or maybe both?
Comment 19 :aceman 2012-09-24 13:25:36 PDT
Ian, what is the rule name? obj.searchattribute.label + obj.searchoperator.label as is shown to the user in the prompt below this?
Comment 20 Ian Neal 2012-09-24 13:49:26 PDT
(In reply to :aceman from comment #19)
> Ian, what is the rule name? obj.searchattribute.label +
> obj.searchoperator.label as is shown to the user in the prompt below this?

Hmmm, yes, just looking at the best way to give as much relevant information in the diagnostics. Thinking about it more, should it be index + 1 seeing users usually start counting at 1 whereas the first rule is zero internally?
Comment 21 :aceman 2012-09-24 14:15:09 PDT
Created attachment 664208 [details] [diff] [review]
patch v7

OK, what about this?
Comment 22 Ian Neal 2012-09-30 02:57:37 PDT
Comment on attachment 664208 [details] [diff] [review]
patch v7

Looks good to me, would any tests / changes to tests be required for TB?
Comment 23 :aceman 2012-09-30 13:39:43 PDT
Fortunately mconley didn't request any.
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-09-30 15:03:24 PDT
https://hg.mozilla.org/comm-central/rev/ba5cfbb941d6

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