-
-
Notifications
You must be signed in to change notification settings - Fork 204
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][ADD] add rma_lot #419
Conversation
Hi @chienandalu, @pedrobaeza, |
ba6362d
to
9cf66f4
Compare
9cf66f4
to
ba48b71
Compare
Hi @pedrobaeza, can you please review that. I'm really interested in migrate this module to v17 because i need and it's probably very important for this repo |
Perform a full review and wait for the changes of the author. |
Maybe @sbejaoui it's better to separate the two new modules in two prs |
I added the onchange functions so that the product and lot update when one or the other changes. I don't agree with making the product field readonly |
Thanks but if you choose an lot_id it's link to product and doesn't associate to another. If lot_id it's not filled it's normal to choose product |
What is your opinion in that case @rousseldenis? |
As both modules are new, I would say it's not really annoying. The problem with that is when changes in one module are accepted and not in the other. That couples the PR lifetime to the slowest. @sbejaoui Could you do a PR for rma search view modification as the module version won't be updated when we will merge this (@peluko00 this is the more annoying case) |
And related to the two new modules? |
|
8d59759
to
1b8b16f
Compare
1b8b16f
to
3a71bc9
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.
LGTM, tested in runboat
Seems it's ready to merge @rousseldenis |
This PR has the |
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, tested in runboat and working as expected
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 68d6a10. Thanks a lot for contributing to OCA. ❤️ |
fixes: #416