-
-
Notifications
You must be signed in to change notification settings - Fork 722
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
[MIG][16.0] product_packaging_type + renaming to product_packaging_level #1215
Conversation
/ocabot migration product_packaging_type |
65ff980
to
3d84894
Compare
@ChrisOForgeFlow @LoisRForgeFlow You should be interested by this. I've put changes in separate commits, so you maybe want to port this to 15 |
771eff1
to
7af73ee
Compare
1b5b2ab
to
f35e290
Compare
There was a problem hiding this 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...
Ok, I check |
f35e290
to
5382722
Compare
@lmignon Seems better now |
There was a problem hiding this 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.
@jbaudoux Maybe an advice here on how to describe functionally this module and how this is different from packaging type? |
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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
5382722
to
86dea0c
Compare
@bealdav Done |
@rousseldenis Can you update the readme and add the missing link between package type & level. Cf #1215 (review) |
There was a problem hiding this 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.
Currently translated at 96.0% (24 of 25 strings) Translation: product-attribute-15.0/product-attribute-15.0-product_packaging_type Translate-URL: https://translation.odoo-community.org/projects/product-attribute-15-0/product-attribute-15-0-product_packaging_type/ca/
e7d3572
to
a8a6300
Compare
@jbaudoux Done |
a8a6300
to
3d2a27f
Compare
There was a problem hiding this 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
/ocabot merge nobump |
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 |
3d2a27f
to
5901a8a
Compare
/ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 33af21a. Thanks a lot for contributing to OCA. ❤️ |
Following this discussion:
#1213
Depends on: