Odoo Addons (MOVED TO GITHUB)

The return packing wizard doesn't take into accout the field "Invoice control"

Bug #1066127 reported by Alexis de Lattre on 2012-10-12

This bug affects 2 people

Affects		Status	Importance	Assigned to	Milestone
	Odoo Addons (MOVED TO GITHUB)	Fix Released	Low	OpenERP Publisher's Warranty Team
	OpenERP Community Backports (Addons)	New	Undecided	Unassigned

Bug Description

Here is the scenario of the bug on OpenERP 6.1 (and also OpenERP trunk) :
1. Create a new DB and install the "sale" module (and "stock" module on OpenERP trunk)
2. Create a new sale order for Agrolait with one sale order line with product CPU1. Set "Invoice Policy" to "Invoice based on deliveries" and confirm this sale order.
3. Go to the delivery order that has been generated. Process it and create the customer invoice (you need to create the customer invoice, otherwise the bug doesn't happen).
4. On the picking, click on the button "Return Products". In the wizard, set "Invoicing" to "No invoicing" and click on the "Return" button to generate the Return picking.
5. Open the return picking and look at the field "Invoice control" : the value is "To be invoiced" ! That's the bug : as we set in the return picking wizard the field "Invoicing" to "No invoicing", the value of the field "Invoice control" should be "Not applicable".

Analysis of the code and solution for OpenERP 6.1 :
If you look at addons-6.1/stock/wizard/stock_return_picking.py line 171, you see that the return picking is copied from the original picking, and, in the dict that is passed to the copy() function, there is "invoice_state": <the_value_of_the_wizard>... so it should work ! But, if you look at addons-61/stock/stock.py line 700, in the definition of the "copy()" function for the object stock.picking, you see :

if picking_obj.invoice_state == 'invoiced':
default['invoice_state'] = '2binvoiced'

So, if when original picking has invoice_state = 'Invoiced", then the return picking always has invoice_state = "To be invoiced", whatever the content of the "default" dict is... so whatever the user sets in the "Return product" wizard !

My patch changes this code : invoice_state is set to "2binvoiced" if the original picking has invoice_state = 'invoiced' AND the "default" dict doesn't have an "invoice_state" key. In fact, there is the same kind of code 5 lines above, in the previous "if", so it seems to be a natural solution for me.

The bug and the solution is the same on addons/trunk.

Tags:

Related branches

lp:~openerp-dev/openobject-addons/6.1-opw-580457-rgo

Approved for merging into lp:openobject-addons/6.1

Naresh(OpenERP): Pending requested 2012-10-16

lp:~openerp-dev/openobject-addons/trunk-opw-580457-port-mma

Merged into lp:openobject-addons at revision 8333

OpenERP Core Team: Pending requested 2012-11-26

Revision history for this message

Alexis de Lattre (alexis-via) wrote on 2012-10-12:

fix-invoice_state_on_return_picking.diff Edit (601 bytes, text/plain)

Vinay Rana (OpenERP) (vra-openerp) on 2012-10-15

Changed in openobject-addons:
assignee:	nobody → OpenERP Publisher's Warranty Team (openerp-opw)
tags:	added: maintenance

Ravi Gohil (OpenERP) (rgo-openerp) on 2012-10-16

Changed in openobject-addons:
status:	New → Confirmed
importance:	Undecided → Low

Ravi Gohil (OpenERP) (rgo-openerp) on 2012-10-16

Changed in openobject-addons:
status:	Confirmed → In Progress

Revision history for this message

Ravi Gohil (OpenERP) (rgo-openerp) wrote on 2012-10-16:

Hello Alexis de Lattre,

We really appreciate your efforts for detailed explanation, the analysis you did and also for the patch.

However, in your fix, there is a small mistake,

- if picking_obj.invoice_state == 'invoiced':
+ if 'invoice_state' not in default and picking_obj.invoice_state == 'invoiced':
default['invoice_state'] = '2binvoiced'

With your fix, the code in the `if` block will never execute as the condition you placed will check for the `invoice_state` key in `default` dictionary, but, when the copy() is called from /stock/wizard/stock_return_picking.py line 171, `invoice_state` key set statically, so the condition `if 'invoice_state' not in default` will never be true.

The reason why it works at your end is, the `Return Products` wizard will send the value for it's `invoice_state` field that is, `none` or `2binvoiced` and this value is going to be sent in `default` dictionary in /stock/stock.py line 702, and the returned picking will be created and it's `Invoice Control` field will have the value of `invoice_state` dict.

As per my investigation, this condition is not needed, because, whatever the `invoice_state` selected in `Return Products` wizard, that should be set in the `Invoice Control` in returned picking! or the fix could be modified like,

- if picking_obj.invoice_state == 'invoiced':
+ if default['invoice_state'] != 'none' and picking_obj.invoice_state == 'invoiced':
default['invoice_state'] = '2binvoiced'

But, as per my opinion, the condition is not needed, hence I will prefer to fix by removing this condition and will create a branch.

@Alexis de Lattre: Thanks for your analysis which traced the root cause of the issue.

Ravi Gohil (OpenERP) (rgo-openerp) on 2012-10-16

Changed in openobject-addons:
status:	In Progress → Fix Committed

Revision history for this message

Alexis de Lattre (alexis-via) wrote on 2012-10-16:

I am a bit suprised by your analysis of the code, because you consider the code of the copy() function of stock.picking only when it is called from stock/wizard/stock_return_picking.py, but not when called by other parts of the code of the addons or extra-addons modules.

I know that when the copy() function is called from stock/wizard/stock_return_picking.py, the code inside the "if" will never be executed... that's what I want, because the "default" dict has an "invoice_state" key ! But, when the copy() function is called from other functions (that may belong to other modules), that code may be usefull... so I am surprised by your merge proposal in which you remove the code !

So I think that my patch is valid and is a good solution to the problem.

Revision history for this message

Ravi Gohil (OpenERP) (rgo-openerp) wrote on 2012-10-17:

Hello Alexis de Lattre,

Thanks for providing details.

I again checked the code and found that the purpose of the check is, copying/duplicating already Invoiced picking should create picking with state 'To be invoiced', and my fix will not treat it. I have also checked your patch and it works well.

I will update the branch with your patch.

Fabien (Open ERP) (fp-tinyerp) on 2012-12-15