[10.0][MIG] mrp - #1076
Conversation
981a9cb to
a69454b
Compare
|
You will need to take into account rename fields when OCA/openupgradelib#84 will be merged. |
d7a4d4a to
93ce204
Compare
|
@pedrobaeza the rename_fields method is now taken into account |
| @@ -1,20 +1,20 @@ | |||
| ---Fields in module 'mrp'--- | |||
| mrp / ir.attachment / priority (selection) : NEW selection_keys: ['0', '1', '2', '3'] | |||
| mrp / mrp.bom / date_start (date) : DEL | |||
There was a problem hiding this comment.
Please don't modify this file
There was a problem hiding this comment.
It was done automatically by PCatinean :S
| 'demo': [ | ||
| 'data/mrp_demo.xml', | ||
| 'data/mrp_lot_demo.yml'], | ||
| # 'data/mrp_lot_demo.yml' |
There was a problem hiding this comment.
Isn't there any other option? I don't like this, as you are disabling demo data that also servers for performing tests. Please put the error you get for debugging the problem. FYI, functions in YAML are also executed on update.
There was a problem hiding this comment.
mrp_demo.xml has <function> tags that later mrp_lot_demo.yml uses.
This <function> tags only are executed on install, not in updates.
When migrating (migration is an update), this tags are not executed and mrp_lot_demo.yml breaks. Thus, mrp_lot_demo.yml is disabled to avoid the breaking.
OpenUpgrade is a tool just for migrating, not for testing nor working.
There was a problem hiding this comment.
No, if you are disabling this, something is not correct. We need to test the migration, and it's done through demo data, so enable it and solve any problem. If not, comment the exact problems you find.
There was a problem hiding this comment.
You cannot test the demo data that is not loaded. The <function> tags in the mrp_demo.xml are not loaded!
There was a problem hiding this comment.
OK, I see this doesn't include demo data. Please add a comment about that.
There was a problem hiding this comment.
The comment is already added in the manifest...
There was a problem hiding this comment.
OK, OK, I didn't see them here in the simplified widget
| # NOTHING TO DO | ||
|
|
||
| mrp / mrp.bom / product_rounding (float) : DEL | ||
| # NOTHING TO DO: Rounding is taken from the product.uom now |
There was a problem hiding this comment.
Is it possible that the rounding is not the same from product one? What to do in that cases?
There was a problem hiding this comment.
What do you propose?
There was a problem hiding this comment.
At least emit a warning. Other possibility is to create a specific uom with that rounding called openupgrade_...
| # Set default values from field definition as this is a new field and | ||
| # the default val resembles the previous behavior best | ||
| default_specs = { | ||
| 'mrp.bom': [('ready_to_produce', None)], |
There was a problem hiding this comment.
No need of this one, as there's already a default value applied:
OpenUpgrade/addons/mrp/models/mrp_bom.py
Line 53 in 4c1d811
| mrp / mrp.bom.line / product_efficiency (float) : DEL required: required, req_default: function | ||
| mrp / mrp.bom.line / product_rounding (float) : DEL | ||
| mrp / mrp.bom.line / property_ids (many2many) : DEL relation: mrp.property | ||
| mrp / mrp.bom.line / product_uom (many2one) : was renamed to product_uom_id [nothing to do] |
There was a problem hiding this comment.
It's not really nothing to do. You have called rename_fields for this field.
There was a problem hiding this comment.
that was previous I used the rename_fields...
| mrp / stock.move / quantity_done_store (float) : NEW | ||
| mrp / stock.move / unbuild_id (many2one) : NEW relation: mrp.unbuild | ||
| mrp / stock.move / unit_factor (float) : NEW | ||
| mrp / stock.move / workorder_id (many2one) : NEW relation: mrp.workorder |
There was a problem hiding this comment.
This one should be set for old workorders not done yet, or better, for all of them for having correctly the history. The same can apply to other fields of this model as well.
There was a problem hiding this comment.
And through which relation can I fill them?
There was a problem hiding this comment.
Again I don't know data structure, but I think it should be there in previous version somewhere
| mrp / stock.move.lots / quantity (float) : NEW | ||
| mrp / stock.move.lots / quantity_done (float) : NEW | ||
| mrp / stock.move.lots / workorder_id (many2one) : NEW relation: mrp.workorder | ||
| # NOTHING TO DO |
There was a problem hiding this comment.
I think you need to fill this table with historic data.
There was a problem hiding this comment.
Which historic data? This table is new
| ('mrp.workorder', 'mrp_workorder', 'date_planned', 'date_planned_start'), | ||
| ] | ||
|
|
||
| _table_renames = [ |
There was a problem hiding this comment.
Use also model_renames method
There was a problem hiding this comment.
how? I proved putting it before and after the table_renames and in both cases fails
| for quant in quants: | ||
| cr.execute( | ||
| """ | ||
| INSERT INTO stock_quant_consume_rel |
There was a problem hiding this comment.
You can make only one query (with openupgrade.logged_query, please) with the insert and the select as source. Check https://www.w3schools.com/sql/sql_insert_into_select.asp
|
|
||
|
|
||
| def update_stock_warehouse(cr): | ||
| cr.execute( |
There was a problem hiding this comment.
You are doing this by SQL, so no recomputation is triggered as you mention on the analysis file (...so as to force the creation of the default picking types and pull rules for manufacturing on that warehouse)
|
In addition, you have to check also mrp_operations possible changes (as now is integrated on mrp). |
| ('state', None, None) | ||
| ], | ||
| 'mrp_workorder': [ | ||
| ('state', None, None) |
There was a problem hiding this comment.
This field is from module mrp_operations, not mrp.
There was a problem hiding this comment.
There was a problem hiding this comment.
But this PR merges both modules before.
There was a problem hiding this comment.
@pedrobaeza this migration script breaks the migration when mrp_operations was not installed in the first place. I fixed it in my PR on this branch.
49276e3 to
45714f6
Compare
|
What's the status here? How can others help? Mostly also, has there since been any work on this outside what we can see on this PR, to not duplicate any work? |
There was a problem hiding this comment.
This is renaming models based on data that's actually about tables, not models.
_model_renames should be renamed to _table_renames and rename_tables called with it, not rename_models. Additionally might have to do something about the model information as well, though it's already filled by the previous VIEW backed model, maybe that can be deleted and then the model rename done too.
Without these changes, the migration actually fails for me, because there will be no mrp_workorder table at all, so first the sequence deletion from _column_copies fails, and afterwards post-migration on mrp_workorder as well.
There was a problem hiding this comment.
"[FIX] without model_renames" commit seems to already fix this up, but then "[ADD] More migration scripts" pushed later messes it up again
There was a problem hiding this comment.
The picking_type_id seems to be failing as-is with this patchset for me (plus the other fixes I've mentioned). There will be no stock.picking.type with code=='mrp_operation', because it seems only created on first need inside _get_manufacture_pull_rules_values, called inside mrp addons stock.warehouse models create_routes additions (amongst other places). But create_routes is only called upon creating a new warehouse - but we are migrating a v9 existing warehouse.
EDIT: For new warehouses, it's actually first created via create_sequences_and_picking_types additions in mrp module, it seems; but the point is the same.
So as far as I can see, the necessary additional warehouse routes will need to be created in mrp pre-migration for each stock.warehouse present.
Otherwise things fall over as follows:
- mrp module upgrade (after pre-migration) creates ir_model_data entry against default warehouses manu_type_id via
addons/mrp/data/mrp_data.yml, which doesn't exist as there's no code=='mrp_operation' stock.picking.type's for it (there would be for fresh databases as stock.warehouse create would have called create_routes) - post-migration script goes looking for suitable mrp_operation code picking types to assign to the new picking_type_id field, but none will be found -- picking_type_id will remain empty for all existing mrp.production records. All picking_type_id will remain unset and NOT NULL contraint addition will still fail after end-migration --update=all calls as well.
odoo.addons.base.ir.ir_model: Deleting 0@stock.picking.type (mrp.picking_type_manufacturing)is done, as it doesn't point to any existing record (expected, just confusing if thinking in terms of stock.picking.type instead of ir.ir_model)- If the database would be used afterwards (e.g. if it was a migration from v9 where MRP was used to setup BOMs and other things, but no actual manufacturing orders yet existed), then there will be picking_type_id unset errors when trying, and curiously in some cases it could end up using an internal transfer stock.picking.type instead of mrp_operation (I have one such instance somehow in my ongoing migration test results).
tl;dr - stock.warehouse/stock.picking.type/etc needs adapting for mrp use cases in mrp pre-migration script
EDIT: Adding env['stock.warehouse'].search([])._create_manufacturing_picking_type() at start of post-migration seems to "solve" it for me, but I haven't reviewed potential other new record needs for mrp stock, and this beats the purpose of adding the column in pre-migration with the intention of trying to have things ready by pre-migration time; but registry doesn't seem to exist in pre-migration, so for now went the easy way via post-migration myself locally to get further.
7291ca5 to
000fdfa
Compare
|
Similarly mrp.workorder qty_produced and qty_producing could use filling up correctly. All done and 0 producing in case of state done. But I suspect this (workorder produced/producing qties) is less important for common use cases compared to mine (I have the equivalent of multiple mrp.workcenter.productivity entries for one mrp.workorder in play in v8 already with custom mods) |
|
I think old mrp.workorder |
|
In v10 mrp.production state='confirmed' is followed by state='planned'. During transition workorders are created. Result with current lack of handling this is that a Manufacturing Order that was in confirmed state in v8 (or presumably v9) already has workorder_ids → user clicks "Create Workorders" to proceed → MO ends up with double the work orders, all of which need production recorded to get MO done. |
There was a problem hiding this comment.
There is much to do here. If this isn't filled up, no manufacturing orders that were brought over will consume any raw materials, because the code in mrp.workorder record_production method won't have a unit_factor, and thus it skips raising consumed qty (quantity_done) when the workorder is done. At least the workorder_id filling is properly done in populate_stock_move_workorder_id, but without unit_factor filled, that isn't useful alone.
If unit_factor isn't calculated for it and filled in, the result is the following for MO's that were created in earlier Odoo version (and weren't in done state):
- We record production, to keep it simple lets say it had nothing produced before and we produce it all now
- record_production() on the last workorder (as stock.move workorder_id IS calculated to point at the last workorder of the production it's a raw material to consume for) will check all stock.moves for that production (via raw_move_ids one2many, the reverse of stock.move workorder_id) unit_factor, and raise quantity_done accordingly. quantity_done stays 0 because unit_factor is not set.
- The production is finished and user clicks "Mark as Done"
- mrp module action_done override is called and the production moves are funneled through move_validate() instead of main action_done()
- move_validate() checks what quantity_done is, and because it has remained zero due to missing unit_factor, it cancels the move instead (button_mark_done does that after post_inventory is done, which ended up in that move_validate)
- The raw material isn't consumed, the reservation is released by stock.move action_cancel, raw material moves are all in cancel state now
- All raw material stock is completely wrong (the quantity that was reserved for production is released back to quantity on hand), because they were obviously consumed in real life production, but Odoo has cancelled the moves instead.
There was a problem hiding this comment.
fill unit_factor with what?
There was a problem hiding this comment.
With the unit factor as it is filled upon new workorder generation for fresh Manufacturing Orders..
I fixed it up for my needs after migration with the following odoo shell script:
moves_to_fix = env['stock.move'].search([('raw_material_production_id','!=',False),('state','not in',['cancel','done']),('unit_factor','=',False)])
for move in moves_to_fix:
move.unit_factor = move.product_uom_qty / ((move.raw_material_production_id.product_qty - move.raw_material_production_id.qty_produced) or 1.0)
Something like that needs doing during migration. Maybe should bother with cancel or done moves as well, in case they get reset at some point, but the workflow used in my instance didn't have a need for it. If it is done for cancel/done moves as well, it'll take a significant amount of time if done via odoo methods; if with pure SQL, should be very fast (but potentially different rounding that needs to be cared for maybe?).
If this is not done, all raw materials used by already generated (prior to migration) MO-s would go to cancel after MO is marked as done, completely breaking stock quantities.
There was a problem hiding this comment.
This has to be filled as well, like also unit_factor #1076 (review)
If this (or unit_factor) is not set, mrp.workorder record_production will NOT consume the move and it will cancel the move afterwards in button_mark_done, resulting in completely wrong quantities in stock for all the raw materials used in manufactoring orders that were created in the older version than v10..
There was a problem hiding this comment.
fill bom_line_id with what?
There was a problem hiding this comment.
Pretty much anything, otherwise as with unit_factor, the stock will go completely wrong when MO is marked as done. The (questionable) check only checks if it's set or not; if not set, it'll not increase quantity_done and end up cancelled in button_mark_done.
In my case I post-processed by checking through all BoM lines of the MO to find the same product - if only one is found (there could be multiple lines for same product), I assigned that to bom_line_id. But that will not solve all of them, as BoM might have changed in time, multiple same products found, etc. To finish it up, I just assigned what is left to basically any bom line of the MO's BoM - in practice it only matters if MO produced quantity is updated via wizard, and my customer doesn't use that. Multiple raw material stock moves of the same MO-s pointing at the same bom line is fine as well, with the same caveat of update wizard not working right. So it's under the understanding that MO Produced Qty Update wizard is not going to be used for old MOs, and then all is good.
That said, v10 code is still rather questionable - nothing stops you from removing a BoM line after it's already assigned to a stock.move, thus breaking the on hand quantities later on as well; for that an issue should be filed against Odoo and at the very least stock.move:bom_line_id made ondelete='restrict' instead of the current default SET NULL. I made that change in my own module for now with issue filing as TODO :(
885ae03 to
e028432
Compare
|
Hey, everybody! This PR is GREEN again. So, I think it would be nice if I squash a bit and we do merge of this PR. Then, if something can be improved, it could be done in another PR instead. @pedrobaeza as you can see in premigration, I think that maybe a generalization of @leio I hope I implemented correctly what you said. |
AaronHForgeFlow
left a comment
AaronHForgeFlow
left a comment
There was a problem hiding this comment.
👍 Working fine from my side. Functional test excluding work.orders cause I do not use them:
-
mrp.production is good: including consumed and production moves. New picking type and the picking type in the procurement group updated fine.
-
bom migrations seems also good: and the bom_line relation in the stock move is correct.
-
The relation between the consumed quants and the stock move is correct IMO.
Thanks
* v9 draft MO: create stock moves after migration. * WH: update manufacturing wh flag and create manufacturing picking type as needed. * MO: fill picking type taking into account WH.
|
@pedrobaeza recently I thought that maybe we could rename the old |
|
That can be a good option. I was waiting to Travis to merge, but if you make this change, I can wait more. |
|
Ok, I will make then. |
|
Any way, as |
|
Merging this as a valid one, and waiting for other PR with the change to see if there's a problem. |
|
Ok :) |
7 participants
Uh oh!
There was an error while loading. Please reload this page.