399446 - Newsgroup: should not be displayed if there is no newsgroup account

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Hideo Oshima]

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]

I observed this bug with 2007101104-trunk/Linux.

Comment 2

[Jens Müller (:tessarakt)]

assign to me, please

Comment 3

[:aceman]

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

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

I mean if you are not attached to fix this specific bug personally, I could take it too.

Comment 6

[Jens Müller (:tessarakt)]

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]

Thanks.

Comment 8

[:aceman]

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

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 [:mkmelin]]

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]

(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 [:mkmelin]]

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

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

(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 [:mkmelin]]

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

(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 [:mkmelin]]

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]

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

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

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

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

Attachment: patch v2

Comment 23

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

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]

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 [:mkmelin]]

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]

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]

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]

Attachment: patch v3

The fix and test.

Comment 29

[Magnus Melin [:mkmelin]]

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]

Attachment: patch v4

Thanks.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/16e20df57d08

Comment 32

[Steve Russie]

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]

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]

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 [:mkmelin]]

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]

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 [:mkmelin]]

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]

And what account will it use for the NNTP protocol? Isn't sending to news just SMTP?

Comment 39

[Magnus Melin [:mkmelin]]

AFAIK, sending to news is just using NNTP (no smtp involved).

Comment 40

[:aceman]

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 [:mkmelin]]

(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 [:mkmelin]]

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

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

Ok, thanks. So the answer is we need a News account in TB for posting to a newsgroup?

Comment 45

[:aceman]

Steve, can you confirm you have a News account configured in TB?

Comment 46

[Steve Russie]

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

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.