[FIX][11.0][contract] analytic invoice/contract lines inheritance #169

katyukha · 2018-06-26T22:14:18Z

Bug description

account.analytic.contract.line inherits
account.analytic.invoice.line

account.analytic.invoice.line defines field analytic_account_id:

comodel='account.analytic.account'

account.analytic.contract.line redefines field analytic_account_id:

comodel='account.analytic.contract'

On attempt to extend account.analytic.invoice.line model adding
field that depends on analytic_account_id.date_start
Odoo fails to update, because it adds this field to
account.analytic.contract.line through inheritance,
and account.analytic.contract model have no this field.

What is done

Change inheritance order:

account.analytic.invoice.line inherits account.analytic.contract.line
all logic from account.analytic.invoice.line moved to account.analytic.contract.line

MiquelRForgeFlow

Interesting. I think you are in the good way.

MiquelRForgeFlow · 2018-07-04T17:33:53Z

contract_variable_quantity/models/contract.py

@@ -46,7 +46,7 @@ def _prepare_invoice_line(self, line, invoice_id):


 class AccountAnalyticInvoiceLine(models.Model):


AccountAnalyticContractLine

MiquelRForgeFlow

I think you could the same, but without moving that many lines. Prove to do the same but just only with renaming the classes, changing inherits and moving the analytic_account_id field. Please, try to minimize the changes.

katyukha · 2018-07-11T19:37:26Z

Hello @mreficent

Sorry for long response.

If i just rename classes and change inherits, but not rename files, then file name will not reference to model it contains. Is it Ok?

Diff will look like:

diff --git a/contract/models/account_analytic_contract_line.py b/contract/models/account_analytic_contract_line.py
index a18d5db..9328178 100644
--- a/contract/models/account_analytic_contract_line.py
+++ b/contract/models/account_analytic_contract_line.py
@@ -4,14 +4,13 @@
 from odoo import fields, models
 
 
-class AccountAnalyticContractLine(models.Model):
-    _name = 'account.analytic.contract.line'
-    _description = 'Contract Lines'
-    _inherit = 'account.analytic.invoice.line'
+class AccountAnalyticInvoiceLine(models.Model):
+    _name = 'account.analytic.invoice.line'
+    _inherit = 'account.analytic.contract.line'
 
     analytic_account_id = fields.Many2one(
-        string='Contract',
-        comodel_name='account.analytic.contract',
+        'account.analytic.account',
+        string='Analytic Account',
         required=True,
         ondelete='cascade',
     )
diff --git a/contract/models/account_analytic_invoice_line.py b/contract/models/account_analytic_invoice_line.py
index 8f547aa..5eff76a 100644
--- a/contract/models/account_analytic_invoice_line.py
+++ b/contract/models/account_analytic_invoice_line.py
@@ -14,8 +14,9 @@ from odoo.exceptions import ValidationError
 from odoo.tools.translate import _
 
 
-class AccountAnalyticInvoiceLine(models.Model):
-    _name = 'account.analytic.invoice.line'
+class AccountAnalyticContractLine(models.Model):
+    _name = 'account.analytic.contract.line'
+    _description = 'Contract Lines'
     _order = "sequence,id"
 
     product_id = fields.Many2one(
@@ -24,8 +25,8 @@ class AccountAnalyticInvoiceLine(models.Model):
         required=True,
     )
     analytic_account_id = fields.Many2one(
-        'account.analytic.account',
-        string='Analytic Account',
+        string='Contract',
+        comodel_name='account.analytic.contract',
         required=True,
         ondelete='cascade',
     )
diff --git a/contract_variable_quantity/models/contract.py b/contract_variable_quantity/models/contract.py
index 1d25df9..dff047a 100644
--- a/contract_variable_quantity/models/contract.py
+++ b/contract_variable_quantity/models/contract.py
@@ -45,8 +45,8 @@ class AccountAnalyticAccount(models.Model):
         return vals
 
 
