Code review comment for lp:~codekaki/openerp-hr/7.0-hr_roster

Daniel Reis (dreis-pt) wrote :

OK, I have a few nitpicks and suggestions. Comments below:

*.py: some small PEP8 issues; try running a PEP8 check for details.
L8: you should add the AGPL license header at the top of this file. Including a short text 'summary' attribute is highly recommended.
L50: 'images' is defined twice;
L69,70,74: I believe that in v7 forms 'col' and 'colspan' are inoperant. Nested groups are what makes that effect.
L86-116:
L114-116: wouldn't if be nicer to hide these on shorter months, instead of just making them readonly?
L192: honestly I think workflow is an overkill here; I would advise removing it.
L339: 'days' is a function field, I don't think you need to set a default for it.
L362: IMO it would be better for department_id to not be mandatory; that would allow for a roster mixing employees from more than one department (sooner or later you'll need this)
L363: the ORM already has "create_uid" and "create_date"; why nor leverage these instead of duplicating?
L371-373: again, defining defaults on function fields
L398: I suggest handling the case where time_out is lower than time_id; example: shift starts at 17:00 and ends at 01:00 if the next day.
L425: I strongly suggest making shift 'code' larger. For larger setups 1 will be clearly enough: it's easy to get hundreds of shift code. I suggest a size=10.

Many are suggestions for improvement, and I think the rest is easy to fix.
Hope to see it merged soon!

review: Needs Fixing

« Back to merge proposal