OCA / community-data-files

https://odoo-community.org/project/tools-maintainers-30
GNU Affero General Public License v3.0
33 stars 171 forks source link

[16.0][MIG] l10n_eu_nace new ShowVoc integration #196

Closed edlopen closed 4 months ago

edlopen commented 5 months ago

Hello you all,

this is a migration and a refactor of the l10n_eu_nace module. Since the RAMON servers from we get te csv data are deprecated this PR contains the functionality to get the data directly from the new ShowVoc web catalogue in the format of a SPARQL endpoint. This get the industries and their translation by a API request and by running a simple wizard.

I would appreciate that this PR is merged rather quickly as it's my intention to migrate similar modules that relays on this new ShowVoc dataset.

Also I included this stale PR's commit.

@moduon MT-1816

@yajo @EmilioPascual @Shide @rafaelbn review this PR when you can.

vincent-hatakeyama commented 5 months ago

I’ve just tested the module.

The wizard need rework. There’s a save/discard button on the bottom and in the centre a big Import button. The import button need to replace the save button, and in the middle a text indicating what would happen would be best.

The import is fast. After import, the list of industries is not shown, so it feels like it did not work. I expected the list of industries to be shown again.

Importing again works as expected. If a language is added, the wizard needs to be run again. It should be indicated in the readme, or automatically done when a new language is activated.

I was also surprised that the module did not run the import during installation.

Odoo SA split the name and full name, only adding the code in the full name. I expected this module to do the same.

The list of industries is also strangely sorted, but I think that has nothing to do with this module.

aronabencherif commented 5 months ago

@edlopen thanks already for this PR. I just reviewed all your code and I honestly admit that yours is much better. However, following the same logic as @vincent-hatakeyama, in Odoo's code base the full name is obtained by combining the code and the name. And what's more, once you have imported the data, you have the impression that nothing happens afterwards, no popup to explain that your code works perfectly :grinning: and no list either to display the imported data. :+1:

edlopen commented 4 months ago

@vincent-hatakeyama @aronabencherif I've updated the wizard and now after installation the import wizard will appear. Also the names and full names are different, with the full names being the code and the nace name. Finally, after running the wizard, the industry tree view is displayed.

I hope this make the module more clear and easy to use. Thank you for your reviews!

OCA-git-bot commented 4 months ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

yajo commented 4 months ago

/ocabot merge patch

OCA-git-bot commented 4 months ago

What a great day to merge this nice PR. Let's do it! Prepared branch 16.0-ocabot-merge-pr-196-by-yajo-bump-patch, awaiting test results.

OCA-git-bot commented 4 months ago

Congratulations, your PR was merged at 86a5f9a9e386f7ff396fe1766e24d3212f225649. Thanks a lot for contributing to OCA. ❤️

edlopen commented 4 months ago

I forgot to add a migration script so that the data is not lost from the res.partner. Currently working to add it in a separate issue and PR.

Thank you all.