300930 - Reply with template is not disabled when there exists no templates

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Emil Hesslow]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050710 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050710 Firefox/1.0+

If you have no templates you can still choose Reply the Template as an filter
action.

Reproducible: Always

Steps to Reproduce:
1. Make sure that you have no Templates
2. Go to Tools -> Message Filters
3. Add a new filter
4. You now can choose Reply with Template
Actual Results:  
The Selectbox which you choose Template from is empty but you can still interact
with it and you can also check the checkbox

Expected Results:  
You should not be able to Reply with Template if you don't have any Templates to
choose from.

Comment 1

[Emil Hesslow]

Attachment: Fixes the bug

The attachment also disables the Reply with Template checkbox when your in a
newsgroupaccout. Before just the selectbox was disabled.

Comment 2

[Scott MacGregor]

Comment on attachment 189437 [details] [diff] [review]
Fixes the bug

This patch is out of date relative to the new UI we use for filter actions.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Keeping Bryan on the radar.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 189437 [details] [diff] [review]
Fixes the bug

Patch has bitrotted.

$ patch --dry-run < ~/Desktop/tbTestPatches/300930.diff 
(Stripping trailing CRs from patch.)
patching file FilterEditor.js
Hunk #1 FAILED at 721.
1 out of 1 hunk FAILED -- saving rejects to file FilterEditor.js.rej
(Stripping trailing CRs from patch.)
patching file FilterEditor.xul
Hunk #1 FAILED at 190.
1 out of 1 hunk FAILED -- saving rejects to file FilterEditor.xul.rej

Comment 5

[:aceman]

Attachment: patch v2

The "disablefornews=true" is already in trunk.
This patch just hides the action if no templates are found.

Comment 6

[Ian Neal]

Comment on attachment 644693 [details] [diff] [review]
patch v2

>+            // Disable Reply with Template if there are no templates
Nit: no full stop.

What happens when there is a filter set to reply with template and that template gets removed?

Comment 7

[:aceman]

From what I could test nothing happens. The filter still references the template, when it is edited it contains the action and the template list is empty (without the patch). Running the filter manually on a matching message does nothing and there is no error in the Error console. Probably other bug?

Comment 8

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

Comment on attachment 644693 [details] [diff] [review]
patch v2

I mostly like it, but I've got a couple of questions…

>+++ b/mailnews/base/search/content/searchWidgets.xml
>@@ -113,20 +113,50 @@
>+            // Disable Reply with Template if there are no templates
>+            let identity = MailServices.accounts.getFirstIdentityForServer(gFilterList.folder.server);

Why are you only checking the first identity for templates?

>@@ -570,36 +600,41 @@
>         <![CDATA[
>-            Components.utils.import("resource:///modules/mailServices.js", this);
>             let identity = MailServices.accounts.getFirstIdentityForServer(gFilterList.folder.server);
>             if (!identity) // typically if this is Local Folders
>               identity = MailServices.accounts.defaultAccount.defaultIdentity;
>+            let enumerator = null;
>+            let msgFolder;
>+            try {
>+              msgFolder = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                                    .getService(Components.interfaces.nsIRDFService)
>+                                    .GetResource(identity.stationeryFolder)
>+                                    .QueryInterface(Components.interfaces.nsIMsgFolder);
>+              enumerator = msgFolder.msgDatabase.EnumerateMessages();
>+            } catch (e) {
>+              // The Templates folder may not exist, that is OK.

Also, does it really make sense to do this logic twice, or could we check for an empty menulist to find out whether to show the item instead?

ui-r=me with those questions answered.

Thanks,
Blake.

Comment 9

[:aceman]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #8)
> >+++ b/mailnews/base/search/content/searchWidgets.xml
> >@@ -113,20 +113,50 @@
> >+            // Disable Reply with Template if there are no templates
> >+            let identity = MailServices.accounts.getFirstIdentityForServer(gFilterList.folder.server);
> 
> Why are you only checking the first identity for templates?
I just copied this code from the function populating the list. Maybe the existing filter backend can't access templates in other identities.
So I can't answer that and must leave the logic as is.

Comment 10

[:aceman]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #8)
> >@@ -570,36 +600,41 @@
> >         <![CDATA[
> >-            Components.utils.import("resource:///modules/mailServices.js", this);
> >             let identity = MailServices.accounts.getFirstIdentityForServer(gFilterList.folder.server);
> >             if (!identity) // typically if this is Local Folders
> >               identity = MailServices.accounts.defaultAccount.defaultIdentity;
> >+            let enumerator = null;
> >+            let msgFolder;
> >+            try {
> >+              msgFolder = Components.classes["@mozilla.org/rdf/rdf-service;1"]
> >+                                    .getService(Components.interfaces.nsIRDFService)
> >+                                    .GetResource(identity.stationeryFolder)
> >+                                    .QueryInterface(Components.interfaces.nsIMsgFolder);
> >+              enumerator = msgFolder.msgDatabase.EnumerateMessages();
> >+            } catch (e) {
> >+              // The Templates folder may not exist, that is OK.
> 
> Also, does it really make sense to do this logic twice, or could we check
> for an empty menulist to find out whether to show the item instead?

I can't see how to do that as the menulist is only populated (templates enumerated) when the menuitem is first selected. I'd try to at least merge the code if that is possible between 2 different <binding>s.

Comment 11

[:aceman]

Attachment: patch v3

Comment 12

[:aceman]

Attachment: patch v4

OK, merged the code. Looks nice.

Comment 13

[Ian Neal]

Comment on attachment 649029 [details] [diff] [review]
patch v4

>+            // Disable "Reply with Template" if there are no templates.
>+            let templatesExist = this.getTemplates(false);
>+            elements = menupopup.getElementsByAttribute("value", "replytomessage");
>+            if (!templatesExist && elements.length == 1)
Couldn't you inline and remove templatesExist now?

>+
>+      <!--
>+        - Check if there exist any templates in this account.
>+        -
>+        - @param populateTemplateList  If true, create menuitems representing
>+        -                              the found templates.
>+        - @param templateMenuList      The menulist element to create items in.
>+        -
>+        - @return  True if at least one template was found, otherwise false.
>+      -->
>+      <method name="getTemplates">
>+        <parameter name="populateTemplateList"/>
>+        <parameter name="templateMenuList"/>
Wouldn't this be better as a helper in FilterEditor.js?

>+            } catch (e) {
>+              // The Templates folder may not exist, that is OK.
>+            }
>+
>+            let templateFound = false;
>+
>+            if (enumerator) {
Couldn't you do instead an early return if enumerator doesn't exist?

>+              while (enumerator.hasMoreElements()) {
>+                let header = enumerator.getNext();
>+                if (header instanceof Components.interfaces.nsIMsgDBHdr) {
>+                  templateFound = true;
>+                  if (populateTemplateList) {
How about instead:
if (!populateTemplateList)
  break;

>+          document.getAnonymousElementByAttribute(this.parentNode,
>+                                                  "class", "ruleactiontype")
>+                  .getTemplates(true, document.getAnonymousNodes(this)[0]);
This looks very uncomfortable, see earlier comment.

Comment 14

[:aceman]

(In reply to Ian Neal from comment #13)
> >+      <method name="getTemplates">
> >+        <parameter name="populateTemplateList"/>
> >+        <parameter name="templateMenuList"/>
> Wouldn't this be better as a helper in FilterEditor.js?
Not sure. As it is basically a part of hideInvalidActions, it makes sense to define it in the same file besides it.

> >+          document.getAnonymousElementByAttribute(this.parentNode,
> >+                                                  "class", "ruleactiontype")
> >+                  .getTemplates(true, document.getAnonymousNodes(this)[0]);
> This looks very uncomfortable, see earlier comment.
What do you mean?

Comment 15

[Ian Neal]

(In reply to :aceman from comment #14)
> (In reply to Ian Neal from comment #13)
> > >+      <method name="getTemplates">
> > >+        <parameter name="populateTemplateList"/>
> > >+        <parameter name="templateMenuList"/>
> > Wouldn't this be better as a helper in FilterEditor.js?
> Not sure. As it is basically a part of hideInvalidActions, it makes sense to
> define it in the same file besides it.
Well it is shared by that and the constructor for the ruleactiontarget-replyto binding.
> 
> > >+          document.getAnonymousElementByAttribute(this.parentNode,
> > >+                                                  "class", "ruleactiontype")
> > >+                  .getTemplates(true, document.getAnonymousNodes(this)[0]);
> > This looks very uncomfortable, see earlier comment.
> What do you mean?
Just looks a bit ugly/complicated.

Comment 16

[:aceman]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #8)
> >+++ b/mailnews/base/search/content/searchWidgets.xml
> >@@ -113,20 +113,50 @@
> >+            // Disable Reply with Template if there are no templates
> >+            let identity = >MailServices.accounts.getFirstIdentityForServer(gFilterList.folder.server);
> 
> Why are you only checking the first identity for templates?
But if rkent says we can use all identities I can try to add that.

Comment 17

[:aceman]

(In reply to Ian Neal from comment #15)
> > > Wouldn't this be better as a helper in FilterEditor.js?
> > Not sure. As it is basically a part of hideInvalidActions, it makes sense to
> > define it in the same file besides it.
> Well it is shared by that and the constructor for the
Still the same file. I do not see any similar code in FilterEditor.js.

> ruleactiontarget-replyto binding.
> > 
> > > >+          document.getAnonymousElementByAttribute(this.parentNode,
> > > >+                                                  "class", "ruleactiontype")
> > > >+                  .getTemplates(true, document.getAnonymousNodes(this)[0]);
> > > This looks very uncomfortable, see earlier comment.
> > What do you mean?
> Just looks a bit ugly/complicated.
Wrapping-wise?

Comment 18

[Kent James (:rkent)]

Comment on attachment 649029 [details] [diff] [review]
patch v4

I did not attempt to run with this patch nor give a detailed look at the code, so this feedback is not with the same detail as a review.

Looking at the issue of the identity, it appears to me that the actual filter action has no provision for setting the identity to be used, and therefore ends up using the default identity (see nsMsgTemplateReplyHelper::OnStopRunningUrl). So I don't really understand why the original UI used the first identity to select a list of templates.

But that is what it used. If you change it at this point to use the default identity, then existing users of this feature will see a different list of templates when they attempt to setup this feature, if the default identity is not the first identity. I don't think we should do that. In any case, the message URI that becomes the template has no identity information in it, so as long as the template still exists it will work.  You could scan all identities to find all possible templates and that would not cause any problems for existing users, but that is beyond the scope of this bug.

So I am happy with the approach of this patch, but while you are touching the code it would be OK to also scan all identities and add any unique templates found. I don't really understand how users actually would use multiple identities here well enough to really understand whether that would have any negative consequences, but I suspect not.

Comment 19

[:aceman]

(In reply to Kent James (:rkent) from comment #18)
> Looking at the issue of the identity, it appears to me that the actual
> filter action has no provision for setting the identity to be used, and
> therefore ends up using the default identity (see
> nsMsgTemplateReplyHelper::OnStopRunningUrl). So I don't really understand
> why the original UI used the first identity to select a list of templates.
> But that is what it used. If you change it at this point to use the default
> identity, then existing users of this feature will see a different list of
> templates when they attempt to setup this feature, if the default identity
> is not the first identity.
The first identity is always the default, see bug 314806. So I'd think the current code is OK if we can only send from the default identity.

> You could scan all
> identities to find all possible templates and that would not cause any
> problems for existing users, but that is beyond the scope of this bug.
> 
> So I am happy with the approach of this patch, but while you are touching
> the code it would be OK to also scan all identities and add any unique
> templates found.

Sorry, this seems like contradicting sentences. What did you mean?

Comment 20

[Kent James (:rkent)]

(In reply to :aceman from comment #19)
> 
> Sorry, this seems like contradicting sentences. What did you mean?

You "could scan all identities" but you do not need to do that in order to fulfill the current bug. So the patch is fine as is. However, if you wanted to tack on a change to scan all identities for templates, that is also OK to me. So I am happy with either approach.

Comment 21

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

Comment on attachment 649029 [details] [diff] [review]
patch v4

This seems basically the same as the previous version, so ui-r still = me.  ;)

Thanks,
Blake.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 649029 [details] [diff] [review]
patch v4

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

::: mailnews/base/search/content/searchWidgets.xml
@@ +123,5 @@
>                elements[i].hidden = !gCustomActions[i]
>                                       .isValidForType(gFilterType, scope);
> +
> +            // Disable "Reply with Template" if there are no templates.
> +            let templatesExist = this.getTemplates(false);

I agree with Ian - we can inline this variable.

@@ +226,5 @@
> +        - @param templateMenuList      The menulist element to create items in.
> +        -
> +        - @return  True if at least one template was found, otherwise false.
> +      -->
> +      <method name="getTemplates">

I'm not wild about how many things this function does. Is there a good reason why we can't have a separate function that populates the menulist with menuitems?

Comment 23

[:aceman]

mconley, there could be separate functions, it was that way before, but bwinton specifically asked for merging of the duplicate code. And I'd agree with that.

Comment 24

[:aceman]

Attachment: patch v5

Added looping over all identities of the account. I tested that sending a reply with a template from the second identity (with another template folder in other account) does work.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 652576 [details] [diff] [review]
patch v5

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

::: mailnews/base/search/content/searchWidgets.xml
@@ +243,5 @@
> +                                      .QueryInterface(Components.interfaces.nsIMsgFolder);
> +                // If we already processed this folder, do not set enumerator
> +                // so that we skip this identity.
> +                if (foldersScanned.indexOf(msgFolder) == -1) {
> +                  foldersScanned.push(msgFolder);

Hold on - so we might get into cases where, while iterating over identities, we find duplicate identity.stationeryFolders? How can we get into that case?

@@ +258,5 @@
> +                let header = enumerator.getNext();
> +                if (header instanceof Components.interfaces.nsIMsgDBHdr) {
> +                  templateFound = true;
> +                  if (!populateTemplateList)
> +                    break identityLoop;

Instead of using break's label feature, can't you simply return true here?

Comment 26

[:aceman]

(In reply to Mike Conley (:mconley) from comment #25)
> Hold on - so we might get into cases where, while iterating over identities,
> we find duplicate identity.stationeryFolders? How can we get into that case?
I think that is actually the most common case. All identities on an account will have the same Template folder (in that account) so I avoid enumerating it everytime. Only in exceptional cases will people have differing template folders set in identities of the same account.

> 
> @@ +258,5 @@
> > +                let header = enumerator.getNext();
> > +                if (header instanceof Components.interfaces.nsIMsgDBHdr) {
> > +                  templateFound = true;
> > +                  if (!populateTemplateList)
> > +                    break identityLoop;
> 
> Instead of using break's label feature, can't you simply return true here?

Probably yes.

Comment 27

[:aceman]

Attachment: patch v6

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 655308 [details] [diff] [review]
patch v6

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

Ok, I think this looks good.

Thanks aceman!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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