[IMP] stock_quant_merge: Merge quant at install + hook for inherit merge #119
Conversation
pedrobaeza
force-pushed
the
8.0-stock_quant_merge-imp
branch
from
fc1cf80
to
3536529
Compare
| @@ -19,14 +19,20 @@ def _mergeable_domain(self): | |||
| ('lot_id', '=', self.lot_id.id), | |||
| ('package_id', '=', self.package_id.id), | |||
| ('location_id', '=', self.location_id.id), | |||
| ('reservation_id', '=', False), | |||
| ('reservation_id', '=', self.reservation_id.id), | |||
There was a problem hiding this comment.
is this change related to the new features or is it a bug fix?
There was a problem hiding this comment.
This is to allow a merging from the init hook of any of possible quants. Imagine that you have reserved 2 quants for the same move. This one allows to merge then in only one.
There was a problem hiding this comment.
I think if we merge quants with different "in date" values, we'll be introducing wrong results in the FIFO/LIFO removal strategy.
There was a problem hiding this comment.
OK, we can add that condition on the mergeable domain.
|
That looks sane, but the tests are not passing anymore. Maybe you need to update them since you use another product? |
|
Sorry about the test. I try only with the second test (the one that I have added), and forget the first one. I'll change it. |
|
@pedrobaeza hehe so it does happen even to the best of us :) |
|
Done |
|
Looks fine from reading the code. I'll try to test it on our big DB (~3M quants) but I fear the install may be too long for default CPU limits. If such is the case, maybe a warning should be added to the README. |
|
I have already added https://github.com/OCA/stock-logistics-warehouse/pull/119/files#diff-674e9db8ba7d8f6cd1fd1c408bc5d61bR22. Isn't enough? |
|
I was specifically thinking of a warning in the line of "this may fail to install on big databases if the CPU limit is too low" |
|
Warning added on the README |
|
Can you please review this again? |
| @@ -7,13 +7,21 @@ Quant merge | |||
| =========== | |||
|
|
|||
| Odoo splits quants each time a reservation is done: this module makes Odoo | |||
| merge them back if they still meet the following requirements: | |||
| merge them back when you unserve them if they still meet the following | |||
|
this looks quite nice. 👍 once small typo is fixed. I'm wondering: why should I install this? Is there a perf gain on a db with a large number of quants? Something else? If so, I think it deserves mentioning in the README. And if there is a huge perf gain, maybe we could make a PR on odoo master? |
|
@gurneyalex, this is more or less like the Windows Disk Defragmenter, but with quants instead of sectors. |
|
@pedrobaeza I was wondering if doing this would not introduce wrong results to the dated valuation? |
|
Well, I don't know, because I haven't digged in that view. It depends on what quantity field takes. Can you check it? |
|
My understanding is that the So I think we should only merge quants which are used in the same moves. |
| @@ -34,7 +40,8 @@ def merge_stock_quants(self): | |||
| for quant in quants: | |||
| if (self._get_latest_move(quant2merge) == | |||
| self._get_latest_move(quant)): | |||
| quant2merge.sudo().qty += quant.qty | |||
| quant2merge.sudo().write( | |||
| self._get_merged_vals(quant2merge, quant)) | |||
| cost += quant.cost | |||
| cont += 1 | |||
There was a problem hiding this comment.
If you're going to merge all the possible quants, some will have different costs, so you should be doing a weighted average like the standard does in the dated valuation (it's changed recently: odoo/odoo@1d7c120)
|
setting needs fixing following @clonedagain's remark |
|
cc @Tecnativa |
|
@pedrobaeza could you please review comments of this PR? Thanks |
|
needs rebasing |
|
Code seems alright, glitches have already been commented. Waiting for fixes and rebase to do functional review (note to self: uninstall module, unreserve quant, install module, only 1 quant should be there). |
|
We have lost interest in this after handling this with the customer in another way. If someone wants to take over the work, please make another PR. I close meanwhile. |
This PR improves 2 things: