Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[17.0][MIG] mail_composer_cc_bcc: Migration to 17.0 #1312

Merged
merged 24 commits into from
Jul 22, 2024

Conversation

trisdoan
Copy link

@trisdoan trisdoan commented Feb 21, 2024

1) Summary of changes in 17.0

mail.template
https://github.com/odoo/odoo/blob/17.0/addons/mail/models/mail_template.py

  • generate_recipients replaced by _generate_template_recipients

mail.mail:
https://github.com/odoo/odoo/blob/17.0/addons/mail/models/mail_mail.py

  • new method _prepare_outgoing_list: odoo/odoo@b1d678b
  • replaces _send_prepare_values
  • new method _personalize_outgoing_body is also introduced

account.move.send: replaces account_invoice_send

  • do not inherit mail.compose.message
  • _action_send_mail replaced by _send_mail
  • introduce new method _get_mail_move_values

2) Consequences

  • We don't need to fully replace mail.mail:_send_mail() as we did before, thanks to _prepare_outgoing_list.
  • override _get_mail_move_values and _send_mail in account.move.send as it does not benefit from mail.compose.message

3) This change

  • major refactoring:
    • roll back to do as odoo: one mail per recipient (will enable compatibility with mail_tracking)
    • keep same all headers (To, Cc, Bcc) in all emails
    • use a context to define the proper smtp_to for each email
    • introduce test_MailTemplate_upstream_file_hash & test_MailComposer_upstream_file_hash to keep track of changes upstream

_compute_partner_ids

  • remove email_cc from partner_ids to keep consistency with previous versions and create no confusion

_action_send_mail

  • update context when composition_mode == 'comment'

  • My colleague Hai is busy on other topics, I will take over as a maintainer of this module from now on.

@trisdoan trisdoan force-pushed the 17.0-mig-mail_composer_cc_bcc branch from a8827f8 to b5b55df Compare February 21, 2024 02:57
@trisdoan trisdoan marked this pull request as draft February 21, 2024 03:11
@trisdoan trisdoan force-pushed the 17.0-mig-mail_composer_cc_bcc branch 3 times, most recently from ac57d71 to 1966425 Compare February 23, 2024 08:37
@trisdoan trisdoan marked this pull request as ready for review February 23, 2024 08:39
@trisdoan trisdoan mentioned this pull request Feb 23, 2024
30 tasks
@llabusch93
Copy link

Hello,

Could you clarify why the module depends on the account? I couldn't find any justification for this dependency. Could you either remove it or explain the reasoning behind its inclusion?

Thank you.

@nilshamerlinck
Copy link

Good catch @llabusch93 !

  • mail_composer_cc_bcc should not depend on account anymore indeed;
  • only mail_composer_cc_bcc_account will (to adapt the Invoice Sending wizard).

This will be fixed soon.

@trisdoan trisdoan force-pushed the 17.0-mig-mail_composer_cc_bcc branch from 1966425 to f964a6a Compare March 11, 2024 07:16
@trisdoan
Copy link
Author

Hi @llabusch93 and @nilshamerlinck, thank you for your review

I updated as you suggested.

@trisdoan trisdoan force-pushed the 17.0-mig-mail_composer_cc_bcc branch 2 times, most recently from 672de7d to 2772470 Compare March 11, 2024 07:36
)
else:
# DIFFERENT FROM ODOO NATIVE:
context = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is imperative. Instead of augmenting the context, it's being overwritten. This leads to unintended consequences in the sales module, such as orders not being flagged as sent upon sending the quote, due to the replacement of the context key that the sales module relies on.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello @llabusch93, I updated as you suggest 🙏

@gurneyalex
Copy link
Member

/ocabot migration mail_composer_cc_bcc

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Apr 9, 2024
@trisdoan trisdoan force-pushed the 17.0-mig-mail_composer_cc_bcc branch 3 times, most recently from dcbc48f to 38027cc Compare April 21, 2024 06:24
Copy link

@llabusch93 llabusch93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now.

