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

Thanks for the valuable suggestions, Daniel. I will get them fixed soon.

*Chang Phui Hock*
CODEKAKI SYSTEMS (R49045/14) - A web-dev company
Address: TB 15588-1 1st Floor, Lrg Kubota, Kubota Square, 91000 Tawau.
Tel: +6018-263 9102
Skype: phuihock
Website: http://www.codekaki.com

On Tue, Apr 29, 2014 at 4:32 AM, Daniel Reis <email address hidden> wrote:

> Review: Needs Fixing
>
> 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!
> --
> https://code.launchpad.net/~codekaki/openerp-hr/7.0-hr_roster/+merge/217116
> You proposed lp:~codekaki/openerp-hr/7.0-hr_roster for merging.
>