[16.0][FIX] mass_edit: Play onchanges before writing

grindtildeath commented 3 weeks ago

@pedrobaeza I did consider doing another module, however:

Not having the onchange functions being played could be considered as a bug, and that's why IMO it must be included in the module and not through an extension.
As there is no module maturity set in manifest, it must be considered as Beta, and such a change is then allowed

I understand it can be annoying having an added dependency, but IMO there's a real benefit to it here.

pedrobaeza commented 3 weeks ago

No, it's not a bug, as you want a lot of times to not execute onchanges. Moreover, that onchanges can have drastic consequences. You have put it as an option in the action itself, as you are aware of this is not something that everyone wants.

I stay on not wanting this to be required for all the installed base.

grindtildeath commented 3 weeks ago

@pedrobaeza

I'm aware we don't necessarily want onchanges to be played all the time and that's why I implemented it this way.

But please try not being so categorical in your takes when, I assume, you know it depends on the context and there's no clear-cut definition of what is a bug, especially here, as it depends on the use case, and that's conditional on how the server action is defined and what it is doing.

Again, since this module doesn't have a maturity level set to stable or mature, there is nothing preventing adding a module dependency to it. IMO reimplementing the code from onchange_helper in this module or creating an extra module for this does not make much sense versus making sure the dependency is installed when upgrading the source code.

@hbrunn @legalsylvain @rousseldenis @ivantodorovich @victoralmau @OCA/tools-maintainers Opinions welcome :pray:

pedrobaeza commented 3 weeks ago

This module is clearly mature, but not set explicitly for to nobody putting that. Anyway, the rule is to not add any extra dependency on an already migrated module until it's something critical and unavoidable, which is not the case here, as you can clearly add this feature as a new module on top of the existing one.

hbrunn commented 3 weeks ago

I'm not aware of https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/oca_module_lifecycle_development_status.rst saying anything about dependencies, and in practice, most deployments will handle this just fine.

For merging this I would however want to see tests and documentation.

pedrobaeza commented 3 weeks ago

@hbrunn it's an issue if in your deployments, you have restricted the available modules, and anyway, when the business model is based on the installed modules (like mine), you don't want to have extra modules to support "for free" with no need.

And again, technically there's no problem in have the other module on top of this one.

rousseldenis commented 3 weeks ago

@hbrunn it's an issue if in your deployments, you have restricted the available modules, and anyway, when the business model is based on the installed modules (like mine), you don't want to have extra modules to support "for free" with no need.

I would say (as already said elsewhere) OCA is not a place where business requirements and technical limitations you put on your side should have influence on improvements on this.

We have major/minor/patch versioning on the whole OCA flows, I would say you can take care on your side which version you install.

This is a barrier with no real arguments (apart those you think important for your business but none for OCA improvements) on contributions.

Please don't blame people that do not follow your own policy.

@OCA/board this is a recurrent argument on how to improve implementations.

@pedrobaeza this is something that I'm still reluctant to and I open (again 😅 ) the debate because I really think that this is important from a community point of view.

pedrobaeza commented 3 weeks ago

We contribute because we do business, so yes, business is important here, and there are no technical barriers to split this feature into the new module, so there are no reasons to do it this way and anger good contributors.

grindtildeath commented 3 weeks ago

We contribute because we do business, so yes, business is important here, and there are no technical barriers to split this feature into the new module, so there are no reasons to do it this way and anger good contributors.

@pedrobaeza

Don't you realize you are actually the one angering good contributors with your posting? I guess most of the contributors are doing business or wouldn't dare contributing at all, so please stop your with your ranting. We did understand your position, there's no need to keep being annoying.

pedrobaeza commented 3 weeks ago

I'm not ranting. I'm opposing to this change as is, and reasoning why, as you can change easily the approach creating an extra module on top.

etobella commented 3 weeks ago

Just to be clear, I am just giving my own opinion on this topic.

I think that adding not needed dependencies is always a bad option for several reasons:

It requires to modify your instances and add new modules. Automatic upgrades might fall
It goes against the idea of mature modules. It is not explicit, but mature modules shouldn't have breaking changes if not needed (changing dependencies is breaking)
We are adding some extra logic that might be interesting but it is not a real requirement for the original module

In this case, it is clear that we have a module that has been there several versions (11-15 it was mass_editing and renamed on 16 with a loss of commit history) and fulfills all the criteria to be considered mature. Personally, I would prefer to have two modules, the original one and a new one with the new functionality (after a fast code review, it is not too hard to split the logic).

