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] shopfloor: add checkout option ask leaf location dest #775

Conversation

TDu
Copy link
Member

@TDu TDu commented Nov 3, 2023

Following this change on the checkout scenario:

A check has been added to force the user to scan a child location if the destination location is not a leaf in the locations tree.

To allow for flexibility this change adds the options to disable this feature.

@TDu TDu force-pushed the 14-sf-add-checkout-option-ask-leaf-destination branch 2 times, most recently from 155a506 to f254bc4 Compare November 3, 2023 10:31
@sebalix sebalix changed the title shopfloor: add checkout option ask leaf location dest [14.0] shopfloor: add checkout option ask leaf location dest Nov 10, 2023
shopfloor/__manifest__.py Outdated Show resolved Hide resolved
@sebalix sebalix force-pushed the 14-sf-add-checkout-option-ask-leaf-destination branch from f254bc4 to e521bd2 Compare November 10, 2023 14:57
Following this change on the checkout scenario

    OCA#618

A check has been added to force the user to scan a child location
if the destination location is not a leaf in the locations tree.

To allow for flexibility this change adds the options to disable this
feature.
@sebalix sebalix force-pushed the 14-sf-add-checkout-option-ask-leaf-destination branch from e521bd2 to e08ad60 Compare November 13, 2023 09:12
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.

Why do you need an option for this?

@@ -1405,13 +1405,14 @@ def done(self, picking_id, confirmation=False):
)
lines_done = self._lines_checkout_done(picking)
dest_location = picking.location_dest_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be the move destination location

Copy link
Contributor

Choose a reason for hiding this comment

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

There is something fishy there:

  1. as said, we should take the destination location of the move
  2. but what move? we get all lines done of picking, so we could get several moves as well, each having a different destination location (e.g. a sub-location of the picking destination location thanks to a put-away)
  3. so we should ask the user to scan a relevant leaf location for each move that targets a non-leaf location?

Right now this code is asking the user to scan a leaf location even if it's probably not necessary IMO 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

We should always scan a destination if the moves target different destinations

)
if len(child_locations) > 0 and child_locations != dest_location:
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 simply test if location_dest_id.usage == "view" then we always need to choose a destination location?

Copy link
Contributor

Choose a reason for hiding this comment

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

Odoo std allows to put internal locations inside internal ones, so checking only usage == view is not enough.
In such case we would need such option to decide what behavior we want for each Shopfloor menu.

Now I'm not sure about the current implementation of this feature, see my comment above.

Copy link
Contributor

@jbaudoux jbaudoux Nov 14, 2023

Choose a reason for hiding this comment

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

Following sebalix remark that we are processing multiple moves, use this condition:
len(lines_done.location_dest_id) != 1 or lines_done.location_dest_id.usage == "view"

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.

Discussed with @sebalix . Looks like proposed changes should solve the issue without the need of a new option.

@TDu
Copy link
Member Author

TDu commented Nov 13, 2023

Discussed with @sebalix . Looks like proposed changes should solve the issue without the need of a new option.

If Odoo allows to store products in stock locations that are not a leaf of the location structure, the shopfloor app should do the same.
This is why the options is needed. IMO

@jbaudoux
Copy link
Contributor

If Odoo allows to store products in stock locations that are not a leaf of the location structure, the shopfloor app should do the same. This is why the options is needed. IMO

It's just that the option is currently providing a feature that nobody needs. So, let's not introduce more complexity. Adapting the if condition will solve the issue.

@TDu
Copy link
Member Author

TDu commented Nov 14, 2023

Following on the comments here, I opened another PR #780 as the fix changes completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants