Merge lp:~akretion-team/openobject-addons/trunk_prepare_refund into lp:openobject-addons

[Benoit Guillot - http://www.akretion.com]

This merge add a method prepare_refund to help the customization of the creation of a refund with the wizard. It also add the context as arguments of the methods def refund and _refund_cleanup_lines.

[Nhomar - Vauxoo (nhomar)]

Wao.

Dude, yesterday we fix a lot of thing in ur both localization writting all the original method.

For V7 this is a must to have ;-)

review: Approve

[Olivier Dony (Odoo) (odo-openerp)]

Nice work, thanks!

It looks good, except what is the purpose of the "refund_id" parameter passed to _prepare_refund(), and seemingly unused?

It would also be a nice touch to add docstrings explaining the meaning and types of the various parameters (not cr, uid, and context, but the others), similarly to what was done in the other _prepare_* methods, like here:
   http://bazaar.launchpad.net/~openerp/openobject-addons/trunk/view/7743/sale/sale.py#L310

Thanks!

review: Needs Information

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

Thanks Nhomar for the approval and Olivier for the comments !

I have added the dosctring for the method prepare.

Concerning the parameter "refund_id", it is a typo, I meant "invoice_id". I have fixed it.
I think it can be useful to have the id of the invoice in the method prepare.

Moreover, I would like to add a new prepare method : _prepare_cleanup_fields()
This method allows us to add new fields for the creation of the refund lines.
Tell me what you think about it please :)

Thank you.

Benoît

[Olivier Dony (Odoo) (odo-openerp)]

On 10/10/2012 05:56 PM, Benoit Guillot - http://www.akretion.com wrote:
> I have added the dosctring for the method prepare.

Thanks! The param type in docstrings is actually meant to be a Python type
(int/str/etc.), but we can fix it when merging, so no big deal.

> Concerning the parameter "refund_id", it is a typo, I meant "invoice_id". I have fixed it.
> I think it can be useful to have the id of the invoice in the method prepare.

Hmm, the thing is this parameter is unused, and redundant because the `invoice`
parameter contains the ID. In general we avoid passing unused parameters
because they risk being removed in the future, and prefer passing higher level
data structures that give you indirect access to more info. In this case the
`invoice` param plays that role.

Now if you prefer having an even higher-level param, you could replace the
`invoice` dict by a browse_record (and update the code accordingly). Not sure
if that's really useful, but it could save extra read() calls in inheriting
methods that need to access more columns of the invoice.

> Moreover, I would like to add a new prepare method : _prepare_cleanup_fields()
> This method allows us to add new fields for the creation of the refund lines.
> Tell me what you think about it please :)

It looks a bit overkill to me, as the main purpose of _refund_cleanup_lines()
is to fix the lines data, no need for a separate one. If your issue is simply
to have this method support custom m2o fields as well, why not remove the
hardcoded list of fields and instead check the type of the fields (via
account.invoice.line._all_columns) and handle all m2o ones properly?

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

Hello Olivier,

I agree with you about the invoice dict. I am ready to spend more time on this to replace it by a browse_record if you think it's worth it. As far as I'm concerned, I think it does, as you said it can save extra read() calls. Then I just have to manage also the refund_cleanup_lines that will change.

Otherwise, if we should keep the dict instead of the browse_record, I can manage the invoice_id in the dict and remove it from the parameters. It is indeed redundant. Moreover, about the method _refund_cleanup_lines(), using "account.invoice.line._all_columns" seems a better idea than using a _prepare() method.

So I can propose the two solutions, but maybe it worth to use this proposal to improve the refund creation with a browse_record instead of several reads.

[Olivier Dony (Odoo) (odo-openerp)]

On 10/11/2012 12:57 PM, Benoit Guillot - http://www.akretion.com wrote:
> I agree with you about the invoice dict. I am ready to spend more time on
> this to replace it by a browse_record if you think it's worth it. As far as
> I'm concerned, I think it does, as you said it can save extra read() calls.
> Then I just have to manage also the refund_cleanup_lines that will change.

Sure, go ahead, this should not break the current API and should be safe :-)

The fields that were read() from the invoice were limited, so it should be easy
to craft a new dictionary from a browse_record instead of a dict, taking care
of fixing m2o values as you go (based on the type coming from _all_columns)

Tip: browse_records support __getitem__ just like dict, so you can simply use
``invoice[f]`` to get the value for field ``f``, without having to use
getattr(invoice, f)

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

