-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
[14.0][IMP] RMA - refactor to use procurement run #350
[14.0][IMP] RMA - refactor to use procurement run #350
Conversation
Hi @chienandalu, @ernestotejeda, |
fd355df
to
e2ab2a3
Compare
FYI: @jbaudoux |
@pedrobaeza and @chienandalu i will redo the PR #334 |
e2ab2a3
to
2be82ae
Compare
cc72bf1
to
2d2a3b4
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.
Thanks
This approach is a lot better. Routes are now properly evaluated.
8b06779
to
e92a980
Compare
ddda60e
to
35f0b4f
Compare
90964e0
to
3c2c3e6
Compare
Sorry, but the modules are right now independent and we don't want to depend on other modules. The feature is interesting, but adding such dependencies not. I don't think the amount of code to repeat is very high. |
stock_helper is in fact not needed, but |
A good answer would explain why 😃 |
It's not the first time I have answered you about this. And you ask us about doing glue modules for certain things, but now want the reverse. We don't want to have to add extra dependencies in all of our customers, and more being for such an old version. Such dependencies implies more code to maintain in our codebase, possible interactions in integration tests, etc. And they are not really adding value to us. Extra glue modules can be added for such needs.
You should add that in a glue module IMO. |
Hmmm. To make this work we need to remove the carrier_id from the proc group and set it like it was done (just with a write) I don't think that this would be a good approach. |
3c2c3e6
to
ac67a3f
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.
Reviewed the last commit
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.
Finally merged in v16 #392 with some changes (e.g. Change reception_move_ids
to reception_move_id
), I think PR should be adapted to the same changes.
Hi @ernestotejeda, @chienandalu, |
Thats good, that there is now finally some progress. May i raise some issue i have with your approach about how pushed an merged your PR through. Why was this PR blocked for so long and your PR was pushed through i don't think that was a convenient way of doing it. |
I am trying to clarify everything a bit. This was left pending from #350 (review) until now (otherwise this would have been taken to later versions and adapted as necessary) in order to start with the revision. These changes were done in v15 (indicating it was a 14.0 FWP) #384 keeping the commit of yours and then adding extra ones with the changes. Finally it was needed already in v16 and that's why #392 was done (there everything was simplified in 1 commit adding you as co-author to avoid that the diff could be reviewed in addition to other necessary changes and reduce the methods that were not necessary). If there are things that you think you need to change I think it is best to do it in v16 and when everything is ok continue with backports (if you need them). |
Yes i have seen what you have done, but exactly thats the point, which i think is not convenient. The annoying thing for me is right now, that i have now more worked to do. Somehow i really don't know why this one was blocked. |
@mt-software-de IMO it could be easier to revert the commit in V16 and reapply yours. Do you plan to make a PR for reintroducing the missing pieces in V16? If not, we can do it on our own while adding any relevant improvements from 2179cc5 and the test from 9bbe997 |
No i am not planing to do it. I also think implementing the relevant improvements here and the revert the changes would be the easiest way. |
bd14bcb
to
5d73971
Compare
5d73971
to
ba06233
Compare
@sbejaoui could you please review this PR. |
@jbaudoux @rousseldenis re-review this? It would be nice to merge this finally and then FW Port it do v16. #403 |
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 PR replaces all lines where a stock.picking or stock.move is created, by using only procurement group runs.
This helps the RMA modules to easier interact with other stock modules like https://github.com/OCA/wms/tree/14.0/stock_warehouse_flow
Depends on:
OCA/stock-logistics-warehouse#1708#351