-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
[16.0][IMP] account_analytic_organization: Added company_id field and fixed spanish translation #697
base: 16.0
Are you sure you want to change the base?
Conversation
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.
LGTM, works as expected in runboat and code review
da75895
to
f75b62b
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.
LGTM, tested in runboat
Hi @pedrobaeza! Is this ready to merge? |
Being your module, it's advisable that any of you put it as maintainer of the module, and then you will be independent to merge these fixes without depending of someone like me. Please do it and I'll merge, and from now on, you will fly alone, hehe. |
f75b62b
to
70213d3
Compare
Done! Thanks for your help :) |
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.
LGTM
Hi @pedrobaeza, sorry for pinging you again. Could you help us to merge this pr? I put me as maintainer on this imp so after this is merge we won't need to ping you anymore. |
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.
Don't change the year of the copyright of all files if they aren't touched. It's adding a lot of noise in the PR.
Hi! Thanks for your comment. This module is new and it was created in 2024. It was'nt correct before, now it's fixed. |
0d27d0b
to
5c75cf3
Compare
In the Analytic Organization field of the partner form, there should be a domain that restricts the displayed organizations to those belonging to the active company. For example: ("company_id", "=", company_id). Additionally, when creating a new analytic organization, the company_id field could be prepopulated with the active company by default. |
5c75cf3
to
9df2b6d
Compare
Thanks for your feedback! Could you review again please? |
I tested, and it's working fine. What if you add a domain at the analytic organization view that filters only by the ones of the active company? |
…xed spanish translation and made organization name appear in partner name
9df2b6d
to
b840ce3
Compare
Great idea! Done, can you try it out please? |
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.
LGTM
On this improvement i added the company_id field to make it work better with multi-company option and fixed spanish translation. I also inherited get_name method in order to show the organization name in the partner name.
cc https://github.com/APSL 160524
@miquelalzanillas @lbarry-apsl @javierobcn @peluko00 @BernatObrador @ppyczko please review