Review: Needs Fixing
Hello, thanks for the merge prop.
There are a few things that bother me:
- You do a search using ('employee_id', '=',
current_attendance_data.employee_id.id) then in the loop you write again
old_attendance.employee_id.id == current_attendance_data['employee_id'].id.
- The early return True seems a bit optimistic.
- Because of all the early returns, I wonder when the code
+ if current_attendance_data['action'] == 'sign_out':
+ return False
return True
might get a chance to be executed...
- The previous code was careful to check the constraint for the current ids,
not for all past data. This is important to not check data that was already
validated.
- The previous code took only action in ('sign_in', 'sign_out'), why not yours?
- Finally, I would like to see this kind of bug fix accompanied with test
cases, especially the one reported in the bug report. This makes sure the bug
is indeed corrected and it won't reappear in the future.
--
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-bug-885387-mdi/+merge/82136
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-addons/trunk-bug-885387-mdi.
_______________________________________________
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
