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

[FIX] mail: templates preview wrong recipients #149563

Conversation

ande-odoo
Copy link
Contributor

@ande-odoo ande-odoo commented Jan 16, 2024

Steps to reproduce:

  1. Activate dev mode
  2. Go to Email Templates
  3. Create a new template
  4. Applies to Task
  5. Email Configuration > To (Partners)
  6. {{[p.id for p in object.message_partner_ids]}}
  7. Click on Preview
  8. Select a record with followers
  9. Look at Recipients
  10. => No or not all followers/partners are included

Cause of the issue:

Caused by cf7ae6b
In the case of partner_to being '[2,3,4]'
Split would output '[2', '3', '4]'
So after the isdigit it would be '3'
It should have been 2, 3, 4

opw-3677069


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Jan 16, 2024

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Jan 16, 2024
@ande-odoo
Copy link
Contributor Author

Alternative solution would be to use re.findall(r'\d+', partner_to)

@ande-odoo ande-odoo marked this pull request as ready for review January 16, 2024 13:18
@C3POdoo C3POdoo requested review from a team January 16, 2024 13:19
@tde-banana-odoo
Copy link
Contributor

tde-banana-odoo commented Jan 16, 2024

Actually partner_to should be a list of partner ids (comma separated list of ids). It is not meant to be used with records, it is not a m2m field, as help on field tells: help="Comma-separated ids of recipient partners"

@tde-banana-odoo
Copy link
Contributor

tde-banana-odoo commented Jan 16, 2024

The real fix is to use ','.join(p.id for p in message.partner_ids).

@nle-odoo
Copy link
Contributor

thanks

so this is not an issue and there should be in step 6, eg. {{str(object.message_partner_ids.ids)[1:-1]}} or {{','.join(str(p.id) for p in object.message_partner_ids)}}

@tde-banana-odoo
Copy link
Contributor

Indeed. Note that we could try to support a stringified list in addition to a comma separated list of ids for that kind of use case, aka considering [1, 2, 3] is as valid as 1, 2, 3 . It would be more defensive (and can be done here while we are at it).

@ande-odoo ande-odoo force-pushed the saas-16.2-OPW_3677069-mail-template_preview_recipients-ande branch from f0310cb to 3b753db Compare January 19, 2024 12:12
@ande-odoo
Copy link
Contributor Author

@tde-banana-odoo Done + modified commit message

@ande-odoo ande-odoo force-pushed the saas-16.2-OPW_3677069-mail-template_preview_recipients-ande branch 2 times, most recently from 7ad39ab to a8faa9d Compare February 2, 2024 14:43
@ande-odoo
Copy link
Contributor Author

Hi @tde-banana-odoo
Concerning the runbot test error, since I'm using literal_eval, input like 2131,2132,0,test would raise a UserError.
How do you think I should approach the issue ? By fixing the test or finding another way of doing _parse_partner_to ?

@tde-banana-odoo
Copy link
Contributor

2131,2132,0,test does not seem like a valid input, so not sure what you mean ?

@ande-odoo ande-odoo force-pushed the saas-16.2-OPW_3677069-mail-template_preview_recipients-ande branch from a8faa9d to 984cf88 Compare February 16, 2024 16:11
@ande-odoo
Copy link
Contributor Author

@tde-banana-odoo I added the fallback to the previous version of the function as requested, what do you think ?

Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

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

Zboing, looked at it again, small improvement proposal + some lint on tests :)

addons/mail/tests/test_mail_template.py Outdated Show resolved Hide resolved
addons/mail/tests/test_mail_template.py Outdated Show resolved Hide resolved
addons/mail/models/mail_template.py Outdated Show resolved Hide resolved
@ande-odoo ande-odoo force-pushed the saas-16.2-OPW_3677069-mail-template_preview_recipients-ande branch from 984cf88 to 5421a1a Compare February 29, 2024 17:37
Steps to reproduce:
1. Activate dev mode
2. Go to Email Templates
3. Create a new template
4. Applies to Task
5. Email Configuration > To (Partners)
6. {{[p.id for p in object.message_partner_ids]}}
7. Click on Preview
8. Select a record with followers
9. Look at Recipients
10. => No or not all followers/partners are included

Cause of the issue:
Caused by odoo@cf7ae6b
In the case of partner_to being '[2,3,4]'
Split would output '[2', '3', '4]'
So after the isdigit it would be '3'
It should have been 2, 3, 4

opw-3677069
@ande-odoo ande-odoo force-pushed the saas-16.2-OPW_3677069-mail-template_preview_recipients-ande branch from 5421a1a to 01d16c6 Compare February 29, 2024 17:38
@ande-odoo
Copy link
Contributor Author

Hi @tde-banana-odoo I updated the code with the best of both worlds, and modified the test, tell me what you think !

@tde-banana-odoo
Copy link
Contributor

@robodoo r+

Thanks :)

robodoo pushed a commit that referenced this pull request Mar 4, 2024
Steps to reproduce:
1. Activate dev mode
2. Go to Email Templates
3. Create a new template
4. Applies to Task
5. Email Configuration > To (Partners)
6. {{[p.id for p in object.message_partner_ids]}}
7. Click on Preview
8. Select a record with followers
9. Look at Recipients
10. => No or not all followers/partners are included

Cause of the issue:
Caused by cf7ae6b
In the case of partner_to being '[2,3,4]'
Split would output '[2', '3', '4]'
So after the isdigit it would be '3'
It should have been 2, 3, 4

opw-3677069

closes #149563

Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
@robodoo robodoo closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants