Merge lp:~roberto-v/account-payment/adding_account_vat_on_payment_7 into lp:~domsense/account-payment/adding_account_vat_on_payment_7

[Roberto Gianassi]

[lp:~roberto-v/account-payment/adding_account_vat_on_payment_7]

112. By Roberto Gianassi on 2014-02-11

[IMP] Copyright fix.

[Lorenzo Battistini (elbati)]

Ciao Roberto,
many thanks for your contribution.

Some remarks:
 - Why is default_has_vat_on_payment a selection instead of a boolean?
 - Now, if the vat_on_payment invoice field is (correctly) set by the onchange_partner_id method, probably the _get_vat_on_payment method should be used to set the default value of default_has_vat_on_payment when creating new partners
 - Please run a PEP8 validator on your code

review: Needs Fixing

[Roberto Gianassi (roberto-v)]

> Ciao Roberto,

Ciao Lorenzo,

> many thanks for your contribution.
>
> Some remarks:
> - Why is default_has_vat_on_payment a selection instead of a boolean?

It is that way to force the user to always fill the right value.
If it were a boolean, it would be rendered on the UI as an initial empty checkbox: it would be very simple to forget to check the flag (when it has to be checked).

> - Now, if the vat_on_payment invoice field is (correctly) set by the
> onchange_partner_id method, probably the _get_vat_on_payment method should be
> used to set the default value of default_has_vat_on_payment when creating new
> partners

Please, could you elaborate a bit more on this?
What is the purpose of the _get_vat_on_payment method?

> - Please run a PEP8 validator on your code

Ok.

Thank you for the review.

[lp:~roberto-v/account-payment/adding_account_vat_on_payment_7]

113. By Roberto Gianassi on 2014-02-11

[IMP] PEP8 fixings.

114. By Roberto Gianassi on 2014-02-12

[FIX] Fixed a bug on XML record creation.

If a module (like fetchmail_invoice) containing a partner record to be
preinstalled through a data.xml file was installed after this one, a
bug occurred.
The bug shows because the record is created with the
default_has_vat_on_payment flag to empty and then validation fails.
To solve this, the create function for partners is changed to check for
the presence of that flag in the vals dictionary. If the flag is found,
we assume the creation happens through the UI and so normal checking goes
on. If the flag is not found, we assume the creation happens for an XML
record and so we add an arbitrary, but valid value for the flag.

115. By Roberto Gianassi on 2014-02-12

[FIX] Typo.

[Lorenzo Battistini (elbati)]

2014-02-11 18:03 GMT+01:00 Roberto Gianassi <email address hidden>:

> > Ciao Roberto,
>
> Ciao Lorenzo,
>
> > many thanks for your contribution.
> >
> > Some remarks:
> > - Why is default_has_vat_on_payment a selection instead of a boolean?
>
> It is that way to force the user to always fill the right value.
> If it were a boolean, it would be rendered on the UI as an initial empty
> checkbox: it would be very simple to forget to check the flag (when it has
> to be checked).
>

I mean, conceptually it is a boolean field, it can accept two values.
if I understand what you mean, you want to force the user to do a choice.
But this can be applied to every boolean field of OpenERP. So, in this
case, the boolean field interface behaviour should be changed system wide.

Moreover, using a boolean field allows you to avoid workaround like
this<http://bazaar.launchpad.net/~roberto-v/account-payment/adding_account_vat_on_payment_7/revision/114>

See below about setting a global default

>
> > - Now, if the vat_on_payment invoice field is (correctly) set by the
> > onchange_partner_id method, probably the _get_vat_on_payment method
> should be
> > used to set the default value of default_has_vat_on_payment when
> creating new
> > partners
>
> Please, could you elaborate a bit more on this?
> What is the purpose of the _get_vat_on_payment method?
>

At the moment, it sets the default value for 'vat_on_payment' field of
account.invoice, retrieving it from the global setting 'vat_on_payment'.
It think, after your changes, the global vat_on_payment setting should be
used for the default_has_vat_on_payment field.

Thanks

[Roberto Gianassi (roberto-v)]

