Skip to content
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

[13.0][REF] base_changeset: remove base model inheritance #2194

Open
wants to merge 1 commit into
base: 13.0
Choose a base branch
from

Conversation

JordiMForgeFlow
Copy link

Using the base model implies adding the changeset fields and methods for all models in the system. This is causing issues when the module is updated, as recorded in #2187.

Usually the practice is not to perform this kind of inheritance, but to create a new Abstract Model to be used to add such functionalities.

This MR intends to perform this modifications so that the models that should be provided with the changeset fields and methods can simply inherit from this new Abstract Model. To do so, new modules can be developed on top of this base module (for example a module purchase_changeset or sale_changeset).

This MR is currently a proposal opened to discussion. The tests have not yet been adapted.

Using the base model implies adding the changeset fields and
methods for all models in the system. This is causing errors
when the module is updated, as recorded in issue OCA#2187.

Usually the practice is not to perform this kind of inheritance,
but to create a new Abstract Model to be used to add such
functionalities.

This MR intends to perform this modifications so that the models that
should be provided with the changeset fields and methods can simply
inherit from this new Abstract Model. To do so, new modules can be
developed for each desired behaviour (for example purchase_changeset,
sale_changeset...).
@JordiMForgeFlow
Copy link
Author

@astirpe
Copy link
Member

astirpe commented Oct 18, 2021

So this way the module become useless by itself, because it will require extra development for each model we want to use this functionality with. This was unwanted in my initial idea of this module.

@JordiMForgeFlow
Copy link
Author

It's true, but inheriting directly from base and adding all the fields for all models seems to cause issues, specially when dealing with DB having lots of models. In other similar cases I have seen that the typical approach is to have a base module adding the core fields and methods in a new abstract model, and then modules are developed on top for the different areas where it could be needed.

I have tried this new approach together with a new purchase_changeset module for purchase related models and worked fine without the issue detected when inheriting from base. I wanted to share it to have opinions on it.

@pedrobaeza
Copy link
Member

One intermediate option can be to attach this feature to mail.thread, that is inherited by most of the models, but not having the issue with the base model.

@JordiMForgeFlow
Copy link
Author

One intermediate option can be to attach this feature to mail.thread, that is inherited by most of the models, but not having the issue with the base model.

I guess this would cover a wide quantity of models, but still models like purchase.order.line or sale.order.line miss that inheritance. At the end, then, we would still need specific modules to leverage the feature in these models.

@astirpe
Copy link
Member

astirpe commented Oct 18, 2021

Attaching this feature to mail.thread would mitigate the requiring of extra development for each model, however I'm afraid there still will be a lot of models excluded, like for example lines (eg.: invoice lines, order lines, ...), configuration records, etc... I'm not against the principle of this PR, I'm just noticing that this PR introduces a regression for the way this module was meant. Is it possible for this module to skip _process_end() somehow?

@JordiMForgeFlow
Copy link
Author

I am not sure on changing such core method behavior...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants