Skip to content

[18.0][MIG] stock_checkout_sync #1979

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

Open
wants to merge 23 commits into
base: 18.0
Choose a base branch
from

Conversation

sebalix
Copy link
Contributor

@sebalix sebalix commented Jun 5, 2025

Migrated from OCA/wms#14.0.

Issue about updating locations directly on moves and on backorders

Starting from Odoo 18.0, source/dest locations on moves are coming directly from the related stock.picking. Fields are stored=True / readonly=False, but when Odoo is putting a backorder move in its own transfer, this is resetting the source/dest locations that were set on it (see https://github.com/odoo/odoo/blob/18.0/addons/stock/models/stock_picking.py#L1585).

Should we split these moves in their own picking (with the expected dest location) when syncing them?

M1 (assigned) A -> B
M2 (assigned) A -> B

Sync both moves to reach C:

M1 (assigned) A -> C
M2 (assigned) A -> C

Validate M1, so we get M2 in its own backorder transfer:

M1 (done)     A -> C
M2 (assigned) A -> B    # /!\ Reset to 'stock.picking' dest location

An idea would be to set the synced dest location not only on the moves themselves, but on their stock.picking, so all moves in this transfert will be synced to reach C.

The issue by doing that is it doesn't fulfill an existing use-case, which is having one of the moves having a different stock.picking as a destination than the others, so not impacted by the sync of other moves:

M1 (assigned) A -> B from PICK1, targetting M3 part of PACK1
M2 (assigned) A -> B from PICK1, targetting M4 part of PACK2

Sync M1 to C:

M1 (assigned) A -> C from PICK1, targetting M3 part of PACK1
M2 (assigned) A -> B from PICK1, targetting M4 part of PACK2

In 14.0 we were able to get different moves (M1 + M2) part of the same transfert (PICK1) but having different dest locations (as soon as they respect the parent location defined by the stock.picking).
Keeping this logic in 18.0 seems dangerous:

  • technically that could still work, excepted when dealing with backorders as seen above (dest location is reset)
  • updating directly location fields on the move seems fragile in 18.0, the system is thought to update locations on the related stock.picking, not directly on moves independently (I see it that way, could be wrong).

EDIT: after discussion with @jbaudoux , the use-case having moves within the same PICK transfer targetting different PACK transfers should not happen / be supported => removing/adapting tests accordingly + sync the location both on stock.picking + moves, no discrepancy allowed.

guewen and others added 20 commits June 5, 2025 17:16
Add an assistant to select the same destination location for all the
moves that will reach the same transfer. Generally used on packing
locations.
m is supposed to be part of moves_to_update.move_dest_ids, and
moves_to_update is already a subset of self so it's always True
* union ids in a set instead of browse for performance in case of many
  moves
* improve summary description in glue module
* remove redundant code in test
* split test common case in "common.py": we shouldn't import a test file
  from another module
Currently translated at 100.0% (35 of 35 strings)

Translation: wms-13.0/wms-13.0-stock_checkout_sync
Translate-URL: https://translation.odoo-community.org/projects/wms-13-0/wms-13-0-stock_checkout_sync/es_AR/
Currently translated at 100.0% (35 of 35 strings)

Translation: wms-14.0/wms-14.0-stock_checkout_sync
Translate-URL: https://translation.odoo-community.org/projects/wms-14-0/wms-14-0-stock_checkout_sync/it/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: wms-14.0/wms-14.0-stock_checkout_sync
Translate-URL: https://translation.odoo-community.org/projects/wms-14-0/wms-14-0-stock_checkout_sync/
Currently translated at 100.0% (35 of 35 strings)

Translation: wms-14.0/wms-14.0-stock_checkout_sync
Translate-URL: https://translation.odoo-community.org/projects/wms-14-0/wms-14-0-stock_checkout_sync/it/
Currently translated at 100.0% (35 of 35 strings)

Translation: wms-14.0/wms-14.0-stock_checkout_sync
Translate-URL: https://translation.odoo-community.org/projects/wms-14-0/wms-14-0-stock_checkout_sync/it/
@sebalix sebalix added this to the 18.0 milestone Jun 5, 2025
@sebalix sebalix requested a review from jbaudoux June 5, 2025 15:42
@sebalix sebalix force-pushed the 18-mig-stock_checkout_sync branch from 0999dd0 to 886d6ab Compare June 5, 2025 15:47
@sebalix sebalix marked this pull request as ready for review June 5, 2025 15:47
Copy link

@twalter-c2c twalter-c2c left a comment

Choose a reason for hiding this comment

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

Technically, LGTM.

comodel_name="stock.picking",
relation="stock_move_checkout_sync_stock_picking_done_dest_rel",
)
show_skip_button = fields.Boolean(default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess this should be enough

Suggested change
show_skip_button = fields.Boolean(default=False)
show_skip_button = fields.Boolean()

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

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

LGTM

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.

As discussed, I'm not sure it's still right to change the destination of the moves and not of the picking as now the destination of the move is computed.

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.

Because it doesn't work anymore on backorder due to the compute. There should be a failing test.

# Sync the source of the destination move too, if it's still waiting.
moves_to_update.move_dest_ids.filtered(
moves_dest = moves_to_update.move_dest_ids.filtered(
# FIXME add partially_available?
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, and confirmed as well

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.

@sebalix you squash the fixups?

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

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.