Thanks for the tip, it has helped me !

I have removed some fields from the dict of the invoice:
     - 'type', 'number' because theses are overwriten
     - 'partner_contact', 'partner_insite', 'partner_ref' because theses seem not exist anymore

I have refactored the method _refund_cleanup_lines(), but maybe it can be useful to create a generic method for instance : _browse_to_dict() which can be something like that (it's just an idea it can be improved):

        def _browse_to_dict(browse):
            for field in browse._all_columns.keys():
                field_type = browse._all_columns[field].column._type
                if field_type == 'many2one':
                    dict_from_browse[field] = browse[field].id
                elif field_type == 'many2many':
                    m2m_list = []
                    for link in browse[field]:
                        m2m_list.append(link.id)
                    dict_from_browse[field] = [(6,0, m2m_list)]
                elif field_type == 'one2many':
                    o2m_list = []
                    for o2m in browse[field]
                        o2m_dict = o2m._browse_to_dict(o2m)
                        o2m_list.append((0, 0, o2m_dict))
                     dict_from_browse[field] = o2m_list
                else:
                    dict_from_browse[field] = browse[field]
            return dict_from_browse

This method can be used in copy() and probably in other cases.

About the merge, I have tested, it seems to work. I haven't try it on the runbot, because akretion has been kicked out from the runbot (probably because of the 'a' of akretion).

[Olivier Dony (Odoo) (odo-openerp)]

On 10/12/2012 02:37 PM, Benoit Guillot - http://www.akretion.com wrote:
> Thanks for the tip, it has helped me !
>
> I have removed some fields from the dict of the invoice:
> - 'type', 'number' because theses are overwriten
> - 'partner_contact', 'partner_insite', 'partner_ref' because theses seem not exist anymore

Looks good, and how about the `journal_id` field, it seems overwritten as well
if I'm not mistaken?

> I have refactored the method _refund_cleanup_lines(), but maybe it can be
> useful to create a generic method for instance : _browse_to_dict() which can
> be something like that (it's just an idea it can be improved):
<SNIP>

It might be useful in other circumstances, however it should not be located in
the account module. In order to speed up the merge I suggest not doing it now.

Also you have an issue with o2m fields now (and m2m to some extent), because
you have removed the list of fields to copy in _refund_cleanup_lines.
I suppose this is to make the code simpler and possibly avoid having to inherit
this method when you add extra fields to the lines. However this may have
dangerous side effects. A module could track individual payment lines or some
special analytic lines and link them to the invoice line using a o2m or m2m -
but these should not be copied over to the refund line!
You do not handle the o2m case at all at the moment, so it will simply crash if
a module ever does this.
I think it would be much safer to keep a list of fields to copy, or at least
skip all o2m/m2m by default and handle `invoice_line_tax_id` as a special case.

As you see, even with a _browse_to_dict() helper method you'd still need to
iterate on the fields to filter them based on their type (to fix the above
problem), so the code might not even be shorter...

> About the merge, I have tested, it seems to work. I haven't try it on the
> runbot, because akretion has been kicked out from the runbot (probably
> because of the 'a' of akretion).

Akretion has not been kicked out of runbot, but the list of registered teams
was recently wiped out by mistake[1]. Sorry about it, you just need to
re-register your team. It would be great to check the status of the tests on
your branch indeed!

Great job, thanks again for your efforts and dedication!

[1] Already happened in the past:
https://twitter.com/antonylesuisse/status/192290768348651520

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

Yes indeed the 'journal_id' field is overwritten, so I removed it.
I have changed also the method refund_cleanup_lines() to skip the o2m and the m2m and handle just the field 'invoice_line_tax_id'.

About the browse_to_dict() method, I was also thinking to put it somewhere else than in the module account, but the purpose of that was just to have a feedback ;)

Thanks for your time spent to discuss about it !

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

Hello Olivier,

Have you verified my last changes ?
What do you think of them ?

I insist because I think we really need this merge for the V7 !

Thanks again for the time you spend on this merge proposal.

Regards,

Benoît

[Olivier Dony (Odoo) (odo-openerp)]

Looks very good to me now, I'm merging it after resolving the latest conflicts.

Thanks for your patience and your excellent contribution! :-)

review: Approve

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

Thanks a lot for the merge !

