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

[16.0][MIG] base vat optional vies: Migration to 16.0 #1572

Merged
merged 43 commits into from
Feb 2, 2023

Conversation

RodrigoBM
Copy link
Contributor

@RodrigoBM RodrigoBM commented Jan 26, 2023

@RodrigoBM RodrigoBM force-pushed the 16.0-mig-base_vat_optional_vies branch from 12a9fd8 to 5dec75f Compare January 26, 2023 14:24
@RodrigoBM RodrigoBM mentioned this pull request Jan 26, 2023
33 tasks
@rafaelbn
Copy link
Member

/ocabot migration base_vat_optional_vies

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jan 28, 2023
@rafaelbn
Copy link
Member

Hello @RodrigoBM , thank you!! 😄

As a know issue in this module and commented recently by @alexis-via in Twitter https://twitter.com/alexisdlattre/status/1618377832090185730 we should improve of FIX this module in this points:

Premise: as de VAT number in the parent (commercial partner) is the same and should be the same for all chilldren , this field vies_valid too!!

So my proposal is simple:

  • Propagate field to children: When VIES is valid and this module mark as true the boolean in the parent propagate it to all children contacts.
  • Improve installation: when the module is installed, and there are already partners created, if we don't make anything the existing partner are not updated.
    • My proposal is create a wizard in the installation that ask to update all existing partners. If this is technically complicated or wrong for performance then create an action or something which objetive is update all existing partners.
    • Maybe the best way is create a installation script to be all recalculated correctly

This changes could be don't know if easy or later in a separate PR. I'm not blocking at all this great migration.

@moduon MT-2178

@rafaelbn rafaelbn requested review from Shide and yajo January 29, 2023 23:36
Copy link
Contributor

@Shide Shide left a comment

Choose a reason for hiding this comment

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

Improvement that can be done in a separate PR, but not blocking migration

Comment on lines 43 to 45
for partner in self:
partner = partner.with_context(vat_partner=partner)
super(ResPartner, partner).check_vat()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe can be improved to propagate vies_passed field to the children contacts:

Suggested change
for partner in self:
partner = partner.with_context(vat_partner=partner)
super(ResPartner, partner).check_vat()
for partner in self.sorted(lambda p: bool(p.commercial_partner_id)):
partner = partner.with_context(vat_partner=partner)
if partner.commercial_partner_id:
partner.update({"vies_passed": partner.commercial_partner_id.vies_passed})
else:
super(ResPartner, partner).check_vat()

@RodrigoBM
Copy link
Contributor Author

with this commit in base_vat odoo/odoo@eff3b14 this mig now fails. I am reviewing it

@RodrigoBM RodrigoBM force-pushed the 16.0-mig-base_vat_optional_vies branch from 215d21e to 359a065 Compare January 31, 2023 01:12
@RodrigoBM RodrigoBM requested review from Shide and removed request for yajo January 31, 2023 01:13
@RodrigoBM RodrigoBM force-pushed the 16.0-mig-base_vat_optional_vies branch from eb3dd33 to 40f5fe4 Compare January 31, 2023 08:27
antespi and others added 16 commits January 31, 2023 14:33
  * Disable VIES test
  * Fixes to avoid exception when using default _construct_constraint_msg method
  * base_vat_optional_vies: Fix pass lower country code in _split_vat function to find parent function check_vat_'xx' instead of check_vat_'XX'
  * base_vat_optional_vies: Convert to upper when write NIF into database
Currently translated at 100.0% (2 of 2 strings)

Translation: account-financial-tools-10.0/account-financial-tools-10.0-base_vat_optional_vies
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-10-0/account-financial-tools-10-0-base_vat_optional_vies/de/
Currently translated at 100.0% (2 of 2 strings)

Translation: account-financial-tools-12.0/account-financial-tools-12.0-base_vat_optional_vies
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-12-0/account-financial-tools-12-0-base_vat_optional_vies/es/
Currently translated at 100.0% (2 of 2 strings)

Translation: account-financial-tools-12.0/account-financial-tools-12.0-base_vat_optional_vies
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-12-0/account-financial-tools-12-0-base_vat_optional_vies/hr/
Currently translated at 100.0% (2 of 2 strings)

Translation: account-financial-tools-12.0/account-financial-tools-12.0-base_vat_optional_vies
Translate-URL: https://translation.odoo-community.org/projects/account-financial-tools-12-0/account-financial-tools-12-0-base_vat_optional_vies/pt/
@RodrigoBM RodrigoBM force-pushed the 16.0-mig-base_vat_optional_vies branch from 40f5fe4 to 604871b Compare January 31, 2023 13:33
@pedrobaeza
Copy link
Member

Does this module now makes sense as Odoo already made the VIES check optional?

Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you @RodrigoBM

@fcvalgar
Copy link

Hello @RodrigoBM , thank you!! smile

As a know issue in this module and commented recently by @alexis-via in Twitter https://twitter.com/alexisdlattre/status/1618377832090185730 we should improve of FIX this module in this points:

Premise: as de VAT number in the parent (commercial partner) is the same and should be the same for all chilldren , this field vies_valid too!!

So my proposal is simple:

  • Propagate field to children: When VIES is valid and this module mark as true the boolean in the parent propagate it to all children contacts.

  • Improve installation: when the module is installed, and there are already partners created, if we don't make anything the existing partner are not updated.

    • My proposal is create a wizard in the installation that ask to update all existing partners. If this is technically complicated or wrong for performance then create an action or something which objetive is update all existing partners.
    • Maybe the best way is create a installation script to be all recalculated correctly

This changes could be don't know if easy or later in a separate PR. I'm not blocking at all this great migration.

@moduon Moduon - Task #2178

Initial validation of existing partners would be critical.

@fcvalgar
Copy link

Does this module now makes sense as Odoo already made the VIES check optional?

It is a very user-friendly validation system without the need for external tools.

It also allows to trace, review and identify validation errors for non-technical users.

For me it is useful as a functional user.

@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). 🤖

@pedrobaeza
Copy link
Member

Fran, but have you seen now how it's done on core? They have put a visible warning.

@fcvalgar
Copy link

fcvalgar commented Jan 31, 2023

Fran, but have you seen now how it's done on core? They have put a visible warning.

It is probably my ignorance but it would only be a warning at the time of creation.

https://www.odoo.com/documentation/16.0/es/applications/finance/accounting/taxation/taxes/vat_validation.html

If I have a team member who wants to verify if that company has passed validation haven't the same information visually.

If this new validation would leave log. The financial and administrative departments ❤️.

@pedrobaeza
Copy link
Member

OK, I agree it can be useful to check it later, not only on VAT number onchange.

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Tested 👍 still needed Odoo core check it but don't allow to safe a VAT number si VIES is not valid. So the need is the same from v8 😄

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1572-by-rafaelbn-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e72fa0b into OCA:16.0 Feb 2, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 9211879. Thanks a lot for contributing to OCA. ❤️

@RodrigoBM RodrigoBM deleted the 16.0-mig-base_vat_optional_vies branch February 2, 2023 15:54
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.