'Due in' information is incorrect for milestones due today

[ryano@…]

I created two milestones, "Milestone 1" is due today (08/23/2010) and "Milestone 2" is due tomorrow. The time is currently 01:14.

On the page /milestone/Milestone 1, it reads: Due in 74 minutes (08/23/2010)

On the page /milestone/Milestone 2, it reads: Due in 23 hours (08/24/2010)

However, the page for Milestone 1 should read 74 minutes late (08/23/2010), not Due in 74 minutes (08/23/2010).

I first noticed this issue on 0.11.7, but confirmed the same behavior exists on 0.12.1dev-r10006.

[Remy Blank]

Any idea where the difference comes from?

[ryano@…]

Okay, the issue here seems to be the question of, what is the due time for a milestone, is it 00:00:00 or 23:59:59?

• If a milestone is due tomorrow, then the milestone will display Due in X hours/minutes. This seems to imply that the due time for a milestone is 00:00:00 on its due date, and one might assume then that a milestone should be considered late if open after that date/time. The timedelta is calculated by dateinfo in trac.web.chrome using pretty_timedelta, which returns the time relative to 00:00 of milestone.due.
• However, the is_late property of the milestone object is only true if self.due.date() < date.today(), which seems to imply that a milestone's due time is 23:59:59 on its due date.

To summarize: On the day the milestone is due, we end up with the milestone reading Due in X hours/minutes (see Roadmap.html​) because milestone.is_late = false, but the X is calculated relative to 00:00:00 of the due date, so at 06:00:00 the milestone reads Due in 6 hours, not Due in 18 hours as it should.

One simple fix to establish consistency would be to change is_late to calculate true when self.date.date() == date.today(): self.due.date() <= date.today().

Another way to establish consistency would be to add 23:59:59 to milestone.due before calculating the timedelta.

I don't really like either of those, however, because it seems like a milestone should only be late after the due date has passed and yet I also don't like that the countdown to the due date displays the number of hours/minutes until 00:00:00 of the due date, because it implies that the milestone is due at the beginning of the day, not on the day.

I think a better solution would be have the milestone display Due today, when self.date.date() == date.today(), and avoid the question of "what is the due time". The question then becomes, how do you calculate the number of hours until a milestone is due? If it is noon today, and the milestone is due tomorrow, do you show 12 hours until the milestone is due, or 36 hours?

A solution to the latter issue might be to only display the number of days until the milestone is due, never the number of hours or minutes. So, if a milestone is due on Thursday, then:

• On Tuesday the milestone will display: Due in 2 days
• On Wednesday it will display: Due in 1 day or Due tomorrow
• On Thursday it will display: Due today.

I'm ready to provide a patch for any of the aforementioned solutions.

[ryano@…]

I noticed another interesting milestone / timezone behavior while testing out the ​th:WikiTicketCalendarMacro, and I think this behavior is entirely correctly, but leads to some strange results and we may want to change the behavior a bit.

I was testing to make sure that the correct day was highlighted on the macro's calendar by changing the timezone. It was just after midnight in my timezone (GMT - 7), so I set the timezone to GMT - 8 to make the local time just after 23:00 and confirmed that the day highlighted on the calendar shifted by -1. All was well, but I noticed that the two milestones I had created shifted by -1 day as well.

When I created a milestone while in GMT - 7 timezone, the milestone.due field was 2010-08-24 07:00:00+00:00, or 2010-08-24 00:00:00 in local time. Now, I switch to GMT - 8 and the milestone due date in local time becomes 2010-08-23 23:00:00.

This all seems technically correct, but in practical terms, do we really want two people working in adjacent timezone to see the due date for a milestone as being two different days?

[Remy Blank]

Excellent analysis, thanks. So this is related to #6369, which suggests being more precise in the due date by adding the due time. Presetting the due time to 12:00 would prevent the due date being on different days for adjacent timezones.

I like the idea of keeping the resolution of the due date at days. Maybe we should combine both: add the due time (preset to 12:00) and keep the display resolution in days?

[hasienda <hoff.st@…>]

Replying to rblank:

[…] I like the idea of keeping the resolution of the due date at days. Maybe we should combine both: add the due time (preset to 12:00) and keep the display resolution in days?

Just like I do with ticket custom due date backed by true custom time fields. Yes, 12:00 would be a reasonable choice.

