Code review comment for lp:~nimit-svnit/gtg/dates
Revision history for this message
| Bertrand Rousseau (bertrand-rousseau) wrote | # |
review:
Needs Fixing
(code inspection)
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
67d34a1
(Get the code!)
Some remarks:
(line numbers are references from your patch)
- I suggest you move line 36 in top of the method code, and remove the line 22. It seems to me that the current code will update the due_date two times if the start date comes after the new due date
- in line 27, you seem to test if the parent has a due date by doing "if par.due_date", shouldn't it be "if par.due_date != Date.no_date()"?
- same remark for the children start date in line 34 ("if sub.start_date")
- you should use the accessor functions when you need to get the start/due date from other object than the current (parents and children tasks)
- if you unset the due date (fulldate == Date.no_date()), shouldn't you skip all parent/children updates? Right now, you skip only the parent update
- you shouldn't change the due date accessor, it seems to me that the previous behavior was correct.