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

[14.0][IMP] shopfloor: zone picking handle complete mix package #621

Merged
merged 4 commits into from
Aug 23, 2023

Conversation

TDu
Copy link
Member

@TDu TDu commented Apr 24, 2023

When a package has different product in it, it will be displayed with multiple move lines in the select_line step. And they will need to be move one after an other.

For such package, this change allows to move such package in one step.

ref.: rau-145

@OCA-git-bot
Copy link
Contributor

Hi @sebalix, @simahawk, @guewen,
some modules you are maintaining are being modified, check this out!

@TDu TDu force-pushed the 14.0-shopfloor-zp-pick-mix-pack branch 4 times, most recently from 324f2bf to 6b69945 Compare April 25, 2023 07:50
@TDu
Copy link
Member Author

TDu commented Apr 28, 2023

@mt-software-de thanks for your review. I updated the code for both of your comments.

@jbaudoux jbaudoux changed the title shopfloor: zone picking handle complete mix package [14.0][FIX] shopfloor: zone picking handle complete mix package May 29, 2023
@jbaudoux jbaudoux changed the title [14.0][FIX] shopfloor: zone picking handle complete mix package [14.0][IMP] shopfloor: zone picking handle complete mix package May 29, 2023
Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

The mobile version is not displaying correctly that a complete package is being moved. Only the desktop version is changed.

shopfloor/actions/packaging.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

I was not able to validate the move anymore. Odoo is complaining the package cannot be on 2 different locations

@TDu TDu force-pushed the 14.0-shopfloor-zp-pick-mix-pack branch from d605b53 to 0782202 Compare June 16, 2023 10:46
@TDu
Copy link
Member Author

TDu commented Jun 16, 2023

I was not able to validate the move anymore. Odoo is complaining the package cannot be on 2 different locations

I pretty sure I fixed that error after the rebase.

@TDu TDu force-pushed the 14.0-shopfloor-zp-pick-mix-pack branch from 0782202 to 010492b Compare June 19, 2023 04:01
self._actions_for("packaging")
data["handle_complete_mix_pack"] = (
self._handle_complete_mix_pack(move_line.package_id)
and not self.work.menu.no_prefill_qty
Copy link
Member Author

Choose a reason for hiding this comment

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

This line seems to be redundant with what is in the _handle_complete_mix_pack function

all the package quantities are reserved.
"""
return self.package_has_several_products(package) and all(
[quant.quantity == quant.reserved_quantity for quant in package.quant_ids]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[quant.quantity == quant.reserved_quantity for quant in package.quant_ids]
quant.quantity == quant.reserved_quantity for quant in package.quant_ids

Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

Also in change_package_lot.change_package here https://github.com/OCA/wms/pull/621/files#diff-81fce66279454c4c0be2adf1f46eb4e629c63edfe409c4cf6bda95940686f9bcR676
it is lacking the new package in the ok func (response for set line destination) in case of complete mix pack

Btw, it is also lacking the qty_done there but not related to this PR

cc @mt-software-de

shopfloor/services/zone_picking.py Outdated Show resolved Hide resolved
Comment on lines 633 to 636
if (
packaging.package_has_several_products(package)
and not handle_complete_mix_pack
):
Copy link
Contributor

Choose a reason for hiding this comment

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

With above suggestion, you can drop this change

shopfloor/services/zone_picking.py Outdated Show resolved Hide resolved
try:
stock.mark_move_line_as_picked(move_line, quantity, check_user=True)
stock.mark_move_line_as_picked(move_lines, quantity, check_user=True)
except ConcurentWorkOnTransfer as error:
response = self._response_for_set_line_destination(
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of line 938 where you set the quantity = None
we should not pass qty_done as argument.
Because we can not set qty_done to None, also it is not needed because it is already done.

kwargs = {}
if not package:
    kwargs["qty_done"] = quantity
response = self._response_for_set_line_destination(
    move_line,
    message={
        "message_type": "error",
        "body": str(error),
    },
    **kwargs
)
return (location_changed, response)```

@TDu
Copy link
Member Author

TDu commented Jul 19, 2023

Last fixup updates all the previous comments

@TDu TDu force-pushed the 14.0-shopfloor-zp-pick-mix-pack branch from fa254b4 to 5b8338b Compare July 20, 2023 12:19
@TDu
Copy link
Member Author

TDu commented Jul 24, 2023

Also in change_package_lot.change_package here https://github.com/OCA/wms/pull/621/files#diff-81fce66279454c4c0be2adf1f46eb4e629c63edfe409c4cf6bda95940686f9bcR676 it is lacking the new package in the ok func (response for set line destination) in case of complete mix pack

This pr has been refactored a few times. And I could not find a reason for passing that package to _response_for_set_line_destination , it actually does not make sense because this is an odoo record that was passed and not some json data.

So it is not needed and removed from previous implementation.

Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

Tested

@TDu TDu force-pushed the 14.0-shopfloor-zp-pick-mix-pack branch from cfba907 to 5727e3f Compare August 2, 2023 08:11
@TDu
Copy link
Member Author

TDu commented Aug 2, 2023

I squashed the commits and rebased.

Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

The move can be partially available

cc @mt-software-de

quantity = None
# Handling all move lines from a complete mix pack
for _move_line in package.move_line_ids:
if _move_line.state != "assigned":
Copy link
Contributor

@jbaudoux jbaudoux Aug 8, 2023

Choose a reason for hiding this comment

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

Suggested change
if _move_line.state != "assigned":
if _move_line.state not in ("assigned", "partially_available"):

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@jbaudoux
Copy link
Contributor

@mt-software-de Can you update your review?

Comment on lines +128 to +134
(
"quant_ids:total_quantity",
lambda rec, fname: sum(rec.quant_ids.mapped("quantity")),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to the mass test, performance problems are gradually appearing. One performance problem is the data loading. Because of that, we should only load the data that is really needed, in the screen. With this change, total_quantity is always loaded for the package data, also in other scenarios where it is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could create a separate method, like _get_package_parser which is getting as argument the record to load and kwargs. And there we can add a kwarg like with_total_quantity.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we do this improvement in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes we can do this.
I think we should create a PR which is doing it for all scenarios then.

@jbaudoux
Copy link
Contributor

@TDu can you squash the last fixup commit?

@TDu TDu force-pushed the 14.0-shopfloor-zp-pick-mix-pack branch 2 times, most recently from 3bb18b8 to 564cd6b Compare August 23, 2023 14:47
@TDu
Copy link
Member Author

TDu commented Aug 23, 2023

squashed and rebased.

When a package has different product in it, it will be displayed with
multiple move lines in the `select_line` step. And they will need to be
move one after an other.

For such package, this change allows to move such package in one step.
In the selection of a line only one line for such package will be
displayed.

The quantity picker will not be displayed.
@TDu TDu force-pushed the 14.0-shopfloor-zp-pick-mix-pack branch from 564cd6b to 7962b3c Compare August 23, 2023 15:18
@TDu
Copy link
Member Author

TDu commented Aug 23, 2023

I had to fix a test after the rebase on test_actions_data.py

@jbaudoux
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-621-by-jbaudoux-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit fc2c45b into OCA:14.0 Aug 23, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

4 participants