[Ryan J Ollos <ryano@…>]

Replying to rblank:

I like the idea of keeping the resolution of the due date at days. Maybe we should combine both: add the due time (preset to 12:00) and keep the display resolution in days?

This sounds good. I've looked at the code, and I'll need some advice on how to proceed.

1. I made a change to add 12 hours to milestone.due when it is set, and attached the patch for comment.
2. In order to display the time until the milestone is due with days resolution, should we replace the dateinfo​ function with an equivalent function that returns the timedelta with days resolution, or add an optional second argument to dateinfo that specifies the resolution? The dateinfo function calls pretty_timedelta, which has a third argument resolution​, which looks like it could be expanded to fit this purpose.

[Ryan J Ollos <ryano@…>]

Patch to set milestone duetime to 12:00.

[Ryan J Ollos <ryano@…>]

The display of the patch that I uploaded is not what I'm used to … still getting accustomed to PyDev and Subsclipse in Eclipse. Anyway the patch seems simple enough to be readable at least.

[Remy Blank]

Instead of just adding 12:00 to the due date, I would make a more radical change, and make the "Due" field on the milestone edit page identical to the "Completed" field:

• Accept a time in addition to a date.
• Add a checkbox to enable or disable the due date.
• If the due date is disabled, set the value of the due date edit box to the current date and a time of 12:00.

This sets a sensible default for the time, while still allowing it to be edited if desired.

I would try to enhance pretty_timedelta(), probably not by overloading the resolution= argument, as it is already used for another purpose, but by adding a smallest= argument specifying the smallest delta to show.

But maybe that's still not the best solution, as we would like to have "Due tomorrow" and "Due today", but pretty_timedelta() has no notion of "now"… except maybe when time2=None. Another option would be a new function point_in_time() that returns a textual representation of a point in time rather than a time interval.

In any case, the i18n aspect is a bit tricky.

(Your patch is in "context diff" format, whereas we usually prefer "unified diff". Subclipse can probably be configured to produce unified diffs.)

[ryano@…]

Replying to rblank:

Instead of just adding 12:00 to the due date, I would make a more radical change, and make the "Due" field on the milestone edit page identical to the "Completed" field:

• Accept a time in addition to a date.
• Add a checkbox to enable or disable the due date.
• If the due date is disabled, set the value of the due date edit box to the current date and a time of 12:00.

This sets a sensible default for the time, while still allowing it to be edited if desired.

If that feature is implemented, I have to wonder if it would be better to leave the resolution in days/hours/minutes, since it appears to be difficult to make this change and it leads to a less precise display of the due date/time (comment:10:ticket:6369 makes a good argument).

My initial thought on this was that, if the milestone only has a duedate resolution of days, then we probably only want to display the time to due in days as well. However, if the due date/time has resolution of seconds, it might make more sense to stick with the existing convention.

This is one of those things that gives me a headache because the more I look at it, the more it seems there is no "definitely right" way to proceed … its very subjective and open to personal preference. I wonder if this warrants an option in trac.ini to allow customizing the behavior.

Seems like there is a bit of work to do here, and lots of possibilities. I could create a patch to add the due time field, which appears to be fairly straightforward, and would closed #6369. After that, I could shift attention back to this ticket for the more complex issue of how to display the resolution of the time to due date.

[Remy Blank]

Replying to ryano@…:

My initial thought on this was that, if the milestone only has a duedate resolution of days, then we probably only want to display the time to due in days as well. However, if the due date/time has resolution of seconds, it might make more sense to stick with the existing convention.

That makes sense.

This is one of those things that gives me a headache because the more I look at it, the more it seems there is no "definitely right" way to proceed … its very subjective and open to personal preference.

Welcome to the wonderful world of Trac development (and, really, any development with more than one user) :-)

I wonder if this warrants an option in trac.ini to allow customizing the behavior.

I don't think so. Too little value (the possibility to configure the behavior) for the additional complexity (in the code and for admins to configure). Sometimes less is more.

Seems like there is a bit of work to do here, and lots of possibilities. I could create a patch to add the due time field, which appears to be fairly straightforward, and would closed #6369. After that, I could shift attention back to this ticket for the more complex issue of how to display the resolution of the time to due date.

Good plan. Go for it!

