Comment is rather big, so for the purpose of readability I'm putting what needs clarifications at the top:
> test_datetime should cover it, I don't think it checks what was imported. It doesn't, nor does it provide various timezones to see what happens. On the other hand, this begs the question: what happens if no timezone is provided, e.g. if the import is done through the API without specifying a context? > I didn't see in the code / documentation any mention about how we could empty > a already defined value. These fields are skipped at the moment, I kept the original semantics (so's not to have to fix files in some localization modules) > In my opinion, it would be a shame to work on the imports without taking some > action for this issue. That's a good point. The issue is that any value other than leaving the cell empty likely already means something (I mean unrealistically we could use a UUID for one or the other, but that'd be quite horrible), so empty can mean either "don't touch it" or "set to empty". I don't have any hard feelings one way or the other, I'd suggest discussing this on whatever mailing list you see fit and then I can change the semantics of the "empty field" whichever way was picked? Would that work? — rest — > usability: shouldn't the Import action be available in Kaban views as well? > Many menus open kanban views by default, and are likely to be frequent import > targets (Contacts, Products, etc.). True. But there's already a "link" there, so I'm not too sure. I guess it'll be added if requested, I just put it where it already was (list view) > the .CSV wikipedia link should have target=_blank? Yeah could be a good idea, less risky. Done. > the drop-down fields for matching CSV columns are weirdly unresponsive/slow Saw that, haven't had the time to wonder about it yet. It might be due to the number of fields we're serializing, I'm not sure the underlying library caches the generated list. > the breadcrumbs show ".../undefined" when opening the more_info action Yeah I don't know why yet. > the xid column header of the ir.model.data list view shows "Complete ID", > while users will presumably be looking for an "External ID" Yeah that's the name it has on ir.model.data, and ir.model.data already uses "external identifier" for "name" (the xid without the module), not sure what to do about this. > does it make sense to show the ir.model.data list as more_info when a > "xxx.id" entry has no match? if you're using ".id" it's probably because you > don't have xids for the records? Well it should show the database ids as well, so the user should be able to pick db ids as well. > usually we avoid documenting cr, uid, ids and context in docstrings, unless > they have something remarkable for that function (e.g. an unusual context > key). I see a few of them ;-) Indeed, my editor tends to add all of them automatically so I just fill them. I removed the crs and uids I saw. > - l.478: in dosctring ``_$fromtype_$column._type``, there should be a 'to_' > somewhere, shouldn't it? Well-spotted, fixed. > - l.642, 2142: misplaced parens makes form view unavailable? if on purpose, > just drop the 'form' tuple wasn't intended no, failed to review the nesting generated by the editor auto-closing some parens. Fixed. > _str_to_datetime: what are we gonna do about timezones? I think we should > treat in the same way as datetimes manually typed in a form view, therefore a > timezone conversion is needed somwehere. Yes, that sounds like a good idea > l.672-676: why not use ir.model.data.get_object_reference() instead of > reimplementing it? Either because I missed it, or because I initially had issues with the ValueError it raises or something. Probably because I missed it. Doesn't seem to break the tests so I'll replace the custom code by it, simpler. > - l.689: Y U NO HAZ TRANSLATION? Didn't really think of translating actual exceptions output as exceptions (especially this one which *really* shouldn't happen). Fixed. > - l.743: intended to be ``return REPLACE_WITH(ids), warnings`` ? Kinda, I might have done that bit before writing/copying the helpers. Fixed. > - l.1464: when context has no 'lang' we should not pass 'en_US' instead, in > order to allow _get_source's short-circuit to skip the DB query. Good point, removed the fallback value (the provided default was also pointless obfuscation). Not even sure where I got that from. -- https://code.launchpad.net/~openerp-dev/openobject-server/trunk-newimport-phase2-xmo/+merge/126658 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-server/trunk-newimport-phase2-xmo. _______________________________________________ Mailing list: https://launchpad.net/~openerp-dev-gtk Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-dev-gtk More help : https://help.launchpad.net/ListHelp
