300117 - recurring event not visible on "until" date

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Tim Olsen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050517 Firefox/1.0.4 (Debian package 1.0.4-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050708 Mozilla Sunbird/0.2+

Recurring events do not display on the "until" date for the event

Reproducible: Always

Steps to Reproduce:
1. add a recurrence to an event that repeats every 1 day and ends on a specific date


Actual Results:  
the event doesn't show up on the "until" date

Expected Results:  
show the event on the "until" date

using CVS as of 7/8/2005 plus "update sunbird callers" patch from bug 299182

I'm not sure which component this bug really belongs to

Comment 1

[Mike Shaver (:shaver emeritus)]

mvl: You think this is a back-end bug, or a front-end one?

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

I think it is backend, as the frontend just gets occurences from the backend,
and doesn't try to interpret the recurrence rules. I need to do some testcasing
to be sure.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Actually, it might be a frontend bug, where the event dialog needs to add one
day to the until date. (the dialog shows an inclusive date, the
calIRecurrenceRule doesn't say if the end date is inclusive or exclusive.)

Comment 4

[Robin Edrenius]

Confirming bug.

This does not apply for allday events, for some reason.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051024 Mozilla Sunbird/0.2+

Comment 5

[Joey Minta]

(In reply to comment #3)
> Actually, it might be a frontend bug, where the event dialog needs to add one
> day to the until date. (the dialog shows an inclusive date, the
> calIRecurrenceRule doesn't say if the end date is inclusive or exclusive.)

It's definitely front-end.  The date-picker returns something like 20051027T000000
for 27 Oct.  This is then earlier than anything (except all-day events) on the 27th.  My feeling is that we should be setting it to 23:59:59 on 27 Oct., so that we match anything on that day, but avoid matching all-day events on the next day.

Comment 6

[Mike Shaver (:shaver emeritus)]

Unfortunately, I do not have cycles to work on Calendar stuff these days (just as it's getting to the good part!), so I am a bad owner for these bugs.  To delete the tragically-large chunk of bugspam, search for gregorianabdication.

Comment 7

[Thomas Benisch]

I'll take over this task.

Comment 8

[Thomas Benisch]

accepted

Comment 9

[Thomas Benisch]

Attachment: 300117.patch sets the time of the endDate to 23:59:59.999

As proposed I set the time of the endDate to 23:59:59.999.
This is done in the saveDialog() function in calendar-recurrence-dialog.js.
Probably someone can enlighten me, if this is the right place.
I thought, that changing the date inside the datepicker or minimonth is
the wrong approach.

Comment 10

[Joey Minta]

Comment on attachment 212222 [details] [diff] [review]
300117.patch sets the time of the endDate to 23:59:59.999

So, from my understanding of the UNTIL property, this patch will not fix the bug for people in timezones east of GMT.  If I have an event from 8pm-9pm local time, but live in GMT+6, this until-time will still be prior to the recurrence on the last day, since the until-time is required to remain in UTC.  Would it not make more sense to specify the UNTIL as a date, rather than a date-time?  That way, it's inclusive nature ought to work nicely for everyone, I think.  Or is my understanding of UNTIL flawed?

Comment 11

[Michael Büttner (no reviews TFN)]

taking over.

Comment 12

[Joey Minta]

Comment on attachment 212222 [details] [diff] [review]
300117.patch sets the time of the endDate to 23:59:59.999

Cancelling review request after discussion with mickey.

Comment 13

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

The first patch already got the job done, since the conversion from the local timezone to UTC was implicetly done by calling jsDateToDateTime(). I also doublechecked this with RFC2445 (see paragraph 4.3.10), the UNTIL-date defines the 'inclusive' bound of the rule, and the datetime MUST be specified in UTC (this is why the endDate is timezone agnostic). I added the appropriate functionality to loadDialog() which was missing for some reason. There was another bug in loadDialog(), which has been triggered if the rule specifies an endDate, in which case an exception has been fired, since the rule does not allow to read the mutually exclusive 'count'-attribute.

Comment 14

[Joey Minta]

Comment on attachment 212489 [details] [diff] [review]
patch v2

To make sure we're all on the same page: As I understand this patch, it relies heavily on the fact that we only create events in the default timezone.  In future versions, where the timezone of the event is not assured to be the timezone that jsDate is in, this will need to be re-written.  Correct?

Comment 15

[Michael Büttner (no reviews TFN)]

(In reply to comment #14)

as far as i know, this is not the case, since the UNTIL datetime is always stored in UTC and converted back and forth as necessary during comparison with local times. getOccurrencesBetween() will convert the UNTIL datetime to the local timezone in order to create the resultset of items. of course the initial datetime depends on the timezone the event was created in. for example if i create an event (in GMT+1) which starts at 1st january, and repeats until 3rd january, the UNTIL datetime will be 3/1/22:59:99Z (which is 23:59:99 converted to zulu-time). if now someone asks for all occurrences of this event, the UNTIL datetime will be converted to the appropriate local timezone before the comparison is made, which seems logical to me. of course, if you're now in GMT-5 the UNTIL datetime in this example would be 3/1/17:59:99, but the start/end-datetime of the event can't be any later than the converted UNTIL datetime (17:59:99), since it was created in my timezone. so it will still show up in your view correctly (i hope that all of this is correct, if not please let me know). the one and only quirk with all of this is that we still mix jsDate and calIDateTime, the former uses the OS timezone, where the latter uses the lightning preferences. the timezone-conversion can only work correctly if both settings are in sync. i think we should get rid of the jsDate object rather soon and stick to calIDateTime, that way we ensure that the timezone conversions are consistent. to make this last statement clearer, we won't need to rewrite the patch later, but get rid of jsDate (in all other places where timezone issues are important as well).

Comment 16

[Joey Minta]

Comment on attachment 212489 [details] [diff] [review]
patch v2

(In reply to comment #15)
> the one and only quirk with all of this is that we still mix
> jsDate and calIDateTime, the former uses the OS timezone, where the latter uses
> the lightning preferences. the timezone-conversion can only work correctly if
> both settings are in sync.
This sync issue is exactly what I'm concerned about.  If it's possible for a user to change the tz of the start-date, then that will, by definition, move it out of sync.  Since we don't offer that functionality yet, I'll accept this for 0.1, but please file a follow-up bug being filed on that issue for later.  mvl and I talked about this a few days ago and came to the conclusion that the best solution is probably to do some post-processing in the calendar-event-dialog.js, based on any changes that may happen to start/endDate.  Again, that's for later.

-            if (rule.count == -1) {
+            if (rule.isByCount) {
+                if (rule.count == -1) {
This change doesn't seem right.  According to both the documentation and code, I can set a 'forever' rule by setting |rule.endDate = null;| and in that case isByCount will return false.  Then you'll get all sorts of nasty errors when you try to get a null datetime in a timezone.  According to the idl, it's only valid to ask for isByCount once you've established that that rule is finite.  It seems like there's maybe an underlying bug in the recurrence code here, which makes it difficult to establish this forever case.  This may be better handled in another bug, but I'll leave that up to you.

+                    setElementValue("recurrence-duration", "forever");
+                }
+                else {
Just a style nit.  The general rule is "preserve the style of the file you're working in."  This file is consistent with keeping else on the same line as the closing brace. } else {

+                var endDate = rule.endDate;
+                var kDefaultTimezone = calendarDefaultTimezone();
+                var endDate = endDate.getInTimezone(kDefaultTimezone);
You shouldn't double declare |var endDate|.  Also, since you're not going to use kDefaultTimezone anywhere else, there's no point in initializing another object for it/adding another variable to this scope.

The actual 23:59.999 changes seem reasonable here, at least for the 0.1 case.  It's the isByCount changes that are the main problem here.

Comment 17

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

new iteration of the patch. i filed a follow-up bug which addresses the timezone conversion issue (https://bugzilla.mozilla.org/show_bug.cgi?id=328442) and another one addressing the issue regarding the confusing count/enddate definition in calIRecurrenceRule (https://bugzilla.mozilla.org/show_bug.cgi?id=328444). the patch has been modified as suggested.

Comment 18

[Joey Minta]

Comment on attachment 213034 [details] [diff] [review]
patch v3

+                if (endDate == null) {
Yeah...you know |if (!endDate) {|

Thanks for the work on this!  I'll fix that prior to checkin. r=jminta

Comment 19

[Joey Minta]

Patch checked in.

Comment 20

[Joey Minta]

*** Bug 329155 has been marked as a duplicate of this bug. ***