Review: Needs Fixing
(Note: assuming L&F still to be polished - no comment for that)
- 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.).
Admittedly this is for the addons part, but I won't review it myself ;-)
- usability (more stuff for addons ;-)):
- the .CSV wikipedia link should have target=_blank?
- the drop-down fields for matching CSV columns are weirdly unresponsive/slow
(tested on Contacts with Chrome), all the time
- the breadcrumbs show ".../undefined" when opening the more_info action
- the xid column header of the ir.model.data list view shows "Complete ID",
while users will presumably be looking for an "External ID"
- usability: 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? Not sure...
- doc: 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 ;-)
- l.478: in dosctring ``_$fromtype_$column._type``, there should be
a 'to_' somewhere, shouldn't it?
- l.642, 2142: misplaced parens makes form view unavailable? if on purpose,
just drop the 'form' tuple
- _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.
(Actually the opposite of what fields.datetime.context_timestamp() already
does, so maybe you could add it as fields.datetime.from_context_timestamp?)
Note that the export must behave accordingly, and should therefore convert
datetime info to the user TZ upon export, considering this is the same as
displaying a datetime value to the user.
test_datetime should cover it, I don't think it checks what was imported.
- l.672-676: why not use ir.model.data.get_object_reference() instead of
reimplementing it?
- l.689: Y U NO HAZ TRANSLATION?
- l.743: intended to be ``return REPLACE_WITH(ids), warnings`` ?
- 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.
This gives a notable perf boost in DB installs and testsuite executions.
When you want to test translations you should explicitly set the language
in the calling context
Very impressive work, btw!
--
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