> 2014-02-11 18:03 GMT+01:00 Roberto Gianassi <email address hidden>:
>
> > > Ciao Roberto,
> >
> > Ciao Lorenzo,
> >
> > > many thanks for your contribution.
> > >
> > > Some remarks:
> > > - Why is default_has_vat_on_payment a selection instead of a boolean?
> >
> > It is that way to force the user to always fill the right value.
> > If it were a boolean, it would be rendered on the UI as an initial empty
> > checkbox: it would be very simple to forget to check the flag (when it has
> > to be checked).
> >
>
> I mean, conceptually it is a boolean field, it can accept two values.
> if I understand what you mean, you want to force the user to do a choice.
> But this can be applied to every boolean field of OpenERP. So, in this
> case, the boolean field interface behaviour should be changed system wide.
>
> Moreover, using a boolean field allows you to avoid workaround like
> this<http://bazaar.launchpad.net/~roberto-v/account-
> payment/adding_account_vat_on_payment_7/revision/114>
>
> See below about setting a global default
>

Yes, I see your point.
The problem is that the vat_on_payment flag has to be set differently for every supplier.
There are some rules (for example the supplier has to be Italian) on when you can use VAT on Payment or not and an error here may have legal/financial consequences.
Technically, the selection will be equivalent to a required field. I know you can make a Boolean required, but, in practice, it has no effect (the user can simply ignore it).
I don't know if there are other ways to force a user to make a Boolean choice. In case, let me know.
I need the patch this way. If this is not possible I will go with a custom module.

Thanks for your time.

[lp:~roberto-v/account-payment/adding_account_vat_on_payment_7]

116. By Roberto Gianassi on 2014-02-14

[ADD] Moved default_has_vat_on_payment on account.fiscal.position.

After some research, the VAT on Payment seems dependent on the account
fiscal position of the customer/supplier.
Moved the default_has_vat_on_payment flag to that model to take that
into account.
The field is now a boolean because is not needed anymore to force the
user to make a choice.

117. By Roberto Gianassi on 2014-02-14

[FIX] When partner field is empty value is False.

The value passed to the onchange_partner_id for the partner is False
when the field is empty. In this case, it is not possible to use such a
value with the browse function or an error arises. Fixed that.

[Lorenzo Battistini (elbati)]

Roberto, thanks for your changes.
I think using fiscal positions is a better choice.

Some remarks:
line 39: 'for partner_id in partner_obj.browse(cr, uid, [partner_id])' can just be 'partner = partner_obj.browse(cr, uid, partner_id, context=context)'
line 41: if 'property_account_position' was not set, partner.property_account_position.default_has_vat_on_payment would raise exception
line 89, I think the _defaults is unnecessary

review: Needs Fixing

[lp:~roberto-v/account-payment/adding_account_vat_on_payment_7]

118. By Roberto Gianassi on 2014-02-24

[FIX] Fixing remarks.

[Lorenzo Battistini (elbati)]

Thanks.

PS: please send a message after the changes, launchpad does not automatically notifies of new commits

review: Approve

[Pedro Manuel Baeza (pedro.baeza)]

This project is now hosted on https://github.com/OCA/account-payment. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

=== modified file 'account_vat_on_payment/__init__.py'
--- account_vat_on_payment/__init__.py	2013-05-15 09:35:50 +0000
+++ account_vat_on_payment/__init__.py	2014-02-24 09:06:14 +0000
@@ -22,3 +22,4 @@
 ##############################################################################
 import account
 import company
+import partner
 
=== modified file 'account_vat_on_payment/__openerp__.py'
--- account_vat_on_payment/__openerp__.py	2013-11-27 14:37:50 +0000
+++ account_vat_on_payment/__openerp__.py	2014-02-24 09:06:14 +0000
@@ -52,6 +52,7 @@
     'update_xml': [
         'account_view.xml',
         'company_view.xml',
+        'partner_view.xml',
         ],
     'demo_xml': [],  # TODO YAML tests
     'installable': True,
 