[Ryan J Ollos <ryano@…>]

Thanks for your help. I should have a first cut at the patch by Wednesday or Thursday.

[Ryan J Ollos <ryano@…>]

Something else I've noticed in the course of working on this ticket, is that the hint is not technically correct when setting a milestone to complete with an incorrect timestamp. The format of the datetime or timestamp is MM/DD/YYYY hh:mm:ss PM. If I try to enter 08/26/2010 13:51:31 PM, I get the following error:

I'll try to resolve this issue as well in this ticket. It seems like the error message should read something like:

Error: Invalid DateTime

"08/26/2010 13:51:31 PM" is an invalid datetime, or the datetime format is not known. Try "MM/DD/YYYY hh:mm:ss PM" instead."

Is there a better term to use than datetime? Perhaps timestamp?

[Ryan J Ollos <ryano@…>]

Here is my first cut at a patch:

• Made the Due field to have the same format as the Completed field, with a datetime format.
• Made these changes to both the Milestone Edit and Admin Milestones Panel.
• Changed the Roadmap and Mitlestone View pages to show datetime format rather than date format.
• Added a parse_datetime function to trac.util which throws an error as described in comment:13. I'm expecting this part of my patch will need some work.
  • Both parse_date and parse_datetime call _parse_datetime, which contains most of what was previously the body of parse_date. However, parse_date and parse_datetime differ in the hint that is displayed when an error is thrown.
  • Another way to approach this might be to modify the existing parse_date function with a third argument that says we are expecting a datetime format. However, we have format_date and format_datetime functions in trac.chrome, so perhaps it makes sense to have a pair of parse functions as well.
• The validation of the Completed field is now done by parse_datetime (comment:13).

What I have not yet attempted to implement:

• Default duetime of 12:00, which seems to be the topic of much discussion in #6369.
  • Do we want to do that in #6369 since the changes implemented in this ticket preserve existing behavior if the user chooses to not enter a time, but changing the default duetime from 00:00 to 12:00 changes the existing behavior?
  • If implemented, do we want a trac.ini parameter to allow specifying a default duetime that applies when only a date in entered?
• I'm not sure what you had in mind for the behavior of Add a checkbox to enable or disable the due date.

[Remy Blank]

A few comments:

• Adding the <div> around the due date field added a margin between the fieldset label and the field. This should probably be fixed in the CSS.
• I would prefer keeping a single parse_date() instead of splitting into parse_date() and parse_datetime(). The only difference between the two is the wording in the errors, where I think keeping the term "date" even for datetime is better, and the hint. We could just add a new argument hint=None to parse_date(), that allows providing the format hint (with the default keeping the current behavior). I don't mind having parse_date() parsing a datetime correctly even if a date is expected, that's what it has been doing all along.
• The "due" edit field still only displays the date.
• Ok for doing the "default to 12:00" in #6369. No need for a trac.ini option for the default duetime, just use 12:00.
• The checkbox is actually meant for #6369. If a milestone has no due time, the "Due" field should be populated with the current date, 12:00. However, just doing this will require the user removing the due time every time if she doesn't want to set one, which is not user friendly. So the suggestion is to do the same as for the "Completed" field, and have a checkbox to explicitly set or disable the due date.

Updated patch coming shortly.

[Remy Blank]

Updated patch according to comment:15.

[Remy Blank]

9582-due-time-r10015.patch​ fixes the first three issues in comment:15. The additional hint argument to parse_date() can be:

• 'date' (the default): show the hint for dates
• 'datetime': show the hint for datetimes
• Anything else: show that as the hint

Please test and comment.

[Ryan J Ollos <ryano@…>]

The patch appears to function well. Your implementation of the datetime hint is very clean, and it was nice to see a better implementation than what I came up with!

The Web Admin panel has a nice consistency now between the Milestone and Versions panel. The Add boxes are identical with exception to their labels, which of course should be different.

The only thing I would change is something that I just now noticed and somewhat unrelated to this ticket. Add 2 px to the width of the Name: textbox on the Versions admin panel, so that it is the same as the width of the Due: textbox (which is a change we already made for the Milestone panel).

trac-0.12-stable-2/trac/admin/templates/admin_versions.html

@@ -53 +53 @@
           <fieldset>
             <legend>Add Version:</legend>
             <div class="field">
