-
-
Notifications
You must be signed in to change notification settings - Fork 693
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_abc_classification #1192
[MIG][16.0] product_abc_classification #1192
Conversation
e0187f4
to
717381f
Compare
@MiquelRForgeFlow I didn't succeded in getting 13.0 and then applying our changes (too much conflicts). I'll try to identify what can be imported from your version |
/ocabot migration product_abc_classification_base |
717381f
to
7d9031f
Compare
Then the cron classification will proceed to assign to these products one of the profile's levels. | ||
|
||
NOTE: If you profile (or unprofile) a product category, then all its | ||
child categories and products will be profiled (or unprofiled). |
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.
Hi. I don't see this feature in the code. (I was expecting an inheritance of product.category
model) Did I missed something ?
Naive question because I don't use this module for the time being.
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.
@legalsylvain Thanks for this. This is WIP for now, some refactoring before the end of the day :-)
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.
@lmignon Do we agree that with multi level on products, setting the levels on category would be harsh to implement ?
What was done before :
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.
@legalsylvain Thanks for this. This is WIP for now, some refactoring before the end of the day :-)
No problem !
Note : you can use "Draft PR" if it's WIP.
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.
@legalsylvain Yes I know, I usually use it when process would last a long time. Here, I knew this would be ready fast
072e0ff
to
b56a5bf
Compare
/ocabot migration product_abc_classification |
7432648
to
340dffd
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.
Thank you @rousseldenis. LGTM (Code + Functional tests with #1196 )
1a0ea49
to
cc97df9
Compare
@rousseldenis What's your roadmap in this regard? As I see from the commit history, right the v13.0 code is just discarded in favour of the one coming from v10.0 |
Indeed as the rebasing was a nightmare. This approach has been improved in some production environments, the essential difference is the multi relationship between products and profiles/levels. As the commits introduced in v13 have no sense with this approach, I haven't taken them apart an improvement on view level by @MiquelRForgeFlow (see PR comment) |
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, code review
I will add tabs as extending the module like in : OCA/stock-logistics-reporting#257 introduce some complexity in the form view. |
d804a71
to
6403c05
Compare
…age on product, level_id, flag, ...)
6403c05
to
81726cf
Compare
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
@rousseldenis your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1192-by-rousseldenis-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
648cf39
to
70beefc
Compare
/ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 6e29ef8. Thanks a lot for contributing to OCA. ❤️ |
Forwardported:
As this is a base module that is intended to be extended by adding profile types to ABC Classification profiles (and corresponding logic), this cannot be tested as is.
You should try this : #1196