-class AccountAnalyticInvoiceLine(models.Model):
-    _inherit = 'account.analytic.invoice.line'
+class AccountAnalyticContractLine(models.Model):
+    _inherit = 'account.analytic.contract.line'
 
     qty_type = fields.Selection(
         selection=[

NL66278 · 2018-07-12T08:36:01Z

For long term maintainance i would rather have a lot of changes in one PR, then some completely misnamed files in perpetuity. But maybe the changes can be minimized by having one commit renaming the files and then another with the minimal changes to give the files their proper role.

I would definitely be interested in backporting the solution adopted to 10.0, where I hit the same problem.

NL66278

👍 LGTM

MiquelRForgeFlow · 2018-07-12T09:12:04Z

@katyukha As @NL66278 said, the best solution is make a commit with the minimum changes and another commit with the rename of the files

Bug description --------------- `account.analytic.contract.line` inherits `account.analytic.invoice.line` `account.analytic.invoice.line` defines field `analytic_account_id`: - comodel='account.analytic.account' `account.analytic.contract.line` redefines field `analytic_account_id`: - comodel='account.analytic.contract' On attempt to extend `account.analytic.invoice.line` model adding field that depends on `analytic_account_id.date_start` Odoo fails to update, because it adds this field to `account.analytic.contract.line` through inheritance, and `account.analytic.contract` model have no this field. What is done ------------ Change inheritance order: - `account.analytic.invoice.line` inherits `account.analytic.contract.line` - no file renames at this stage (this wil be done in next commit)

katyukha · 2018-07-12T10:54:32Z

Reimplemented it as two commits: first - minimal changed and second - only file renames.

And during this work, i see there is some hacks in module code to handle incorrect inheritance. Something like:

if self._name != 'account.analytic.invoice.line':
    return

This code is used in computation methods on line's dates fields. May be it have sense to move these fields to account.analytic.invoice.line and set proper depends on compute methods?

MiquelRForgeFlow · 2018-07-12T11:07:39Z

Something you missed, as travis ci failed.

Also, can you please divide the renaming commit in two commits step?

Like:

first rename commit:
account_analytic_contract_line.py -> account_analytic_contract_line_bis.py
account_analytic_invoice_line.py -> account_analytic_invoice_line_bis.py
second rename commit:
account_analytic_contract_line_bis.py -> account_analytic_invoice_line.py
account_analytic_invoice_line_bis.py -> account_analytic_contract_line.py

Then you may squash the first rename commit with the fix inheritance just to have 2 commits in all instead of 3 if you want, but I think that it's already fine with 3 commits.

Seems a bit stupid, but it would be very nice!! 😊

In previous commit changed inheritance order of 'account.analytic.*.line' models, thus classes and models were renamed. This commit only renames files to temporary names. This commit does not change file contents.

In previous commit 'account_analytic_*_line' model files were renamed to temporary names. This commit renames these files to correct names

katyukha · 2018-07-12T12:43:26Z

All done! :)

MiquelRForgeFlow · 2018-07-12T12:49:59Z

remove the [wip] in the title 😉

katyukha · 2018-07-12T12:57:04Z

removed :)

JordiBForgeFlow · 2018-07-12T16:22:12Z

@katyukha I understand that this change impacts on the data model, does it? in that case, changes to the data model should include a migration script.

katyukha · 2018-07-12T16:45:38Z

@jbeficent This PR changes only inheritance order. Data structure (database tables and fields) is not changed, so it seems that no migration needed.

Side effects may happen in other addons that inherit account.analytic.invoice.line and expects that changes will be forwarded to account.analytic.contract.line. This happened in contract_variable_quantity_addon, and in this case that addons have to be modified to inherit from account.analytic.contract.line instead.

MiquelRForgeFlow · 2018-07-12T17:53:14Z

@katyukha could you add in this PR a fix for contract_variable_quantity module? A PR should not break any already installable module of the same repo.

katyukha · 2018-07-12T18:00:39Z

@mreficent This fix already here. See diff

MiquelRForgeFlow · 2018-07-12T18:01:03Z

Ah, thanks!

katyukha · 2018-07-12T18:04:47Z

One thing i've missed is to update versions of addons...
Is it ok to do it as separate commit, or it would be better to ammend last one?

MiquelRForgeFlow · 2018-07-12T18:06:44Z

As you prefer, I think there isn't preference about this. I would do a new commit.

- contract - contract_variable_quantity

katyukha · 2018-07-13T08:31:43Z

Versions have been updated!
But there is strange error on travis: build failed to start.

