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][16.0] product_packaging_type + renaming to product_packaging_level #1215

Merged
merged 50 commits into from
Sep 18, 2023

Conversation

rousseldenis
Copy link
Sponsor Contributor

@rousseldenis
Copy link
Sponsor Contributor Author

/ocabot migration product_packaging_type

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Nov 29, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Nov 29, 2022
55 tasks
@rousseldenis rousseldenis force-pushed the 16.0-mig-product-packaging-level-dro branch from 65ff980 to 3d84894 Compare November 29, 2022 07:48
@rousseldenis
Copy link
Sponsor Contributor Author

@ChrisOForgeFlow @LoisRForgeFlow You should be interested by this.

I've put changes in separate commits, so you maybe want to port this to 15

@rousseldenis rousseldenis force-pushed the 16.0-mig-product-packaging-level-dro branch 2 times, most recently from 771eff1 to 7af73ee Compare November 29, 2022 07:54
@rousseldenis rousseldenis force-pushed the 16.0-mig-product-packaging-level-dro branch 2 times, most recently from 1b5b2ab to f35e290 Compare November 29, 2022 08:27
Copy link
Sponsor Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

@rousseldenis The UI isn't working at all.... It's not possible to add new packaging on the product forms.. the packaging type remains always the same etc...

@rousseldenis
Copy link
Sponsor Contributor Author

@rousseldenis The UI isn't working at all.... It's not possible to add new packaging on the product forms.. the packaging type remains always the same etc...

Ok, I check

@rousseldenis
Copy link
Sponsor Contributor Author

@lmignon Seems better now

Copy link
Sponsor Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

This code LGTM (also functional tests) but I've some difficulties to understand the difference between packaging type and packaging level. Can you elaborate and add some informations into the readme plz.

@rousseldenis
Copy link
Sponsor Contributor Author

rousseldenis commented Dec 5, 2022

@jbaudoux Maybe an advice here on how to describe functionally this module and how this is different from packaging 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.

It is missing a small feature to ease the configuration. On the package type, we should be able to configure a package level and when we select a package type on the product packaging, it should set the right package level on the product packaging.

product_packaging_level/readme/DESCRIPTION.rst Outdated Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

@rousseldenis I have a question. Why putting this update of the table in a pre-init-hook instead of putting it in a pre-migration.py script in the migration directory of this module ?

Is the pre-init-hook launched when performing an update of the module ? I thought that it only runs when installing the module, but I may be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure on my side but I suppose there is no update here.
You install a new module (new name).
Installing it, you move data with this hook.
It seems logic to me but @rousseldenis will give the good response

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

@remytms @bealdav Because as the module is renamed, it is not yet installed. So, pre-init is the good entry point.

Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

Thanks for this renaming, make it more explicit.
I would like to test with runboat before make final review.
Could you try to regenerate it ?
Thanks

@rousseldenis
Copy link
Sponsor Contributor Author

Thanks for this renaming, make it more explicit. I would like to test with runboat before make final review. Could you try to regenerate it ? Thanks

@bealdav Done

@jbaudoux
Copy link
Contributor

jbaudoux commented Jun 8, 2023

@rousseldenis Can you update the readme and add the missing link between package type & level. Cf #1215 (review)

Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

LGTM, really thanks.

Thanks to update doc.

@rousseldenis rousseldenis force-pushed the 16.0-mig-product-packaging-level-dro branch from e7d3572 to a8a6300 Compare September 18, 2023 10:13
@rousseldenis
Copy link
Sponsor Contributor Author

@jbaudoux Done

@rousseldenis rousseldenis force-pushed the 16.0-mig-product-packaging-level-dro branch from a8a6300 to 3d2a27f Compare September 18, 2023 11:10
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.

Let's merge this. The small missing feature can be done in a later PR

@jbaudoux
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Sorry @jbaudoux you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@rousseldenis rousseldenis force-pushed the 16.0-mig-product-packaging-level-dro branch from 3d2a27f to 5901a8a Compare September 18, 2023 12:37
@rousseldenis
Copy link
Sponsor Contributor Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1215-by-rousseldenis-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 6ac7f20 into OCA:16.0 Sep 18, 2023
7 of 9 checks passed
@OCA-git-bot
Copy link
Contributor

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