Merge lp:~mathieu-julius/prestashoperpconnect/prestashoperpconnect-get-good-taxes-in-lines into lp:prestashoperpconnect

[Mathieu Vatel - Julius Network Solutions]

This is a fix to get the good taxes in the lines (delivery line) or even in sale order line if there's no taxes in prestashop.
The taxes in delivery lines should be improved by put the function in e-commerce-addons

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

The whole _add_order_extra_line seems redundant with what we have in the connector_ecommerce.

I think that you probably need to re-use most of what is done in connector_ecommerce (extracting / splitting the things in methods that you need to reuse).

Do you use this method? I can't find a single call to it.

By the way, for information, I started a refactoring of the code which add the extra order lines because 2 things bother me in the actual code:
 - it should not be part of the 'sale.order' model but rather use the ConnectorUnit mechanism
 - the temporary fields put in the record and then removed back really really hurt me
The branches are here:
https://code.launchpad.net/~openerp-connector-core-editors/openerp-connector/7.0-e-commerce-addons-refactor-so-extra-lines
https://code.launchpad.net/~openerp-connector-core-editors/openerp-connector/7.0-magentoerpconnect-refactor-so-extra-lines
https://code.launchpad.net/~openerp-connector-core-editors/openerp-connector/7.0-connector-mapper-refactor

review: Needs Fixing

[lp:~mathieu-julius/prestashoperpconnect/prestashoperpconnect-get-good-taxes-in-lines]

238. By Mathieu Vatel - Julius Network Solutions on 2013-07-15

[IMP] move the method in the good model (sale.order)

[Mathieu Vatel - Julius Network Solutions (mathieu-julius)]

The method was defined in the wrong orm.Model

It should be defined in the Model sale.order to replace the method done in ~/connector_ecommerce/sale.py

I don't know how to inherit this function instead of replace it. But with this replacement the lines of deliveries are taking the good tax rate (based on what was done in prestashop).
Maybe this should be directly in connector_ecommerce instead but I don't know how are working other e-commerce on this point, that the reason why I've done it there.

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

> The method was defined in the wrong orm.Model
>
> It should be defined in the Model sale.order to replace the method done in
> ~/connector_ecommerce/sale.py
>
> I don't know how to inherit this function instead of replace it. But with this
> replacement the lines of deliveries are taking the good tax rate (based on
> what was done in prestashop).
> Maybe this should be directly in connector_ecommerce instead but I don't know
> how are working other e-commerce on this point, that the reason why I've done
> it there.

No opposition to put it here if this is a special case, but you should not override completely the _add_order_extra_line method. Rather, you should inherit it (like any other OpenERP method), the correct way to do it it to split the original method in connector_ecommerce in parts (including hooks if you need) so you can reuse every parts which are common and to propose a MP on the connector_ecommerce's branch.

What do you need to be able to do that?

review: Needs Fixing

[Mathieu Vatel - Julius Network Solutions (mathieu-julius)]

> > The method was defined in the wrong orm.Model
> >
> > It should be defined in the Model sale.order to replace the method done in
> > ~/connector_ecommerce/sale.py
> >
> > I don't know how to inherit this function instead of replace it. But with
> this
> > replacement the lines of deliveries are taking the good tax rate (based on
> > what was done in prestashop).
> > Maybe this should be directly in connector_ecommerce instead but I don't
> know
> > how are working other e-commerce on this point, that the reason why I've
> done
> > it there.
>
> No opposition to put it here if this is a special case, but you should not
> override completely the _add_order_extra_line method. Rather, you should
> inherit it (like any other OpenERP method), the correct way to do it it to
> split the original method in connector_ecommerce in parts (including hooks if
> you need) so you can reuse every parts which are common and to propose a MP on
> the connector_ecommerce's branch.
>
> What do you need to be able to do that?