=== modified file 'account_vat_on_payment/account.py'
--- account_vat_on_payment/account.py	2013-11-28 07:11:33 +0000
+++ account_vat_on_payment/account.py	2014-02-24 09:06:14 +0000
@@ -336,6 +336,22 @@
             new_move_lines.append(line_tup)
         return new_move_lines
 
+    def onchange_partner_id(
+            self, cr, uid, ids, type, partner_id, date_invoice=False,
+            payment_term=False, partner_bank_id=False, company_id=False):
+        res = super(account_invoice, self).onchange_partner_id(
+            cr, uid, ids, type, partner_id, date_invoice, payment_term,
+            partner_bank_id, company_id)
+        # default value for VAT on Payment is changed every time the
+        # customer/supplier is changed
+        partner_obj = self.pool.get("res.partner")
+        if partner_id:
+            p = partner_obj.browse(cr, uid, partner_id)
+            res['value']['vat_on_payment'] = p.property_account_position\
+                and p.property_account_position.default_has_vat_on_payment\
+                or False
+        return res
+
     _inherit = "account.invoice"
     _columns = {
         'vat_on_payment': fields.boolean('Vat on payment'),
 
=== added file 'account_vat_on_payment/partner.py'
--- account_vat_on_payment/partner.py	1970-01-01 00:00:00 +0000
+++ account_vat_on_payment/partner.py	2014-02-24 09:06:14 +0000
@@ -0,0 +1,36 @@
+# -*- coding: utf-8 -*-
+##############################################################################
+#
+#    OpenERP, Open Source Management Solution
+#    Copyright (C) 2011-2012 Domsense s.r.l. (<http://www.domsense.com>).
+#    Copyright (C) 2012-2013 Agile Business Group sagl
+#    (<http://www.agilebg.com>)
+#    Copyright (C) 2014 Develer srl (<http://www.develer.com>)
+#
+#    This program is free software: you can redistribute it and/or modify
+#    it under the terms of the GNU Affero General Public License as
+#    published by the Free Software Foundation, either version 3 of the
+#    License, or (at your option) any later version.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU Affero General Public License for more details.
+#
+#    You should have received a copy of the GNU Affero General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+##############################################################################
+
+
+from openerp.osv import fields, orm
+from openerp.tools.translate import _
+
+
+class account_fiscal_position(orm.Model):
+    _inherit = 'account.fiscal.position'
+
+    _columns = {
+        'default_has_vat_on_payment': fields.boolean(
+            'VAT on Payment Default Flag'),
+    }
 
=== added file 'account_vat_on_payment/partner_view.xml'
--- account_vat_on_payment/partner_view.xml	1970-01-01 00:00:00 +0000
+++ account_vat_on_payment/partner_view.xml	2014-02-24 09:06:14 +0000
@@ -0,0 +1,48 @@
+<?xml version="1.0"?>
+
+<!--
+# -*- coding: utf-8 -*-
+##############################################################################
+#
+#    OpenERP, Open Source Management Solution
+#    Copyright (C) 2011-2012 Domsense s.r.l. (<http://www.domsense.com>).
+#    Copyright (C) 2012-2013 Agile Business Group sagl
+#    (<http://www.agilebg.com>)
+#    Copyright (C) 2014 Develer srl (<http://www.develer.com>)
+#
+#    This program is free software: you can redistribute it and/or modify
+#    it under the terms of the GNU Affero General Public License as
+#    published by the Free Software Foundation, either version 3 of the
+#    License, or (at your option) any later version.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU Affero General Public License for more details.
+#
+#    You should have received a copy of the GNU Affero General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+##############################################################################
+-->
+
+
+<openerp>
+    <data>
+
+        <!-- res.partner customizations -->
+
+        <record id="view_partner_form" model="ir.ui.view">
+            <field name="name">account.fiscal.position.vat.default.view</field>
+            <field name="model">account.fiscal.position</field>
+            <field name="inherit_id" ref="account.view_account_position_form"/>
+            <field name="arch" type="xml">
+                <field name="active" position="after" version="7.0">
+                    <field name="default_has_vat_on_payment" class="oe_inline"/>
+                </field>
+            </field>
+        </record>
+
+
+    </data>
+</openerp>