561762 - Add additional error information to "This filter cannot be saved"

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Kent James (:rkent)]

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)]

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.]

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]

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)]

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]

(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)]

(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]

Attachment: patch

Something like this?

Comment 8

[Kent James (:rkent)]

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]

Attachment: patch v2

Like this?

Comment 10

[Kent James (:rkent)]

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) (:⚙️)]

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)]

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]

Attachment: patch v3

Done.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

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]

Attachment: patch v4

Sure, thanks.

Comment 16

[:aceman]

Attachment: patch v5

Comment 17

[:aceman]

Attachment: patch v6

Comment 18

[Ian Neal]

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]

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]

(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]

Attachment: patch v7

OK, what about this?

Comment 22

[Ian Neal]

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]

Fortunately mconley didn't request any.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/ba5cfbb941d6