-              <label>Name:<br /><input type="text" name="name" id="name" /></label>
+              <label>Name:<br /><input type="text" name="name" id="name" size="22"/></label>
             </div>
             <div class="field">
               <label>Released:<br />

[Ryan J Ollos <ryano@…>]

Replying to rblank:

• Ok for doing the "default to 12:00" in #6369. No need for a trac.ini option for the default duetime, just use 12:00.
• The checkbox is actually meant for #6369. If a milestone has no due time, the "Due" field should be populated with the current date, 12:00. However, just doing this will require the user removing the due time every time if she doesn't want to set one, which is not user friendly. So the suggestion is to do the same as for the "Completed" field, and have a checkbox to explicitly set or disable the due date.

Looking forward to implementing this next.

I'm not sure of the best way to default to 12:00:00, but I've given it some thought. In the _do_save method of the MilestoneModule class in roadmap.py, we have:

        due = req.args.get('duedate', '')
        milestone.due = due and parse_date(due, req.tz, 'datetime') or None

It seems like we want to set the default time to 12:00:00 in parse_date, because prior to parse_date, we are working with a string rather than a datetime object, which seems tedious. After the call to parse_date, we have no way of knowing if the user entered 00:00:00 for the duetime, or if they entered no duetime.

However, I have to wonder if this is an appropriate change to make to parse_date given the likelihood that it is used elsewhere is Trac.

[Ryan J Ollos <ryano@…>]

I found a lingering issue with the fix so far …

Going back to the original issue that I filed the report about, it seems that the overall problem was that calculations were done by treating the milestone due field as having resolution of seconds, but the user was only allowed to specify the due date with a resolution of days. Now that we have a patch that increases the resolution with which a user can specify a milestone, I have to wonder if we want to modify the islate property of the Milestone object accordingly. Currently, it only treats milestone.due as having a resolution of days:

    is_late = property(fget=lambda self: self.due and \
                                         self.due.date() < date.today())

We still see one of the originally reported problems. I created a milestone just now and set the due date to about 2 hours ago. The milestone says Due in 109 minutes (08/29/2010 01:00:00 AM), which is incorrect, because our is_late calculation is still incorrect.

[Remy Blank]

Replying to Ryan J Ollos <ryano@…>:

The only thing I would change is something that I just now noticed and somewhat unrelated to this ticket. Add 2 px to the width of the Name: textbox on the Versions admin panel, so that it is the same as the width of the Due: textbox (which is a change we already made for the Milestone panel).

Those are characters, not pixels. And the width of the date box is calculated from the length of the date hint, so it's not constant. But yes, we should be consistent between version and milestone, and a larger field is certainly more user-friendly.

[Remy Blank]

Replying to Ryan J Ollos <ryano@…>:

It seems like we want to set the default time to 12:00:00 in parse_date, because prior to parse_date, we are working with a string rather than a datetime object, which seems tedious. After the call to parse_date, we have no way of knowing if the user entered 00:00:00 for the duetime, or if they entered no duetime.

Correct, and I wouldn't even try to guess. I would just set a default with 12:00 when displaying the milestone edit page and no due date is currently set. Look what happens when a milestone has no "completed" date, and you edit the milestone: the "Completed" checkbox is unchecked, and the "Completed" text box has the current date and time. So if you check the checkbox, the default value for the completion date is the current date and time.

We could do the same for the due date, except with a time of 12:00. If there is no due date, the "Due" checkbox is unchecked, and the "Due" text box is populated with the current date, and a time of 12:00. So when I check the "Due" checkbox, I have a sensible default, and I just need to edit the date and leave the time at 12:00.

Sure, the best option would be to detect when the user only enters a date, and set the time to 12:00 in that case, but I'm not sure we can even detect this (with all possible date / time formats). If you find a way to do that, great! Assuming a time of 00:00 means "no time" could be good enough, though.

Replying to Ryan J Ollos <ryano@…>:

I have to wonder if we want to modify the islate property of the Milestone object accordingly.

Yes, absolutely, I had forgotten about that. I'll fix that and the version box width, and apply the patch. Then I'll wait for your patch for the default due time.

[Remy Blank]

Patch (including fixes from comment:21) applied in [10022]. I'll close this ticket, and we can continue with the "12:00 default" (or 18:00 or whatever) in #6369.