Skip to content

[18.0] [ADD] mail_force_email_notification#42

Merged
OCA-git-bot merged 1 commit intoOCA:18.0from
camptocamp:18.0-add-mail_force_email_notification
Apr 2, 2025
Merged

[18.0] [ADD] mail_force_email_notification#42
OCA-git-bot merged 1 commit intoOCA:18.0from
camptocamp:18.0-add-mail_force_email_notification

Conversation

@rlimaeco
Copy link
Copy Markdown
Contributor

@rlimaeco rlimaeco commented Mar 26, 2025 •
edited
Loading

This module is used to add a condition to mail.thread using context key while sending messages to set notification to be sent by email

@rlimaeco rlimaeco force-pushed the 18.0-add-mail_force_email_notification branch 2 times, most recently from e04e4c5 to 92bb84c Compare March 28, 2025 12:51
@rlimaeco
Copy link
Copy Markdown
Contributor Author

rlimaeco commented Mar 28, 2025

@sebalix @simahawk could you please review this PR

@rlimaeco rlimaeco marked this pull request as ready for review March 28, 2025 14:51
twalter-c2c
twalter-c2c approved these changes Mar 29, 2025
View reviewed changes
Copy link
Copy Markdown

@twalter-c2c twalter-c2c left a comment

Choose a reason for hiding this comment

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

LGTM. Nitpicking: copyright year is not consistent across different files.

@sebalix
Copy link
Copy Markdown
Contributor

sebalix commented Mar 31, 2025 •
edited
Loading

@twalter-c2c it's not an issue, copyright year should not be updated. If one mentions 2019, it applies for next years automatically.
Then the year could change from one file to another depending on the contribution date of this specific file.

EDIT: sorry @twalter-c2c , didn't see this module was a new one (not even copied from a local module), so yes it could make sense to align copyrights everywhere

@rlimaeco rlimaeco force-pushed the 18.0-add-mail_force_email_notification branch from 92bb84c to d612b7d Compare March 31, 2025 12:16
sebalix
sebalix approved these changes Mar 31, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

LG

Comment thread mail_force_email_notification/tests/test_mail_thread.py
@classmethod
def setUpClass(cls):
super().setUpClass()
cls.env = cls.env(context=dict(cls.env.context, tracking_disable=True))
Copy link
Copy Markdown
Contributor

@sebalix sebalix Mar 31, 2025

Choose a reason for hiding this comment

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

Could leverage BaseCommon class to remove this line

Copy link
Copy Markdown
Contributor

@simahawk simahawk Apr 2, 2025

Choose a reason for hiding this comment

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

not a blocker

@OCA-git-bot
Copy link
Copy Markdown
Contributor

OCA-git-bot commented Mar 31, 2025

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). 🤖

1 similar comment
@OCA-git-bot
Copy link
Copy Markdown
Contributor

OCA-git-bot commented Mar 31, 2025

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). 🤖

@simahawk
Copy link
Copy Markdown
Contributor

simahawk commented Apr 2, 2025

/ocabot merge nobump

@OCA-git-bot
Copy link
Copy Markdown
Contributor

OCA-git-bot commented Apr 2, 2025

On my way to merge this fine PR!
Prepared branch 18.0-ocabot-merge-pr-42-by-simahawk-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 35315c7 into OCA:18.0 Apr 2, 2025
7 checks passed
@OCA-git-bot
Copy link
Copy Markdown
Contributor

OCA-git-bot commented Apr 2, 2025

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

@simahawk simahawk simahawk approved these changes

+2 more reviewers

@sebalix sebalix sebalix approved these changes

@twalter-c2c twalter-c2c twalter-c2c approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved merged 🎉 ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rlimaeco @sebalix @OCA-git-bot @simahawk @twalter-c2c