271405 - Implement optional warning/confirmation prompt when sending bulk mail to many recipients without using BCC: [plenty/a lot/lots of To or CC recipients: suggest/propose using BCC instead]

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement, P2)

Description

[Søren Thing Pedersen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; da-DK; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; da-DK; rv:1.7.3) Gecko/20040910

In order to limit accidental bulk mails being sent with to: or cc: the user
should be prompted whether the use of to: or cc: is deliberate or whether all
recipients should be put in BCC:. 

The threshold could be variable but for a start I suggest 10 recipients in to:
or cc: combined should activate the warning prompt. The user should also be able
to deactive the warning prompt in preferences and in a "Do not show this prompt
again" checkbox.

Obviously, this is an idea for both suite and Thunderbird.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Christian Schmidt]

I second this suggestion.

Sending email to many recipients is probably not a common situation for most
people, so warning them about potential dangers will be very educating. Acting
irresponsibly may not only harm the sender but also the recipients.

Here is a few examples of situations where using the To or Cc field for sending
bulk mail is dangerous:

- A company wants to send an email to all its customers. For competetive reasons
the company does not want to keep its customer list secret.

- A company wants to send an email to all its customers. The customers may want
to keep it a secret that they are customers of this company (e.g. if the company
sells Viagra).

- Somebody wants to send an email to a lot of people who do not know each other
(e.g. "I have moved - here is my new address"). One of these people have a
computer virus that sends junk to all addresses found in the person's inbox.

- Somebody wants to send an email to a lot of people who do not know each other
(e.g. "I have moved - here is my new address"). One of these people wants to
congratulate the sender and presses "Reply all" and sends a message to the
sender - and everyone else. A few other does the same. Now a few of the
recipients get annoyed and send messages to everyone saying "Please stop
spamming". This makes another person write "I don't think it's spam". And so on.

Comment 2

[Eric Forgeot]

This would be a really good feature, I hope the developper will hear our voice !

Comment 3

[kwoid]

In my comment https://bugzilla.mozilla.org/show_bug.cgi?id=310901#c3 on Bug 310901 I made a similar proposal.

Comment 7

[Age Bosma]

I, for one, would love to see this feature implemented.
A default threshold of 10 as suggested in the initial request is too high imho. I would start at more than 5 or 3 even. Being able to set a threshold in the normal options isn't really required. I doubt that people will use it. Once they are aware of the issue and decide that they should use BCC when sending an e-mail to a specific amount of people they will do so in the future so just a "Do not show this prompt again" checkbox is sufficient.

Comment 8

[Neale Upstone]

This would be a big media win for TB3.  I have just received over 200 email addresses as cc:'d people in an email from our local police!   This could really help the adoption rate of TB.

If this is available as a plugin, I'd suggest making it a builtin...

Comment 9

[Thilo Pfennig]

Yeah, this is should be one of the essential features. I could not find a plugin that does it either. this is much more important than an empty subject line.

Comment 10

[Thomas D. (:thomas8)]

This adds a lot of value and should be seriously considered for TB 3.x.
It's also something we could sell very well, where Mozilla contributes to web hygiene (don't hurt the web by making spammers' life too easy).

