Error saving purchase order if purchase_tax_include is installed (function field with 'store' overwriten)

[Borja López Soilán (NeoPolus)]

On 5.0 and 6.0, if the purchase_tax_include module (from extra-addons) is installed, saving a purchase order will throw an exception: "column "amount_tax" of relation "purchase_order" does not exist"

The problem seems to be that the 'amount_tax' field (a function field with store={...}) is being redefined by purchase_tax_include as a function field without 'store'.

Current 5.0 amount_tax field is defined like this:

        'amount_tax': fields.function(_amount_all, method=True, digits=(16, int(config['price_accuracy'])), string='Taxes',
            store={
                'purchase.order.line': (_get_order, None, 10),
            }, multi="sums"),

Current 6.0 amount_tax field is defined like this:

        'amount_tax': fields.function(_amount_all, method=True, digits_compute= dp.get_precision('Purchase Price'), string='Taxes',
            store={
                'purchase.order.line': (_get_order, None, 10),
            }, multi="sums"),

And purchase_tax_include amount_tax_field is defined like this:

        'amount_tax': fields.function(_amount_tax, method=True, string='Taxes'),

As you can see, the last definition is not compatible with the previous ones (does not have 'store').
It seems that the bug is present since 2008-12-05 (on revno 2047 of the 5.0 addons the 'store' options where added to the amount_tax field).

I'll fix this bug for this module, but my question for the Framework Experts is:

- Shouldn't the framework be able to handle this case (a function field with 'store' being overwritten by a function field without 'store')?

[Nhomar - Vauxoo (nhomar)]

The framework should inform this.... +1 for the Idea...

Here we have two issues:

1.- What you are saying (technical issue).
2.- How something than _important_ like the tax in a document can be bad
implemented in a module on Extra-Addons....

If you can refer to the workshops http://piratepad.net/communitydays-merges

We took this point in exrta-addons this kind of problem should be avoided by
us, we didn't try this module yet, but for example, the purchase_discount
module overwrite all the functions about cost calculation with several
accounting issues, This is a similar problem in my point of view....

How technically do you think we can avoid this kind of errors on
addons-extra?

2010/7/8 Launchpad Bug Tracker <email address hidden>

> You have been subscribed to a public bug by Borja López Soilán (Pexego)
> (borjals):
>
> On 5.0 and 6.0, if the purchase_tax_include module (from extra-addons)
> is installed, saving a purchase order will throw an exception: "column
> "amount_tax" of relation "purchase_order" does not exist"
>
> The problem seems to be that the 'amount_tax' field (a function field
> with store={...}) is being redefined by purchase_tax_include as a
> function field without 'store'.
>
> Current 5.0 amount_tax field is defined like this:
>
> 'amount_tax': fields.function(_amount_all, method=True, digits=(16,
> int(config['price_accuracy'])), string='Taxes',
> store={
> 'purchase.order.line': (_get_order, None, 10),
> }, multi="sums"),
>
> Current 6.0 amount_tax field is defined like this:
>
> 'amount_tax': fields.function(_amount_all, method=True,
> digits_compute= dp.get_precision('Purchase Price'), string='Taxes',
> store={
> 'purchase.order.line': (_get_order, None, 10),
> }, multi="sums"),
>
> And purchase_tax_include amount_tax_field is defined like this:
>
> 'amount_tax': fields.function(_amount_tax, method=True,
> string='Taxes'),
>
> As you can see, the last definition is not compatible with the previous
> ones (does not have 'store').
> It seems that the bug is present since 2008-12-05 (on revno 2047 of the 5.0
> addons the 'store' options where added to the amount_tax field).
>
>
> I'll fix this bug for this module, but my question for the Framework
> Experts is:
>
> - Shouldn't the framework be able to handle this case (a function field
> with 'store' being overwritten by a function field without 'store')?
>
> ** Affects: openobject-addons
> Importance: Undecided
> Status: New
>
> --
> Error saving purchase order if purchase_tax_include is installed (function
> field with 'store' overwriten)
> https://bugs.launchpad.net/bugs/603100
> You received this bug notification because you are subscribed to
> OpenObject.
>

--
Saludos Cordiales