I will be able to inherit well the method with this replacement of the original method (extract the vals of the _add_order_extra_line method):

    def _get_order_extra_line_vals(self, cr, uid, vals, option, context=None):
        return {
            'product_id': product.id,
            'name': product.name,
            'product_uom': product.uom_id.id,
            'product_uom_qty': 1,
            'price_unit': price_unit
        }

    def _add_order_extra_line(self, cr, uid, vals, option, context=None):
        """ Add or substract amount on order as a separate line item
        with single quantity for each type of amounts like: shipping,
        cash on delivery, discount, gift certificates...

        :param dict vals: values of the sale order to create
        :param option: dictionary of options for the special field to process
        """
        if context is None:
            context = {}
        sign = option.get('sign', 1)
        if (context.get('is_tax_included') and
                vals.get(option['price_unit_tax_included'])):
            price_unit = vals.pop(option['price_unit_tax_included']) * sign
        elif vals.get(option['price_unit_tax_excluded']):
            price_unit = vals.pop(option['price_unit_tax_excluded']) * sign
        else:
            return self._clean_special_fields(option, vals)
        model_data_obj = self.pool.get('ir.model.data')
        product_obj = self.pool.get('product.product')
        __, product_id = model_data_obj.get_object_reference(
                cr, uid, *option['product_ref'])
        product = product_obj.browse(cr, uid, product_id, context=context)

        extra_line = self._get_order_extra_line_vals(cr, uid, vals, option, context=context)

        ext_code_field = option.get('code_field')
        if ext_code_field and vals.get(ext_code_field):
            extra_line['name'] = "%s [%s]" % (extra_line['name'],
                                              vals[ext_code_field])
        vals['order_line'].append((0, 0, extra_line))

        return self._clean_special_fields(option, vals)

[Mathieu Vatel - Julius Network Solutions (mathieu-julius)]

with "product" and "price_unit" in the attributes of the new method (which was missing):

    def _get_order_extra_line_vals(self, cr, uid,
            vals, option, product, price_unit, context=None):
        return {
            'product_id': product.id,
            'name': product.name,
            'product_uom': product.uom_id.id,
            'product_uom_qty': 1,
            'price_unit': price_unit
        }

    def _add_order_extra_line(self, cr, uid, vals, option, context=None):
        """ Add or substract amount on order as a separate line item
        with single quantity for each type of amounts like: shipping,
        cash on delivery, discount, gift certificates...

        :param dict vals: values of the sale order to create
        :param option: dictionary of options for the special field to process
        """
        if context is None:
            context = {}
        sign = option.get('sign', 1)
        if (context.get('is_tax_included') and
                vals.get(option['price_unit_tax_included'])):
            price_unit = vals.pop(option['price_unit_tax_included']) * sign
        elif vals.get(option['price_unit_tax_excluded']):
            price_unit = vals.pop(option['price_unit_tax_excluded']) * sign
        else:
            return self._clean_special_fields(option, vals)
        model_data_obj = self.pool.get('ir.model.data')
        product_obj = self.pool.get('product.product')
        __, product_id = model_data_obj.get_object_reference(
                cr, uid, *option['product_ref'])
        product = product_obj.browse(cr, uid, product_id, context=context)

        extra_line = self._get_order_extra_line_vals(cr,
                uid, vals, option, product, price_unit, context=context)

        ext_code_field = option.get('code_field')
        if ext_code_field and vals.get(ext_code_field):
            extra_line['name'] = "%s [%s]" % (extra_line['name'],
                                              vals[ext_code_field])
        vals['order_line'].append((0, 0, extra_line))

        return self._clean_special_fields(option, vals)

[lp:~mathieu-julius/prestashoperpconnect/prestashoperpconnect-get-good-taxes-in-lines]

239. By Mathieu Vatel - Julius Network Solutions on 2013-07-15

[IMP] - inherit the method which has been merge in the revno 333 of e-commerce-addons branch

[Mathieu Vatel - Julius Network Solutions (mathieu-julius)]

[IMP] => inherit the new method which have been merged in revno 333 of e-commerce-addons instead of replacing totally the original method

review: Needs Resubmitting

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Thanks for your MP on e-commerce-addons!

Would be nice with a test, it would explain your intention which is hard to guess.

In Magentoerpconnect we removed totally the option which try to infer the tax from the tax rate, because it was not accurate: it takes the first tax found, that can be wrong.

Instead, the tax must be defined on the product used for the shipping (hence this MP [0] to use the shipping product of the delivery method).

Is it different with Prestashop?

However, I noted that the 'is_tax_included' is never set in the context so the change seems not complete.