We should remember that one of the greatest things of Odoo is the modularity. I don't see a problem on using it in our own benefit.

legalsylvain commented 3 weeks ago

Hi @grindtildeath. First thanks for your contribution ! whatever the final design (a new module, or the current patch), it's a great addition IMO ! Thanks for pinging me. (I worked on a refactoring of this module some years ago : https://github.com/OCA/server-ux/pull/94 and this module is very important for my company)

There is a lot of topics here, but before making a complete answer, I have a naive question :

Pedro said : No, it's not a bug, as you want a lot of times to not execute onchanges. Moreover, that onchanges can have drastic consequences. You have put it as an option in the action itself, as you are aware of this is not something that everyone wants.

You said : I'm aware we don't necessarily want onchanges to be played all the time and that's why I implemented it this way.

My naive question : in which use case you don't want onchange to be played ? If I take a very basic example : create a mass edition with the following configuration model : res.partner ; field state_id. If a user changes the state of many partners, he allways expects that onchange to be executed (https://github.com/odoo/odoo/blob/16.0/odoo/addons/base/models/res_partner.py#L499) Don't you think ?

Thanks for your answers.

rousseldenis commented 3 weeks ago

It requires to modify your instances and add new modules.

Of course, but in the case you upgrade the module version you want.

Automatic upgrades might fall

I don't catch what you want to say.

It goes against the idea of mature modules. It is not explicit, but mature modules shouldn't have breaking changes if not needed

(changing dependencies is breaking)

I suggest you to test this tool which is really great :sweat_smile: that allows to upgrade a module and get dependencies aligned. https://pypi.org/project/pip-deepfreeze/

I would say (as here above) we have major/minor/patch versioning for OCA modules (that are python packages in fact). The guideline is always to mark major version changes as breaking change. That allows to benefit from improvements on a module you want to upgrade or keep the previous version.

We are adding some extra logic that might be interesting but it is not a real requirement for the original module

This has to be clarified as @legalsylvain said, in which case don't we want to trigger the onchanges ??

etobella commented 3 weeks ago

I suggest you to test this tool which is really great 😅 that allows to upgrade a module and get dependencies aligned. https://pypi.org/project/pip-deepfreeze/

I think that one of the greatest things of this community is that we are proposing modules, not a way of managing our companies. I know that Tecnativa is using Doodba, Akretion have their own odoo docker and probably the same for Acsone. Right now, we have several options to deploy and manage your instances and some might be compatible with your proposal and some not. Personally, I don't like the idea of deepfreeze, I prefer to update with the last version in order to avoid regressions and issues (that is the idea of CI/CD). I think that there are room for all options if we keep the things ordered.

I would say (as here above) we have major/minor/patch versioning for OCA modules (that are python packages in fact). The guideline is always to mark major version changes as breaking change. That allows to benefit from improvements on a module you want to upgrade or keep the previous version.

But as PSCs we need to check if the major change is really required and have sense. In this case we have a way of doing it without breaking so many things.

We are adding some extra logic that might be interesting but it is not a real requirement for the original module

This has to be clarified as @legalsylvain said, in which case don't we want to trigger the onchanges ??

In any case, it is not a problem on splitting the functionality because the harm doesn't fill the benefits IMO.

ValentinVinagre commented 3 weeks ago

Hello everyone, After reading all the comments, it can be summarized in 2 points:

Extension of the base module: As indicated, it is not a FIX, it would be an extension of the module. It is true that a dependency would have to be added, but the functionality would have to be included in the base. I understand that it may be "work" to add this dependency to the clients in production, but then this change can be easily transferred to future versions.
Create an extension module: The problem is that this point is food for today, hunger for tomorrow. Doing this option gives an easy introduction of the change, but it will have to be added in future migrations in the base module, on the other hand, a migration script will also have to be created.

Both solutions are correct, the only debate in my opinion is the impact on the clients that are in production. If we look at it based on business, it is better to do it in a separate module. If we look at it based on OCA, it is better to do it in the same module.

That is my opinion, both options seem correct to me. This is where the PSCs come in and decide the best way to implement the changes.

Greetings,

grindtildeath commented 3 weeks ago

Thanks everyone for chiming in.

It goes against the idea of mature modules. It is not explicit, but mature modules shouldn't have breaking changes if not needed (changing dependencies is breaking)

