-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
[16.0][MIG] product_configurator: Migration to 16.0 #101
[16.0][MIG] product_configurator: Migration to 16.0 #101
Conversation
…tom_type and val_custome_field method
…butes onchange and constraint methods
… other code improvements
…_extra method and write test cases
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 the PR!
I'm just now beginning to work on this module and looks great, I tried it and seems to be working good.
I have reviewed the code and suggested some changes in ursais#3, let me know what you think.
Could you please squash bot commits as explained in https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests? This PR already has a lot of commits.
I see there are also some edits due to #100, could you please remove them? A rebase on 16.0
might be enough since the PR has been merged.
For some reason the maintainer @PCatinean has not been tagged in the PR (doing it now), what do you think of this migration?
EDIT: I also noticed that the changes of #92 are missing, could you please include them?
"field_prefix": "__attribute-", | ||
"custom_field_prefix": "__custom-", | ||
"field_prefix": "__attribute_", | ||
"custom_field_prefix": "__custom_", |
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.
Just curious, why this change? This causes a lots of changes in the tests too
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.
I just took that from the original PR assuming there were Javascript changes that made that necessary
Add dependency on PR OCA#101 for test builds
Thank you @dreispt this actually works well, also @SirAionTech has great improvements would be great if this could be merged in with his suggested adjustments |
Add dependency on PR OCA#101 for test builds
Add dependency on PR OCA#101 for test builds
Add dependency on PR OCA#101 for test builds
Add dependency on PR OCA#101 for test builds
@SirAionTech @dannyadair Proposed changes merged. If I get approval I can go ahead with the merge. |
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.
Thank you @dreispt LGTM
/ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 8c58fbb. Thanks a lot for contributing to OCA. ❤️ |
Closes #96