Comment on lines 63 to 66
self.move_ids, wizard.mail_template_id
)
wizard.partner_bcc_ids = self._get_default_mail_partner_bcc_ids(
self.move_ids, wizard.mail_template_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.move_ids, wizard.mail_template_id
)
wizard.partner_bcc_ids = self._get_default_mail_partner_bcc_ids(
self.move_ids, wizard.mail_template_id
wizard.move_ids, wizard.mail_template_id
)
wizard.partner_bcc_ids = self._get_default_mail_partner_bcc_ids(
wizard.move_ids, wizard.mail_template_id

you are iterating over possibly multiple wizards

Comment on lines 32 to 36
def _get_default_mail_partner_cc_ids(self, move, mail_template):
partners = self.env["res.partner"].with_company(move.company_id)
if mail_template.email_cc:
for mail_data in tools.email_split(mail_template.email_cc):
partners |= partners.find_or_create(mail_data)
return partners

def _get_default_mail_partner_bcc_ids(self, move, mail_template):
partners = self.env["res.partner"].with_company(move.company_id)
if mail_template.email_bcc:
for mail_data in tools.email_split(mail_template.email_bcc):
partners |= partners.find_or_create(mail_data)
return partners
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're almost doing twice the same thing here. Would it be possible to factorize this code ?
Also, building a new recordset of partners from a set of partner ids with browse is more efficient than appending records multiple times.

Comment on lines 115 to 112
new_message = move.with_context(
no_new_invoice=True,
mail_notify_author=self.env.user.partner_id.id in partner_ids,
is_from_composer=True,
partner_cc_ids=self.partner_cc_ids,
partner_bcc_ids=self.partner_bcc_ids,
).message_post(
message_type="comment",
**kwargs,
**{
"email_layout_xmlid": "mail.mail_notification_layout_with_responsible_signature", # noqa: E501
"email_add_signature": not mail_template,
"mail_auto_delete": mail_template.auto_delete,
"mail_server_id": mail_template.mail_server_id.id,
"reply_to_force_new": False,
},
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not blocking, but I would prefer having this in more steps.

Suggested change
new_message = move.with_context(
no_new_invoice=True,
mail_notify_author=self.env.user.partner_id.id in partner_ids,
is_from_composer=True,
partner_cc_ids=self.partner_cc_ids,
partner_bcc_ids=self.partner_bcc_ids,
).message_post(
message_type="comment",
**kwargs,
**{
"email_layout_xmlid": "mail.mail_notification_layout_with_responsible_signature", # noqa: E501
"email_add_signature": not mail_template,
"mail_auto_delete": mail_template.auto_delete,
"mail_server_id": mail_template.mail_server_id.id,
"reply_to_force_new": False,
},
)
move_with_context = move.with_context(
no_new_invoice=True,
mail_notify_author=self.env.user.partner_id.id in partner_ids,
is_from_composer=True,
partner_cc_ids=self.partner_cc_ids,
partner_bcc_ids=self.partner_bcc_ids,
)
extra_args = {
"email_layout_xmlid": "mail.mail_notification_layout_with_responsible_signature",
"email_add_signature": not mail_template,
"mail_auto_delete": mail_template.auto_delete,
"mail_server_id": mail_template.mail_server_id.id,
"reply_to_force_new": False,
"message_type": "comment"
}
kwargs.update(extra_args)
move_with_context.message_post(**kwargs)

@gurneyalex
Copy link
Member

@trisdoan can you check @mmequignon's comments?

@trisdoan trisdoan force-pushed the 17.0-mig-mail_composer_cc_bcc branch from d102f75 to f9826d2 Compare June 1, 2024 04:04
@trisdoan
Copy link
Author

trisdoan commented Jun 1, 2024

Hello @mmequignon, @gurneyalex I took your suggestions into account 🙏

@trisdoan trisdoan requested a review from mmequignon June 24, 2024 02:09
@trisdoan
Copy link
Author

Hello @mmequignon's , how does it look to you?

nilshamerlinck and others added 20 commits July 21, 2024 22:04
Currently translated at 83.3% (15 of 18 strings)

Translation: social-16.0/social-16.0-mail_composer_cc_bcc
Translate-URL: https://translation.odoo-community.org/projects/social-16-0/social-16-0-mail_composer_cc_bcc/es/
Currently translated at 100.0% (18 of 18 strings)

Translation: social-16.0/social-16.0-mail_composer_cc_bcc
Translate-URL: https://translation.odoo-community.org/projects/social-16-0/social-16-0-mail_composer_cc_bcc/es/
Currently translated at 100.0% (18 of 18 strings)

Translation: social-16.0/social-16.0-mail_composer_cc_bcc
Translate-URL: https://translation.odoo-community.org/projects/social-16-0/social-16-0-mail_composer_cc_bcc/it/
@trisdoan trisdoan force-pushed the 17.0-mig-mail_composer_cc_bcc branch from f9826d2 to 217a993 Compare July 21, 2024 15:08
@trisdoan
Copy link
Author

Hello @gurneyalex, could you please remerge

@hbrunn
Copy link
Member

hbrunn commented Jul 22, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-1312-by-hbrunn-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a7d3b2b into OCA:17.0 Jul 22, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 8270ee8. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.