[15.0][FIX] mail_show_follower: Use company from the document to render messages correctly #1464
Conversation
|
Hi @yajo, |
This module aims to show the recipients of an email in the email header, but the previous implementation was not suitable for internal notes. When **logging an internal note** with tagged users and/or partners, those specific recipients should be displayed as well as the document's followers subscribed to this message subtype. When **sending an email**, the recipients should be the document's followers. This is achieved by using the union of `mail.mail`'s `recipient_ids` and `notified_partner_ids`.
carlos-lopez-tecnativa
force-pushed
the
15.0-fix-mail_show_follower-multicompany
branch
from
666a2f5 to
a8af850
Compare
|
Backport of #1391 |
| # recipients from any Notification Type (i.e. email, inbox, etc.) | ||
| recipients = mail.notification_ids.res_partner_id | ||
| record = self.env[mail.model].browse(mail.res_id) | ||
| company = getattr(record, "company_id", self.env.company) |
There was a problem hiding this comment.
suggestion: This is not the recommended way to get a field that you don't know if exists, because that attribute could perfectly be a method (imagine def company_id():) and break your logic. Hardly probable, but possible.
The rule of thumb way to make sure it's really a field:
| company = getattr(record, "company_id", self.env.company) | |
| company = record["company_id"] |
Of course there can be a KeyError which you can circumvent with a try-except block, or a if "company_id" in record guard.
| recipients = mail.notification_ids.res_partner_id | ||
| record = self.env[mail.model].browse(mail.res_id) | ||
| company = getattr(record, "company_id", self.env.company) | ||
| show_internal_users = company and company.show_internal_users_cc |
There was a problem hiding this comment.
suggestion: This guard is unnecessary:
| show_internal_users = company and company.show_internal_users_cc | |
| show_internal_users = company.show_internal_users_cc |
| # it is saved in the body_html field so that it does | ||
| # not appear in the odoo log | ||
| mail.body_html = final_cc + mail.body_html | ||
| group_user = self.env.ref("base.group_user") |
There was a problem hiding this comment.
issue: although the PR description explains a clear and concise problem, the code I see here is a medium refactor of a very important (and yes, spaghethic) piece of code.
For example, you are going away from testing portal group membership into testing backend user membership.
So, the PR goes beyond scope and gets harder to review. Could you split the fix and the refactor in 2 different PRs? Or could you explain a little bit more what's this refactor doing, so it's easier to review?
…sages correctly + proper lang handle In a multi-company environment, if one company uses the show_followers_partner_format by partner_name and another company uses partner_email, a user with permissions in both companies may experience issues. If a document belongs to Company A, but the current company is set to Company B, the message format is taken from the current company (Company B) instead of the document’s company (Company A). This fix ensures that the message format is rendered based on the document's company. It also fixes the lang handling for cases with False values.
pedrobaeza
force-pushed
the
15.0-fix-mail_show_follower-multicompany
branch
from
a8af850 to
1de314a
Compare
|
@yajo this is a backport of #1391, with only an extra patch in a separated commit, so all your comments should go to the 16.0 PR when it was done, and it was you who approve it. We are not going to invest more time in that part, as we will finally get to the newest version with the refactoring already done. The only patch from us applied right now is the company one, but we have changed also the lang part, which indeed can have some side effects (but this is not new). We will fw-port it to 16 if this PR is accepted. Please accept it as it is to get both versions with the good refactoring, or we will let it die as a PR injected where needed. |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at b786332. Thanks a lot for contributing to OCA. ❤️ |
|
@carlos-lopez-tecnativa please fw-port the second patch to 16 |
In a multi-company environment, if one company uses the
show_followers_partner_formatbypartner_nameand another company usespartner_email, a user with permissions in both companies may experience issues. If a document belongs to Company A, but the current company is set to Company B, the message format is taken from the current company (Company B) instead of the document’s company (Company A). This fix ensures that the message format is rendered based on the document's company.TT51022 @Tecnativa
@pedrobaeza @chienandalu could you please review it, thanks