[14.0][FIX] shopfloor_reception: Fix qty todo computation #726

mmequignon · 2023-08-16T14:02:40Z

We had a concurrent update introduced here #672

Updating product uom qties were causing issues.
This is basically fixed by modifying the product_qty in responses instead of updating the move lines qties.

OCA-git-bot · 2023-08-16T14:02:44Z

Hi @JuMiSanAr,
some modules you are maintaining are being modified, check this out!

jbaudoux

What about the uom?

shopfloor_reception/services/reception.py

mt-software-de · 2023-09-14T15:54:35Z

shopfloor_reception/services/reception.py

+        # then selected_line is the only one related to this move.
+        # In such case, we must ensure there's another move line with the remaining
+        # quantity to do, so selected_line is extracted in a new move as expected.
+        selected_line._split_partial_quantity()


Suggested change

selected_line._split_partial_quantity()

new_move_line = selected_line._split_partial_quantity()

We should call unmark_move_line_as_picked and set the location_dest_id back to stock.move's location_dest_id
if a split was happening

stock = self._actions_for("stock") stock.unmark_move_line_as_picked(new_move_line) new_move_line.location_dest_id = new_move_line.move_id.location_dest_id

camptocamp#118

Hello @mt-software-de , I updated the branch, can you please rebase? I'm checking this.
Thanks!

This is correct but you also need to recompute the putaway.
It is easier if you always set product_uom_qty to 0, then there is no move line backorder (i.e. nothing to split).
I would do:

if product_uom_qty: selected_line.product_uom_qty = 0

and drop the _split_partial_quantity

Hello @mt-software-de , I updated the branch, can you please rebase? I'm checking this. Thanks!

rebase was done

shopfloor_reception/models/shopfloor_menu.py

shopfloor_reception/services/reception.py

jbaudoux · 2023-09-18T07:55:15Z

shopfloor_reception/services/reception.py

+        # If user only processed 1/5 and is the only one working on the move,
+        # then selected_line is the only one related to this move.
+        # In such case, we must ensure there's another move line with the remaining
+        # quantity to do, so selected_line is extracted in a new move as expected.
+        selected_line._split_partial_quantity()


This block requires a if product_uom_qty:

shopfloor_reception/services/reception.py

jbaudoux · 2023-09-18T08:04:57Z

shopfloor_reception/services/reception.py

+        # then selected_line is the only one related to this move.
+        # In such case, we must ensure there's another move line with the remaining
+        # quantity to do, so selected_line is extracted in a new move as expected.
+        selected_line._split_partial_quantity()


This is correct but you also need to recompute the putaway.
It is easier if you always set product_uom_qty to 0, then there is no move line backorder (i.e. nothing to split).
I would do:

if product_uom_qty: selected_line.product_uom_qty = 0

and drop the _split_partial_quantity

jbaudoux · 2023-09-18T08:10:43Z

shopfloor_reception/services/reception.py

