-
-
Notifications
You must be signed in to change notification settings - Fork 749
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
9.0 account chart update migration #420
9.0 account chart update migration #420
Conversation
491c3a9
to
2ebb295
Compare
Why this new PR? |
Can you please answer why you open a new PR? |
@pedrobaeza The PR #381 requires to be fixed. How this could have been done other than creating a new branch with the fixes and a new PR? |
Well, first asking the contributor to fix it, or making a PR over his branch. Anyway, I see that you have respected his commit history, so let's continue with this one. |
I'm still making some more tests because tax name was not shown in wizard and tax accounts where not loaded. |
Perfect, let me know when I can review it |
Any good reason why find_tax_by_templates tries to match an existing tax based on its description? The description is what is displayed on the invoice. |
There are some localizations that only fill that field. Maybe for v9 the situation has been normalized. What is the field name which you want to remove? description or name? |
c1bc4f0
to
f80bb74
Compare
@pedrobaeza No improvement on this in 9.0, so I leave it like that. I'll manage to have different description on those taxes. I'm done fixing the issues I found. This PR is now ready for review |
@sbidoul Could you also review this module that allows to install the l10n_belgium PR you reviewed after having the chart of accounts/taxes loaded. Thanks |
Could this PR be merged? |
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 looks good, still lacking tests sadly.
Did not check runbot since it is unavailable ATM.
"Odoo Community Association (OCA)", | ||
'website': "http://odoo-community.org", | ||
'depends': ["account"], | ||
'category': "Generic Modules/Accounting", | ||
'category': "Accounting & Finance", | ||
'contributors': [ |
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.
Does this key really do anything? 🤔
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.
Well, it's loaded, but not shown in any part. We can keep it anyway.
@tools.ormcache("code") | ||
def padded_code(self, code): | ||
"""Return a right-zero-padded code with the chosen digits.""" | ||
return "{:0<{}}".format(code, self.code_digits) |
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.
Only unicode strings are allowed to be used with .format
(per OCA standard).
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.
Uhm, format can be used with both kind of strings, so no real problem I think.
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.
>>> "Vivo en {}".format(u"España")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf1' in position 4: ordinal not in range(128)
>>> u"Vivo en {}".format(u"España")
u'Vivo en Espa\xf1a'
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.
But it's not the case here. Anyway, the same happens with "Vivo en %s" % "España"
if you don't put the u
.
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 can see discussion on OCA/pylint-odoo#65, I'd still recommend it as a good rule of thumb.
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.
OK, let's change it...
On a tentative update, there's this error:
so something fails. |
06366a6
to
0ac8ce9
Compare
@jbaudoux I have reordered a bit commits. The changes you have made to the fiscal position part are not working, with the message I put the other day. Can you please check? |
<?xml version="1.0" encoding="utf-8"?> | ||
<!-- © 2016 Jairo Llopis <jairo.llopis@tecnativa.com> | ||
License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). --> | ||
<openerp> |
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.
s/openerp/odoo/g
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 only.
@@ -1,156 +1,133 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<!-- © 2016 Jairo Llopis <jairo.llopis@tecnativa.com> | |||
License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). --> | |||
<openerp> |
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.
s/openerp/odoo/g
@pedrobaeza I cannot reproduce your error. |
mapping = self.env["account.fiscal.position.tax"].search([ | ||
("position_id", "=", pos_id), | ||
("tax_src_id", "=", src_id), | ||
], limit=1) |
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 works fine if fiscal position map 1 tax with more than 1 tax.
As there's a constraint that forbids to write the code of the account if it has moves, we delete the corresponding entry on update only, because the method that prepares the values is the same for new and existing accounts.
* Process price_include field * Detect change in price_include field
- New-style license headers. - Remove .pot file. - Remove tax codes stuff, now removed from v9. - Refactor methods for search, create, update, delete. Now they are smaller, fitter, happier, more productive. - Only update fields that have any kind of change on any updated record. - Place the wizard in the configuration page, instead of its own menu item. - Display amount of disabled taxes at ending page.
eb58f67
to
a735d30
Compare
a735d30
to
5d2ef73
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.
I have fixed the fiscal position part, so this one is finally ready to merge.
Replaces PR #381