Actually, this is not a breaking change. If you update the code of this module with this PR and have onchange_helper installed or available to install, you won't notice that anything changed in what server_action_mass_edit is doing. A breaking change would have been to introduce the onchange without the ability to deactivate it. Moreover, it is needed in cases where the logic depends on an onchange function. I agree it might not be needed all the time, but if you take an update of country_id or state_id on res.partner for example as suggested, then it's something that is needed.

We are adding some extra logic that might be interesting but it is not a real requirement for the original module

Again, it depends on the use case. From my POV and for what I'm trying to solve here, same as for the example I just mentioned, it is a requirement to have the onchange played to avoid having records with inconsistent data.

My naive question : in which use case you don't want onchange to be played ?

I'm not sure, but I guess it could make sense to deactivate it so that a single write can be done instead of having to loop over the records and write. It's more about performance than logic, because IMO from a logical perspective onchange need to be considered anyway. That's why I left the door open to avoid introducing a breaking change in how the module is working and be nice with existing implementations.

Finally, IMO it doesn't make sense to have a separated module for what is part of the same scope, and what can be considered a bug in some cases. As it was just said, this feature would have to be integrated in this module anyway in another version. So why not having a better module in the first place? If the way some people are deploying their projects cannot handle such a change in the dependencies, IMO it's not a reason to reject contributions.

Now, I spent almost 2 hours arguing here for a change that is providing a fix (that I agree might not be required in some cases but is still a fix). I won't spend any more time because this is going nowhere as people are entitled to their opinions and some favour avoiding extra work on their own deployment instead of having a more robust module in the OCA.

etobella commented 3 weeks ago

@grindtildeath it is breaking because you need to have the module there. Otherwise, everything will start to fail.

It is like adding a new library dependency. It is breaking, because you don't know if the instance has the library there.

We are not against the idea of the module, we are just asking for a different approach. I don't see the problem on that. I have done a lot of changes like this in my history inside OCA.

dreispt commented 3 weeks ago

I reviewed the code but did not read the discussion (sorry). I dislike adding dependencies, and it will break existing installations on code update. I also not sure about the use case being solved here, seems like it is to avoid a known limitation of onchanges, that today is mostly solved through computed fields.

In my opinion, this should be in a separate module, extending mass_edit. If you need it, you install (and maintain) it. If you don't need it, you're also fine.

grindtildeath commented 3 weeks ago

FWIW it already happened in the past that Pedro complained about introducing breaking changes in a released version of a module and blocked the PR before they (Tecnativa) did it their way when they needed it. :roll_eyes: cf https://github.com/OCA/stock-logistics-workflow/pull/1010 and https://github.com/OCA/stock-logistics-workflow/pull/1135

pedrobaeza commented 3 weeks ago

Yes, and how this compares to the current case?

Starting with, the PR you are referring to, in the state where I blocked it, contained potential harmful DB schema changes that I extendedly reasoned about on:

https://github.com/OCA/stock-logistics-workflow/pull/816#issuecomment-806443777

To propose to create a new module was for not blocking at all the contribution, so both gets happy: you having resolved your problem, and me, not suffering the technical problems that the PR was bringing.

I asked my colleague @CarlosRoca13 to review it anyway in https://github.com/OCA/stock-logistics-workflow/pull/1010#issuecomment-1309511131, but he seems to not remember about it.

And then came with a solution with no DB schema change when we faced the same problem in https://github.com/OCA/stock-logistics-workflow/pull/1135, with no dark intentions. But obviously, not having conflicts nor debatable breaking changes, it was merged following usual review mechanism, including an approval from other person outside Tecnativa.

mmequignon commented 3 weeks ago

As indicated, it is not a FIX, it would be an extension of the module.

I'm not sure to agree with this. I'd rather disable the onchanges with an option than having to enable them, as I'm guessing the cases where we do not want them are rarer. IMO onchanges should be played by default.

Having this option enabling the onchanges and defaulting to False is a good compromise I think, which will avoid disrupting the behavior for partners that are happy with this.

If you don't want it, you don't configure it, and we're good.

legalsylvain commented 3 weeks ago

Hi all.

Well. What a debate ! In my point of view :

On one side, in some comments, this debate is starting to go off in all directions,
In the other hand, certain subjects need to be discussed elsewhere as said by @ivantodorovich,
Also I find that some of the comments are a bit harsh. Starting by saying ‘thank you for your contribution’ when someone takes the time to suggest an improvement to an OCA module should be the rule. Especially when the contributor is asked to refactor their entire commit by creating a new module ! Moving on. I understand that we go straight to the point when we're reviewing a lot of PR, but I think we have to be careful. I'm not blaming anyone. It's just a point of attention.