MiquelRForgeFlow · 2018-07-13T12:09:16Z

@katyukha It's full green again 😉

I just have run an analysis with openupgrade_records just to be sure there is not need of migration scripts 👍 @jbeficent @pedrobaeza

katyukha changed the title ~~[FIX] analytic invoice/contract lines inheritance~~ [WIP] [FIX] analytic invoice/contract lines inheritance Jun 26, 2018

katyukha force-pushed the 11.0-fix-1 branch from 31f71de to f47eb9a Compare June 28, 2018 21:16

rafaelbn added this to the 11.0 milestone Jun 29, 2018

rafaelbn added the needs review label Jun 29, 2018

rafaelbn requested review from pedrobaeza and carlosdauden June 29, 2018 15:47

rafaelbn changed the title ~~[WIP] [FIX] analytic invoice/contract lines inheritance~~ [WIP][FIX][11.0][contract] analytic invoice/contract lines inheritance Jun 29, 2018

MiquelRForgeFlow approved these changes Jul 4, 2018

View reviewed changes

MiquelRForgeFlow suggested changes Jul 4, 2018

View reviewed changes

katyukha mentioned this pull request Jul 11, 2018

[10.0] Overriding field analytic_account_id in account.analytic.contract.line breaks link between account.analytic.invoice.line and account.analytic.account. #124

Closed

NL66278 approved these changes Jul 12, 2018

View reviewed changes

katyukha force-pushed the 11.0-fix-1 branch from f47eb9a to 999bf43 Compare July 12, 2018 10:38

katyukha added 2 commits July 12, 2018 14:33

Rename account_analytic_*_line files

a3da404

In previous commit changed inheritance order of 'account.analytic.*.line' models, thus classes and models were renamed. This commit only renames files to temporary names. This commit does not change file contents.

Rename account_analytic_*_line files

31df1b7

In previous commit 'account_analytic_*_line' model files were renamed to temporary names. This commit renames these files to correct names

katyukha force-pushed the 11.0-fix-1 branch from 999bf43 to 31df1b7 Compare July 12, 2018 11:54

MiquelRForgeFlow approved these changes Jul 12, 2018

View reviewed changes

katyukha changed the title ~~[WIP][FIX][11.0][contract] analytic invoice/contract lines inheritance~~ [FIX][11.0][contract] analytic invoice/contract lines inheritance Jul 12, 2018

Update addon versions

4bffe59

- contract - contract_variable_quantity

JordiBForgeFlow merged commit b5d5ea3 into OCA:11.0 Jul 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX][11.0][contract] analytic invoice/contract lines inheritance #169

[FIX][11.0][contract] analytic invoice/contract lines inheritance #169

katyukha commented Jun 26, 2018 •

edited

Loading

MiquelRForgeFlow left a comment

MiquelRForgeFlow Jul 4, 2018

MiquelRForgeFlow left a comment

katyukha commented Jul 11, 2018

NL66278 commented Jul 12, 2018

NL66278 left a comment

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018 •

edited

Loading

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

JordiBForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018 •

edited

Loading

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 13, 2018

MiquelRForgeFlow commented Jul 13, 2018

		@@ -46,7 +46,7 @@ def _prepare_invoice_line(self, line, invoice_id):


		class AccountAnalyticInvoiceLine(models.Model):

[FIX][11.0][contract] analytic invoice/contract lines inheritance #169

[FIX][11.0][contract] analytic invoice/contract lines inheritance #169

Conversation

katyukha commented Jun 26, 2018 • edited Loading

Bug description

What is done

MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

MiquelRForgeFlow Jul 4, 2018

Choose a reason for hiding this comment

MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

katyukha commented Jul 11, 2018

NL66278 commented Jul 12, 2018

NL66278 left a comment

Choose a reason for hiding this comment

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018 • edited Loading

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

JordiBForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018 • edited Loading

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 12, 2018

MiquelRForgeFlow commented Jul 12, 2018

katyukha commented Jul 13, 2018

MiquelRForgeFlow commented Jul 13, 2018

katyukha commented Jun 26, 2018 •

edited

Loading

MiquelRForgeFlow commented Jul 12, 2018 •

edited

Loading

MiquelRForgeFlow commented Jul 12, 2018 •

edited

Loading