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.

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

[MIG] partner contact nationality: Migration to 16.0 #1421

Conversation

FernandoRomera
Copy link
Contributor

No description provided.

Jairo Llopis and others added 30 commits November 30, 2022 12:58
Really I'm just renaming partner_contact_base.
The new name is more self-explanatory.
This reverts commit e170541.
This reverts commit e170541.
@OCA-git-bot
Copy link
Contributor

Sorry @FernandoRomera you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for porting this module.

Dont understand the design. Nationality should be a many2many, don't you think ?

@FernandoRomera
Copy link
Contributor Author

@legalsylvain
The nationality must be a single country, as well as the VAT number.

Nationality is for individuals, and it is true that they can have more than one, but if we manage it we would have to start administering more than one VAT number and associate it to each nationality.

@legalsylvain
Copy link
Contributor

It is possible to add a constrain, dont you think ?
If it is a company, limit to 1 value.

@FernandoRomera
Copy link
Contributor Author

This is a migration pull request, I had not planned to add functionality. Once this one is approved, we can decide what is right.

@rousseldenis
Copy link
Contributor

/ocabot migration partner_contact_nationality

@rousseldenis
Copy link
Contributor

It is possible to add a constrain, dont you think ? If it is a company, limit to 1 value.

I don't see the use case as for a company, the nationality has to be the country used for its address (legally)

@rousseldenis
Copy link
Contributor

For the multiple instances case, why not using partner_identification_number module and add a country field ?

@FernandoRomera
Copy link
Contributor Author

Nationality is only for individuals, not for companies. The dependency of this module is clearly identified: partner_contact_personal_information_page

Copy link

@DoDoViVi DoDoViVi left a comment

Choose a reason for hiding this comment

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

Tested on Runboat,
work as expected
LGTM
Thanks for the contribution

Copy link

@MRomeera MRomeera left a comment

Choose a reason for hiding this comment

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

Ok

@rousseldenis
Copy link
Contributor

@legalsylvain Could you update your review ?

@legalsylvain
Copy link
Contributor

Thanks for the ping.

Well, AFAIK, this module is only for individuals contact, as it depends on partner_contact_personal_information_page module. Right ? So no problem with VAT and other stuff, as individual contact doesn't have VAT number. And as @rousseldenis for the companies, we should use the field country_id of the address part.

In that case, the field should be a M2M. Don't you think ?
Once decided, the question is when to do that changes. Now or in another PR.

@FernandoRomera
Copy link
Contributor Author

I think it is best to leave this module for those use cases where the primary or sole nationality is identified. In case you want to be able to assign more than one nationality to the partner, you would have to use the module "partner_identification" and not this one.

Copy link

@MRomeera MRomeera left a comment

Choose a reason for hiding this comment

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

Ok

@rafaelbn
Copy link
Member

rafaelbn commented Dec 9, 2022

Please @yajo make here a review as original author, it could be of your interest. thank you!

@FernandoRomera
Copy link
Contributor Author

FernandoRomera commented Dec 14, 2022

@yajo
it looks like it's up to you, that this pr will merge. You could check it and then we can move forward. And #1422

@rousseldenis
Copy link
Contributor

@FernandoRomera In fact, I see this is a duplicate of #1380

@FernandoRomera
Copy link
Contributor Author

@rousseldenis
Sorry, we did not see the PR with the migration to 16.0.
For us you can merge the previous PR and we close this one.

@yajo
Copy link
Member

yajo commented Dec 19, 2022

Closing in favor of #1380. Code there is more up to date.

@yajo yajo closed this Dec 19, 2022
@yajo
Copy link
Member

yajo commented Dec 19, 2022

BTW people here: please go there and approve to merge.

@FernandoRomera FernandoRomera deleted the 16.0-mig-partner_contact_nationality branch December 19, 2022 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.