Nhomar G. Hernandez M.
[phone]
[phone]
[phone] ext 124
[phone]
Skype: nhomar00
Web-Blog: http://geronimo.com.ve
Servicios IT: http://openerp.netquatro.com
Linux-Counter: 467724
Correos:
<email address hidden>
<email address hidden>

[Borja López Soilán (NeoPolus) (borjals)]

Thanks for the comments Nhomar.

*** About the technical / framework problem ***

The root of the problem is the framework not dealing gracefully with function fields being overwritten:

  - It does not allow to easily extend the method used by the function field. So the developers must overwrite the full function field definition to use their method implementation. (That's why the 'amount_tax' needs to be redefined on purchase_tax_include). It would be interesting to have a way to redefine some attributes of the field alone. Like changing the methods used on function fields, or changing the caption, without having to overwrite the full field.

  - When redefining a function field, if the store attribute is not specified, the ORM seems to 'remove' the field from the table, but still tries to store it, so it raises those nasty "column X of relation Y does not exist".

*** About the purchase_tax_include module itself ***

The truth is that is *severely* broken :(

After fixing the previous bug (the "column 'amount_tax' of relation 'purchase_order' does not exist" / "no store" problem), I could use the module a bit, and discovered that:

  - The module shows incorrect 'amount_untaxed' and 'amount_total' on the purchase order (though the lines values are the expected), cause only 'amount_tax' is overwritten.
    (* I already corrected this on my branch).

  - The "price_subtotal_incl" is not getting added to the purchase order lines of the order form view (so the user can not see the total value with taxes).

  - When the order is invoiced, the created invoice does not use the "tax_included" price type, so the values don't match the order.

So, I'm starting to think that this module never worked at all... :(

Anybody using it and willing to exchange experiences?

[Carlos Liebana (carlos-liebana)]

Hi Borja,

I don't know about purchase_tax_included, but we've used sale_tax_included in the past (most of e-commerce implementations requires it), with no problem. No one of the problems listed for purchase appears there, have you tried it? I suppose most of the code there can be refactored for purchase.

[Borja López Soilán (NeoPolus) (borjals)]

Carlos, I haven't tested sale_tax_included thoroughly, but I remember that it at least seemed to work :)

purchase_tax_included on the other end made impossible to confirm a purchase (cause it raised the exception) [I already fixed this on the branch I'll publish later], did not properly calculate the order totals [also fixed on my branch], did not display the "subtotal with taxes" on the lines [fixed on my branch], did not set the price_type of the invoices when creating invoices from the order [already fixed] nor when creating invoices from the picking list [fixing it today].

So:

1. The module was unusable! :(

2. Most of the problems could be solved in a day, but it seems that nobody tried to fix them on the last year and a half (the module is totally broken since the 'purchase.order' improvement back in 2008-12-05).

I'll publish a branch with all those fixes later.

[Borja López Soilán (NeoPolus) (borjals)]

By the way Carlos, I just found some problems on the sale_tax_included module:

- When creating invoices from orders, the invoice price type is not set as the one from the order (so if the order had included taxes prices, the invoice will not, and they won't match).

- When creating invoices from picking lists, it checks the original orders to check the price type to use, but it matches the pickings with its sale orders by origin (which is user editable free text, OMG!) and name. It does this:
       for picking in self.browse(cursor, user, ids, context=context):
           sale_ids = sale_obj.search(cursor, user, [('name','=',picking.origin)],context=context)
  I think it should do something like this instead (use the many2one reference to the sale order; it is there for a purpose!):
       sale_ids = [picking.sale_id.id for picking in self.browse(cursor, user, ids, context=context) if picking.sale_id]

Maybe I should register a bug report for the sale_tax_include too :(

[Borja López Soilán (NeoPolus) (borjals)]

Sorry for so much "branch dancing".

I just proposed to merge two branches (one for 5.0 and one for trunk/6.0) that fix all the mentioned problems for the module. As it is almost a full rewrite of the module it would be a good idea for someone to review it :)

[Borja López Soilán (NeoPolus) (borjals)]

The fixes where merged on revno 4462 of the stable (5.0) extra-addons, and revno 4743 of the trunk (6.0) extra-addons.
Thanks for yours comments people :)