[16.0] revert of #392 and FWP #350

[sbejaoui]

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

[OCA-git-bot]

Hi @pedrobaeza, @chienandalu, some modules you are maintaining are being modified, check this out!

[pedrobaeza]

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).

[mt-software-de]

Thx. This will help a lot. @pedrobaeza i will update today my pr for v14 and then those commits can be used. They will not include any datamodel change.

[etobella]

@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 :thinking:

[pedrobaeza]

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.

[mt-software-de]

@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 🤔

No i will not backport everything. I didn't agreed on this. I agreed on making the needed changes like (not changing reception_move_id) and then revert PR v16 and FW Port v14. Like it is done here.

[mt-software-de]

I updated now https://github.com/OCA/rma/pull/350 @sbejaoui i moved the changes for rma_delivery into a own commit. Could you please do a cherry-pick again?

[victoralmau]

I do not agree with these changes (reverse something merged), other contributors have commented https://github.com/OCA/rma/pull/403#pullrequestreview-2156994950 + https://github.com/OCA/rma/pull/403#issuecomment-2206905849 + https://github.com/OCA/rma/pull/403#issuecomment-2206913838

[pedrobaeza]

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.

[jbaudoux]

@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?

[mt-software-de]

@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.

[pedrobaeza]

@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:

• The datamodel change from m2o to m2m that you already know.
• Some bugfixing that we detected.
• The "coding style" avoiding some over-engineered methods.

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.

[lmignon]

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). It's sad that the original proposal was rejected the first time and that there doesn't seem to have been any willingness to take a constructive approach to improve it. Instead, all we can see is that the work was taken up in another PR, incorporating numerous changes but without ever consulting the original author to find out whether these changes were also compatible with his needs. I think that as a community we have to be attentive to everyone's needs. And if no compromise can be found, there's nothing to stop us forking the module to promote another approach, even if we'd prefer to avoid this type of situation.

[pedrobaeza]

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 Co-Authored-By, as the name that should be there is him, not Michael's). The prepare hooks have been all preserved. And if not, it's as easy as tell us or propose a PR adding such hook with the corresponding reasoning.

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: https://github.com/OCA/rma/pull/350#pullrequestreview-1497227473

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 mis-builder or queue, where you put your own rules that are against the OCA common practices.

[jbaudoux]

I also would like to contribute positively to this rma repository and move forward. :) I find those modules a good base for managing what it serves and I'd like to see some improvements. I'll open several issues so that the features can discussed separately. My focus is currently on v16.0

[rousseldenis]

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.