The thoughts of the Akretion's people would be appreciable here.

[0] https://code.launchpad.net/~openerp-connector-core-editors/openerp-connector/7.0-e-commerce-addons-shipping-setup-product/+merge/168456

[Mathieu Vatel - Julius Network Solutions (mathieu-julius)]

I agree on the fact we can get twice or 3 times a tax with the same tax rate, but the thing is sometimes, we've got tax on prestashop on the shipping and sometimes there is no tax in the line which can be actually fix with a fiscal position defined on the partner or directly on the sale order.

I think we have to speak about it. Maybe get the Akretion's people thoughts and yours too ! Don't merge it now ;)

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

Hi Mathieu.
For the e-commerce we have two posibility
- using the external tax and mapping it.
- ask openerp to apply the good taxe.

The first solution seem better/easier but In many case your customer not only take care about the taxe but also to the account. For exemple a sale in US will be in the account "vente à a l'export" and a French sale will be in "Vente de marchandise 19.6".
Also depending of the taxe 19.6 or 5.5 you may need to use different account like "vente de marchandise 19.6" and "vente de marchandise 5.5".
So for real customer in production that use accounting you have to configure correctly the fiscal_position and install the module "account_fiscal_position_rule_sale" (https://code.launchpad.net/openerp-fiscal-rules), and also "l10n_fr_rule" (https://code.launchpad.net/~account-core-editors/openerp-fiscal-rules/l10n_fr_rule)
When you have installed this module and configured correctly the fiscal position, the best solution is to ask OpenERP to recompute the tax and check the total amount. This that solution you will be sure that in prestashop and OpenERP there is no error in the tax configuration. A lot a people do ugly mistake in tax configuration in Prestashop or Magento, with this double check all of error are found easily.

We already discuss of this point with Guewen, and the think that for all e-commerce solution the default is to recompute again and check the result.

If we still want to use the external tax (maybe in some country we do not care of the account, or maybe the customer do not care of the accounting) I propose to create an extra module that use the external tax. But I am not sure it's worth, a least for the demonstration it's very interesting because you have nothing to do but for real production not sure it's a good idea to skip the fiscal position configuration.

So I propose to you to extract you work in a new module "prestashoperpconnect_external_tax"

Thanks

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

review: Needs Fixing

Unmerged revisions

239. By Mathieu Vatel - Julius Network Solutions on 2013-07-15

[IMP] - inherit the method which has been merge in the revno 333 of e-commerce-addons branch

238. By Mathieu Vatel - Julius Network Solutions on 2013-07-15

[IMP] move the method in the good model (sale.order)

237. By Mathieu Vatel - Julius Network Solutions on 2013-07-15

[IMP] make possible to get the lines with or without taxes (which is defined in prestashop)

=== modified file 'prestashoperpconnect/sale.py'
--- prestashoperpconnect/sale.py	2013-07-15 14:26:03 +0000
+++ prestashoperpconnect/sale.py	2013-07-15 14:50:35 +0000
@@ -30,7 +30,6 @@
 from .connector import get_environment
 from backend import prestashop
 
-
 class sale_order_state(orm.Model):
     _name = 'sale.order.state'
 
@@ -43,7 +42,6 @@
         ),
     }
 
-
 class prestashop_sale_order_state(orm.Model):
     _name = 'prestashop.sale.order.state'
     _inherit = 'prestashop.binding'
@@ -63,7 +61,6 @@
         ),
     }
 
-
 class sale_order_state_list(orm.Model):
     _name = 'sale.order.state.list'
 
@@ -99,6 +96,28 @@
         ),
     }
 
