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 #176

Closed Abranes closed 5 months ago

Abranes commented 10 months ago

Standard migration to 16.0

flotho commented 10 months ago

hum... runboat not available! issued a 404

Abranes commented 10 months ago

hum... runboat not available! issued a 404

It works for me, can you try it one more time?

iziwas commented 10 months ago

Hi,

Thanks for the PR request. Functional tests are OK for me too. However, I have a warning about parent_path field in res.parter.nace : "parent_path field on model 'res.partner.nace' should have unaccent disabled. Add unaccent=False to the field definition."

Is it possible to fix it before please ?

Rad0van commented 9 months ago

After analyzing this module, I think it needs a refactor. Please let me explain.

This module introduces a duplicate structure in Odoo. Odoo CE already has a res.partner.industry model. In OCA, the partner_industry_secondary module implements:

  • Parent-child relationship.
  • Many2many relation between res.partner and res.partner.industry.

So, if this module depends on partner_industry_secondary, which is already migrated to v16, we can drop most of the code. This would just contain the NACE codes in CSV, which would be declared in the manifest and loaded at install; period.

IMHO the migration is probably the best opportunity to do it, as this involves structural changes in the data model. It would mean directly dropping all xml and python code from here, so I think it's a simple refactor that'd be very helpful.

FWIW the only "complex" thing would be a migration script, which wouldn't be very complex anyways.

WDYT?

Looking at the modules I totally agree with you.

pedrobaeza commented 9 months ago

The only problem I see is that the industry model already contains other records that are not NACE ones.

Rad0van commented 9 months ago

The only problem I see is that the industry model already contains other records that are not NACE ones.

Which ones? By quickly looking (at least for v16) it seems right to me...

pedrobaeza commented 9 months ago

I mean these ones:

https://github.com/odoo/odoo/blob/16.0/odoo/addons/base/data/res_partner_data.xml#L57-L160

Rad0van commented 9 months ago

I mean these ones:

https://github.com/odoo/odoo/blob/16.0/odoo/addons/base/data/res_partner_data.xml#L57-L160

Yes, but these are the main existing NACE groups. See https://en.wikipedia.org/wiki/Statistical_Classification_of_Economic_Activities_in_the_European_Community or https://nacev2.com/en

pedrobaeza commented 9 months ago

Ok then. Just a proper mapping should be done for recognizing them as the parents.

yajo commented 9 months ago

partner_industry_secondary makes the parent-child relationship possible, so it shouldn't be a big problem, apart from a relatively simple migration script.

cvinh commented 7 months ago

Hello @Abranes, are you going to change your PR according to comments above ?

rafaelbn commented 6 months ago

/ocabot migration l10n_eu_nace

OCA-git-bot commented 6 months ago

Sorry @rafaelbn you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

edlopen commented 5 months ago

I have an open PR that uses the res.parter.industry as @yajo said here. And also updated the module so that it uses the new ShowVoc Endpoint as the RAMON servers are deprecated.

yajo commented 5 months ago

Thanks! Closing here, as the other PR is more suited for merge.

rafaelbn commented 1 month ago

Migrated in https://github.com/OCA/community-data-files/pull/196