-
-
Notifications
You must be signed in to change notification settings - Fork 519
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][FIX] contract: Usage of analytic distribution #1019
[16.0][FIX] contract: Usage of analytic distribution #1019
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.
I agree to follow the same procedures as in sale.order. I think this PR would fix the issue that the analytic_account_id is not copied to the invoice when used in contract.line.
Nevertheless there is a general different between sale.order and contract.contract, where I am not sure if this is intended or should be changed.
While the analytic_account_id is on sale.order the analytic_account_id in contract is on contract.line instead of contract.contract.
(I even do not understand why there is an analytic_account_id in sale.order at all and not only an analytic_distribution on sale.order.line and that's it ... maybe usability)
Shouldn't we either remove analytic_account_id at all (migrate the data into analytic_distribution) or move analytic_account_id into "contract.contract" to create the same logic as in sale.order? (analytic_account_id on sale.order is continued on 17.0, so there might be a reason I do not understand)
I am willing to prepare a PR, but I would like to have some opinions first.
@pedrobaeza @wpichler @rousseldenis
[1] https://github.com/OCA/OCB/blob/16.0/addons/sale/models/sale_order.py#L854 |
Analytic account on contract lines was there for transferring such analytic line to the invoice lines, so it's something needed. In v16, it should be switched to the analytic distribution, to be transferred the same way. |
This script works in an post-migration.py, but I cannot access env["contract.line"] when it is in a pre-migration.py. And the analytic_account_id is not available anymore, when we remove it from contract.line. Is a better way than creating temporary table in a pre-migration script storing the data and deleting this table in the post-migration step?
@fkantelberg do you want to add the discussed changes on this PR or shell I create an updated PR?
|
89dbf5c
to
4691a28
Compare
@CRogos I added the migration and dropped the |
4691a28
to
210b043
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.
LGTM: Code reviewed, tested migration and analytic distribution is copied to invoices.
contract/models/contract_line.py
Outdated
@@ -559,6 +555,7 @@ def _prepare_invoice_line(self): | |||
self.last_date_invoiced, self.recurring_next_date | |||
) | |||
name = self._insert_markers(dates[0], dates[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 introduce a useless line break inside a method (and more being unrelated).
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.
Removed again. Was left because I could remove the initial code here.
210b043
to
f7c4b5c
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.
/ocabot merge nobump
On my way to merge this fine PR! |
Congratulations, your PR was merged at 80fdfc7. Thanks a lot for contributing to OCA. ❤️ |
Because of the changes in 16.0 analytic accounting the
analytic_distribution
is more important now and only this will be copied onto the invoices.I copied the invoice line preparation mostly from
sale.order.line
to generate the distribution also from theanalytic_account_id
. Therefore no migration is required.