Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.0][FIX] stock: add move lines from inventory moves #1633

Merged
merged 1 commit into from
Jan 16, 2019

Conversation

MiquelRForgeFlow
Copy link
Contributor

This PR adds the stock moves lines from inventory moves.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@pedrobaeza
Copy link
Member

Why is this needed? Isn't stock.move coming from inventory lines already in done state?

@MiquelRForgeFlow
Copy link
Contributor Author

Yes, they are already in done state, but it's a "just in case". It's not bad to have them. I am just trying to have a complete stock.move.line migration.

@pedrobaeza
Copy link
Member

You are duplicating stock.move.line then, so this is not valid. I'm afraid "just in case" is not enough for accepting this.

@MiquelRForgeFlow
Copy link
Contributor Author

You are duplicating stock.move.line then

I am not duplicating anything. Each method gives different stock move lines.

@pedrobaeza
Copy link
Member

I don't get it then: if they are in done state, then they should be inserted in the other query. The group by sm.picking_id or maybe if there is a JOIN instead of LEFT JOIN are the only things that can waste that, but better to fix it instead of duplicating code.

@MiquelRForgeFlow
Copy link
Contributor Author

It's a bit difficult to merge the two methods, they are too different.

@pedrobaeza
Copy link
Member

OK, I'll check next week as today I'm holidays.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said, the extra method is not needed at all, as it's already populated by the general method create_stock_move_line, so please remove it.

addons/stock/migrations/11.0.1.1/post-migration.py Outdated Show resolved Hide resolved
addons/stock/migrations/11.0.1.1/post-migration.py Outdated Show resolved Hide resolved
@pedrobaeza
Copy link
Member

I fix in #1639 the main problem

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 11.0-fix-mig-stock-moves-inventory branch 2 times, most recently from e0b29bf to 8ece51c Compare November 29, 2018 11:03
@MiquelRForgeFlow
Copy link
Contributor Author

As said, the extra method is not needed at all, as it's already populated by the general method create_stock_move_line, so please remove it.

That's not true. The create_stock_move_line method only creates stock moves lines from stock pack operations. But you may have stock move lines that are not associated with stock pack operations. Thus, this create_stock_move_line_from_inventory_moves method handles an specific case that the general method doesn't not handle.

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Nov 29, 2018

When you have a product and then you do "update qty on hand", it generates this stock move lines associated with stock moves that are linked to stock.inventory

@pedrobaeza
Copy link
Member

OK, that's what the kind of explanation that I was expecting from the beginning, not now. Let me check again then.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the concept is correct, but several incorrect things in the SQL.

addons/stock/migrations/11.0.1.1/post-migration.py Outdated Show resolved Hide resolved
addons/stock/migrations/11.0.1.1/post-migration.py Outdated Show resolved Hide resolved
addons/stock/migrations/11.0.1.1/post-migration.py Outdated Show resolved Hide resolved
addons/stock/migrations/11.0.1.1/post-migration.py Outdated Show resolved Hide resolved
addons/stock/migrations/11.0.1.1/post-migration.py Outdated Show resolved Hide resolved
@pedrobaeza
Copy link
Member

pedrobaeza commented Nov 29, 2018

Other thing: have you taken into account possible inventories in another UoM different from product one?

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 11.0-fix-mig-stock-moves-inventory branch 2 times, most recently from 565df76 to a912df7 Compare November 29, 2018 15:04
@MiquelRForgeFlow
Copy link
Contributor Author

Other thing: have you taken into account possible inventories in another UoM different from product one?

Yes, I recently tested that. Works super fine. Want screenshots? :)

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 11.0-fix-mig-stock-moves-inventory branch 2 times, most recently from 8001ec7 to be7cb6c Compare December 3, 2018 11:58
@MiquelRForgeFlow
Copy link
Contributor Author

Ready to review and merge

Copy link
Sponsor Member

@JordiBForgeFlow JordiBForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to process millions of moves, so better get this query the quickest possible!

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 11.0-fix-mig-stock-moves-inventory branch from be7cb6c to 0c84b8b Compare December 5, 2018 10:14
@MiquelRForgeFlow
Copy link
Contributor Author

I updated the PR as you requested, but I don't like this way without subquery. It will create duplicated stock move lines in cases where a stock move is linked to more than one quant with lot.

@pedrobaeza
Copy link
Member

