-
-
Notifications
You must be signed in to change notification settings - Fork 615
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
[14.0][ADD] mail_chatter_company_tracking #1461
base: 14.0
Are you sure you want to change the base?
[14.0][ADD] mail_chatter_company_tracking #1461
Conversation
7ab8744
to
b8d200c
Compare
Functionally reviewed. LGTM |
LGTM!! |
b8d200c
to
407efb7
Compare
There was a problem hiding this 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:
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
"license": "AGPL-3", | ||
"category": "social", | ||
"version": "14.0.1.0.0", | ||
"depends": ["mail", "company_dependent_attribute"], |
There was a problem hiding this comment.
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.
"depends": ["mail", "company_dependent_attribute"], | |
"depends": ["mail"], |
407efb7
to
ad76b6c
Compare
This PR has the |
This add-on includes the company name in the email chatter tracking and filters tracking based on allowed companies.