[FIX] base_vat: correct the check and the reference of Israeli VAT numbers #172760
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
2dc52b1 to
769f241
Compare
769f241 to
ea36b21
Compare
nle-odoo
approved these changes
Jul 11, 2024
There was a problem hiding this comment.
looks good to me
side note: you should remove the direct link to https://github.com/arthurdejong/python-stdnum/pull/436 of the commit message (and PR message) so the PR outside of odoo repo doesn't get spammed by all the forward-port/PR/rebase and so on
you could do put it like arthurdejong/python-stdnum #436 or https://github.com/arthurdejong/python-stdnum/ PR436 I think
ea36b21 to
8ffb406
Compare
alialfie
reviewed
Jul 16, 2024
Issue: The reference displayed when entering a non valid VAT number for Israel is incorrect. And the check used is not the right one. Steps to reproduce: - In app Contact, create a new one - Select Israel as country and enter a non valid VAT number - The default ref is displayed: 'CC##' (CC=Country Code, ##=VAT Number) - And the number 039225313 should be accepted Cause: The check used by the library stdnum is not up to date. There is a PR to modify the library stdnum: https://github.com/arthurdejong/python-stdnum/ PR436 as the law has changed. Before only corporations could have a VAT number, now individuals can also have one. The new regulations give different formats for comanies and individuals. Solution: Use the right check (tdnum.il.idnr) for individuals, which is available in the library stdnum. opw-395467
8ffb406 to
844b4f0
Compare
alialfie
approved these changes
Jul 17, 2024
|
@FlorianGilbert can you review please? A client is pushing to solve this as fast as possible. |
| @@ -820,6 +821,10 @@ def check_vat_de(self, vat): | |||
| is_valid_stnr = stdnum.util.get_cc_module("de", "stnr").is_valid | |||
| return is_valid_vat(vat) or is_valid_stnr(vat) | |||
|
|
|||
| def check_vat_il(self, vat): | |||
| check_func = stdnum.util.get_cc_module('il', 'hp').is_valid if self.is_company else stdnum.util.get_cc_module('il', 'idnr').is_valid | |||
There was a problem hiding this comment.
I don't get why we're doing that kind of stuff depending of if it's a company or not?
Couldn't we do the same than the fix in the module and always use idnr? 🤔
Is it for the check on the first digit?
There was a problem hiding this comment.
@FlorianGilbert The format is different if the contact is a company or not. The difference is only on the first digit, it must be a 5 for companies.
We could use only idnr but it would be more accurate to select the right one depending on if it is a company or not.
I agree it can be confusing for the users if the format is changing.
Tell me if you think I need to change and only use idnr.
There was a problem hiding this comment.
@FlorianGilbert @mathcoutant I don't believe it is correct to access self.is_company here, as the function is called on potentially multiple partners in https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170
So either you'll need to override _run_vat_test or change the line above to work on partner and not self
There was a problem hiding this comment.
@FlorianGilbert @mathcoutant I don't believe it is correct to access
self.is_companyhere, as the function is called on potentially multiple partners in https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170So either you'll need to override
_run_vat_testor change the line above to work onpartnerand notself
Indeed, you're right, @mathcoutant may I ask you to fix it?
Thank you :)
There was a problem hiding this comment.
@FlorianGilbert I have looked into the proposed changes and changing partner to self seem to work.
But I also noticed that I forgot to add 'is_company' as constraint of check_vat with this PR, to check when only is_company is changed. But this change causes problem when changing both vat and is_company so I changed the write method to ensure that the check for the change of is_company is called after the one of vat. There may be a better way to do this...
There is the new PR: #174817
Can I have your opinion on this? Maybe the best solution is to only use idnr and remove the specific check for the company...
There was a problem hiding this comment.
Hello @mathcoutant
As my first opinion on this PR was to only use idnr, let's change the code for that. I would prefer avoid adding a new constraint, etc etc.
Thank you, feel free to assign me as reviewer 😉
robodoo
pushed a commit
that referenced
this pull request
Jul 19, 2024
Issue: The reference displayed when entering a non valid VAT number for Israel is incorrect. And the check used is not the right one. Steps to reproduce: - In app Contact, create a new one - Select Israel as country and enter a non valid VAT number - The default ref is displayed: 'CC##' (CC=Country Code, ##=VAT Number) - And the number 039225313 should be accepted Cause: The check used by the library stdnum is not up to date. There is a PR to modify the library stdnum: https://github.com/arthurdejong/python-stdnum/ PR436 as the law has changed. Before only corporations could have a VAT number, now individuals can also have one. The new regulations give different formats for comanies and individuals. Solution: Use the right check (tdnum.il.idnr) for individuals, which is available in the library stdnum. opw-395467 closes #172760 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com>
This was referenced Jul 19, 2024
mathcoutant
added a commit
to odoo-dev/odoo
that referenced
this pull request
Jul 29, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674
robodoo
pushed a commit
that referenced
this pull request
Jul 30, 2024
In this PR: #172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes #174817 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com>
fw-bot
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
Jul 30, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 X-original-commit: dacabd6
fw-bot
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
Jul 30, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 X-original-commit: dacabd6
fw-bot
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
Jul 30, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 X-original-commit: dacabd6
fw-bot
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
Jul 30, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 X-original-commit: dacabd6
fw-bot
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
Jul 30, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 X-original-commit: dacabd6
robodoo
pushed a commit
that referenced
this pull request
Jul 31, 2024
In this PR: #172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes #174947 X-original-commit: dacabd6 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Signed-off-by: Mathieu Coutant (mcou) <mcou@odoo.com>
robodoo
pushed a commit
that referenced
this pull request
Jul 31, 2024
In this PR: #172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes #174990 X-original-commit: dacabd6 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Signed-off-by: Mathieu Coutant (mcou) <mcou@odoo.com>
robodoo
pushed a commit
that referenced
this pull request
Jul 31, 2024
In this PR: #172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes #175006 X-original-commit: dacabd6 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Signed-off-by: Mathieu Coutant (mcou) <mcou@odoo.com>
robodoo
pushed a commit
that referenced
this pull request
Jul 31, 2024
In this PR: #172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes #175027 X-original-commit: dacabd6 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Signed-off-by: Mathieu Coutant (mcou) <mcou@odoo.com>
robodoo
pushed a commit
that referenced
this pull request
Jul 31, 2024
In this PR: #172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes #174971 X-original-commit: dacabd6 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Signed-off-by: Mathieu Coutant (mcou) <mcou@odoo.com>
Dasanchez1992
pushed a commit
to Nosolotec/odoo
that referenced
this pull request
Aug 22, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes odoo#174947 X-original-commit: dacabd6 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Signed-off-by: Mathieu Coutant (mcou) <mcou@odoo.com>
riccardoSANEHO
pushed a commit
to resultrum/odoo
that referenced
this pull request
Sep 30, 2024
In this PR: odoo#172760 the check_vat_il function is calling self.company. It appears it was not a good idea and several things were not done well: - It could lead to errors as run_vat_test can be called on multiple partners: https://github.com/odoo/odoo/blob/16.0/addons/base_vat/models/res_partner.py#L170. - The previous PR is also checking the VAT number depending on the field is_company but did not put it as a constraint. This can be fixed, but with more changes than needed. It has been decided to use the `` check for all Israeli VAT numbers, regardless of the partner type. With this change the VAT check for companies is not complete: it is not tested that they begin by a 5. But it is good enough. opw-3954674 closes odoo#174947 X-original-commit: dacabd6 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Signed-off-by: Mathieu Coutant (mcou) <mcou@odoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue:
The reference displayed when entering a non valid VAT number for Israel is incorrect. And the check used is not the right one.
Steps to reproduce:
Cause:
he check used by the library stdnum is not up-to-date.
There is a PR to modify the library stdnum: https://github.com/arthurdejong/python-stdnum/ PR436 as the law has changed. Before only corporations could have a VAT number, now individuals can also have one.
Solution:
Use the right check (tdnum.il.idnr), which is available in the library stdnum.
opw-3954674