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

[14.0][MIG] base_location: Migration to 14.0 #1003

Merged
merged 40 commits into from
Nov 30, 2020

Conversation

pedrobaeza
Copy link
Member

  • Standard procedure
  • invisible > attrs in view
  • Changed method

@Tecnativa TT26286

nbessi and others added 30 commits October 18, 2020 12:46
…some views. Added two columns in respective tree views.
…ooks

* Added Icon.
* Improve module description and extracted to README.rst.
* Pass country instead of country_id for advance comparisons.
* Allow to transform city name.
* Some code style.
* Do not remove all entries of a country, but only not found.
* Include hooks for transforming some things.
* Include spanish translation.
Brussels Belgium city
…y, state and country in order to allow search using '%' from the m2o widget
* Headers shortened
* Move cities management to settings
* [IMP] base_location: Add lat & long to `better.zip`
* Add latitude and longitude columns to `better.zip`

* [IMP] base_location_geonames_import: Add lat/long
* Add support for latitude & longitude to genomes importer
Incredibly not included in Odoo core.
Currently translated at 66.7% (24 of 36 strings)

Translation: partner-contact-11.0/partner-contact-11.0-base_location
Translate-URL: https://translation.odoo-community.org/projects/partner-contact-11-0/partner-contact-11-0-base_location/es/
Currently translated at 100,0% (36 of 36 strings)

Translation: partner-contact-11.0/partner-contact-11.0-base_location
Translate-URL: https://translation.odoo-community.org/projects/partner-contact-11-0/partner-contact-11-0-base_location/de/
This module has now been refactored to be more consistent with what base_address_city offers to the location management.
Added dependency to contacts so that I could change the menu location for cities / zip management.

Now, every res.city record has a relation One2many to res.city.zip (old res.better.zip). This way, every zip has a realted city too.
Zips can be searched through city code, zip or city name (same as before).

Modified tests and deleted not needed tests.

Added sql contraints so that zips and cities are unique within it's country / state / city.
Steps:

 - Open company form
 - Set 'city completion' field

Get 'The state of the partner My Company differs from that in location X'

Disabling _check_zip while writing 'zip' fields from company, as incompatible with the sequence of write operations.

Automatic test is added too.
Currently translated at 57.6% (19 of 33 strings)

Translation: partner-contact-12.0/partner-contact-12.0-base_location
Translate-URL: https://translation.odoo-community.org/projects/partner-contact-12-0/partner-contact-12-0-base_location/es/
* Don't need cities for migrating

  Previous script required to have cities populated for working, but that's not
  usual thing, as people in v11 may choose to not import them, and for older databases
  they even weren't that option.

  With this improve script, now we populate city table for ZIP entries without city,
  so the rest of the queries are properly executed in any case.

* Cover case of res_better_zip w/o country_id

* Avoid error on null zip numbers
* Standard procedure
* Change v13 specifics
* Adapt tests + correct some practices
@OsoTranquilo
Copy link

Correct functionality test

@alexis-via
Copy link
Contributor

Sorry if my question is stupid... I haven't followed the development of base_location since v12. Why do we need the res.city.zip object ? I really don't understand the role of this object: res.city is enough, isn't it ?

@alexis-via
Copy link
Contributor

Another question: res.city object has a zipcode field (cf https://github.com/odoo/odoo/blob/14.0/addons/base_address_city/models/res_city.py#L13), and that field existed since the beginning of base_address_city, but its value is not set by base_location_geonames_import, cf https://github.com/OCA/partner-contact/blob/13.0/base_location_geonames_import/wizard/geonames_import.py#L98
So it would mean that we don't want to use zipcode on res.city, but we use res.city.zip to store the zipcode ? Maybe it's the answer to my previous question...

@alexis-via
Copy link
Contributor

FYI, as geonames.org is down (https://download.geonames.org/export/zip/ is empty), I can't test easily the base_location + base_location_geonames_import modules... which makes things more difficult.

@pedrobaeza
Copy link
Member Author

pedrobaeza commented Nov 27, 2020

@alexis-via Geonames is up again. About the data model, it was the same in v12. The idea of the refactoring of this module was to have a normalized data model, where there's an specific model that represents a city, not a city + zip. Your colleague @rvalyi claimed for it sometime ago in v7/v8. Then, we have a cardinality of 1..N from city to zips linked to that city. There's an ongoing conversation in #1020 for relaxing current constraint for having a hybrid data model.

I'm finishing the conversion to computed writable for having it as example for things like OCA/odoo-community.org#50. I will write back when finished.

@alexis-via
Copy link
Contributor

I'll study it more in depth and talk with @rvalyi about it. Now that geonames is back up and running, I will be easier for me to test. I'll see if I arrive to the conclusion that res.city.zip is needed or if it should be dropped.

@alexis-via
Copy link
Contributor

Just to facilitate the debate:

@pedrobaeza
Copy link
Member Author

I have adapted the code to new computed writable fields for depicting the possibilities.

Please take a look to this new "style", and if you give your blessings, I finally merge this.

* Adapt code to this style
* Adapt tests and use Form for mimicking UI behavior
@OCA-git-bot
Copy link
Contributor

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). 🤖


This module introduces a zip model that allows you to manage locations in a better way.

The zips will allow the users to complete automatically all address-related fields by just filling the zip.
Copy link
Contributor

@NL66278 NL66278 Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually not all address related fields. You will not be able to fill in house_number and door_number from the zip. In most countries also not the street. Maybe replace 'all' with 'many'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, that fields are added through extension modules, so it's OK to say that with current dependency set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only a minor point anyway. Very glad with the module as is. But street is also not filled automatically, and this is definitely also address related.

Copy link
Member Author

@pedrobaeza pedrobaeza Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I get your point about the street, although it's impossible to fill a not known data derived from the ZIP. If I read the text, I understand that the "address-related" text refers exactly to this: the address fields that are linked with the ZIP, not the non related ones, and street (and the others) is not related, as a ZIP code contains a lot of streets.

Copy link

@OsoTranquilo OsoTranquilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works well

@pedrobaeza
Copy link
Member Author

Can I merge?

@HaraldPanten
Copy link
Contributor

Can I merge?

👍

@alexis-via
Copy link
Contributor

I talked with @rvalyi about res.city.zip object, and he told me that we doesn't use base_location in Bazil, so he's not concerned about the issue.
As I still think that base_location is now useless, I developed this afternoon a new module base_address_city_geonames_import, that does NOT depend on base_location. KISS concept. I wonder if I should propose this module in OCA/partner-contact or keep it in some Akretion Github project.

@pedrobaeza
Copy link
Member Author

OK, I still see valuable this structure as base instead of the base_address_city one. You can propose if you want the other module to OCA for having both options.

Merging this one:

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-1003-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c140a11 into OCA:14.0 Nov 30, 2020
@OCA-git-bot
Copy link
Contributor

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

@alexis-via
Copy link
Contributor

Unless if somebody explicitly says in this thread that he's interested in removing the res.city.zip object and in the associated simplification of the code, I think I'll publish my module base_address_city_geonames_import for v14 under akretion/odoo-usability (I'll do that probably tomorrow). The extra work needed for publishing in OCA is not worth it if I'm the only person interested.

@alexis-via
Copy link
Contributor

I just published my new module base_address_city_geonames_import in a dedicated Github project:
https://github.com/akretion/geonames-import-simple
I'm very happy with the result:
base_location + base_location_geonames_import = 208 + 212 lines of python (tests excluded) = 420 lines total
base_address_city_geonames_import = 186 lines of python (tests excluded)
55% less lines of python code. KISS.

As a consequence, I'll stop maintaining l10n_fr_base_location_geonames_import in OCA/l10n-france (I'll move the equivalent code elsewhere).
If one day the strategy changes and the object res.city.zip is dropped, I may propose my module base_address_city_geonames_import to OCA or go back to using base_location + base_location_geonames_import.

@fmdl
Copy link

fmdl commented Dec 2, 2020

@alexis-via I interessed to have this module in OCA.

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

Successfully merging this pull request may close these issues.