There are basically two questions here : 1) A "Devops question": Is it allowed to add new module in the 'depends' key of a manifest of an existing OCA module that has been released, that is quite mature and that is currently used in production on many instances ? 2) A "Design question": In absolute terms (= in a design point of view), should this modification be in server_action_mass_edit module (current proposal) or in a new addon ? (something like server_action_mass_edit_onchange).

Then, here are the possible futures, once we've answered these two questions. (I hope this code is understandable)

If answer_of_question_2 == "server_action_mass_edit":
    If answer_of_question 1 == "add_new_dependency_allowed": 
        # Merge this PR.
    else:
        # A) Introduce this improvment in a not merged version of server_action_mass_edit
        #     (version 18 is not merged : https://github.com/OCA/server-ux/pull/951)
        # B) (Option) Develop a temporary module named server_action_mass_edit_onchange
        #     that will be marked as merged in server_action_mass_edit in version 18.0,
        #     in the apriori.py file of openUpgrade project
        # (Alternatively) let this PR opened, and people can use if
        #     they want, and the feature will be native in the V18 module.
else:
    # Close this PR and make a new PR with a dedicated module server_action_mass_edit_onchange

I think question 1 has not be debated here. As said by @hbrunn in this comment and @ivantodorovich here, there is actually no OCA rule that forbid such changes. This question is not related to the current PR (it has been said in other OCA PRs). I created a dedicated issue on https://github.com/OCA/odoo-community.org repo. So please, regarding that topic, please comment and argue over here.

Question 2 has to be debated here.

On that point, I'm in favor to include this feature in the core module. (now, or in version 18, depending on the answer of the question 1).

Pro

This is totally within the scope of the module.
I think that admin user that configures a server action should ask himself whether he wants to launch the onchange or not: Disable that option can generate inconsistent data vs make a mass update on a huge amount of item will take more time if onchange option is checked.
If an important feature is in an extra modules, its distribution will be much weaker. A lot of people just won't know that there's a new module introducing new option that should be interesting. Remember that there are around 4,000 OCA modules. How to find out about the existence of a new peripheral module hidden in this mass ?
Port two modules costs more time that port one module. The current OCA rule is to make a PR per module. if an extra module exists, it will require to port first the main module, then make a second PR, add a reference to the first one. Once the first PR is merged, go to the second one and remove the temporary commit that add a reference to the first PR... That's basically more time.
Excluding tests and manifest file, the current module contains 326 python lines of code. The version of the module modified by grindtildeath contains 365 lines of code. (+39 lines / +11%). This does not significantly increase the complexity or maintenance of this module.
I asked myself the theoretical question: _"If this module didn't exist and was proposed today with a dependency on onchange_helper, would I think there was a problem, and would I ask the contributor to create 2 modules?"_ I don't think so. Would you ?
Since recent Odoo version (at least V16) there is "basic" mass editing feature in Odoo. It is enabled in tree view, if the option multi_edit="1" is set. Go in Contact > display tree view > select many items and click on a field. you can for exemple change an email and the new value will be applied to all the selected items.

I therefore wondered how Odoo had managed this onchange ‘subject’ ! For that, show optional hidden fields state_id and country_id and try to change the country (US -> BE for exemple) to see if the state is reset. Well, it is not allowed, because that field are readonly in the tree view. See tree view definition. The reason is very well explained in that commit. If I can summary, Odoo SA considers that mass editing without running onchange should be prohibited.

Cons

Excluding possible deployment issues, the only argument ‘against’ this change that I see in the comments is: "Personally, I'm not going to use this new feature. As I don't want to complicate my technical stack, I prefer two modules". As it is implemented as an option, I don't think that's an argument that outweighs all the other disadvantages of having 2 modules.

Please complete or reword if I don't have all the arguments.

Thanks for reading !

hbrunn commented 3 weeks ago

how about a compromise in the spirit of graceful degradation?

in ir_actions_server.py:


has_onchange_helper = fields.Boolean(compute=lambda self: [
    this.update(dict(has_onchange_helper=hasattr(self, 'play_onchanges'))) for this in self
])

in ir_actions_server.xml:

<field name="has_onchange_helper" invisible="True" />
<field name="apply_onchanges" attrs="{'invisible': [('has_onchange_helper', '=', False)]}" />

in tests/test_mass_editing.py:

if not hasattr(self, 'play_onchanges'):
    return

and lose the hard dependency? The README then should talk about the feature and pros and cons anyways, and there mention it's only available with onchange_helper installed.

rousseldenis commented 3 weeks ago