@mreficent that is not going to happen as business logic avoids that: although technically the field is a many2many that allows several values, on practice, this is not happening, as you are only able to introduce one lot per inventory line, and thus, that many2many table will have only one record (or none) per each line.

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Dec 5, 2018

@pedrobaeza Your statement is False. I have a v10 database, and I have inventory stock moves linked to more than one quant with lot_id

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 11.0-fix-mig-stock-moves-inventory branch from 0c84b8b to 5e98057 Compare December 5, 2018 10:51
@MiquelRForgeFlow
Copy link
Contributor Author

Also, you can have inventory stock moves that are linked to several quants without any lot_id. Without any way to restrict it to just one quant than using a subquery, I don't know how to do the plain FROM query without creating duplicated stock move lines. Please, just put explicit your ideal query and we will see.

@pedrobaeza
Copy link
Member

@mreficent I'm not talking moves in general. I'm talking about moves coming from inventory lines, which is the source of this query. Please check this case.

@MiquelRForgeFlow
Copy link
Contributor Author

It's what I checked.

@MiquelRForgeFlow
Copy link
Contributor Author

This query includes all stock move lines that are generated from stock moves that have the inventory_id filled. If you think there are different subcases, then maybe the good way to handle it is to have one different query for any of these subcases.

@pedrobaeza
Copy link
Member

Can you guide me to a case where multiple lots are for the same move? Maybe Odoo groups all the inventory lines with different lots in one move?

@MiquelRForgeFlow
Copy link
Contributor Author

In the case with inventory moves linked to more than one quant with lot_id, the lot_id is the same, not different.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 11.0-fix-mig-stock-moves-inventory branch from 5e98057 to fafa5ff Compare December 5, 2018 11:25
@pedrobaeza
Copy link
Member

@mreficent I don't get it. Maybe we can perform a hangout and check it.

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Dec 5, 2018

Case 1 (without lots):

  • create a stockable product A.
  • after created, click on update qty on hand and put 100 units (check that this create a move mA linked to just one quant without lot)
  • do the same for antoher product B (and check move mB).
  • create a bill of materials with product A being created using 1 unit of product B.
  • then, create a Manufacture Order with product A, then check availability, then produce it and mark as done.

Now, check that in the move_quant table, for the move mB there is TWO quants! Thus, if I do your "plain FROM query" will create two stock move lines, one for each quant, and that is a wrong duplication!

Case 2 (without lots):
(as a continuation of case 1)

  • go to inventory adjustments
  • create one with a product only and put product A
  • start one, change quantity, then validate.

This create a move linked to a quant without lot. No problem with this case.

Case 3 (fusion of case 2 and 1, but with lots):
(track lots and serial numbers in settings, and mark option Manage Lots and Serial Numbers user rights)

  • create stockable products C and D with 0 qty (without updating the qty on hand)
  • create bill of materials with product C being created using 1 unit of product D
  • go to inventory control-> lots/serials numbers
  • create a lot Ld with product D
  • go to inventory adjustments
  • start an inventory with product only and put product D
  • before validation, in the inventory line put the lot Ld, put real qty 50 and validate
  • then create a manufacture order with product C, qty 1, check availabity, then produce it and mark as done.

Now check that in the move table, there is a move linked to the inventory adjustment when we updated qty to 50. Then, check in the move_quant table that this move has TWO quants, each quant with lot Ld (one quant has qty 1 and the other has qty 49).

@pedrobaeza
Copy link
Member

OK, I see the problem and it's due to how the lots are computed for the field lot_ids, which is a dirty trick. Possible solutions:

  1. Leave it as is, although the performance is going to suffer. Have you tried in your real DB to see if it's acceptable?
  2. Create a intermediate table for storing the subquery with the group by, simulating the computed m2m table, and perform the plain SQL from this table.
  3. Look for another way of getting same information.

@JordiBForgeFlow
Copy link
Sponsor Member

@pedrobaeza We will check how long does it take to migrate with the current approach

@pedrobaeza
Copy link
Member

Any news on this?

@pedrobaeza
Copy link
Member

Merging this one meanwhile. If someone detects a great bottleneck, then we reconsider the SQL.

@pedrobaeza pedrobaeza merged commit d2751ae into OCA:11.0 Jan 16, 2019
@MiquelRForgeFlow MiquelRForgeFlow deleted the 11.0-fix-mig-stock-moves-inventory branch January 16, 2019 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants