OCA / server-brand

GNU Affero General Public License v3.0
58 stars 150 forks source link

[12.0][MIG] res_config_settings_enterprise_remove #7

Closed astirpe closed 5 years ago

astirpe commented 6 years ago

https://github.com/OCA/server-brand/issues/6

eLBati commented 6 years ago

Hi @astirpe , could you also add this https://github.com/OCA/server-brand/pull/8/commits/a017bc5aed10f92933f37d9399e53a673a22081a ?

pedrobaeza commented 6 years ago

The problem with <delete> records is that they give a warning when the record is not found, so updating 2 times the module results in an ugly log, but right now I can't think in a better solution.

astirpe commented 6 years ago

@eLBati @pedrobaeza as stated in the description on the readme, this module is not meant to delete the enterprise modules from the modules list. It's only meant to remove them from the config settings view. I would rather create a separate module for that. Or discuss the possibility to modify this module, but in a separate PR. I honestly don't like to introduce warnings in the log when doing an update all. So if it's OK for you, I would keep this PR as it is: a pure porting of the module.

pedrobaeza commented 6 years ago

OK, let's keep this PR as is, but introduce later that.

eLBati commented 6 years ago

Ok

levkar commented 6 years ago

I made a simple 12.0 migration PR https://github.com/OCA/server-brand/pull/11 before i noticed this PR.

Here is the way to avoid warning messages:

``

``

But the name of the module ("res_config_settings_enterprise_remove") can be misleading. Maybe it should be another module named "module_enterprise_remove" ? Or a new module named "enterprise_remove" can be introduced to include both functionalities. I can cancel my PR and/or help with a new PR. Let me know what you think.

One last question, why don't we have modules in this repo with auto_install=True?

astirpe commented 6 years ago

@levkar thank you for the tip! I agree with you, an extra module is needed. Or an alternative "enterprise_remove" module would be great, but this solution will prevent the user to install enterprise modules! Both proposals are fine to me. Would you like to go ahead and open a PR? Let me know...

levkar commented 5 years ago

@astirpe I created a new pull request #12.

astirpe commented 5 years ago

Closing in favour of #12