-
-
Notifications
You must be signed in to change notification settings - Fork 205
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] revert of #392 and FWP #350 #403
Conversation
…nsfers" This reverts commit 08a492c.
Hi @pedrobaeza, @chienandalu, |
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.
Sorry, but we don't agree. We are the maintainers of this module, and the way things were done in the other PR are not of our taste, so we have preserved the authorship, but don't want such changes here.
We squashed the commits for avoiding a big diff, as Michael refactor a lot of code to its "style", which is not the styling guidelines applied here, and redo all of this meant a lot of code again. Apart, there were several datamodel changes that were not correct (like changing a m2o to m2m). |
Thx. This will help a lot. |
@sbejaoui Why do you need to revert? I think it was agreed on the v14 PR that the changes of v16 will be backported, not from 14 to 16 🤔 |
We can also backport the changes to v14 if you want. We have done the work on 15, 16 and 17 already, so it's not a problem to do it on one extra version. |
No i will not backport everything. I didn't agreed on this. |
def _prepare_outgoing_procurement_values(self, warehouse=None, scheduled_date=None): | ||
values = self._prepare_procurement_values(warehouse, scheduled_date) | ||
values.update( | ||
{"rma_id": self.id, "route_ids": self.warehouse_id.rma_out_route_id} |
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.
The rma_out_route_id shouldn't be set, or is not needed. The correct route/rule will by selected by odoo default implementation https://github.com/odoo/odoo/blob/11cf3db0b50a70eaa385cd93c0bb6fc51fd9223b/addons/stock/models/stock_rule.py#L513
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.
The current behavior is as expected, the route is used so that the picking_type_id
is correct and the location is used (RMA)
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.
@mt-software-de ,
Without explicitly setting the route, Odoo will return the delivery route as it matches the criteria of the procurement
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.
Ok i was just our use case that we want to route the goods without the rma out route. But for that we can simply set the field to False.
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.
@sbejaoui i will implement it in my commits as well. So you can do the cherry-pick?
I do not agree with these changes (reverse something merged), other contributors have commented #403 (review) + #403 (comment) + #403 (comment) |
We don't want a fight, but you are not respecting our role of both creators and maintainers during a lot of time. You are reverting our work (which has been also a significant amount of time) improving Michael's proposal, and proposing again something that is not completed, even removing our part. We have already explained the reasons for the squashing, as for the blame things, having 2 commits for the same thing doing one thing and the next commit undoing is a mess in our experience. Next time, we will keep it if that bothers you so much, but even the attribution has been respected. Sorry, but I'm closing this. You have our commitment that if something is incorrect, just tell us and we put the patch. |
@mt-software-de Now that we have the diff between what has been integrated in regards of your initial PR, can you point out what, in all this, is really required for your usage? |
For now it will not spend anymore time on contributing to the rma repo. I will have a talk about this internally, how i and my customer want to proceed with this. |
@mt-software-de we are sorry for this dislike, but, what is the problem with the code that has landed in the branch? We have respected all your main stuff, except:
I think the result is better for both. In the v14 branch, that I suppose your paying customer is there, we can have a bit more freedom to merge your PR (but avoiding the datamodel change as well), and when you migrate, everything will be smooth and transparent. |
It's all pretty sad. Without wishing to judge the need to reduce the number of commits in the history or the willingness of everyone to work to improve this module, I am nonetheless concerned about the path that has led to the current situation. From an intellectual point of view, is it normal to modify code in the name of its original author without his consent? Nor have I seen in this whole process any real requests for changes to code that has been deemed over-engineered from some people's point of view. Perhaps this "over-engineered " style also met a need for extension points for particular uses. (Many of us have asked Odoo to modify its code to add extension points. These were also sometimes seen as an unnecessary complexity for them). |
We haven't rejected anything. We have just done the work in later branches, as we needed for that version. We have merged together the work of the other author for the reasons stated, but never trying to make things appear as he is the original one (it's true that my colleague made a mistake putting in the commit message the As maintainers of these modules, we have to be comfortable with the code to maintain, because at the end, people is not willing to maintain it. You are claiming that there hasn't been requests for changing things. That's not really true, as you can see in my first round of requested changes: #350 (review) But it was too much to ask, that we took the direction of letting the v14 branch to him and do the equivalent ones for upper versions, because we also need to move forward with our customers. I agree this is not ideal to have all these factors together, but we are willing to contribute to finish this properly (obviously without reverting). You are the ones that only wants an impossible path. And again, please respect a bit the author & maintainer role we have, as we do with other repos of yours, like |
I also would like to contribute positively to this rma repository and move forward. :) |
I would say, as Jacques-Etienne, the rma repo deserves good contributions (every). The problem is often the form (the way we interact) that can refrain some people to contribute and should be avoided. Let's move forward on more formal things like issues and PR's that will be proposed. |
This PR reverts #392 and forward ports the work of @mt-software-de made in #350 to v16.
The way commits were squashed in #392 made it difficult to follow which parts were taken from the original commit and what was dropped.
cc/ @jbaudoux , @lmignon , @rousseldenis