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

[14.0][ADD] mail_chatter_company_tracking #1461

Merged
merged 3 commits into from
Dec 6, 2024
Merged

[14.0][ADD] mail_chatter_company_tracking #1461

merged 3 commits into from
Dec 6, 2024

Conversation

adasatorres
Copy link

@adasatorres adasatorres commented Oct 9, 2024 •
edited
Loading

This add-on includes the company name in the email chatter tracking and filters tracking based on allowed companies.

@adasatorres adasatorres force-pushed the 14.0-add-mail_chatter_company_tracking branch 3 times, most recently from 7ab8744 to b8d200c Compare October 9, 2024 13:06
@Abimael1321
Copy link

Functionally reviewed. LGTM

@Yoana-RB
Copy link

LGTM!!

@adasatorres adasatorres force-pushed the 14.0-add-mail_chatter_company_tracking branch from b8d200c to 407efb7 Compare October 10, 2024 12:49
Christian-RB
Christian-RB approved these changes Oct 10, 2024
View reviewed changes
Copy link

@Christian-RB Christian-RB left a comment

Choose a reason for hiding this comment

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

I think it looks great that way:
image
Please take a look at my comments regarding the dependencies. I’d like to know your thoughts on this. It’s true that by adding the module, we avoid duplicating the same logic, but without it, we make the module more independent. It's just a point and im not sure wich one should be the best approach. So LGTM

mail_chatter_company_tracking/__manifest__.py Outdated
"license": "AGPL-3",
"category": "social",
"version": "14.0.1.0.0",
"depends": ["mail", "company_dependent_attribute"],
Copy link

Choose a reason for hiding this comment

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

I understand that this can be useful for understanding how the module works, but technically, it's not necessary. A good approach might be to include it in the README as a helpful optional feature that users can choose to install.

Suggested change
"depends": ["mail", "company_dependent_attribute"],
"depends": ["mail"],

mail_chatter_company_tracking/models/mail_message.py Outdated Show resolved Hide resolved
@adasatorres adasatorres force-pushed the 14.0-add-mail_chatter_company_tracking branch from 407efb7 to ad76b6c Compare October 10, 2024 14:05
@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). 🤖

@adasatorres adasatorres requested a review from hbrunn December 6, 2024 17:12
@hbrunn
Copy link
Member

hbrunn commented Dec 6, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-1461-by-hbrunn-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 95eba18 into OCA:14.0 Dec 6, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@Yoana-RB Yoana-RB Yoana-RB approved these changes

@Abimael1321 Abimael1321 Abimael1321 approved these changes

@Christian-RB Christian-RB Christian-RB approved these changes

@hbrunn hbrunn hbrunn approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@adasatorres @Abimael1321 @Yoana-RB @OCA-git-bot @hbrunn @Christian-RB