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.

Sign up for GitHub

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

Jump to bottom

[16.0][FIX][REF] mail_show_follower: internal notes #1391

Merged
merged 2 commits into from
Jul 10, 2024
Merged

[16.0][FIX][REF] mail_show_follower: internal notes #1391

merged 2 commits into from
Jul 10, 2024

Conversation

vvrossem
Copy link
Contributor

@vvrossem vvrossem commented Jun 10, 2024 •
edited
Loading

This module aims to show the recipients of a message 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 recipient of each notification line

@OCA-git-bot
Copy link
Contributor

Hi @yajo,
some modules you are maintaining are being modified, check this out!

@vvrossem
Copy link
Contributor Author

Tests are failing because of mail_gateway_telegram.

yankinmax
yankinmax approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@yankinmax yankinmax left a comment

Choose a reason for hiding this comment

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

LGTM: code review

@vvrossem vvrossem force-pushed the 16.0-ref-mail_show_follower branch from bca3372 to 0153eaf Compare June 11, 2024 07:28
yajo
yajo requested changes Jun 11, 2024
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Some issues detected.

Please also check those test failures...

Please @fcvalgar, @Gelojr or @rafaelbn do a functional review.

Thanks!

mail_show_follower/models/mail_mail.py Outdated Show resolved Hide resolved
mail_show_follower/models/mail_mail.py Outdated Show resolved Hide resolved
mail_show_follower/models/res_partner.py Outdated Show resolved Hide resolved
mail_show_follower/models/res_partner.py Outdated Show resolved Hide resolved
@vvrossem vvrossem force-pushed the 16.0-ref-mail_show_follower branch from 0153eaf to 1819daf Compare June 12, 2024 12:56
@vvrossem
[16.0][FIX][REF] mail_show_follower: internal notes
33cbb12 
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`.
@vvrossem vvrossem force-pushed the 16.0-ref-mail_show_follower branch from 1819daf to 33cbb12 Compare June 12, 2024 13:16
@vvrossem vvrossem changed the title [16.0][FIX][REF] mail_show_follower: use mail recipient_ids [16.0][FIX][REF] mail_show_follower: internal notes Jun 12, 2024
@vvrossem
Copy link
Contributor Author

@etobella I'm encountering issues with tests failing on mail_gateway_telegram.
For example:

  File "/__w/social/social/mail_gateway_telegram/models/mail_gateway_telegram.py", line 71, in _get_channel_vals
    update.message.chat.description or False,
AttributeError: 'Chat' object has no attribute 'description'. Did you mean: 'set_description'?

Any idea what could be the issue ?

@etobella
Copy link
Member

I think they made a change on the library. I will pin a version in order to avoid the issue. I will make it ASAP (tonight or tomorrow morning)

@etobella etobella mentioned this pull request Jun 13, 2024
Merged
@rafaelbn rafaelbn added this to the 16.0 milestone Jun 19, 2024
@rafaelbn rafaelbn requested a review from fcvalgar June 19, 2024 22:34
yajo
yajo requested changes Jun 20, 2024
View reviewed changes
mail_show_follower/models/mail_mail.py Outdated Show resolved Hide resolved
mail_show_follower/models/mail_mail.py Outdated Show resolved Hide resolved
mail_show_follower/models/res_partner.py Outdated Show resolved Hide resolved
mail_show_follower/models/mail_mail.py Outdated Show resolved Hide resolved
mail_show_follower/models/mail_mail.py Outdated Show resolved Hide resolved
@vvrossem
Copy link
Contributor Author

Thank you @yajo for your additional review 🙏 . I'll handle it asap :)

@vvrossem vvrossem requested a review from yajo June 24, 2024 13:33
fcvalgar
fcvalgar approved these changes Jul 3, 2024
View reviewed changes
Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

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

Functional review,

LGTM, thank you @vvrossem

imageç

image

@yajo yajo requested a review from fcvalgar July 5, 2024 08:07
yajo
yajo approved these changes Jul 5, 2024
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Code review OK.

@fcvalgar please update your functional review. The part you highlight in the Mailhog interface does not belong to this module. Instead, this module adds recipients info in the message body, and I cannot see any of that in the screenshot. Maybe it was not installed when you tried it?

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@fcvalgar
Copy link

fcvalgar commented Jul 9, 2024

Code review OK.

@fcvalgar please update your functional review. The part you highlight in the Mailhog interface does not belong to this module. Instead, this module adds recipients info in the message body, and I cannot see any of that in the screenshot. Maybe it was not installed when you tried it?

I think the behaviour is not as expected. The followers of the notes do not appear in the body of the message.

image

image

image

yajo
yajo approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

I've double-checked the functional test. All seems to be working fine.

As you can see here, if Marc Demo is subscribed to discussions and handles notifications in Odoo, he still appears in the CC list:
image

I create a new user with mail [email protected] and subscribe to that record's notes by email. I send an internal note:

image

Now I remove Marc Demo's subscription to notes and send another message:

image

There's no extra header. This was probably the case you triggered, @fcvalgar. IMHO it's OK because there are no more people subscribed, so that header wouldn't add much value.

So this seems like a functional improvement.

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1391-by-yajo-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 6d63564 into OCA:16.0 Jul 10, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 7f4afd9. 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
Reviewers

@yajo yajo yajo approved these changes

@yankinmax yankinmax yankinmax approved these changes

@fcvalgar fcvalgar Awaiting requested review from fcvalgar

Assignees
No one assigned
Labels
approved merged 🎉 needs review ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@vvrossem @OCA-git-bot @etobella @fcvalgar @yajo @yankinmax @rafaelbn