openerp-community-reviewer team mailing list archive

[Nicolas Bessi - Camptocamp <nicolas.bessi@xxxxxxxxxxxxxx>]

Review: Needs Fixing

Oups I missed that.

Actually, I do not agrees with you, but I won't argue too long on this as for me the branch organisation problem is more deep. Let's see what other think about it. 

Some comments Belows:
Some PEP8 in manifest:

328	+                _("The content of the file doesn't correspond to the "
329	+                    "selected country."))

I will add row value and country info it will be more easy for support.

A question with :
+        if res_request.status_code != requests.codes.ok:
349	+            raise orm.except_orm(
350	+                _('Error:'),
351	+                _('Got an error %d when trying to download the file %s.')
352	+                % (res_request.status_code, url))

It's been a long time since I used geonames, if I'm correct in case of wrong country it returns 404 that right. If it the case I'm ok with this else we may have to treat a 200 with a message


+        if bzip_ids_to_delete:
356	+            bzip_obj.unlink(cr, uid, bzip_ids_to_delete, context=context)
357	+            logger.info(
358	+                '%d better zip entries deleted for country %s'
359	+                % (len(bzip_ids_to_delete), wizard.country_id.name))

This is a quite a direct approach, I agree create, update, unactivate is quite not easy to put in place, but as other development can depends on base location model there should at least be a Big warning in manifest.

Also it would be a good idea to add a small lock to ensure atomicity during the import.
A query "for update no wait" a the begining of transaction would be nice.

Tests are also missing. Without depending on the geoname services having a local excrept to base the tests would be great.

Thanks for the contributions, keep up the good work.

Regards

Nicolas



-- 
https://code.launchpad.net/~akretion-team/partner-contact-management/base-location-geonames-import/+merge/214564
Your team Partner and Contact Core Editors is subscribed to branch lp:partner-contact-management.