[mrp] Regression in mrp.procurement.action_confirm() (5.0)

[Dukai Gábor]

Hi!
5.0 latest bzr.
In a setup where sales is configured 'Delivery Order Only' (Output location is auto-linked to Customers). When sales orders create pickings, the moves stay with the 'Output' location as the destination but they should be changed to 'Customers'.

Regression appeared in: Revision ID: <email address hidden>
Reverting the change in mrp.py/mrp.procurement.action_confirm() solves this bug.

[Stephane Wirtel (OpenERP) (stephane-openerp)]

I informed the developper

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

Hmm, and I took the time to write test cases to check regressions for this one-line patch!
Let me extend them to cover this case and find the appropriate fix.

Although I can't directly see the connection between my small patch and the reported error, this once again highlights the need for a full regression test suite. Well, at least we're starting to build one with your help.

Thanks for the quick report, Dukai Gábor!

[Dukai Gábor (gdukai)]

I've seen sale_test.xml and I'm impressed that there's already a test embedded in v5.0, just it needs to be extended.

Actually I found out about the bug quite soon as I was crazy enough to put it in production...

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

Cool, Don Kirby has already completed the tests to cover this bug :-) (not sure why his merge proposal is not showing on this bug)
It's at https://code.launchpad.net/~donkirkby/openobject-addons/complete-procurement/+merge/20694

This will make it even easier for me, thanks!

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

Hrm sorry, still need to add a regression test, Don Kirkby's branch is for bug 532148 instead (which of course explains why it does not show on this bug ;-))

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

Fix landed with revision 2637. This was in fact due to an old piece of code that directly wrote to the status of stock moves instead of calling the appropriate workflow-like method.
My original patch had made this piece of code more exposed, which is why this issue appeared.

Revision 2637 also includes updated regression tests for sale and mrp_jit modules to cover this.

This will be included in 5.0.8.

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

Note: make that revision 2638, in which I introduced separate tests for auto-chained locations and manually-chained locations.
Better to test both separately, just in case.

[Dukai Gábor (gdukai)]

Hello Olivier,

Thanks for all your efforts! I've reported >100 bugs for OpenERP but this is the first time I get a test case to make sure the bug wont happen again:)
Please take a look at https://bugs.launchpad.net/openobject-addons/+bug/535058, that's also a chained location related one.

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

On 09/03/10 15:03, Dukai Gábor wrote:
> Thanks for all your efforts! I've reported >100 bugs for OpenERP but
> this is the first time I get a test case to make sure the bug wont
> happen again:)

You're welcome! This was perhaps the first time, but definitely not the
last time.. This incremental creation of a test suite for OpenERP is
part of our efforts for improving the general quality of the system.
I am doing it myself but will also be watching the bugfixes and have the
rest of the team do the same thing.

And the best thing is that I see members of the community picking this
up as well (like yourself or Don Kirkby), which will help us even further.

It is still quite tedious to write the XML test cases at the moment, but
this should become a lot easier in 5.2, with:
- support for YAML syntax
- support for recording test scenarios with base_module_record in YAML
format
- better support for calling wizards from test cases
- and more...
Then there should be no excuses for not having regression tests
committed with every bugfix.

:-)

> Please take a look at bug 535058, that's also a chained location
> related one.

Sure, will watch it as well.

[Don Kirkby (donkirkby)]

I'm still pretty new to the project, so I often can't contribute an actual fix for the bugs I report. I figure if I contribute a failing test, it helps motivate other project members to find a fix.

Don

Olivier Dony wrote:
> And the best thing is that I see members of the community picking this
> up as well (like yourself or Don Kirkby), which will help us even further.

[Numérigraphe (numerigraphe)]

FYI, I've just been hit by this bug, but another way: our module redefined stock.move.action_confirm(), and our code was not beeing used anymore when confirming orders because the move state was changed by procurements.

Anyway, the fix shows that changing the move state directly is incorrect - but maybe there are other parts of the code where this has been done?
As a defensive measure, isn't there a way we could use the ORM to make sure action_confirm() is called every time it's needed?
Lionel.