+        # then selected_line is the only one related to this move.
+        # In such case, we must ensure there's another move line with the remaining
+        # quantity to do, so selected_line is extracted in a new move as expected.
+        selected_line._split_partial_quantity()
        new_move = selected_line.move_id.split_other_move_lines(


If some other move lines have a product_uom_qty, you need to modify them so that the new_move qty done is not smaller than the remaining move lines product_uom_qty. To prevent this complexity, it is easier if all move lines have a product_uom_qty of 0.

Suggested change

new_move = selected_line.move_id.split_other_move_lines(

selected_line.move_id.move_line_ids.filtered("product_uom_qty").product_uom_qty = 0

new_move = selected_line.move_id.split_other_move_lines(

We decided to revert the change where I introduced the "align_qties" method, and to modify the product_uom_qty in the response instead, in order to not alter a move line the user is not working on.
I'm, we should set the qty in _scan_line__find_or_create_line every time, whether we create a new line or find one.

Drop the call to split_other_move_lines and do sth like:

if selected_line.qty_done < selected_line.move_id.qty_todo: split_move_vals = self._split(selected_line.qty_done) split_move = self.create(split_move_vals) split_move.move_line_ids = selected_line split_move._action_confirm(merge=False) split_move._recompute_state() split_move._action_assign() selected_line.move_id._recompute_state()

@jbaudoux can you make another pass please?

mmequignon · 2023-09-20T14:20:55Z

@jbaudoux Just checked, and we need the product uom qty to be set on the move line when we split the move.
In split_other_move lines, we use this product_uom_qty to determine the qty to be done on the new move.
Before we post the move though, we can use 0 as a product uom qty, that's ok, I guess.

jbaudoux

Looks promising. I still have a few remarks

shopfloor_reception/services/reception.py

TDu · 2023-11-03T12:47:48Z

@jbaudoux @mt-software-de Could I get a review update on this one, thanks

mt-software-de

I think we need some locks. Otherwise working in parallel will not work.

shopfloor_reception/data/shopfloor_scenario_data.xml

mt-software-de · 2023-11-05T15:41:21Z

shopfloor_reception/services/reception.py

-        if line:
-            # The line quantity to do needs to correspond to
-            # the remaining quantity to do of its move.
-            line.product_uom_qty = move.product_uom_qty - move.quantity_done
-        else:
-            qty_todo_remaining = max(0, move.product_uom_qty - move.quantity_done)
-            values = move._prepare_move_line_vals(quantity=qty_todo_remaining)
+        if not line:
+            values = move._prepare_move_line_vals()


Shouldn't there not a kind of lock?
If users work in parrallel, they will get serialize errors.
Because in the method _scan_line__assign_user the picking.user_id will be set and also of course the shopfloor_user_id of the stock.move.line.
Also i think we should remove the self._assign_user_to_picking from _scan_line__assign_user

If a line was found, shouldn't we change the product_uom_qty to 0. Otherwise _auto_post_line can maybe make some trouble if we work in parallel.

I think removing self._assign_user_to_picking from _scan_line__assign_user is a good idea because the user assigned to picking is not used in this scenario.
@jbaudoux ?

About reseting product_uom_qty to zero it is probably already done enough in this scenario (Before line is posted, all are reset)

I mean if a user starts and there is already an unassigned move.line where product_uom_qty is already set. I think it would be safe to set it to 0, then it would be the same as creating a new move.line

If two users start a the same time with the same product this will cause an serialize access error, because the will both get maybe the same existing move.line if there is one.
And if we are not remove the assign_user_to_picking this will cause also an serialize access error.

This function has been improved by the commit shopfloor_reception: improve move line assignation to user
First to lock a row being assigned to the user.
Second the first implementation missed some sorting and the first line being choosen could have been one without shopfloor_user assigned although a line with the current user assigned does exist.

mt-software-de · 2023-11-05T15:53:52Z

shopfloor_reception/services/reception.py

+        if lines_with_qty_todo:
+            lines_with_qty_todo.product_uom_qty = 0


We should ensure that there is only one line with product_uom_qty on _scan_line__find_or_create_line we should ensure that the product_uom_qty is 0. Otherwise we can find maybe more then one line?

With this scenario there can be multiple shopfloor users on the same move. Each one will have its own stock.move.line assigned. The product_uom_qty are kept to zero on all stock.move.line until they are being posted. This is to avoid concurrency issues, when users are scanning more than what is required by the stock.move.
So it is actually the goal to update all lines with a positive value on product_uom_qty back to zero.
All the done line that we would not want to update have been extracted on done.

I also improved the filter above to exclude on the state as well, just in case and to be more explicit.

What i mean. In _post_line you are setting the product_uom_qty
This line will be found bei auto_post_line normally it should only find the selected_line,
because other lines from other users should also have set a qty of 0.
But in _scan_line__find_or_create_line there could be a existing unassigned move.line returned with a qty set.

Also i think we can remove lines_with_qty_todo and always use the selected line.

Is it really needed to check for other stock.move.lines with a product_uom_qty > 0?
Because i think every scanned line should have a product_uom_qty of 0, until _post_line is called.

yes, because nobody prevents anybody to modify this from the UI.
Hence, we might have issues if sum(move.move_line_ids.mapped("product_uom_qty")) > move.product_uom_qty, when recomputing the state of the move.

We can set product_uom_qty = 0 everywhere in the code, but what really matters, here, is that those are aligned when recomputing the state of the move.
Otherwise the move state moves from assigned to partially available, and is not visible in shopfloor.

Thx for the explanation. I tested this quick.
If i'm working with two users. Both are scanning both getting an own move.line.
The first one creates a new package and sets the the destination and enters the _post_line method.
Within the _auto_post_line the split is happening a new picking is created in state done.
And the move._recompute_state() sets the state to "confirmed".
So the move state is not anymore 'assigned' as you said.
What is clear if i am looking at the code.
https://github.com/odoo/odoo/blob/f572335756ef60402236cf5e4e11c0c843c3879e/addons/stock/models/stock_move.py#L1653
https://github.com/odoo/odoo/blob/f572335756ef60402236cf5e4e11c0c843c3879e/addons/stock/models/stock_move.py#L1659C25-L1659C26
It will only be set to 'assigned' if the move.reserved_availability ( sum of move.lines product_uom_qty) is the same as the move's product_uom_qty, and this isn't the case.

I could reproduce the issue you mentioned.
My last commit fixes that.

shopfloor_reception/services/reception.py

TDu · 2023-11-07T16:03:34Z

Rebased on 14.0

shopfloor_reception/services/reception.py

TDu · 2023-11-13T09:58:18Z

@mt-software-de PR updated could you give another review, thank you

shopfloor_reception/services/reception.py

jbaudoux · 2023-11-13T10:09:39Z

@TDu Can you squash fixups?

jbaudoux · 2023-11-15T11:29:09Z

@mt-software-de I think everything as been addressed now. I have not re-tested. Can you update your review?

mt-software-de · 2023-11-15T10:46:18Z

shopfloor_reception/services/reception.py

+        if not line:
+            values = move._prepare_move_line_vals()
            line = self.env["stock.move.line"].create(values)


I think we should ensure that the product_uom_qty is always 0.

Suggested change

if not line:

values = move._prepare_move_line_vals()

line = self.env["stock.move.line"].create(values)

if not line:

values = move._prepare_move_line_vals()

line = self.env["stock.move.line"].create(values)

else:

line.product_uom_qty = 0

We do this on the post for backward compatibility with previous implementation (lines that would already be there with a qty != 0). Also if you have only one line pre-generated by odoo, you have a qty != 0 and this is not causing any issue.
When posting, it is required we care other lines (if any) are well set to 0 otherwise you cannot post.

#726 (comment)

mt-software-de · 2023-11-15T11:07:22Z

shopfloor_reception/services/reception.py

+        if lines_with_qty_todo:
+            lines_with_qty_todo.product_uom_qty = 0


Is it really needed to check for other stock.move.lines with a product_uom_qty > 0?
Because i think every scanned line should have a product_uom_qty of 0, until _post_line is called.

mt-software-de · 2023-11-15T11:58:01Z

@mt-software-de I think everything as been addressed now. I have not re-tested. Can you update your review?
Not everything. Could you have a look at my last review?

TDu · 2023-11-17T16:24:37Z

Rebased

TDu · 2023-11-27T12:06:09Z

Rebased

mt-software-de · 2023-11-28T13:14:48Z

shopfloor_reception_mobile/static/src/scenario/reception.js

@@ -153,6 +153,7 @@ const Reception = {
                        <v-row align="center">
                            <v-col class="text-center" cols="12">
                                <btn-back/>
+                                <cancel-button/>


Suggested change

<cancel-button/>

</v-col>

<v-col class="text-center" cols="12">

<cancel-button/>

A own v-col would be great i think.

With suggestion, it would look like this.

Do we need both back & cancel ?

I don't know, but
the Cancel button will call the back end to cancel the done quantity on the current line (line will be removed).
the Back button goes back to the previous screen without calling the backend.

I have done the change to have the button spaced out.

In my opinion, it should be either cancel or back. So cancel should replace the back button. Maybe @mmequignon can clarify the intention here?

When creating a move line, set product_uom_qty = 0. Only when such line is posted this value will be aligned with qty_done.

Because many users can work on the same picking at the same time and the scenario never uses this information. It is not needed.

Add the skip locked option.

When 2 users work on the same move and one user completes his picking, The move containing the quantity left to do could not be switch back to assigned.

mt-software-de

LGTM now :-). Thanks

jbaudoux · 2023-12-04T09:28:37Z

/ocabot merge minor

OCA-git-bot · 2023-12-04T09:28:49Z

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-726-by-jbaudoux-bump-minor, awaiting test results.

OCA-git-bot · 2023-12-04T09:42:43Z

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

jbaudoux reviewed Aug 17, 2023

View reviewed changes

shopfloor_reception/services/reception.py Outdated Show resolved Hide resolved

shopfloor_reception/services/reception.py Show resolved Hide resolved

jbaudoux changed the title ~~shopfloor_reception: Fix qty_done computation~~ shopfloor_reception: Fix qty todo computation Aug 22, 2023

jbaudoux changed the title ~~shopfloor_reception: Fix qty todo computation~~ [14.0][FIX] shopfloor_reception: Fix qty todo computation Aug 22, 2023

mt-software-de suggested changes Sep 14, 2023

View reviewed changes

mmequignon force-pushed the shopfloor_reception_qty_done_fixes branch 2 times, most recently from ee85400 to dd90a41 Compare September 15, 2023 10:22

jbaudoux requested changes Sep 18, 2023

View reviewed changes

mmequignon force-pushed the shopfloor_reception_qty_done_fixes branch 2 times, most recently from 002e7ad to 5daa717 Compare September 25, 2023 10:54

mt-software-de mentioned this pull request Sep 29, 2023

[14.0][FIX] shopfloor_reception: multiple moves with same product #759

Merged

jbaudoux requested changes Oct 10, 2023

View reviewed changes

TDu reviewed Oct 11, 2023

View reviewed changes

shopfloor_reception/services/reception.py Outdated Show resolved Hide resolved

TDu force-pushed the shopfloor_reception_qty_done_fixes branch from 2e4b758 to cbdc725 Compare October 20, 2023 11:36

mt-software-de suggested changes Nov 5, 2023

View reviewed changes

TDu force-pushed the shopfloor_reception_qty_done_fixes branch from 1afa337 to f535ce3 Compare November 7, 2023 16:03

TDu force-pushed the shopfloor_reception_qty_done_fixes branch from f32f528 to d56d148 Compare November 8, 2023 17:54

mt-software-de suggested changes Nov 8, 2023

View reviewed changes

shopfloor_reception/services/reception.py Show resolved Hide resolved

jbaudoux reviewed Nov 13, 2023

View reviewed changes

shopfloor_reception/services/reception.py Outdated Show resolved Hide resolved

TDu force-pushed the shopfloor_reception_qty_done_fixes branch 2 times, most recently from d839dae to d76aabc Compare November 15, 2023 10:37

jbaudoux approved these changes Nov 15, 2023

View reviewed changes

mt-software-de suggested changes Nov 15, 2023

View reviewed changes

TDu force-pushed the shopfloor_reception_qty_done_fixes branch 2 times, most recently from 5ac327c to cd39385 Compare November 17, 2023 16:24

mt-software-de mentioned this pull request Nov 21, 2023

[FIX] shopfloor_reception: Unmark as picked on partial picking camptocamp/wms#118

Closed

mmequignon added 4 commits November 27, 2023 12:45

shopfloor_reception: Fix qty_done computation

dfd20da

shopfloor_reception: Split moves before posting

f49eb1c

shopfloor_reception: Fix uom computation

74692b4

shopfloor_reception: Make select picking by product optional

7e9aaa1

TDu force-pushed the shopfloor_reception_qty_done_fixes branch from cd39385 to 348faa1 Compare November 27, 2023 12:05

mt-software-de suggested changes Nov 28, 2023

View reviewed changes

mmequignon and others added 9 commits November 29, 2023 08:32

shopfloor_reception: set_quantity - Add cancel button

6133aa7

shopfloor_reception: fix product_uom_qty at move line create

9cb1425

When creating a move line, set product_uom_qty = 0. Only when such line is posted this value will be aligned with qty_done.

shopfloor_reception: more integration tests

285a68a

shopfloor_reception: Fix product_uom_qty on move line

36c795c

sf_reception: remove assigning user to picking

90ee401

Because many users can work on the same picking at the same time and the scenario never uses this information. It is not needed.

shopfloor_base: improve lock for update action

6d381aa

Add the skip locked option.

shopfloor_reception: improve move line assignation to user

ed5e320

shopfloor_reception: fix lock on move when done

20560a6

shopfloor_reception: fix auto post option

8e35df0

When 2 users work on the same move and one user completes his picking, The move containing the quantity left to do could not be switch back to assigned.

TDu force-pushed the shopfloor_reception_qty_done_fixes branch from 348faa1 to 8e35df0 Compare November 29, 2023 07:32

mt-software-de approved these changes Nov 29, 2023

View reviewed changes

OCA-git-bot added the approved label Nov 29, 2023

OCA-git-bot added the bot is merging ⏳ label Dec 4, 2023

OCA-git-bot merged commit b600b1e into OCA:14.0 Dec 4, 2023
5 of 7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Dec 4, 2023

	selected_line._split_partial_quantity()
	new_move_line = selected_line._split_partial_quantity()

	new_move = selected_line.move_id.split_other_move_lines(
	selected_line.move_id.move_line_ids.filtered("product_uom_qty").product_uom_qty = 0
	new_move = selected_line.move_id.split_other_move_lines(

		if lines_with_qty_todo:
		lines_with_qty_todo.product_uom_qty = 0

[14.0][FIX] shopfloor_reception: Fix qty todo computation #726

[14.0][FIX] shopfloor_reception: Fix qty todo computation #726

Conversation

mmequignon commented Aug 16, 2023

OCA-git-bot commented Aug 16, 2023

jbaudoux left a comment

Choose a reason for hiding this comment

mt-software-de Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaudoux Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmequignon commented Sep 20, 2023

jbaudoux left a comment

Choose a reason for hiding this comment

TDu commented Nov 3, 2023

mt-software-de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TDu commented Nov 7, 2023

TDu commented Nov 13, 2023

jbaudoux commented Nov 13, 2023

jbaudoux commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mt-software-de commented Nov 15, 2023

TDu commented Nov 17, 2023

TDu commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TDu Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mt-software-de left a comment

Choose a reason for hiding this comment

jbaudoux commented Dec 4, 2023

OCA-git-bot commented Dec 4, 2023

OCA-git-bot commented Dec 4, 2023

mt-software-de Sep 14, 2023 •

edited

Loading

jbaudoux Sep 25, 2023 •

edited

Loading

TDu Nov 29, 2023 •

edited

Loading