Last Comment Bug 399446 - Newsgroup: should not be displayed if there is no newsgroup account
: Newsgroup: should not be displayed if there is no newsgroup account
Status: RESOLVED FIXED
: polish
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 24.0
Assigned To: :aceman
:
:
Mentors:
: 744635 (view as bug list)
Depends on: 922614
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-11 06:51 PDT by Hideo Oshima
Modified: 2013-11-11 12:35 PST (History)
11 users (show)
acelists: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.50 KB, patch)
2013-05-24 08:31 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v2 (2.75 KB, patch)
2013-06-03 12:53 PDT, :aceman
mkmelin+mozilla: review-
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v3 (7.47 KB, patch)
2013-06-08 07:48 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v4 (7.48 KB, patch)
2013-06-08 12:37 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Hideo Oshima 2007-10-11 06:51:47 PDT
Steps to reproduce:
1. Make sure you don't have newsgroup account.
2. Open Compose Window.
3. Click combo box displayed "To:".

Exptected result:
No Newsgroup: in the drop down list.

Actual result:
There is Newsgroup: in the drop down list.

Original bug report written in Japanese is
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=4130
Comment 1 Hideo Oshima 2007-10-11 06:53:49 PDT
I observed this bug with 2007101104-trunk/Linux.
Comment 2 Jens Müller (:tessarakt) 2012-09-20 15:30:38 PDT
assign to me, please
Comment 3 :aceman 2013-05-23 09:40:46 PDT
Jens, are you working on this?

Also, should this enable "Newsgroup" option if there exists any NNTP account in the profile? Or only if a NNTP account is chosen in the From field?
Comment 4 Jens Müller (:tessarakt) 2013-05-23 11:02:11 PDT
(In reply to :aceman from comment #3)
> Jens, are you working on this?

Sorry, not yet. First wanna wrap up bug 99019 properly ...

> Also, should this enable "Newsgroup" option if there exists any NNTP account
> in the profile? Or only if a NNTP account is chosen in the From field?

Hu, this is a tough question. I'd prefer the latter ...
Comment 5 :aceman 2013-05-23 14:25:26 PDT
I mean if you are not attached to fix this specific bug personally, I could take it too.
Comment 6 Jens Müller (:tessarakt) 2013-05-23 14:37:06 PDT
Yes, please do. Once I manage to get the infrastructure for my first case working, I'll hopefully be faster with the following ones ...
Comment 7 :aceman 2013-05-24 00:30:37 PDT
Thanks.
Comment 8 :aceman 2013-05-24 08:31:09 PDT
Created attachment 753804 [details] [diff] [review]
patch

This seems to do it.

For mkmelin:
This intentionally changes "accountname" to "accountkey" in the statement:
identityElement.selectedItem.getAttribute("accountkey");
identityElement.setAttribute("accountkey", accountKey);

It seems "accountname" was a mistake as the attribute on the menuitems is accountkey. It was not detected that we were getting always "" as there does not seem to be any user of this functionality. But I think we can keep it as it may be useful. The question is whether to also change the attribute name on the patent menulist.
Comment 9 Jens Müller (:tessarakt) 2013-05-24 12:12:21 PDT
Comment on attachment 753804 [details] [diff] [review]
patch

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

Looks fine to me ... I think I already looked at that file before, but I find it quite hard to understand due to the lack of comments ...

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4146,5 @@
> +        let addrWidget = document.getElementById("addressingWidget");
> +        for (let row = 0; row < addrWidget.itemCount; row++) {
> +          let addrRow = addrWidget.getItemAtIndex(row);
> +          if (!addrRow.hasAttribute("_isDummyRow")) {
> +            let addrType = addrRow.firstChild.firstChild;

For me, it would help to explain what each ".firstChild" is.
Comment 10 Magnus Melin 2013-05-30 03:24:48 PDT
Comment on attachment 753804 [details] [diff] [review]
patch

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

Some thoughts:
 - we should probably do this for mail too, for people who only use tb as a newsreader
 - Followup-To is news specific, and i think we can hide it if there are no news accounts set up. You can set it in mails too, and it will be shown but have to be manually clicked for replies to go there. I suppose that's another bug. Anyway i doubt it's a common use case.
 - probably good to put the functionality in a separate function like hideIrrelvantAddressingOptions.
 - not sure it should be in loadidentity or in onopopushowing

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4149,5 @@
> +          if (!addrRow.hasAttribute("_isDummyRow")) {
> +            let addrType = addrRow.firstChild.firstChild;
> +            if (addrType.id.startsWith("addressCol1#"))
> +              addrType.querySelector('[value="addr_newsgroups"]')
> +                      .collapsed = hideNews;

can't you just querySelector this from querySelector?
Comment 11 :aceman 2013-05-30 04:50:34 PDT
(In reply to Magnus Melin from comment #10)
> Comment on attachment 753804 [details] [diff] [review]
> patch
> 
> Review of attachment 753804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some thoughts:
>  - we should probably do this for mail too, for people who only use tb as a
> newsreader
I test if this is even possible yet. So you mean to hide 'To', 'CC' and 'BCC' ?
But in the Account manager it is possible to set predefined CC and BCC on a News account too (the Copies&Folders pane). Or do you propose only to hide 'To'?

>  - Followup-To is news specific, and i think we can hide it if there are no
> news accounts set up. You can set it in mails too, and it will be shown but
> have to be manually clicked for replies to go there.
> I suppose that's another bug.
So I didn't understand this. Is it supposed to do anything on non-News on not?

> Anyway i doubt it's a common use case.
>  - probably good to put the functionality in a separate function like
> hideIrrelvantAddressingOptions.
Will do.

>  - not sure it should be in loadidentity or in onopopushowing
LoadIdentity will probably be used about once per window. The address type popup may be used several times (and identity has not changed). So if no other difference, the LoadIdentity way may be more efficient.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +4149,5 @@
> > +          if (!addrRow.hasAttribute("_isDummyRow")) {
> > +            let addrType = addrRow.firstChild.firstChild;
> > +            if (addrType.id.startsWith("addressCol1#"))
> > +              addrType.querySelector('[value="addr_newsgroups"]')
> > +                      .collapsed = hideNews;
> 
> can't you just querySelector this from querySelector?

???
Comment 12 Magnus Melin 2013-05-30 05:00:19 PDT
(In reply to :aceman from comment #11)
> (In reply to Magnus Melin from comment #10)
> > Comment on attachment 753804 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 753804 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Some thoughts:
> >  - we should probably do this for mail too, for people who only use tb as a
> > newsreader
> I test if this is even possible yet. So you mean to hide 'To', 'CC' and
> 'BCC' ?

Yeah, or strictly speaking you may need to only have an smtp set, didn't check how that works in practice though.

> But in the Account manager it is possible to set predefined CC and BCC on a
> News account too (the Copies&Folders pane). Or do you propose only to hide
> 'To'?

Arguably the auto options should be disabled unless an email account is set, imho.

> >  - Followup-To is news specific, and i think we can hide it if there are no
> > news accounts set up. You can set it in mails too, and it will be shown but
> > have to be manually clicked for replies to go there.
> > I suppose that's another bug.
> So I didn't understand this. Is it supposed to do anything on non-News on
> not?

I don't know if it has a defined behavior... 

> > can't you just querySelector this from querySelector?
> 
> ???

Sorry, i meant can't you directly from addrWidget?
Comment 13 :aceman 2013-05-30 11:48:43 PDT
OK, I have tested it and when sending to a Newsgroup I could add a To and Bcc addressee (myself) and it worked (message delivered). So I think I can not disable those (in compose and in account manager).

The Followup-to is still open. Can anybody explain what it does and if it can be set when sending from a non-News account?
Comment 14 Jens Müller (:tessarakt) 2013-05-30 12:13:20 PDT
(In reply to :aceman from comment #13)
> The Followup-to is still open. Can anybody explain what it does and if it
> can be set when sending from a non-News account?

https://tools.ietf.org/html/rfc5536#section-3.2.6 defines it for Netnews articles.

https://tools.ietf.org/html/rfc5322, in contrast, does not define such a header.
Comment 15 Magnus Melin 2013-05-30 12:21:43 PDT
(In reply to :aceman from comment #13)
> OK, I have tested it and when sending to a Newsgroup I could add a To and
> Bcc addressee (myself) and it worked (message delivered). So I think I can
> not disable those (in compose and in account manager).

Huh? Without any mail account set up, how would that be possible?
Comment 16 :aceman 2013-05-30 12:31:03 PDT
(In reply to Magnus Melin from comment #15)
> Huh? Without any mail account set up, how would that be possible?

I had mail accounts set up. But I specifically composed from the Newsgroup account (selecting a newspost and clicking Reply). So I don't think it should have touched any of the mail accounts.
Comment 17 Magnus Melin 2013-05-30 12:43:46 PDT
If you have a mail account it will work (probably uses the default), but without mail set up it can't work.
Comment 18 :aceman 2013-05-30 13:21:00 PDT
I did test it, empty profile, only NNTP account set up, add SMTP server. Reply to a news post, change Newsgroup in the dropdown to To, write some email address. Message arrived just fine.
Comment 19 Joshua Cranmer [:jcranmer] 2013-05-30 13:56:05 PDT
(In reply to :aceman from comment #13)
> The Followup-to is still open. Can anybody explain what it does and if it
> can be set when sending from a non-News account?

Followup-To is to Newsgroups as Reply-To is to To. Nothing gets sent to it; it's merely an indicator to clients to how to treat reply.
Comment 20 :aceman 2013-05-30 14:06:17 PDT
(In reply to Joshua Cranmer [:jcranmer] from comment #19)
> Followup-To is to Newsgroups as Reply-To is to To. Nothing gets sent to it;
> it's merely an indicator to clients to how to treat reply.

So it not needed when posting from a non-News account? Can we hide it?
Comment 21 Joshua Cranmer [:jcranmer] 2013-05-30 14:33:03 PDT
(In reply to :aceman from comment #20)
> (In reply to Joshua Cranmer [:jcranmer] from comment #19)
> > Followup-To is to Newsgroups as Reply-To is to To. Nothing gets sent to it;
> > it's merely an indicator to clients to how to treat reply.
> 
> So it not needed when posting from a non-News account? Can we hide it?

It only makes sense if you add a Newsgroup header.
Comment 22 :aceman 2013-06-03 12:53:52 PDT
Created attachment 757569 [details] [diff] [review]
patch v2
Comment 23 Blake Winton (:bwinton) (:☕️) 2013-06-04 08:28:17 PDT
Comment on attachment 757569 [details] [diff] [review]
patch v2

It would be cool if this updated the list when I opened the compose window, and then created a new newsgroup account, but I'm pretty happy with it the way it is if you didn't feel like changing that.  :)

ui-r=me.
Comment 24 :aceman 2013-06-04 09:26:41 PDT
It should update the list of addressee types if you select the newly created account in the From dropdown.
If you say the account is not yet available in the dropdown, then that is a different bug :)
Comment 25 Magnus Melin 2013-06-05 13:12:34 PDT
Comment on attachment 757569 [details] [diff] [review]
patch v2

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

I get a busted addressing area (missing To:/Cc: dropdowns) if i start by selecting a newsgroup and clicking write, then try to change to a mail account. Didn't look into why, but nothing in the error console.

Also, would be nice with a mozmill test for this.
Comment 26 :aceman 2013-06-08 05:41:04 PDT
In my previous tests even when the "Newsgroup" item was selected, collapsing it in the dropdown did not affect the appearance of the menu ("Newsgroup" was still shown even if no longer selectable). In your scenario, collapsing the item collapses the whole menu. If I find no better sollution, maybe changing collapse to disabled would work.
Comment 27 :aceman 2013-06-08 05:59:11 PDT
Sorry, wrong. The switch to querySelectorAll caused it to return also the parent menulist together with the menuitems so it got collapsed explicitly too.
I fixed that and collapse can be used. Working on the test now.
Comment 28 :aceman 2013-06-08 07:48:50 PDT
Created attachment 760117 [details] [diff] [review]
patch v3

The fix and test.
Comment 29 Magnus Melin 2013-06-08 11:37:56 PDT
Comment on attachment 760117 [details] [diff] [review]
patch v3

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

Nice! r=mkmelin

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4140,5 @@
> +    .querySelectorAll('menuitem[value="addr_newsgroups"], menuitem[value="addr_followup"]');
> +  // Collapsing the menuitem only prevents it getting chosen, it does not
> +  // affect the menulist widget display when Newsgroup is already selected.
> +  for (let item of newsTypes)
> +    item.collapsed = hideNews;

please always use braces for for loops
Comment 30 :aceman 2013-06-08 12:37:02 PDT
Created attachment 760140 [details] [diff] [review]
patch v4

Thanks.
Comment 31 Ryan VanderMeulen [:RyanVM] 2013-06-10 05:14:42 PDT
https://hg.mozilla.org/comm-central/rev/16e20df57d08
Comment 32 Steve Russie 2013-09-27 12:57:15 PDT
I loaded Thunderbird 24.0 and can no longer send messages to my internal newsgroups (as the to newsgroup option has disappeared).   Please restore this function
Comment 33 :aceman 2013-09-30 00:20:28 PDT
What are internal newsgroups? What type of account do you use to send to them? Can you tell the exacts steps you try to do?
Comment 34 Steve Russie 2013-09-30 09:26:10 PDT
We have newsgroups that are behind our firewall at news.internaldomain.com they are on an AIX unix machine running an inn newsgroup server.   We connect to the server vi NNTP at port 119, until patch 24 we could cc the newsgroup because "newsgroup" was in the "TO" dropdown when we composed or replied to an email.

The email client is IMAP to port 142 of the same Unix server.

Let me know if you need more details, 
Thanks,
Steve
Comment 35 Magnus Melin 2013-09-30 10:58:30 PDT
It's visible if you're writing from a newsgroup account (and you can temporarily switch to one). Granted, i agree this makes mail+news postings very difficult. Would be better if it hid depending on the presence of a newsgroup account.
Comment 36 :aceman 2013-09-30 11:21:09 PDT
Magnus, if you have selected a mail account as the From and still post to both mail & news addresses, is that supposed to work? But it will send using your mail account that may no have the proper settings.
Comment 37 Magnus Melin 2013-09-30 11:26:01 PDT
That's supposed to work yes. It will send to the mail addresses using mail protocols, and do a post to the newsgroup using nntp.
Comment 38 :aceman 2013-09-30 11:29:33 PDT
And what account will it use for the NNTP protocol? Isn't sending to news just SMTP?
Comment 39 Magnus Melin 2013-09-30 11:37:30 PDT
AFAIK, sending to news is just using NNTP (no smtp involved).
Comment 40 :aceman 2013-09-30 11:48:52 PDT
I don't think so. There is SMTP server setting in the account manager when you view a news account. I think sending to News is through normal SMTP just downloading is over NNTP protocol.
Comment 41 Magnus Melin 2013-09-30 12:04:56 PDT
(In reply to :aceman from comment #40)
> I don't think so. There is SMTP server setting in the account manager when
> you view a news account. I think sending to News is through normal SMTP just
> downloading is over NNTP protocol.

jcranmer?
Comment 42 Magnus Melin 2013-09-30 12:12:16 PDT
I'd expect that to be the smtp used when sending from a news identity, and e.g. ccing mail people (just in case you have multiple mail accounts set up you get to choose)
Comment 43 Joshua Cranmer [:jcranmer] 2013-09-30 12:42:24 PDT
(In reply to :aceman from comment #40)
> I don't think so. There is SMTP server setting in the account manager when
> you view a news account. I think sending to News is through normal SMTP just
> downloading is over NNTP protocol.

And you would be wrong. The SMTP setting is to support sending to both newsgroups and other people, such as when I cross-post between m.d.a.t (a newsgroup) and tb-planning (a mailing list).

(In reply to :aceman from comment #38)
> And what account will it use for the NNTP protocol? Isn't sending to news
> just SMTP?

If you are using a non-news identity, I believe it picks the news server that has that newsgroup; if none of them have it, I believe it defaults to the "first" one. There are some bugs somewhere about having an explicit default news account for this sort of stuff (and no-authority news URIs).
Comment 44 :aceman 2013-09-30 12:50:59 PDT
Ok, thanks. So the answer is we need a News account in TB for posting to a newsgroup?
Comment 45 :aceman 2013-09-30 13:40:16 PDT
Steve, can you confirm you have a News account configured in TB?
Comment 46 Steve Russie 2013-10-01 05:42:23 PDT
(In reply to :aceman from comment #45)
> Steve, can you confirm you have a News account configured in TB?

Yes, we have 2 accounts set up, a standard imap email account and a news group account (pointing to port 119 on the same server as the imap email).

We use the news group as a common customer specific repository, so we often forward to or cc the newsgroup when sending emails from the imap account.
Comment 47 :aceman 2013-10-01 06:20:46 PDT
OK, I have split the problem into bug 922614 (to ease the tracking into which branches the fix must go) and will work on it.
Comment 48 Magnus Melin 2013-11-11 12:35:29 PST
*** Bug 744635 has been marked as a duplicate of this bug. ***

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