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

[MIG] stock_storage_type: Migration to 16.0 #470

Merged
merged 145 commits into from
Jul 13, 2023

Conversation

@rousseldenis
Copy link
Contributor Author

/ocabot migration stock_storage_type

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Oct 18, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 18, 2022
18 tasks
@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch 3 times, most recently from 2d2dab5 to 7fcf545 Compare October 18, 2022 12:42
@rousseldenis
Copy link
Contributor Author

@jbaudoux @lmignon I think this is a first testable version.

@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch from 7fcf545 to f6445b9 Compare October 18, 2022 14:19
@rousseldenis rousseldenis reopened this Oct 19, 2022
@rousseldenis rousseldenis reopened this Oct 19, 2022
@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch from f6445b9 to e89ddcc Compare October 19, 2022 15:07
@jbaudoux
Copy link
Contributor

@rousseldenis Do you think we can replace stock.location.storage.type by the new standard stock.storage.category and add a tab on the "Storage Category" called "Capacity by Storage" where you show the list of available storage types?
Drawback I see is:

  • the max_height won't be supported for the capacity by package type or product
  • the location can only have a single storage category but that's not bad limitation

@rousseldenis
Copy link
Contributor Author

@rousseldenis Do you think we can replace stock.location.storage.type by the new standard stock.storage.category and add a tab on the "Storage Category" called "Capacity by Storage" where you show the list of available storage types? Drawback I see is:

  • the max_height won't be supported for the capacity by package type or product
  • the location can only have a single storage category but that's not bad limitation

Not yet tested but that can make sense to get back to standard.

@jbaudoux
Copy link
Contributor

jbaudoux commented Oct 20, 2022

@rousseldenis Coming back on this:

  • the location can only have a single storage category but that's not bad limitation

I checked more deeply the implications and that would mean that the allow_new_product needs to be also added on the relation between the storage category and the product/pack storage. It would be an optional setting overriding the default one from the category.

Today we are using this config: Location A has:

  • location storage type LST-1 is empty for product/pack storage type PST-X
  • location storage type LST-2 is do not mix for product/pack storage type PST-Y

That would become: location A has:

  • location storage category LC-12 for product/pack storage type PST-X in mode is empty and for PST-Y in mode do not mix

@lmignon
Copy link
Contributor

lmignon commented Oct 20, 2022

@rousseldenis Coming back on this:

  • the location can only have a single storage category but that's not bad limitation

I checked more deeply the implications and that would mean that the allow_new_product needs to be also added on the relation between the storage category and the product/pack storage. It would be an optional setting overriding the default one from the category.

Today we are using this config: Location A has:

* location storage type LST-1 is empty for product/pack storage type PST-X

* location storage type LST-2 is do not mix for product/pack storage type PST-Y

That would become: location A has:

* location storage category LC-12 for product/pack storage type PST-X in mode is empty and for PST-Y in mode do not mix

If we add information on the relation between storage category and pack/product storage, we could also add the max_weight at the same level

@jbaudoux
Copy link
Contributor

If we add information on the relation between storage category and pack/product storage, we could also add the max_weight at the same level

The max weight/height is a location characteristic not depending on what you put in. According to me, no need to put that on the relation.

@lmignon
Copy link
Contributor

lmignon commented Oct 20, 2022

The max weight/height is a location characteristic not depending on what you put in. According to me, no need to put that on the relation.

It makes sens. That means we can remove all the logic used to compute this max_height on the stock_location and declare the field on the stock_location if it doesn't exist.

@lmignon
Copy link
Contributor

lmignon commented Oct 20, 2022

@jbaudoux

Since stock.package.storage.typeis now a stock.package.type, we already have the relation on stock.storage.category. The only missing piece according to our last comment is that we should add an allow_new_product optional field on this relation. (Implemented by stock.storage.category.capacity)

@jbaudoux
Copy link
Contributor

jbaudoux commented Oct 20, 2022

Since stock.package.storage.typeis now a stock.package.type, we already have the relation on stock.storage.category. The only missing piece according to our last comment is that we should add an allow_new_product optional field on this relation. (Implemented by stock.storage.category.capacity)

No, you cannot replace stock.package.storage.typeby stock.package.type. It is a complete different concept. The package type will say for example that it is a pallet. The storage type will tell the putaway configuration to apply. We need the 2 concepts as 2 different products on a pallet can require completely different putaway configuration.

@jbaudoux
Copy link
Contributor

It makes sens. That means we can remove all the logic used to compute this max_height on the stock_location and declare the field on the stock_location if it doesn't exist.

It can be replaced by a related readonly to the storage category.

@rousseldenis
Copy link
Contributor Author

No, you cannot replace stock.package.storage.typeby stock.package.type. It is a complete different concept.

@jbaudoux Have you tested it on runboat ? I made it working with nothing changed at tests levels.

@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch 2 times, most recently from cf0c5f3 to ca7599a Compare October 20, 2022 12:45
@jbaudoux
Copy link
Contributor

jbaudoux commented Oct 20, 2022

No, you cannot replace stock.package.storage.typeby stock.package.type. It is a complete different concept.

@jbaudoux Have you tested it on runboat ? I made it working with nothing changed at tests levels.

The issue is that it is impossible to configure because it is 2 different concepts. We need multiple package storage types for a same package type. The package storage type represent the putaway rule you set for the package type. And we have many rules (package storage type) for a box or a pallet (package type).

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.

You cannot replace stock.package.storage.type by stock.package.type.
See previous comments

@jbaudoux
Copy link
Contributor

The issue is that it is impossible to configure because it is 2 different concepts. We need multiple package storage types for a same package type. The package storage type represent the putaway rule you set for the package type. And we have many rules (package storage type) for a box or a pallet (package type).

After discussion, we can use the new stock.package.type instead of stock.package.storage. However, this is not to be confused by the package type OCA module (https://github.com/OCA/product-attribute/tree/14.0/product_packaging_type) which has then to not be replaced by the new stock.package.type

@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch from 7a29e71 to 5d3249b Compare March 16, 2023 08:03
@rousseldenis
Copy link
Contributor Author

@jbaudoux I think this is ready

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 README needs to be rewritten.

I would also rename all pack_putaway_strategy into package_type_putaway_strategy

@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch 2 times, most recently from 4003944 to 1bcd098 Compare April 14, 2023 07:49
@rousseldenis
Copy link
Contributor Author

@jbaudoux @lmignon This one seems ready and cover now the flow without package (put in pack) for the allowed destinations

@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch from 75b2630 to 705fc81 Compare April 28, 2023 13:10
…s to apply allowed destinations without packages

Rename the putaway strategy method name to take into account package type

As in some flows, it could have no package defined (no put in pack), fallback on
the product default package type and apply the allowed destination strategy
@rousseldenis rousseldenis force-pushed the 16.0-mig-stock_storage_type-dro branch from 705fc81 to 46b937e Compare April 28, 2023 13:15
…ructure

If a view location has a strategy == 'none' and a sequence to be applied, the first call
to _get_putaway_strategy in Odoo core will return the first internal child (and no strategy
will be applied at all).

So, if the location is a view and has no putaway rules for that product, substitute the
child_internal_location_ids field value to self.
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.

LG now

@rousseldenis
Copy link
Contributor Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-470-by-rousseldenis-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f44ad54 into OCA:16.0 Jul 13, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 731f26f. 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.