(In reply to comment #7)
> I, for one, would love to see this feature implemented.
> A default threshold of 10 as suggested in the initial request is too high imho.

Maybe, maybe not. On the other hand, with less than 10 recipients the probability of unintended harm is much lower, and we don't want to get on people's nerves for no reason. Especially, we DON'T want them to switch off this feature, as this will increase the likeliness of big accidents in the future which we really want to avoid.

> I would start at more than 5 or 3 even.
No (as explained above). Actually, 10 seems like a reasonable default threshold, definitely it shouldn't be below 5 as users will immediately switch that off.

> Being able to set a threshold in the
> normal options isn't really required. I doubt that people will use it. Once
> they are aware of the issue and decide that they should use BCC when sending
> an e-mail to a specific amount of people they will do so in the future so
> just a "Do not show this prompt again" checkbox is sufficient.

No. Different users have different needs. E.g. for a private person, 10 recipients might be a lot, but in a business context, users might only want the warning for bigger numbers of recipients. In the long run, this surely needs to be configurable, so it's probably better to go for configurable from the start (UI needed). Actually, we shouldn't put the checkbox on the dialogue as it's too easy to switch off. Instead, add a note saying "You can customize or turn off this warning in Tools > Options > Composition > Recipients"

An alternative UI might be a red-ish notification bar (similar to Attachment Reminder notification bar). When user adds too many recipients in to or cc, we show notification bar at the top of msg:

+-----------------------------------------------------------------------------+
| You are sending your message too more than _10_ recpients (To or CC). All   |
| recipients will see each other's email addresses, and viruses might harvest | | these addresses for unsolicited mails (spam). You should use BCC so that the| email addresses of the other recipients are hidden. What do you want to do?   |
| Customize this warning...     [Hide email addresses, use BCC] [Don't hide     mail addresses]                                                               |
+-----------------------------------------------------------------------------+
Maybe something shorter with a "More Info" link to expand.

Another interesting thing in this context is Bug 491368 (Add a warning when sending mail to certain recipients).

Comment 11

[Eric Forgeot]

It would definitively be great to propose to switch to BCC when there are too many recipied, so people annoyed by the reminder would allow to automatically hide recipients when they are forwarding their **** funny powerpoint slideshow to their whole address book.

Comment 13

[Thomas D. (:thomas8)]

Adding search words for better retrievability.

Comment 16

[:aceman]

I like this idea.
I would propose to make a pref for the number of recipients to trigger this. Also I propose to not have the "never remind me later" as most users would turn it off at first sight and then it would never kick in when actually needed. Knowing users and enterprises would easily change the pref if needing bulk emails.

I'll check if the number of recipients is available from the compose UI (also expanding mailing lists from addressbook).

Comment 17

[Javi Rueda :javirid]

(In reply to :aceman from comment #16)
> I would propose to make a pref for the number of recipients to trigger this.

I have opened bug 1081608 for this part of the fix. A decision has not been taken, so I did it with UNCONFIRMED status and without filling dependency fields.

Comment 18

[Magnus Melin [:mkmelin]]

aceman: should we add this to the 38 feature list?

Comment 19

[:aceman]

I can look at it and if it passes by you we can think about making it an official 38 feature :)

Comment 21

[Javi Rueda :javirid]

This bug would need a preference to set the threshold. That is why I filed bug 1081608. Althought that is a simple bug which only adds the default value of the preference, some discussion could arise in order to set the value of the threshold.

As that discussion is irrelevant of the way the feature is finally implemented here, I have added bug 1081608 as a dependancy. This way the discussions could be made in a more untidy way.

Then, I will not ask for bug 1081608 to be landed on c-c until this one is going to land also.

Comment 22

[Jim Porter (:squib)]

There's really no need to have a separate bug just to create a pref for this bug. All that does is split the discussion over two bugs with no clear benefit.

Comment 23

[:aceman]

Attachment: patch

This could be it. We just need proper wording. I think there should be some question in the dialog so that the user can answer OK/Cancel or Yes/No.

Comment 24

[Jim Porter (:squib)]

(In reply to :aceman from comment #23)
> This could be it. We just need proper wording. I think there should be some
> question in the dialog so that the user can answer OK/Cancel or Yes/No.

Please try to avoid OK/Cancel or Yes/No. They're usually not very clear. Some other options would be "Continue/Cancel" (ok) or "Send Anyway/Go Back" (better). There might be something better still. I'll let others chime in on the wording here.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 8673309 [details] [diff] [review]
patch

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

I think this should go into a notification bar instead.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +146,5 @@
>  addressInvalid=%1$S is not a valid e-mail address because it is not of the form user@host. You must correct it before sending the e-mail.
>  
> +## Strings used by the alert that tells the user that an e-mail address is invalid.
> +manyPublicRecipientsTitle=Too many public recipients
> +manyPublicRecipients=You are sending this message to many recipients which will see each other. Consider using the Bcc field instead of To and Cc to protect their privacy.

Maybe something like

All the N recipients will see each others e-mail addresses. To prevent this and protect their privacy you can use the Bcc field.  [Use Bcc instead]

Comment 26

[Richard Marti (:Paenglab)]

(In reply to Magnus Melin from comment #25)
> Comment on attachment 8673309 [details] [diff] [review]
> 
> I think this should go into a notification bar instead.

Yes, this would not or less break the user's workflow. But then the check should be made immediately by reaching the limit and not at sending.

> Maybe something like
> 
> All the N recipients will see each others e-mail addresses. To prevent this
> and protect their privacy you can use the Bcc field.  [Use Bcc instead]

I like this. The [Use Bcc instead] implies a automatism. Maybe a [Let me change]   [Continue without change] where the "Let me change" could focus the first To: dropdown and shows the notification again or at least at sending and the "Continue without change" dismisses the notification bar until the next new message.

Comment 27

[:aceman]

Let's not make a new module out of this similar to the attachment checker :)
For a notification bar the check would have to run periodically or on a keypress or on recipient input, so could become CPU heavy as the attachment checker (which parses whole text). We already have a checker like that for enabling the Send button.

But it seems to me the wording you guys is still about a dialog with 2 buttons. Should there be a notification bar (just informative) and then a dialog on send with the 2 options?

Comment 28

[Magnus Melin [:mkmelin]]

It'd only have to be run on adding a recipient.
My suggestion was for the notification bar text + a button that would change addresses to use Bcc instead.

Comment 29

[Richard Marti (:Paenglab)]

When no notification bar then Magnus' text would also work in a dialog. Then the buttons could be [Let me change]   [Continue].

Comment 30

[:aceman]

(In reply to Magnus Melin from comment #28)
> It'd only have to be run on adding a recipient.
> My suggestion was for the notification bar text + a button that would change
> addresses to use Bcc instead.

Automatically change all To/Ccs to Bcc?

Comment 31

[Magnus Melin [:mkmelin]]

Yes. Remember this feature would be useful not to have people send their mail to 200 people. That's not something anyone wants to try to change manually.

Comment 32

[Richard Marti (:Paenglab)]

There exists servers which need at least one To: address. Then the sender address should be added to To:

Comment 33

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #32)
> There exists servers which need at least one To: address. Then the sender
> address should be added to To:

This is handled in the code by adding "undisclosed-recipients" as recipient.

Comment 34

[Richard Marti (:Paenglab)]

Aceman, are you awaiting my ui-r? Or is all clear how you would go further?

Comment 35

[:aceman]

What is the suggested location of that notification bar? If it is at the same spot as the attachment notification, I think one of them will obscure the other if fired simultaneously.

Comment 36

[Magnus Melin [:mkmelin]]

Notifications have a priority, we just decide which is more important. (Once you close one, the one "under it" becomes visible.

Comment 37

[Richard Marti (:Paenglab)]

At the same position like the attachment notification.

From the workflow I would say first should the attachment notification be shown. By pressing the send button, after the user chooses through the requester he wants not to add the attachment, the bulk mail warning can be shown. When he adds an attachment the bulk warning appears automatically as it's the only one now.

Comment 38

[:aceman]

(In reply to Magnus Melin from comment #36)
> (Once you close one, the one "under it" becomes visible.

That is the catch I ask about. We probably want the bar here to be ignorable so that the user does not need to close it.

Comment 39

[Magnus Melin [:mkmelin]]

Thinking about it I'd say they could come just in order of appearance. They come from different parts (adding addresses vs writing the mail) of the work-flow so one would assume the user would have seen the earlier notification already.

Comment 40

[Wayne Mery (:wsmwk)]

is this complex for a good first bug?

Comment 41

[:aceman]

This is a good first bug. There is even a patch that implements it :) Somehow we just want to convert it to a notification. Hopefully running it on adding any recipient will not break the 4000 recipients use cases.

Comment 43

[Vader]

_13 YEARS_ WITHOUT reaction and implemetation? Please set higher priotity of this bug!

Comment 44

[Vader]

(In reply to Magnus Melin from comment #25) 
> Maybe something like
> 
> All the N recipients will see each others e-mail addresses. To prevent this
> and protect their privacy you can use the Bcc field.  [Use Bcc instead]

Good idea but also there should be legal information. Part of people "has nothing to hide". 

Additional buttons:
first - people don't read massege boxes and clicks the first button - [Help] - here description about privacy and visibility of e-mail adresses. 
second - [Go back]
4th- [move all from CC to BCC]
5th- [move all from To to BCC]

Comment 45

[:aceman]

I should get back to this :)
Meanwhile, there is an addon that does this: https://addons.mozilla.org/thunderbird/addon/use-bcc-instead/

Comment 47

[Nadia]

This function should be a standard due to GDPR (2016/679) law in EU. Not addition but standard function. Default Threshold 5 persons (configurable) and there should ba a guide why and when to use BCC. Majority of people do not know why ans when should use the BCC. 

Because people doesn't read warnings and info boxes, the first dialog box should contain information and second dialog box should contain question "Do you really want to send e-mail to %d people without use of BCC?" and link to guide second time.

In my opinion it is well-known security problem. Mainly in companies.

Comment 48

[Richard van den Berg]

It is also important to note that the add on mentioned in comment #45 no longer works in TB 60. Please make this standard functionality.

Comment 49

[surbhianand231994]

Hi everyone, I am new to the Open source world but would like to take this up. Please let me know if I can start with it and what are the things that I should start with.

Comment 50

[Magnus Melin [:mkmelin]]

Hello, see the patch attached in this bug and comment 25.

Pointer for using notification bar: https://searchfox.org/comm-central/search?q=msgNotificationBar&case=false&regexp=false&path=mail

Comment 51

[surbhianand231994]

(In reply to Magnus Melin [:mkmelin] from comment #28)
> It'd only have to be run on adding a recipient.
> My suggestion was for the notification bar text + a button that would change
> addresses to use Bcc instead.

So, it should run only when number of receiver's shoot max limit. For this, there need to be some checker that continously, checks the current receiver's list count. Is such a checker required, if yes, Is it already there? If not, any pointers to how can I add such a checker.

Comment 52

[Magnus Melin [:mkmelin]]

I don't think there is one. 
document.querySelectorAll("#addressingWidget .addressingWidgetItem").length gives you the number of items

See https://searchfox.org/comm-central/source/mail/test/mozmill/composition/test-reply-addresses.js#78

Comment 53

[PeterPan]

Why not implement three radio button like: 

-> "Notification to use BCC instead"

In settings -> compostion -> general

As soon as more than one recipient is chosen the sender will get a notification ("You are sending to a group of recipients. Do you want to use BCC instead?) and the possibility (Button) Yes/No.

-> The other radio button could be "Send mails generally as BCC". 

-> Another: "Do not ask". 

Another possibility would be to be able to define email groups to be sent as BCC or To generally. So during to creation of your mail groups you could chose whom to put into the "BCC" mail group and whom to put in a "To" mail group.

This is really an important feature.

Comment 55

[Nadia]

Reported: 15 years ago and STILL not solved? Please set P2 priority. It looks never be solved!!!

Comment 56

[[e-mail]]

This is a necessity, really.
Highest priority to protect humans from themselves.

Up vote!

Comment 57

[[e-mail]]

P1

Comment 58

[Wayne Mery (:wsmwk)]

https://addons.thunderbird.net/en-US/thunderbird/addon/use-bcc-instead-c/ is another workaround, but apparently only for v64 and newer.

Comment 59

[PeterPan]

(In reply to Wayne Mery (:wsmwk) from comment #58)

https://addons.thunderbird.net/en-US/thunderbird/addon/use-bcc-instead-c/ is another workaround, but apparently only for v64 and newer.

It says here about this addon while trying to install:

"Not compatible with thunderbird 60.6.1"

(64bit version) on Mint Cinammon 19.1

Comment 60

[Nadia]

I use 32-bit version of Thunderbird and I also need this addon or proposed patch. $ years ago aceman published workaround (in attachment). Why it is not added to thunderbird?

Comment 61

[:aceman]

Attachment: 271405.patch v2 WIP

WIP patch with the notification bar.
Open for comments if this is the right direction and what needs polishing yet.

Comment 62

[:aceman]

(In reply to Nadia from comment #60)

I use 32-bit version of Thunderbird and I also need this addon or proposed patch. $ years ago aceman published workaround (in attachment). Why it is not added to thunderbird?

Because it was deemed to not be the right solution. I have now uploaded a new one according to the requests.

Comment 63

[Richard Marti (:Paenglab)]

Comment on attachment 9061185 [details] [diff] [review]
271405.patch v2 WIP

f- because when I choose "Use Bcc instead" the notification bar hides and it sends still with To. And only the first recipient gets the mail.

I've also set the pref to 2 to see the notification earlier. After entering the second email address with return (the focus goes to the third, empty To: field) the notification pops up and speaks of 3 recipients. This is wrong, I entered only two. The notification should appear when three recipient are entered.

Comment 64

[Magnus Melin [:mkmelin]]

Comment on attachment 9061185 [details] [diff] [review]
271405.patch v2 WIP

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

Didn't try it, but I think the direction looks ok

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +169,5 @@
>  addressInvalidTitle=Invalid Recipient Address
>  addressInvalid=%1$S is not a valid e-mail address because it is not of the form user@host. You must correct it before sending the e-mail.
>  
> +## Strings used by the notification about many public recipients.
> +manyPublicRecipients=All the %S recipients will see each others e-mail addresses. To prevent this and protect their privacy you can use the Bcc field.

the second sentence could be

You can protect their privacy by using the Bcc field, which means recipients can't see who else got this message.

::: mailnews/mailnews.js
@@ +851,5 @@
>  // When there is no disclosed recipients (only bcc), we should address the message to empty group
>  // to prevent some mail server to disclose the bcc recipients
>  pref("mail.compose.add_undisclosed_recipients", true);
>  
> +// The number of public recipients before we offer BCC addressing.

unclear from the description if it's < or <=

@@ +852,5 @@
>  // to prevent some mail server to disclose the bcc recipients
>  pref("mail.compose.add_undisclosed_recipients", true);
>  
> +// The number of public recipients before we offer BCC addressing.
> +pref("mail.compose.warn_max_public_recipients", 10);

10 is fairly small. maybe 15?

Comment 65

[Nadia]

10 is fairly small. maybe 15?

No. Due to GDPR the default value should be 5 + possibility of changing the value in settings.

Comment 66

[Nadia]

Additionally if domains are different, the limit should be 2.

Comment 67

[Nadia]

Additionally if domains are different, the limit should be 2 (max 2 different domains in "To" + "CC" fields).

Comment 68

[Age Bosma]

@Nadia, can you back this claim with proof? I find it hard to believe the GDPR is this specific about sending emails.

Comment 69

[bugzilla_mozilla]

(In reply to Nadia from comment #66)

Additionally if domains are different, the limit should be 2.

What? Sending an email to three members of my family, who use different freemailers, already should trigger this? I should send email as BCC to my family members? Because of GDPR????

Hundreds of mails in my inbox would match this condition. And for 99% of them, BCC is not the better option.

If you trigger this warning all the time, this will only cause people to disable the warning completely or increase the limit to 999. Better choose a reasonable default so it triggers only in cases where it is required.

10 is a good start I think.

Comment 70

[Age Bosma]

I'd say 5 is a good start, GDPR or not. Less is overkill with too much chance BCC is not desired and thus receiving too many notifications. More than ten allows for too many e-mails to slip through.
It's too bad we don't really have any statistics. In my case maybe 95% of my e-mails have a single recipient, 4% have two to three recipients and only 1% have four or more.

Comment 71

[bugzilla_mozilla]

Here are my personal stats. I analyzed ~ 4500 mails I sent myself since 2002, not the ones I received. This is a private email account.

5,2% of the mails I sent have more than one To-recipient, i.e. at least 2.
1% has at least 3 To-recipients
0,09% have at least 4 To-recipients
Only a single mail had 5 To-recipients (and no mail had more).

10,5% have at least one CC recipient
0,9% have at least two CC recipients
0,09% have at least three CC recipients
No mail has more than three CC recipients.

1,2% have at least one BCC recipient.

Note these statistics only consider the number of CC- or To-recipients, but do not add both. This is due to the simple grep way I used to get these numbers, it would be more complicated to get better numbers.

From these numbers, it seems a limit of 5 would have only triggered once or twice in 17 years. But I am also not somebody who is sending massmails.

I believe a corporate mail account would look a lot different, as people tend to CC their boss, colleagues, ticketing-systems and so on. I may do a statistics of a corporate mail account later.

Comment 72

[Nadia]

My statistics are similar. Good will be 5 persons for personal use and work use. I only considered work related e-mail but not personal ones.

Due to GDPR will be good solution to ask during installation, how Thunderbird will be used (for work or personal stuff)?
In work related use a limit on domain count can be crucial. Sending of one e-mail to 4 clients may lead to legal isseus.

Comment 73

[Magnus Melin [:mkmelin]]

This is about avoiding unintentional mass mailings to large number of people. There is no need to have the notification for small numbers of people. Even a small project, or some mailing to say family can easily involve more than 10 people.

Comment 75

[Meteor0ID]

Since this feature is about preventing data leaks, would it not make sense to set a priority on this enhancement request? This has been open for 15 years now, think about how many people could have been warned before making a mistake.

Comment 77

[Dave Royal]

I took over https://addons.thunderbird.net/en-US/thunderbird/addon/use-bcc-instead/ from the original author, because I used it: https://addons.thunderbird.net/en-US/thunderbird/addon/use-bcc-instead-c/
The old and new versions together have about 800 users.

My use case is sending out newsletters and announcements to members of local meeting groups. Typically I have a mailing list of 10 to 20 people and I want to avoid putting it on the TO or CC line.

The addon also has the facility to set the default recipient on the compose window to TO. I know some that people use the addon only for that.

Comment 78

[Dave Royal]

Correction:
The addon also has the facility to set the default recipient on the compose window to BCC.

Comment 79

[Thomas D. (:thomas8)]

(In reply to Meteor0ID from comment #75)

Since this feature is about preventing data leaks, would it not make sense to set a priority on this enhancement request? This has been open for 15 years now, think about how many people could have been warned before making a mistake.

Absolutely. 11 duplicates are testimony, many of them recent.

(In reply to Magnus Melin [:mkmelin] from comment #73)

This is about avoiding unintentional mass mailings to large number of people.

Sounds like a must-have for enterprise deployment, might be less frequent/relevant (but still relevant) for private use.

There is no need to have the notification for small numbers of people.

What number of recipients is considered "small" (aka no warning needed) may depend much on scenario. Different users, different needs (see comment 77). There's certainly no one-for-all good number here. Definitely should have a pref which allows setting the number both higher and lower than the default which we'll ship.

(In reply to Dave Royal from comment #77)

My use case is sending out newsletters and announcements to members of local meeting groups. Typically I have a mailing list of 10 to 20 people and I want to avoid putting it on the TO or CC line.

Perfectly valid use case and thanks David for respecting the right to privacy for those small groups of yours.
Companies might need a different threshold so that they can send their internal mails to enough recipients without getting the warning.

Comment 80

[Lasana Murray]

Attachment: bug271405.patch

Cleaned up the patch a bit, using fluent instead of string bundle, implemented the bcc transfer.

Comment 81

[Dave Royal]

I thought it worth mentioning here that the latest incarnation of the old Use BCC Instead addon
https://addons.thunderbird.net/en-GB/thunderbird/addon/limit-non-bcc-recipients/
includes an option, which I called 'fixed limit', to disallow exceeding the limit of public recipients. It removes the 'OK' ('keep') button - your only option is to change the public recipients to BCC or return to the compose window.

My addon didn't originally have this option, though it's in the UseBCC Instead ones:
https://addons.thunderbird.net/en-GB/thunderbird/addon/use-bcc-instead/
https://addons.thunderbird.net/en-US/thunderbird/addon/use-bcc-instead-c/

I added the option - it's a bit of a kludge - at the request of someone in charge of a non-profit organisation who wanted to ensure that his staff could not accidently compromise the privacy of his clients (who were alcoholics in therapy IIRC).

So you might want to consider whether to include this option. I'm not advocating for it, and I don't know whether it's a widely used feature of my addon or the old ones (which have about 800 users in total).

Comment 82

[Thomas D. (:thomas8)]

Comment on attachment 9211950 [details] [diff] [review]
bug271405.patch

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

I am snatching this review for now because
- I started advocating for this feature 12 years ago
- I've been working a lot on the addressing area with Alex
- This is enterprise-relevant

Thank you very much Lasana for working on this! This will be a great feature addition!

This is going in the right direction. I like the gRecipientObserver!
Let's iterate on this to fix some functional flaws, code style nits, and polish pref and variable names.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3820,5 @@
> +      }
> +    });
> +  });
> +  gRecipientObserver.observe(document.getElementById("toAddrContainer"), {
> +    childList: true,

Elegant! Observing the address row childList looks like a smart thing to do.

@@ +5034,5 @@
>  
> +/**
> + * Check if there are too many public recipients and offer to send them as BCC.
> + */
> +function CheckTooManyPublicRecipients() {

camelCase for new functions pls.
Maybe we could call this `checkPublicRecipientsLimit()`?

@@ +5035,5 @@
> +/**
> + * Check if there are too many public recipients and offer to send them as BCC.
> + */
> +function CheckTooManyPublicRecipients() {
> +  let recipLimit = Services.prefs.getIntPref(

warnPublicRecipientsThreshold (camel case version of the pref name which I am suggesting below)

@@ +5036,5 @@
> + * Check if there are too many public recipients and offer to send them as BCC.
> + */
> +function CheckTooManyPublicRecipients() {
> +  let recipLimit = Services.prefs.getIntPref(
> +    "mail.compose.warn_max_public_recipients"

We might add another pref later to enforce the threshold in enterprise/organisational environments (see https://bugzil.la/271405#c81), so let's be ready for that. I'm not sure if we have best practices for pref naming, but I think using dashes looks lighter (less bulky than underscore) and is easier to read and use (also better than camelCase).

"mail.compose.warn-public-recipients.threshold"

Then later, we might add:

"mail.compose.warn-public-recipients.enforce"

@@ +5039,5 @@
> +  let recipLimit = Services.prefs.getIntPref(
> +    "mail.compose.warn_max_public_recipients"
> +  );
> +
> +  let addressPills = document.querySelectorAll(

Let's be more descriptive for better readability: let publicAddressPills

@@ +5040,5 @@
> +    "mail.compose.warn_max_public_recipients"
> +  );
> +
> +  let addressPills = document.querySelectorAll(
> +    "#toAddrContainer > mail-address-pill"

CC addresses are also public, so please include them in the selector!

@@ +5043,5 @@
> +  let addressPills = document.querySelectorAll(
> +    "#toAddrContainer > mail-address-pill"
> +  );
> +
> +  let shouldNotify = addressPills.length > recipLimit;

Please inline this comparison into the condition.

@@ +5047,5 @@
> +  let shouldNotify = addressPills.length > recipLimit;
> +  let notification = gComposeNotification.getNotificationWithValue(
> +    "manyPublicRecipientsReminder"
> +  );
> +  if (!shouldNotify) {

Pls add a comment.

@@ +5055,5 @@
> +    return;
> +  }
> +
> +  if (notification) {
> +    gComposeNotification.removeNotification(notification);

We should not remove the notification if we still need it, but update strings (the count) only instead (if possible), see below. If you'd always want to remove the notification, this should go above the if (!shouldNotify) check, and be removed from inside that conditional.

@@ +5058,5 @@
> +  if (notification) {
> +    gComposeNotification.removeNotification(notification);
> +  }
> +
> +  // Construct the notification as we don't have one.

This looks very expensive wrt performance, as we'll reconstruct the entire notification every time a pill gets added or deleted. With lots of pills, deletion via holding backspace is actually visibly slowed down with hickups.
Is it possible to create this notification outside the function once, and only update strings (including the count) via the generic fluent methods? Then we also won't need to remove the notification if we know it's staying around.

Unfortunately I'm not a notification expert.

@@ +5063,5 @@
> +  let msg = document.createXULElement("hbox");
> +  msg.setAttribute("flex", "100");
> +
> +  let msgText = document.createXULElement("label");
> +  let args = JSON.stringify({ count: addressPills.length });

pls inline args where you use it below

@@ +5073,5 @@
> +
> +  let bccButton = {
> +    "l10n-id": "manyPublicRecipientsBCC",
> +    callback() {
> +      let addresses = [];

publicAddresses

@@ +5074,5 @@
> +  let bccButton = {
> +    "l10n-id": "manyPublicRecipientsBCC",
> +    callback() {
> +      let addresses = [];
> +      for (let addr of addressPills) {

This would be `let pill of publicAddressPills`, but it's much easier to use spread operator (...).
For in-place array creation, pls do something similar as in moveSelectedPills():
https://searchfox.org/comm-central/rev/6ed21f162dcf7bb6378d139520aa8578c75a5e5b/mail/base/content/mailWidgets.js#2985-2987

      // Store all the selected addresses inside an array.
      let selectedAddresses = [...this.getAllSelectedPills()].map(
        pill => pill.fullAddress
      );

(Adjust accordingly, as we're not using selected pills)

@@ +5075,5 @@
> +    "l10n-id": "manyPublicRecipientsBCC",
> +    callback() {
> +      let addresses = [];
> +      for (let addr of addressPills) {
> +        addresses.push(addr.emailAddress);

Unfortunately, this will eliminate all display names. You want pill.fullAddress to fill the array (see above)

@@ +5079,5 @@
> +        addresses.push(addr.emailAddress);
> +      }
> +
> +      let toInput = document.querySelector("#toAddrInput");
> +      recipientClearPills(toInput);

pls inline the selector

@@ +5082,5 @@
> +      let toInput = document.querySelector("#toAddrInput");
> +      recipientClearPills(toInput);
> +
> +      let bccInput = document.querySelector("#bccAddrInput");
> +      let bccLabel = document.querySelector("#addr_bcc");

These would need to be inlined, but then, we won't need them

@@ +5083,5 @@
> +      recipientClearPills(toInput);
> +
> +      let bccInput = document.querySelector("#bccAddrInput");
> +      let bccLabel = document.querySelector("#addr_bcc");
> +      bccInput.value = addresses.join(",");

I think we can avoid adding temporary text to the input and create pills directly, like this:

awAddRecipientsArray("addr_bcc", publicAddresses, true);

@@ +5084,5 @@
> +
> +      let bccInput = document.querySelector("#bccAddrInput");
> +      let bccLabel = document.querySelector("#addr_bcc");
> +      bccInput.value = addresses.join(",");
> +      showAddressRow(bccLabel, "addressRowBcc");

showAddressRow not needed if you use awAddRecipientsArray()

@@ +5101,5 @@
> +  };
> +
> +  notification = gComposeNotification.appendNotification(
> +    "",
> +    "manyPublicRecipientsReminder",

"warnPublicRecipientsNotification"

@@ +5106,5 @@
> +    null,
> +    gComposeNotification.PRIORITY_WARNING_MEDIUM,
> +    [bccButton, ignoreButton]
> +  );
> +  notification.setAttribute("id", "manyPublicRecipientsReminder");

"warnPublicRecipientsNotification"

@@ +5494,5 @@
>    if (!automatic) {
>      gContentChanged = true;
>    }
> +  if (gRecipientObserver) {
> +    CheckTooManyPublicRecipients();

checkPublicRecipientsLimit();

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +115,5 @@
>  button-return-receipt =
>      .label = Receipt
>      .tooltiptext = Request a return receipt for this message
> +
> +#   $count (Number) - the count of addresses in the to field

... in the To and CC fields

@@ +116,5 @@
>      .label = Receipt
>      .tooltiptext = Request a return receipt for this message
> +
> +#   $count (Number) - the count of addresses in the to field
> +manyPublicRecipients = All of the {$count} recipients will see each others e-mail addresses. To prevent this and protect their privacy you can use the Bcc field.

Quite good, I like this message. Some spelling/grammar nits, and we must mention To or Cc because there might be existing Bcc recipients already.

manyPublicRecipientsNotification = All of the {$count} recipients in the To or CC fields will see each other's names and e-mail addresses. To prevent this and protect their privacy, you can use the Bcc field instead.

For bonus points, check in which of To/CC/or both there are pills and dynamically adjust the fluent string accordingly.

@@ +120,5 @@
> +manyPublicRecipients = All of the {$count} recipients will see each others e-mail addresses. To prevent this and protect their privacy you can use the Bcc field.
> +
> +manyPublicRecipientsBCC = 
> +  .label = Use Bcc instead
> +  .accesskey = B

We may use B as accesskey for Bcc address row, so better to use U here.

@@ +123,5 @@
> +  .label = Use Bcc instead
> +  .accesskey = B
> +
> +manyPublicRecipientsIgnore=
> +  .label = Keep recipient types

Keep recipients public

::: mailnews/mailnews.js
@@ +945,5 @@
>  // to prevent some mail server to disclose the bcc recipients
>  pref("mail.compose.add_undisclosed_recipients", true);
>  
> +// The number of public recipients before we offer BCC addressing.
> +pref("mail.compose.warn_max_public_recipients", 15);

pref name as suggested above

Comment 83

[Thomas D. (:thomas8)]

Also, please warn if (publicAddressPills.length >= warnPublicRecipientsThreshold).
The threshold logic should be "warn if 15 pills or more" when the pref is set to 15.

Comment 84

[Thomas D. (:thomas8)]

(In reply to Dave Royal from comment #81)

option to disallow exceeding the limit of public recipients.
... a non-profit organisation who wanted to ensure that staff could not accidently compromise the privacy of clients
So you might want to consider whether to include this option.

Thanks Dave, I think that's a great idea for a followup bug!

Comment 85

[Magnus Melin [:mkmelin]]

Re pref namings: no dashes please.
For the pref about enforcing, I don't think we want that. But, we could/should make this similar to the attachment reminder which also has the attachment_reminder_aggressive pref which forces you to ok the send if in a dialog if you didn't take action.

Comment 86

[Lasana Murray]

(In reply to Thomas D. (:thomas8) from comment #82)

Comment on attachment 9211950 [details] [diff] [review]
bug271405.patch

Review of attachment 9211950 [details] [diff] [review]:

I am snatching this review for now because

• I started advocating for this feature 12 years ago
• I've been working a lot on the addressing area with Alex
• This is enterprise-relevant

Magnus, you ok with this?

Comment 87

[Magnus Melin [:mkmelin]]

Well, let me take the review once you updated the patch. Most of the review comments look valid.

Comment 88

[Lasana Murray]

@@ +5047,5 @@

• let shouldNotify = addressPills.length > recipLimit;
• let notification = gComposeNotification.getNotificationWithValue(
• "manyPublicRecipientsReminder"
• );
• if (!shouldNotify) {

Pls add a comment.

About what exactly?

We should not remove the notification if we still need it, but update strings (the count) only instead (if possible), see below. If you'd always want to remove the notification, this should go above the if (!shouldNotify) check, and be removed from inside that conditional.
...

This looks very expensive wrt performance, as we'll reconstruct the entire notification every time a pill gets added or deleted. With lots of pills, > deletion via holding backspace is actually visibly slowed down with hickups.
Is it possible to create this notification outside the function once, and only update strings (including the count) via the generic fluent methods? > Then we also won't need to remove the notification if we know it's staying around.

Unfortunately I'm not a notification expert

It does not look like that is supported as far as I can tell. I think the DOM will still be modified each time so it may be better to debounce the function.

Comment 89

[Lasana Murray]

Attachment: bug271405v2.patch

Implemented most of the feedback. Added a function to debounce display of the warning while the user is still making changes.

Comment 90

[Magnus Melin [:mkmelin]]

Comment on attachment 9212508 [details] [diff] [review]
bug271405v2.patch

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

The patch has bitrotted so I didn't try it out yet.
Could be worth moving to phab.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5036,5 @@
> +/**
> + * Check if there are too many public recipients and offer to send them as BCC.
> + */
> +function checkPublicRecipientsLimit() {
> +  console.error("bang bang");

remove

@@ +5052,5 @@
> +    document.querySelectorAll("#toAddrContainer > mail-address-pill")
> +  );
> +  let ccAddrPills = Array.from(
> +    document.querySelectorAll("#ccAddrContainer > mail-address-pill")
> +  );

No need to use Array.from since you're going to spread them below anyway.

::: mailnews/mailnews.js
@@ +945,5 @@
>  // to prevent some mail server to disclose the bcc recipients
>  pref("mail.compose.add_undisclosed_recipients", true);
>  
> +// The number of public recipients before we offer BCC addressing.
> +pref("mail.compose.warn_public_recipients.threshold", 2);

Far too small. Let's make it 15.

Comment 91

[Lasana Murray]

Attachment: Bug 271405 - Warn users about sending email to too many addresses. r=mkmelin

Comment 92

[Lasana Murray]

Un-bitrotted and debug code cleaned up, moved to phab.

Comment 93

[Lasana Murray]

Attachment: Bug 271405 - Add tests for public recipients warning. r?mkmelin

Depends on D110343

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c1dd78a39801
Warn users about sending email to too many addresses. r=mkmelin
https://hg.mozilla.org/comm-central/rev/8e3f134f37fe
Add tests for public recipients warning. r=mkmelin

Comment 95

[Óvári]

In addition to Bcc, can users please also be referred to the Mail Merge add-on by Alexander Bergmann?

What do you think?

Thank you

Comment 96

[Magnus Melin [:mkmelin]]

That is a different use case. And we don't really advertise add-ons in our UI.

Comment 97

[Óvári]

we don't really advertise add-ons in our UI.

Wasn't the Lightning add-on previously advertised until it was merged into core?

Surely the Mail Merge add-on by Alexander Bergmanncan be advertised until there is a suitable feature in core?

Otherwise, surely you can advertise the Add-ons for Thunderbird in the UI? Perhaps there could be a message to Search Add-ons for Thunderbird for how to individually send a message to a group of recipients. Advertising and promoting Add-ons for Thunderbird is beneficial to the Thunderbird program, Add-on Developers and Thunderbird users. A real win-win-win scenario.

What do you think?

Thank you

Comment 98

[Magnus Melin [:mkmelin]]

It was never advertised no.
We have the add-on manager in the menu. I don't think there's much more we should do.

Comment 99

[Mitra Ardron]

Regressions: 171245

Comment 100

[Mitra Ardron]

I'd like to request that this change be backed out of the beta channel (v 89) as because of bug #1706204 it is data-loosing, so we've now got a data-loosing bug in a version that people auto-update to ! (It just bit me, with significant , real world consequences as people didn't get the message and were silently dropped from a conversation chain).

Comment 101

[Hans de Goede]

Question is there any way to turn the new warning off, or tweak the limit at which it triggers? (a quick duckduckgo did not find anything)

I understand and even appreciate why it was added, but for some workflows it just gets in the way.

As a kernel developer I regularly get emails with quite a few people in the Cc and the kernel-netiquette is to do reply-to-all so that everyone who is interested gets the entire thread without them needing to be subscribed to various lists. Moving to Bcc would break the reply-to-all.

Let me know if I should file a RFE bug to make this configurable.

Comment 102

[Hans de Goede]

Never mind, looking at the patch implementing this I see that there is a "mail.compose.warn_public_recipients.threshold" config setting for this, which answers my question.

For anyone reading along and who does not like the new warning, simply bump "mail.compose.warn_public_recipients.threshold" to a higher value, e.g. 50.