Review: Needs Fixing
Hi,
The code looks clean, and the embedded mini-spec is a nice touch. However I
would have loved to see an actual code review before merging the branch in
trunk. Some of the things I noticed with a quick and mostly-syntactic review
look like real bugs, so I hope I misunderstood something... ;-)
You might specifically want to double-check lines 168, 309 and 393.
- By convention, only methods that actually use the `context` should test it
for None. This otherwise adds a lot of useless noise, and passing
context=context downstream is perfectly fine even when context is None. Most of
the methods added in this branch do check for context being None in a perfectly
useless manner. 20 less lines of code in the diff, always good to take ;-)
- ir_needaction seems like a weird and quite obfuscated name for this feature,
and unfortunately it leaks in menus, objects description and the mixin. It's
probably still time to change it, so why not use something more generic like
'ir.user_subscription' or 'ir.record_notification' or a full
'ir.user_record_subscription' or 'ir.user_record_notification' or something
like that? (and update the rest accordingly)
- "ir_needaction_mixin" class: why prepend "ir" in the class name? Presumably
it is namespaced in openerp.addons.base.ir, so do we need the second mention of
"ir". Furthermore, as it is a mixin class, shouldn't it be a osv_abstract or
even better a new AbstractModel? Using osv.osv will create a useless database
table.
- l.42: the doc seems to refer to base.needaction that looks like it was
renamed to ir.needaction_mixin?
- l.168: it looks like that line should be ``map(itemgetter('user_id'),...`` !
- l.168 bis: in general it is always better to pass an explicit list of columns
to read() for performance reasons, as omitting it means reading *all*
columns... and it could have helped spot the previous mistake.
- l.168 ter: it looks like this line will return duplicate user_id values if
there are some, and given how the results are used (including comparing the
size of the list), this will cause bugs. Filtering it with list(set(...))
should do the trick.
- l.190: all([cur_user in user_ids for cur_user in cur_users])
^^^^^^ all() works on any iterable, so using a generator is sufficient
in general, not need to have a full-blown in-memory list comprehension, even if
I doubt it would make a noticeable performance difference here:
all(cur_user in user_ids for cur_user in cur_users)
- l.309: it looks like this line should be ``map(itemgetter('res_model',
'res_id')...`` !
- l.323: initializing ``res`` with dict.fromkeys(ids) seems strange as 2 lines
later it is overwritten with {} inside the loop, making the reader wonder which
one of the default values is actually intended to be returned: None or {}?
- l.376: shouldn't get_needaction_info() be a private method? Seeing that it
only supports certain kinds of domains (string literals, not referring to
'uid', etc.) and is mostly meant to be used by the function fields on
ir.ui.menu, it seems a sensible choice. Especially as it is calling eval() on
one of its arguments! (fortunately, safe_eval is already used in orm.py, but
nevertheless). This eval() a bit brittle, btw...
- l.390: the doc for get_needaction_info() seems to mention a 2-item returned
value, while the implementation returns 3 of them
- l.393: passing limit=8192 to needaction_get_record_ids() is very strange! If
the intent is to turn off the search() limit, the right thing to do is to pass
limit=None!
--
https://code.launchpad.net/~openerp-dev/openobject-server/trunk-need_action-tde/+merge/97207
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-server/trunk-need_action-tde.
_______________________________________________
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
