[19.0][FIX] mail_composer_cc_bcc: one email per recipient, no Bcc disclosure - #2
Conversation
Two issues show up on 19.0 when sending from the composer with Cc/Bcc: * the Cc-only email Odoo builds when mail.email_to is empty (odoo/odoo@46bad8f0) is a duplicate here, since every Cc partner is a recipient and already gets its own email. It was dropped by comparing lengths and popping the last entry, which actually removed the *last recipient's* email instead: with two Bcc recipients, the second one never received anything. Filter that entry out explicitly instead. * 19.0 fills the 'X-Msg-To-Add' header with every external recipient (MailThread._notify_by_email_get_headers) and merges it into the To header at send time (IrMailServer._alter_message__) to enable Reply-All. This module builds the whole To / Cc itself, so the header has to be dropped: otherwise the Cc and Bcc addresses end up in the To of every email. Both are covered by tests.
|
@skanndar After a quick test, it seems to work for Sale Orders, but I could not get it to work for invoices. |
|
The current BCC handling seems to be tied to the mail composer flow because it relies on: This works for composer-based flows (e.g. Sale Orders), but it seems that other template-based sending flows like invoice sending ( I also noticed this comment in This might be worth considering, as invoice sending appears to use a different recipient generation flow compared to the mail composer. Since the module is named A possible direction could be to propagate the template This would make template BCC work consistently for different modules (e.g. invoices, sales orders, etc.) without introducing module-specific dependencies. I only did a quick review/test, so I may be missing some details, but this seems to explain why it works for Sale Orders but not for invoices. |
NICO-SOLUTIONS
left a comment
There was a problem hiding this comment.
@skanndar
Sorry for the noise. I was initially irritated because I noticed that BCC is not applied when sending invoices.
After reviewing the implementation more closely, it seems the current behavior is correct for the intended scope of mail_composer_cc_bcc. The module extends the Mail Composer flow, and the usage of is_from_composer makes sense in this context. Composer-based flows, such as Sale Orders using the email composer wizard, are covered, and the overall flow and scope look correct.
I also verified the actual email delivery with Sale Orders. The BCC recipient receives the email without being exposed in the visible To/Cc headers, and the regular recipient does not see the BCC recipient. So the BCC handling itself looks correct.
Invoice sending (account.move.send) follows a different template/recipient generation path in core and does not go through the Mail Composer, so it is expected that this module does not affect that flow. The comment in account.move.send about aligning with the standard composer/template recipient management is still worth considering, but this would be a separate improvement to the general template-based email generation flow rather than an issue with this module.
If BCC support is also intended for direct template-based sending flows, a separate module or a more general mail_template_cc_bcc approach might make sense here. That could provide BCC support independently of the Mail Composer flow.
Maybe a small note in the documentation could clarify that the current module covers composer-based emails only and that direct template-based sending flows are not covered.
From my side, code review completed and functional testing performed with Sale Orders. LGTM 👍
Sending flows that do not go through mail.compose.message — invoices via account.move.send being the main one, which calls message_post() directly — are not covered by this module. Requested during review.
|
Thanks both for the reviews, and no noise at all @NICO-SOLUTIONS — the invoice question was worth asking, and your reading is right. I checked it on 19.0: I pushed your documentation suggestion as @Borruso I see you closed #1 — thanks. This branch still carries your commit unchanged, so merging here brings both fixes. @cyrilmanuel this now has two approvals; whenever you merge it into |
Follow-up to my review on OCA#104, pushed as commits as @NICO-SOLUTIONS suggested.
This branch builds on @Borruso's #1 — his commit is included unchanged, because his two fixes (renaming the override to
_prepare_email_message__, which 19.0 renamed, and copying theheadersdict) are what make the Bcc emails reach the SMTP server at all. Merging this brings both; if you would rather keep them separate, tell me and I will rebase onto19.0-mail_composer_cc_bcconce #1 is in.My commit adds two things on top.
1. One email per recipient (currently the last one is silently lost)
When
mail.email_tois empty Odoo builds an extra Cc-only email (odoo/odoo@46bad8f0). Here it is a duplicate: every Cc partner is a recipient and already gets its own email. It was being dropped by comparing lengths:recipientsis aset, and a Cc partner is counted once for the Cc-only entry and once for its own, so the count is short by one andpop()removes the last entry — the last recipient's email — instead of the duplicate. With one To, two Cc and two Bcc partners on 19.0:cc1@bcc2@Filtering the Cc-only entry explicitly (
res = [m for m in res if m["email_to"]]) fixes both, and the length heuristic goes away.2. Drop
X-Msg-To-AddNew in 19.0:
MailThread._notify_by_email_get_headers()fills that header with every external recipient, andIrMailServer._alter_message__()merges it into theToheader at send time to enable Reply-All. Since this module builds the whole To/Cc itself, the header has to go — otherwise the Cc and Bcc addresses end up in theToof every email. I verified this A/B: with the header kept, every recipient (including the plain To one) receivesTo: to@…, cc1@…, bcc1@…, bcc2@….Tests
Two tests, both failing before this commit and passing after:
test_email_cc_bcc_one_email_per_recipient— 5 recipients → 5 emails, same To/Cc on all of them, no Bcc address visible in any To/Cc, and each Bcc recipient gets its own email.test_email_cc_bcc_no_reply_all_header— noX-Msg-To-Addsurvives.The existing
test_email_cc_bccassertedlen(self._mails) == 5and passed either way, because the count was right while the recipients were not.How this was checked
Official
odoo:19image (build19.0.20260723), fresh database, only this module installed, and the SMTP session replaced by a stub recording exactly what Odoo hands to the server (send_message(message, smtp_from, smtp_to_list)) — so the envelopes and headers above are the real ones, not a reading of the code. Full module test suite green (9 tests).