=== modified file 'account/account_invoice.py'
--- account/account_invoice.py	2012-10-08 12:17:45 +0000
+++ account/account_invoice.py	2012-10-16 16:41:28 +0000
@@ -1124,72 +1124,96 @@
             ids = self.search(cr, user, [('name',operator,name)] + args, limit=limit, context=context)
         return self.name_get(cr, user, ids, context)
 
-    def _refund_cleanup_lines(self, cr, uid, lines):
+    def _refund_cleanup_lines(self, cr, uid, lines, context=None):
+        clean_lines = []
         for line in lines:
-            del line['id']
-            del line['invoice_id']
-            for field in ('company_id', 'partner_id', 'account_id', 'product_id',
-                          'uos_id', 'account_analytic_id', 'tax_code_id', 'base_code_id'):
-                if line.get(field):
-                    line[field] = line[field][0]
-            if 'invoice_line_tax_id' in line:
-                line['invoice_line_tax_id'] = [(6,0, line.get('invoice_line_tax_id', [])) ]
-        return map(lambda x: (0,0,x), lines)
-
-    def refund(self, cr, uid, ids, date=None, period_id=None, description=None, journal_id=None):
-        invoices = self.read(cr, uid, ids, ['name', 'type', 'number', 'reference', 'comment', 'date_due', 'partner_id', 'partner_contact', 'partner_insite', 'partner_ref', 'payment_term', 'account_id', 'currency_id', 'invoice_line', 'tax_line', 'journal_id', 'company_id'])
-        obj_invoice_line = self.pool.get('account.invoice.line')
-        obj_invoice_tax = self.pool.get('account.invoice.tax')
+            clean_line = {}
+            for field in line._all_columns.keys():
+                if line._all_columns[field].column._type == 'many2one':
+                    clean_line[field] = line[field].id
+                elif line._all_columns[field].column._type not in ['many2many','one2many']:
+                    clean_line[field] = line[field]
+                elif field == 'invoice_line_tax_id':
+                    tax_list = []
+                    for tax in line[field]:
+                        tax_list.append(tax.id)
+                    clean_line[field] = [(6,0, tax_list)]
+            clean_lines.append(clean_line)
+        return map(lambda x: (0,0,x), clean_lines)
+
+    def _prepare_refund(self, cr, uid, invoice, date=None, period_id=None, description=None, journal_id=None, context=None):
+        """Prepare the dict of values to create the new refund from the invoice.
+            This method may be overridden to implement custom
+            refund generation (making sure to call super() to establish
+            a clean extension chain).
+
+            :param integer invoice_id: id of the invoice to refund
+            :param dict invoice: read of the invoice to refund
+            :param string date: refund creation date from the wizard
+            :param integer period_id: force account.period from the wizard
+            :param string description: description of the refund from the wizard
+            :param integer journal_id: account.journal from the wizard
+            :return: dict of value to create() the refund
+        """
         obj_journal = self.pool.get('account.journal')
+
+        type_dict = {
+            'out_invoice': 'out_refund', # Customer Invoice
+            'in_invoice': 'in_refund',   # Supplier Invoice
+            'out_refund': 'out_invoice', # Customer Refund
+            'in_refund': 'in_invoice',   # Supplier Refund
+        }
+        invoice_data = {}
+        for field in ['name', 'reference', 'comment', 'date_due', 'partner_id', 'company_id',
+                'account_id', 'currency_id', 'payment_term']:
+            if invoice._all_columns[field].column._type == 'many2one':
+                invoice_data[field] = invoice[field].id
+            else:
+                invoice_data[field] = invoice[field] and invoice[field] or False
+
+        invoice_lines = self._refund_cleanup_lines(cr, uid, invoice.invoice_line, context=context)
+
+        tax_lines = filter(lambda l: l['manual'], invoice.tax_line)
+        tax_lines = self._refund_cleanup_lines(cr, uid, tax_lines, context=context)
+        if journal_id:
+            refund_journal_ids = [journal_id]
+        elif invoice['type'] == 'in_invoice':
+            refund_journal_ids = obj_journal.search(cr, uid, [('type','=','purchase_refund')], context=context)
+        else:
+            refund_journal_ids = obj_journal.search(cr, uid, [('type','=','sale_refund')], context=context)
+
+        if not date:
+            date = time.strftime('%Y-%m-%d')
+        invoice_data.update({
+            'type': type_dict[invoice['type']],
+            'date_invoice': date,
+            'state': 'draft',
+            'number': False,
+            'invoice_line': invoice_lines,
+            'tax_line': tax_lines,
+            'journal_id': refund_journal_ids and refund_journal_ids[0] or False,
+        })
+        if period_id:
+            invoice_data.update({
+                'period_id': period_id,
+            })
+        if description:
+            invoice_data.update({
+                'name': description,
+            })
+        return invoice_data
+
+    def refund(self, cr, uid, ids, date=None, period_id=None, description=None, journal_id=None, context=None):
         new_ids = []
