-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
[WIP][10.0][MIG] account_asset_analytic #102
[WIP][10.0][MIG] account_asset_analytic #102
Conversation
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.
Little changes, but overall OK
account_asset_analytic/__init__.py
Outdated
@@ -0,0 +1,5 @@ | |||
# -*- coding: utf-8 -*- | |||
# © 2016 Antonio Espinosa - <antonio.espinosa@tecnativa.com> |
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 remove copyright from init files.
@@ -0,0 +1,22 @@ | |||
# -*- coding: utf-8 -*- | |||
# © 2016 Antonio Espinosa - <antonio.espinosa@tecnativa.com> |
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.
Replace ©
by Copyright
and add yours
"summary": "Adds analytic account per asset", | ||
"version": "10.0.1.0.0", | ||
"category": "Analytic Accounting", | ||
"website": "http://www.tecnativa.com", |
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.
https
"application": False, | ||
"installable": True, | ||
"depends": [ | ||
"account_asset", |
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.
Doesn't it depend on analytic
?
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.
Yes, but throw this route:
account_asset > account_accountant > account > analytic
@@ -0,0 +1,8 @@ | |||
# -*- coding: utf-8 -*- | |||
# © 2016 Antonio Espinosa - <antonio.espinosa@tecnativa.com> |
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.
Remove copyright here
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 drop in coverage means that there are certain things untested. See here what it's untested and check, because I think that the analytic account propagation to the asset is not correct:
Runbot can't build due to other modules test |
Runbot is building, but it fails with a warning. Anyway, it's possible to test the module in runbot. |
@cubells can you review? |
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 and tested
f4f3eed
to
3bf212e
Compare
Fix assignation on recordset with len > 1
Account asset analytic
This module allows you to define an analytic account per asset and creates
expenses moves with that analytic account instead of the analytic account
per category, as the standard does.
It also propagates analytic account from invoice line to analytic account line.
cc @Tecnativa