-
-
Notifications
You must be signed in to change notification settings - Fork 946
[16.0][IMP] partner_company_type: allow to set up countries and states, display shortcut in name #1604
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
Conversation
944bccc to
914e55c
Compare
44a8411 to
5350466
Compare
JordiMForgeFlow
left a comment
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.
Code review LGTM
5350466 to
9c4d7ef
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
AaronHForgeFlow
left a comment
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.
Code review LG
LoisRForgeFlow
left a comment
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.
Code review 👍
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
|
@rousseldenis could we get a review here? :) |
|
@OriolMForgeFlow @len-foss Isn't it the same problematic ? I would say I'd prefer an EXCLUDE |
| rec.partner_company_type_id.country_ids | ||
| and rec.country_id not in rec.partner_company_type_id.country_ids | ||
| ): | ||
| raise UserError( |
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.
You should raise ValidationErrorin a contraint.
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.
@rousseldenis corrected :)
|
@rousseldenis Here there's a check to choose a company type to match the partner's country which is nice though. Since it seems that different use-cases want for different constraint, it would make sense to have a configuration option to choose what is allowed. Therefore entirely drop the sql constaint for the python check. |
9c4d7ef to
ac966c5
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
ac966c5 to
1068af9
Compare
|
@len-foss @rousseldenis could we get this one merged? |
|
@rousseldenis could we move this one forward? |
| def _check_partner_company_type_country_state(self): | ||
| for rec in self: | ||
| if not rec.partner_company_type_id: | ||
| return |
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.
Should this not be continue?
NL66278
left a comment
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.
This are very extensive changes without any reflection in the README. To me it is completely unclear what problem this PR is supposed to solve.
1068af9 to
73fa29e
Compare
|
@NL66278 you are right, README is now updated to reflect the purpose. We don't have a problem with the module, we are simply proposing an improvement in order to manage the relation of Legal Forms with countries and their states. Legal Forms are very structured and predefined in most countries so it is a way to ensure and validate that the data is correct. |
|
@JordiMForgeFlow It is becoming more clear now. Would it be possible to consistently use the vocabulary of "Legal Entity Type" instead of Legal Form. The word "Form" is confusing at it suggest some kind of document or form that must be filled. |
73fa29e to
cccddcb
Compare
|
@NL66278 done :) |
| @@ -0,0 +1,11 @@ | |||
| # Copyright 2023 ForgeFlow S.L. <http://www.forgeflow.com> | |||
| # License LGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). | |||
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.
The module itself has an AGPL license.
| name="partner_company_type_id" | ||
| options='{"no_open": True}' | ||
| attrs="{'invisible': [('is_company', '=', False)]}" | ||
| domain="[ '|', ('country_ids', '=', False), ('country_ids', 'in', [country_id]), '|', ('state_ids', '=', False), ('state_ids', 'in', [state_id])]" |
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.
This can actually be split over multiple lines, would be nicer.
cccddcb to
e203008
Compare
|
Two last small remarks and we should be good to go. |
…play shortcut in name
e203008 to
717b7ea
Compare
|
@NL66278 thank you for the reviews, should be now ready :) |
NL66278
left a comment
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.
👍 Thanks for your contribution! LGTM
|
/ocabot merge major |
|
This PR looks fantastic, let's merge it! |
|
Congratulations, your PR was merged at 820958d. Thanks a lot for contributing to OCA. ❤️ |
No description provided.