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.
--
Mailing list: https://launchpad.net/~openerp-community-reviewer
Post to : openerp-community-reviewer@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community-reviewer
More help : https://help.launchpad.net/ListHelp
