[MIG][10.0] Migrate 'res_config_settings_enterprise_remove'

[mstuttgart]

• Migrate res_config_settings_enterprise_remove module to Odoo 10.0 api.
• Adjust in code format

[yajo]

This way of importing is weird... why not use _inherit?

[mstuttgart]

I only migrate the original code. But I changed it to use inherit

[mstuttgart]

@yajo code updated with inherit.

[lasley]

LGTM thanks @mstuttgart

[sebalix]

Tested on runbot (login : admin // H463zbquEe}5 ), two things:

• The group title of Enterprise features is not hidden on some configuration panels (e.g. Shipping Connectors and Extra Features titles on the Inventory config panel are visible), but I don't know if it was already the case on 9.0...
• all Enterprise features are visible on the Manufacturing config panel. Is there a way to handle these Enterprise options properly?

So it is 👎 for me as the module does not act as expected.

[mstuttgart]

@sebalix fixed.

[lasley]

@sebalix - regarding a global handling of the Enterprise options, we looked into this. This was as good as it got in terms of scalability unfortunately. The history of the endeavor is in #499

Side note - I admittedly only code reviewed. I just tried Runbot and it had a changed pass, so I'm rebuilding and will do a functional test as well.

[lasley]

I just went through all the menus of all the apps, looks like we got them all now after that last review round! 👍

[sebalix]

Cool, thank you @mstuttgart for fixing this.
It remains the license header in the README, is there any reason to remove it?

[pedrobaeza]

Working as expected. I would expect some tests for avoiding the coverage drop.

[mstuttgart]

@sebalix thks. I remove the license icon because it not be loaded in module description of Odoo module list screen (though the image link was corrent). The second reason is improve appearance of README.

[lasley]

I remove the license icon because it not be loaded in module description of Odoo

Looks fine on my local & on the v9 Runbot. What error was appearing in web developer console?

The second reason is improve appearance of README.

This badge is part of the OCA template, which should be adhered to. If you have suggestions on the appearance, that change should be submitted to the template instead please. In this specific instance, I think it's there for a reason - but that discussion is out of scope for this PR IMO.

[sebalix]

@mstuttgart Thank you for the migration. About the icon, you're right it seems the license image is not displayed. But I prefer to not block this PR for that (maybe this issue impacts other modules, and if we find the root cause this will be fixed in another PR).
👍

[sebalix]

This header is part of the OCA template (see https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst), you should not remove it.

[lasley]

Please revert this change, as @sebalix mentioned this is part of the OCA template - https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst

[pedrobaeza]

Why the coverage drop? Can you add tests?

[mstuttgart]

@pedrobaeza this original module not has tests. I can add it, but not in this moment.

[pedrobaeza]

OK, I prefer to wait and complete the module with full coverage as this has no rush to be merged.

[lasley]

@mstuttgart why close? IMO it would be better to let us know that you don't want to add the tests & we could decide if we should merge or if someone wants to take over the PR. Wasting code/work sucks 😦

[mstuttgart]

@lasley sorry guys, my mistake. :D
I use this module in production enviroment and it is work. The original module don't have tests, then I add it in another PR, but not is this moment (it is not priority for me).

[lasley]

@pedrobaeza - The testing of this module presents a unique situation due to the fact that this module does not depend on the modules it edits.

Adding the dependencies would mean that anyone wanting to use this module has to install a lot of modules that are potentially not needed. We originally submitted something like 8 modules to edit each view independently, but this more abstract approach was preferred.

This module came from my side of the house & we originally made an exception due to the complications of the module dependencies above.

If you would like, I will commit to applying some mock wizardry so that we have tests here. It would be a false green though because we would just be testing that the xpath was called with the queries, instead of whether they actually work.

Alternatively we could create a test_ module, but I find that pretty dirty. Maybe less dirty than no tests though - I'm torn.

[lasley]

Please revert this change, as @sebalix mentioned this is part of the OCA template - https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst

[mstuttgart]

@lasley fixed. The icon is back (commit rebased)

[lasley]

PSC - hold merge for @pedrobaeza response to #665 (review)

[pedrobaeza]

Hi, @lasley, the testing can consist in some mocking, or even simpler, directly create a fake XML view with the looked elements to see if the result removes that elements. We are not testing the real scenario, but it's valid for sure and I don't think this is going to fail in real scenarios in the future if the test passes.

[lasley]

directly create a fake XML view with the looked elements to see if the result removes that elements.

Ah yes, better. But still we're just testing the implementation of it on the current version though, so we're going to end up needing to change our tests each iteration forward & could create some ambiguity for someone upgrading the module due to the tests passing but the code being non-functional in context.

Hrmmmmm so what about if we directly access the view files from the existing Odoo installation in the tests, then feed the data directly to the method? We could then evaluate the output to make sure that no enterprise classes exist.

I think this would provide us our failing build if need be during forward migrations for checking if the EE class exists at all, vs checking whether we're removing the specific elements.

The one thing we would be missing is if new config pages are added that contain the class. I think that's fairly reasonable downfall though, and not worth the effort of figuring out.

Sounds like this may be plausible?

[pedrobaeza]

OK, look, this is better: create a demo view with this widget in the module and test it against it!

[lasley]

@BMW95 - you have any thoughts or insight to these testing proposals?

[woodbrettm]

hmmm, well I guess it depends whether people are really adding enterprise-only features to the community platform or not. I doubt they would be, but I haven't had a ton of exposure to a huge amount of modules so not sure where the truth in that lies.

I also took a look at several of the config views in the odoo repo, and they seem to be the same or at least pretty close to 9.0. Change seems infrequent within the same version as well.

With that I'd say it's probably safe to just copy the account config view and store it in a test file to test against. Then just recheck everything from v10 to v11, v12, etc.

As per @sebalix's comment.

The group title of Enterprise features is not hidden on some configuration panels (e.g. Shipping Connectors and Extra Features titles on the Inventory config panel are visible), but I don't know if it was already the case on 9.0...
all Enterprise features are visible on the Manufacturing config panel. Is there a way to handle these Enterprise options properly?

Just as a note, those titles were specifically requested be kept in place in 9.0 in case other developers were adding features under those headers, currently, or for future purposes. Would that argument still be valid?

Also thanks for pointing out the issues with MRP. In 9.0 we were looking for upgrade_boolean widget fields, but it seems perhaps that the widget="upgrade_radio" is for the same thing? If so, there's one in website as well: https://github.com/odoo/odoo/blob/2013dc387d68d7c13abde30acb51d7a33ba4b89f/addons/website/views/res_config_views.xml#L114

Anyways those are my $0.02 :)

[lasley]

Just as a note, those titles were specifically requested be kept in place in 9.0 in case other developers were adding features under those headers, currently, or for future purposes. Would that argument still be valid?

Yup we inject into the Shipping connectors section in our connector-easypost, for example. I guess we could add some internal switches to enable/disable the headers, then have a module change it in order to inject, but then that would create a circular dependency. Rock and a hard place, really.

[ghost]

Forget Remove Enterprise Fields Project settings Forecasts, planning and Gantt charts

[mstuttgart]

@neps1192 Project settings fixed

[ghost]

mstuttgart Thank you for this work