openerp-community-reviewer team mailing list archive

[Nhomar - Vauxoo <nhomar@xxxxxxxxx>]

Review: Needs Information

@Raph:

http://bazaar.launchpad.net/~savoirfairelinux-openerp/partner-contact-management/city-move/revision/31

Our original implementation was with State Id but I don't understand yet why it was remove by @Sandy. @Sandy can you help us with you reasons please.

@maxime:

About France:

Region == State ?? (technically speaking)

@Pedro:

Sadly this time i think we need to improve our own revisions answers, it means:

If we are agreed a +1 is fine.
If not (please explain everything better and extended) your actual answer:

1.- This reasons>>
- It makes a direct replacing of the field city, which breaks automatically any existing installation with data in city field, because there are string in the DB on that field, and with this module, it is expected to exist an ID (integer).
- It is incompatible with other modules that expect to have a string on that field. For example, CRM.
- It doesn't provide a menu entry for managing cities.
- It is not adapted to v7 conventions.

Are completely valid (but totally different from the first one)  and - with exception of the CRM part - it takes 20 minutes to solve but are technically irrelevant in the main discussion topic base_location vs city vs "Normal city feature".

About the compatibility, let me tell you we test it with runbot some time ago "Several" time ago and it is green:

http://runbot.openerp.com/openerp-mexico-maintainer.html

And may be we need is develop the test and plan with an rgrep "but honestly" in this first stage of merge modules these specific reason is irrelevant, "We have more than 40 modules in this specific case", we need first have the inventory on modules correctly setted and __then__ dedicate time to avoid invisible broken parts.

Yes you are right, if you see our original implementation was adding city_id, and you are right, it should be easily solved. ( I didn't realize this problem in the Savoir proposal )

About this reason>>

- Removing the requirement of the zip field. City field is still required. We have to change a little 'name_get' method and add a 'search_name' method to fulfill complete ORM compatibility.

Yes this is my point, if you are agreed to remove 100% of not state/city relation from base_location and build the correct country/state/city relation on it +1 for this option.

- My point is the same as Raph.

If the module is called base_whatever or only simple "city_state" or base_city_state it is fine for me.

The question is that if we don't have a clean of zip module done we don't have the feature at all and all localizations are re-implementing the wheel every time.

Regards.
-- 
https://code.launchpad.net/~savoirfairelinux-openerp/partner-contact-management/city-move/+merge/196023
Your team Partner and Contact Core Editors is subscribed to branch lp:partner-contact-management.