+    def _get_order_extra_line_vals(self, cr, uid, vals, option,
+                                   product, price_unit, context=None):
+        line_vals = super(sale_order, self)._get_order_extra_line_vals(cr,
+            uid, vals, option, product, price_unit, context=context)
+        tax_rate = vals.get(option['tax_rate_field']) and vals.pop(option['tax_rate_field'])
+        if tax_rate and float(tax_rate):
+            tax_rate = float(tax_rate)
+            if tax_rate > 1.0:
+                tax_rate = tax_rate / 100.0
+            included = context.get('is_tax_included', False)
+            tax_obj = self.pool.get('account.tax')
+            tax_ids = tax_obj.search(cr, uid, [
+                ('type', '=', 'percent'),
+                ('amount', '=', tax_rate),
+                ('type_tax_use', 'in', ('sale','all')),
+                ('price_include', '=', included),
+                ], context=context)
+            if not tax_ids:
+                raise ValueError('Can\'t found a tax %s, with percentage %s.'
+                    %(included and 'included' or 'not included',str(tax_rate)))
+            line_vals.update({'tax_id': [(6, 0, [tax_ids[0]])]})
+        return line_vals
 
 class prestashop_sale_order(orm.Model):
     _name = 'prestashop.sale.order'
@@ -121,11 +140,19 @@
         'prestashop_delivery_number': fields.char('PrestaShop Delivery Number',size=64),
     }
 
-
 @prestashop
 class PrestaShopSaleOrderOnChange(SaleOrderOnChange):
     _model_name = 'prestashop.sale.order'
 
+    def _get_product_id_onchange_param(self, line, previous_lines, order):
+        # This is done to be able to get or not the tax
+        # with the onchange on delivery lines.
+        args, kwargs = super(PrestaShopSaleOrderOnChange,
+            self)._get_product_id_onchange_param(line, previous_lines, order)
+        update_tax = line.get('update_tax') and line.pop('update_tax') or False
+        if not update_tax:
+            kwargs['update_tax'] = False
+        return args, kwargs
 
 class sale_order_line(orm.Model):
     _inherit = 'sale.order.line'
@@ -138,7 +165,6 @@
         ),
     }
 
-
 class prestashop_sale_order_line(orm.Model):
     _name = 'prestashop.sale.order.line'
     _inherit = 'prestashop.binding'
@@ -178,13 +204,11 @@
             context=context
         )
 
-
 @prestashop
 class SaleOrderStateAdapter(GenericAdapter):
     _model_name = 'prestashop.sale.order.state'
     _prestashop_model = 'order_states'
 
-
 @prestashop
 class SaleOrderAdapter(GenericAdapter):
     _model_name = 'prestashop.sale.order'
@@ -194,13 +218,11 @@
         self._prestashop_model = 'order_histories'
         self.create(datas)
 
-
 @prestashop
 class SaleOrderLineAdapter(GenericAdapter):
     _model_name = 'prestashop.sale.order.line'
     _prestashop_model = 'order_details'
 
-
 @prestashop
 class SaleStateExport(ExportSynchronizer):
     _model_name = ['prestashop.sale.order']
@@ -216,7 +238,6 @@
         }
         self.backend_adapter.update_sale_state(prestashop_id, datas)
 
-
 # TODO improve me, don't try to export state if the sale order does not come
 #      from a prestashop connector
 # TODO improve me, make the search on the sale order backend only
@@ -252,7 +273,6 @@
             )
     return True
 
-
 @job
 def export_sale_state(session, model_name, record_id, new_state):
     inherit_model = 'prestashop.' + model_name

=== modified file 'prestashoperpconnect/unit/mapper.py'
--- prestashoperpconnect/unit/mapper.py	2013-07-15 14:26:03 +0000
+++ prestashoperpconnect/unit/mapper.py	2013-07-15 14:50:35 +0000
@@ -292,9 +292,11 @@
     def shipping(self, record):
         shipping_tax_incl = float(record['total_shipping_tax_incl'])
         shipping_tax_excl = float(record['total_shipping_tax_excl'])
+        shipping_tax_rate = float(record['carrier_tax_rate'])
         return {
             'shipping_amount_tax_included': shipping_tax_incl,
             'shipping_amount_tax_excluded': shipping_tax_excl,
+            'shipping_tax_rate': shipping_tax_rate,
         }
 
     @mapping
@@ -377,6 +379,13 @@
     ]
 
     @mapping
+    def taxes(self, record):
+        update_tax = True
+        if record.get('total_price_tax_incl') == record.get('total_price_tax_excl'):
+            update_tax = False
+        return {'update_tax': update_tax}
+    
+    @mapping
     def product_id(self, record):
         return {'product_id': self.get_openerp_id(
             'prestashop.product.product',