[14.0][FIX] pos_order_return: make returns properly

[chienandalu]

Before this PR the returns weren't functional when a stock picking was involved.

cc @Tecnativa TT42730

please review @pedrobaeza @CarlosRoca13

[pedrobaeza]

@chienandalu please check tests

[legalsylvain]

Hi. Using test framework (out of tests) is not a good idea in term of performance.
In the meantime, i dont understand why it is needed in this case. Could you avoid it ?
Thanks !

[chienandalu]

Hi. Using test framework (out of tests) is not a good idea in term of performance.

That's normally true and we found it out the hard way time ago :) In this case, anyway, we're dealing with a single record and the overhead can be assumed IMO. In exchange, we cover onchange workflows by default without pulling extra deps like the onchange helper module.

[legalsylvain]

In this case, anyway, we're dealing with a single record and the overhead can be assumed IMO

could you provide before / after performance ?

could you also take a look on the CI that is red ?

[chienandalu]

In this case, anyway, we're dealing with a single record and the overhead can be assumed IMO

could you provide before / after performance ?

The module was broken. So the gain is 100% 😅 Jokes aside in our experience is reliable for light singleton operations like this (run a wizard)

could you also take a look on the CI that is red ?

It should be fixed now

[pedrobaeza]

Please push again, as there's no CI checks.

[chienandalu]

All green

[zamberjo]

Thanks for the fix, it seems that the error no longer occurs.

As far as I can see, the picking created in the return remains in Ready status, while the normal order picking is changed to Done. Is it possible to use the PR to transfer the picking? do you think that would be ok?

Note: this only occurs when we choose 'In real time', otherwise if we choose 'At the session closing' option all pickings are transferred.

[chienandalu]

As far as I can see, the picking created in the return remains in Ready status, while the normal order picking is changed to Done. Is it possible to use the PR to transfer the picking? do you think that would be ok?

Sure. It should be close. Check again, please

[zamberjo]

Thanks! 👏

[pedrobaeza]

@legalsylvain please unblock

[pedrobaeza]

Merging as no further answer from @legalsylvain. If a serious performance problem is faced, we can proceed other way:

/ocabot merge patch

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-981-by-pedrobaeza-bump-patch, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 4a5c96a. Thanks a lot for contributing to OCA. ❤️

[legalsylvain]

Merging as no further answer from @legalsylvain.

@pedrobaeza, I was looking for the long debate with @rousseldenis , @Cedric-Pigeon, @lmignon, which had concluded (except you) not to use odoo.test.Forms (outside the tests of course) and I could not find it.
I just found it again, so I put this link here to allow everyone to understand the issues of the debate "onchange_helper" vs odoo.test.Forms : OCA/sale-workflow#1239

I din't changed my mind (follow OCA members conclusion, see link below). I have nothing to add.

Merging a PR in this context, because the OCA Representative of the repo (and original author of the module) who is blocking the PR has not responded for a few days is not correct.

The module was broken. So the gain is 100% sweat_smile Jokes aside in our experience is reliable for light singleton operations like this (run a wizard)

@chienandalu : It would have been interesting to spend some time to analyze the execution time with / without odoo.test.Form, to have some figures.
In my previous benchmark, the execution time was multiplied by 8 to 11, which is not insignificant.

That's being said :

• Let's see the good side of things! this PR fixes a bug, it's a good news.
• A priori, return order is not a common operation, and as said by @chienandalu realized on a singleton. The degradation of the UX due to the slowness introduced by this PR, although undesirable, is not dramatic.
• This module is in the Alpha development status
• With the new "Refund" button present in Odoo 16.0 CE, this module will maybe not be usefull anymore, or at least, highly refactored.

so I will add soon in the ROADMAP.rst file of the module a text to indicate the need to replace the use of odoo.test.Forms by onchange_helper if we want to improve UX.

[pedrobaeza]

That thread is because they want to do operations in batch. This is for only one record. And all that context was taken into account, but this was required to fix the problem, and it was paralyzed by you for 2 weeks, so I consider the merging correct. And your measures of 8x/11x of increase were on sale.order.line, where lots of computations/onchanges are done, not here in this specific wizard.