@hbrunn IMHO such approaches should be avoided as adding code for things the module 'should' depend on is bad. My two cents.

pedrobaeza commented 3 weeks ago

Please read https://github.com/OCA/odoo-community.org/issues/193#issuecomment-2448846096, which expresses clearly right now how I feel.

simahawk commented 3 weeks ago

This is a bug and should be fixed. Yet, by experience, running onchanges might have unexpected result, especially when a module has been used as is since 2 years. Sure, this is solved by the flag.

Personally I don't see any issue w/ adding a dep. However, saying that we can use "major" version won't solve the problem but it will introduce a new one. Since we don't have 1 repo per module, we cannot maintain 2 versions of a module on the same odoo version. Which means that ppl not willing/able to introduce the dep won't be able to contribute back on the former version. Correct me if I'm wrong.

All in all, assuming that we have proper hooks in place I'm favor of the less impacting option of the glue module. I would mention the bug in the readme and point to this new module as a solution.

Finally, I'm not an author nor a maintainer of the module and IMO the final word belongs to them. If any of them is against the change we have to respect their will. This is important.

V18 can have a different future, for sure.

ivantodorovich commented 3 weeks ago

The purpose of onchange_helper is to re-use these ~70 lines of code : https://github.com/OCA/server-tools/blob/16.0/onchange_helper/models/base.py

This module was created to avoid code repetition, but it is not strictly needed.

Thus, I propose a middle ground approach: copy & paste the play_onchanges method here, adding a roadmap item and a "TODO" comment to remove this code by replacing it with the onchange_helper dependency in a future migration. This way we can have the feature/fix into the module without the extra dependency.

simahawk commented 3 weeks ago

Thus, I propose a middle ground approach: copy & paste the play_onchanges method here,

I thought about that, indeed... but I probably know the answers :wink:

pedrobaeza commented 3 weeks ago

FWIW, I still don't want such feature in the base module. It's to give more fuel to my users to screw up database. That's why I also insist on not having this feature here in the base module.

It's not a bug, it's a new feature. If not, it's not understandable that this "bug" has been this way during 8 versions. And as Daniel said, there are less onchanges now with the computed writable fields. Computed functions are more solid also, as they usually take into account the states and other values. Onchanges, being for the manual data entry in screen, are like Attila, replacing values with no mercy. That's why applying onchanges is so risky.

legalsylvain commented 3 weeks ago

Onchanges, being for the manual data entry in screen, are like Attila, replacing values with no mercy. That's why applying onchanges is so risky.

Thank you for your clarifications @pedrobaeza. Could you elaborate? Provide a use case where applying onchange would be dangerous? I can't picture the situation.

Whatever the design selected, we need to write a correct helper on the new boolean field, that explain the impact of this setting. (For the time being, I have in mind : unchecked : risk of loss of data integrity (state_id / country_id example). Checked : slows down if the item selection is large.)

Thanks!

pedrobaeza commented 3 weeks ago

Sorry, I'm not investing more time in this. If you don't consider my judgment and do something very simple, which is to have 2 modules, go ahead. I will do what I consider then.

legalsylvain commented 3 weeks ago

Sorry, I'm not investing more time in this.

No please. You just said it was risky here. And when I ask for clarification, because I don't understand the risk, you stop answer. As an experienced developer, your opinion is precious. So please provide the details so that everyone here understands what's at stake.

HaraldPanten commented 3 weeks ago

Hi guys,

First of all I would like to thank @grindtildeath for his contribution. I think that this is an interesting feature that might be useful for a lot of users/partners. For us, as well.

I've read all the conversation, and I'd like to ask you something. (Please, remember that mine is not a technical POV, but I understand Odoo and OCA ecosystem perfectly).

Shouldn't 2 separate modules satisfy both parts?

IMO, modules should be improved once there's agreement between all the "stakeholders". In the OCA ecosystem these "stakeholders" would be PSCs or usual contributors, for example.

In case we can't find that agreement, I think that the less "harmful" decision is to separate this functionality. We have faced this situation lots of times, as well, and it hasn't been a big problem for us.

Far from supporting one, or the other side, I think that we should invest our efforts in collaborate instead of "fighting".

Personally, I appreciate all the good contributors in the OCA (and I know some of you in person). In this conversation, we've brought together the input from partners who probably carry out more than 50% of all OCA maintenance. Why are we arguing? We all support each other.

My 2 cents.

OCA / server-ux

[16.0][FIX] mass_edit: Play onchanges before writing #963

Pro

Cons