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

[11.0][mig][base_location_geonames_import] #494

Merged
merged 49 commits into from
Nov 21, 2017

Conversation

JordiBForgeFlow
Copy link
Member

@JordiBForgeFlow JordiBForgeFlow commented Oct 13, 2017

Migration to 11.0.
Relevant changes:

  • The module now creates res.city if the country requires it (flag 'enforce_cities' is set for that country)

Depends on

I discussed with @pedrobaeza The possibility to import only cities. But after reviewing, it can be very confusing to do so in this module, and in any case we could discuss about creating a separate base_address_city_geonames_import that does not depend on base_location, but only on base_address_city.

@pedrobaeza pedrobaeza mentioned this pull request Oct 13, 2017
53 tasks
@JordiBForgeFlow JordiBForgeFlow force-pushed the 11.0-mig-base_location_geonames_import branch from 10a0c9f to 9a36ea1 Compare October 16, 2017 14:14
@rafaelbn rafaelbn added this to the 11.0 milestone Oct 16, 2017
@JordiBForgeFlow JordiBForgeFlow force-pushed the 11.0-mig-base_location_geonames_import branch 3 times, most recently from fe3f925 to 3f0e97c Compare October 19, 2017 13:08
Copy link

@rgarnau rgarnau left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Just a minor comment

@@ -0,0 +1,3 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Coding is no longer necessary, as it can be checked in the migration document to 11.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Because for Python 2.7 is still needed...

Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow Nov 6, 2017

Choose a reason for hiding this comment

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

Oh, yes!! 😂

# <alexis.delattre@akretion.com>)
# Copyright 2014 Lorenzo Battistini <lorenzo.battistini@agilebg.com>
# Copyright 2016 Pedro M. Baeza <pedro.baeza@tecnativa.com>
# Copyright 2017 Efient Business and IT Consulting Services, S.L.
Copy link
Contributor

Choose a reason for hiding this comment

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

Eficent

Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jesusVMayor jesusVMayor left a comment

Choose a reason for hiding this comment

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

LGTM

@pedrobaeza
Copy link
Member

Any possible improvement in speed?

Copy link

@AaronHForgeFlow AaronHForgeFlow 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 correctly 👍 Couldn't find a way to make it faster.

@yajo
Copy link
Member

yajo commented Nov 15, 2017

Can you rebase now that #488 is merged? (BTW I don't understand how bots were ✔️ )

@JordiBForgeFlow JordiBForgeFlow force-pushed the 11.0-mig-base_location_geonames_import branch from 3f0e97c to c4851d0 Compare November 17, 2017 17:50
@JordiBForgeFlow
Copy link
Member Author

@pedrobaeza @yajo Ready to be merged.

@pedrobaeza
Copy link
Member

There are some duplicated commits at the beginning. please squash them.

@JordiBForgeFlow JordiBForgeFlow force-pushed the 11.0-mig-base_location_geonames_import branch from c4851d0 to 8eeaf2b Compare November 17, 2017 18:34
@JordiBForgeFlow
Copy link
Member Author

@pedrobaeza I am not sure to understand which ones you mean. I found a few Transifex commits that were one after each other and I squashed them.

@pedrobaeza
Copy link
Member

Wow, it seems that right now everything has blown away with lots of commits that doesn't belong to this...

@yajo
Copy link
Member

yajo commented Nov 20, 2017

Yeah, it seems something weird happened in the branch, please check that 😕

@JordiBForgeFlow
Copy link
Member Author

@pedrobaeza Will you do the honors to merge?

@pedrobaeza
Copy link
Member

Yes, of course.

@pedrobaeza
Copy link
Member

I see that your name is not correct yet (Efient instead of Eficent). Do you want to fix it?

@JordiBForgeFlow JordiBForgeFlow force-pushed the 11.0-mig-base_location_geonames_import branch 2 times, most recently from 205025d to ec11e6e Compare November 21, 2017 13:47
@JordiBForgeFlow
Copy link
Member Author

@pedrobaeza fixed!

pedrobaeza and others added 22 commits November 21, 2017 17:57
… imported country

Monaco country is very little and it allows to save some downloaded bytes plus making
a test for entries deletion
For avoiding constant problems with this test, as Monaco data changes a lot,
what we are testing now is the existence of the data, not the exact match of them.
Add option to put city name all upper case
* Tests
* New menu location
* Wizard options fixed
* Speed improvement applying cache for not doing duplicated searches
  over states.
* Tests change to SavepointCase, which only passes over setUp one time.
* [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
@pedrobaeza pedrobaeza force-pushed the 11.0-mig-base_location_geonames_import branch from ec11e6e to bb1ca1e Compare November 21, 2017 16:57
@pedrobaeza pedrobaeza merged commit dbfc9d1 into OCA:11.0 Nov 21, 2017
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.