-        for invoice in invoices:
-            del invoice['id']
-
-            type_dict = {
-                'out_invoice': 'out_refund', # Customer Invoice
-                'in_invoice': 'in_refund',   # Supplier Invoice
-                'out_refund': 'out_invoice', # Customer Refund
-                'in_refund': 'in_invoice',   # Supplier Refund
-            }
-
-            invoice_lines = obj_invoice_line.read(cr, uid, invoice['invoice_line'])
-            invoice_lines = self._refund_cleanup_lines(cr, uid, invoice_lines)
-
-            tax_lines = obj_invoice_tax.read(cr, uid, invoice['tax_line'])
-            tax_lines = filter(lambda l: l['manual'], tax_lines)
-            tax_lines = self._refund_cleanup_lines(cr, uid, tax_lines)
-            if journal_id:
-                refund_journal_ids = [journal_id]
-            elif invoice['type'] == 'in_invoice':
-                refund_journal_ids = obj_journal.search(cr, uid, [('type','=','purchase_refund')])
-            else:
-                refund_journal_ids = obj_journal.search(cr, uid, [('type','=','sale_refund')])
-
-            if not date:
-                date = time.strftime('%Y-%m-%d')
-            invoice.update({
-                'type': type_dict[invoice['type']],
-                'date_invoice': date,
-                'state': 'draft',
-                'number': False,
-                'invoice_line': invoice_lines,
-                'tax_line': tax_lines,
-                'journal_id': refund_journal_ids
-            })
-            if period_id:
-                invoice.update({
-                    'period_id': period_id,
-                })
-            if description:
-                invoice.update({
-                    'name': description,
-                })
-            # take the id part of the tuple returned for many2one fields
-            for field in ('partner_id', 'company_id',
-                    'account_id', 'currency_id', 'payment_term', 'journal_id'):
-                invoice[field] = invoice[field] and invoice[field][0]
+        for invoice in self.browse(cr, uid, ids, context=context):
+            invoice = self._prepare_refund(cr, uid, invoice, 
+                                                date=date,
+                                                period_id=period_id,
+                                                description=description,
+                                                journal_id=journal_id,
+                                                context=context)
             # create the new invoice
-            new_ids.append(self.create(cr, uid, invoice))
+            new_ids.append(self.create(cr, uid, invoice, context=context))
 
         return new_ids
 

=== modified file 'account/wizard/account_invoice_refund.py'
--- account/wizard/account_invoice_refund.py	2012-09-30 17:02:56 +0000
+++ account/wizard/account_invoice_refund.py	2012-10-16 16:41:28 +0000
@@ -146,7 +146,7 @@
                     raise osv.except_osv(_('Insufficient Data!'), \
                                             _('No period found on the invoice.'))
 
-                refund_id = inv_obj.refund(cr, uid, [inv.id], date, period, description, journal_id)
+                refund_id = inv_obj.refund(cr, uid, [inv.id], date, period, description, journal_id, context=context)
                 refund = inv_obj.browse(cr, uid, refund_id[0], context=context)
                 inv_obj.write(cr, uid, [refund.id], {'date_due': date,
                                                 'check_total': inv.check_total})

=== modified file 'point_of_sale/wizard/pos_return.py'
--- point_of_sale/wizard/pos_return.py	2012-03-30 08:27:51 +0000
+++ point_of_sale/wizard/pos_return.py	2012-10-16 16:41:28 +0000
@@ -278,7 +278,7 @@
             location_id=res and res[0] or None
 
             if order_id.invoice_id:
-                invoice_obj.refund(cr, uid, [order_id.invoice_id.id], time.strftime('%Y-%m-%d'), False, order_id.name)
+                invoice_obj.refund(cr, uid, [order_id.invoice_id.id], time.strftime('%Y-%m-%d'), False, order_id.name, context=context)
             new_picking=picking_obj.create(cr, uid, {
                             'name':'%s (return)' %order_id.name,
                             'move_lines':[], 'state':'draft',