Closed
Bug 333363
Opened 18 years ago
Closed 17 years ago
Some providers return an allday event on the day of the event, and the day after
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: mvl, Assigned: dbo)
References
Details
(Keywords: dataloss)
Attachments
(2 files, 8 obsolete files)
17.31 KB,
patch
|
Details | Diff | Splinter Review | |
30.81 KB,
patch
|
sebo.moz
:
review+
Fallen
:
review+
|
Details | Diff | Splinter Review |
Follow-up from bug 306157. In that bug, it was discovered that at least the memory and the storage calendar can return an all-day event on the day of the event, and on the day after (when querying for events in one day). The problem seems to be in using calIDateTime.nativeTime, which isn't really defined for an all-day event. Also, there are problems with timezone conversion.
Comment 1•18 years ago
|
||
I think this could lead to dataloss issues if one tried to manipulate the bogus occurrence.
Keywords: dataloss
Comment 2•18 years ago
|
||
Marking as major, since this involves incorrect display of calendar data.
Severity: normal → major
Comment 3•18 years ago
|
||
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
Comment 4•18 years ago
|
||
Bug 330496 seems to be related to this one.
Updated•18 years ago
|
Flags: blocking0.3+
Updated•18 years ago
|
Whiteboard: [needs patch]
Reporter | ||
Comment 5•18 years ago
|
||
I don't see how this can lead to dataloss. bug 306157 makes the items that should not be returned from the providers not show up, so the user doesn't have access to the bogus occurences.
Reporter | ||
Updated•18 years ago
|
Flags: blocking0.3+ → blocking0.3?
Comment 6•18 years ago
|
||
As much as it pains me, I think we can live without this for 0.3.
Flags: blocking0.3? → blocking0.3-
Keywords: relnote
Whiteboard: [needs patch] → [needs patch][cal relnote]
Updated•17 years ago
|
Whiteboard: [needs patch][cal relnote] → [needs patch]
Updated•17 years ago
|
Flags: blocking-calendar0.7?
Comment 7•17 years ago
|
||
0.7 is released- removing blocking flag and proposing wanted0.8
Flags: blocking-calendar0.7? → wanted-calendar0.8?
Assignee | ||
Updated•17 years ago
|
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Comment 8•17 years ago
|
||
A partial fix is provided in bug 354198.
Assignee | ||
Comment 9•17 years ago
|
||
- Due to the fact that the memory calendar's queries are hardly maintainable, I've rewritten that code using calUtils' checkIfInRange. - It has shown up that the inherent problem of e.g. bug 354198 (recurring all-day events) is (once again) calRecurrenceInfo's expansion code, namely calculateDates.
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #287690 -
Flags: review?(sebo.moz)
Assignee | ||
Comment 10•17 years ago
|
||
This changes the storage calendar's query to not incorporate the end date into the result set. Maybe I am wrong, then maybe mvl might tell me... :) These two patches fix bug 354198 for me as well.
Attachment #287692 -
Flags: review?(mvl)
Assignee | ||
Comment 11•17 years ago
|
||
Since code in this area is very sensitive to changes, I'd want some additional testing of the attached patches; setting QAWANTED.
Comment 12•17 years ago
|
||
(In reply to comment #10) > Created an attachment (id=287692) [details] > fix - calStorageCalendar > Wouldn't this fail for zero-length items starting at rangeStart?
Assignee | ||
Comment 13•17 years ago
|
||
@sebo: correct. This patch should include zero-length items; we need a unit test for this. Omitting review for now since this patch is only roughly tested, although comments/further testing is greatly appreciated, especially on tasks.
Attachment #287692 -
Attachment is obsolete: true
Attachment #287692 -
Flags: review?(mvl)
Comment 14•17 years ago
|
||
(In reply to comment #13) > comments/further testing is greatly appreciated, especially on tasks. > If a due date or a start date is not set it gets assigned "null" (calStorageCalendar::setDateParamHelper). I think a check for that is needed as well, as we have assigned duedate = startdate in other places in these situations. Would that work? WHERE (event_end > :range_start AND event_start < :range_end) OR (event_end = :range_start AND event_start = :range_start) OR (event_end = null AND event_start = :range_start) OR (event_end = :range_start AND event_start = null)
Reporter | ||
Comment 15•17 years ago
|
||
Those zero-length events make no real sense to me. The end-time is exclusive, so they start at a certain time, and end just a little bit before that same time. I fail to understand that. But I already wrote that in bug 333362. > Wouldn't this fail for zero-length items starting at rangeStart? Is that really a bug? The event ends just before the rangeStart, so I don't think it needs to be included in the result set. Although that does lead to problems for zero-length events at midnight when using the dayview, because those events would not show. But that's what you get for having events that make no sense...
Comment 16•17 years ago
|
||
two unit tests for the memory and the storage calendar. Note that at the moment, the test.sdb database for the storageCalendar test needs to be removed between each run of the unit test. The storageCalendar fails for the third todo entry (no DTSTART set), while memoryCal does not fail.
Reporter | ||
Updated•17 years ago
|
Attachment #288070 -
Attachment is patch: true
Attachment #288070 -
Attachment mime type: application/octet-stream → text/plain
Comment 17•17 years ago
|
||
Daniel, I cannot build at home atm due to bug 387239. I tested your patch during last week and it seems to work well. Final review will be during next week.
Comment 18•17 years ago
|
||
This is a new version of the unit test for the memory calendar. It showed an error for daily recurring all-day events. If the range is set to the same times as the parent item, getItems() returns the parent item and the following occurrence. That means endRange seems not to be exclusive.
Attachment #288070 -
Attachment is obsolete: true
Comment 19•17 years ago
|
||
The bug I mentioned in comment 18 seems to be addressed in bug 353887. I have verified, that it is not a regression of the proposed patch. As a workaround, one could check in calReccurrenceInfo::calculateDates if startDate isDate. Consequently, rangeStart and rangeEnd should be set to isDate as well. I will attach your patch with this small modification.
Comment 20•17 years ago
|
||
Assignee | ||
Comment 21•17 years ago
|
||
(In reply to comment #18) > Created an attachment (id=288584) [details] > new version of unit test for memory calendar > > This is a new version of the unit test for the memory calendar. It showed an > error for daily recurring all-day events. If the range is set to the same times > as the parent item, getItems() returns the parent item and the following > occurrence. That means endRange seems not to be exclusive. > I can't reproduce this. Removing the break statement from your (new) unit tests >+ } else { >+ dump("Problem\n"); >+ break; >+ } the tests pass successfully: make[1]: Leaving directory `/d/moz18/sbird-debug/calendar/test' ../../dist/bin/test_all.sh ../../dist/bin/test_calendar ../../dist/bin/test_calendar/test_attendee.js: PASS ../../dist/bin/test_calendar/test_datetime.js: PASS ../../dist/bin/test_calendar/test_ics_roundtrip.js: PASS ../../dist/bin/test_calendar/test_memoryCal.js: PASS ../../dist/bin/test_calendar/test_recur.js: PASS (In reply to comment #19) > The bug I mentioned in comment 18 seems to be addressed in bug 353887. I have > verified, that it is not a regression of the proposed patch. > As a workaround, one could check in calReccurrenceInfo::calculateDates if > startDate isDate. Consequently, rangeStart and rangeEnd should be set to isDate > as well. I don't consider this being ok since enforcing a DATE effectively cuts the range.
Assignee | ||
Comment 22•17 years ago
|
||
(In reply to comment #15) > Those zero-length events make no real sense to me. The end-time is exclusive, > so they start at a certain time, and end just a little bit before that same > time. I fail to understand that. But I already wrote that in bug 333362. True. Moreover 2445bis states [1] "The value type of this property MUST be the same as the "DTSTART" property, and its value MUST be later in time than the value of the "DTSTART" property." [1] <http://tools.ietf.org/rfcmarkup?doc=draft-ietf-calsify-rfc2445bis-07#section-3.8.2.2> But even though we are actually (once again) facing that wrong data and should reflect it in the UI, so users could correct it. Admittedly, one could easily produce those items with Sunbird/Lightning; we should correct that (known bug?).
Comment 23•17 years ago
|
||
A new version of the unit tests, storage Calendar inclusive. I'm also obsoleting patch 288831, Daniel pointed out on IRC, that this would cut a range if it is not on midnight.
Attachment #288584 -
Attachment is obsolete: true
Attachment #288831 -
Attachment is obsolete: true
Comment 24•17 years ago
|
||
(In reply to comment #21) > > I can't reproduce this. Removing the break statement from your (new) unit tests > > >+ } else { > >+ dump("Problem\n"); > >+ break; > >+ } > > the tests pass successfully: > Sorry, I submitted before reloading the bug. The events that fail (with and without the patch applied) are commented out! > (In reply to comment #19) > I don't consider this being ok since enforcing a DATE effectively cuts the > range. > I agree.
Comment 25•17 years ago
|
||
Just to eliminate any confusion, I'm adding the tests with the events uncommented. They should now really fail.
Attachment #288841 -
Attachment is obsolete: true
Comment 26•17 years ago
|
||
Comment on attachment 287690 [details] [diff] [review] fix - calRecurrenceInfo, calMemoryCalendar r- based on the failing unit tests.
Attachment #287690 -
Flags: review?(sebo.moz) → review-
Assignee | ||
Comment 27•17 years ago
|
||
Passes all tests, but dump("Test 19\n"); // Test a zero-length event with DUE set at the start of range and no DTSTART testGetItems("BEGIN:VTODO\n" + "DUE:20020402T000000Z\n" + "END:VTODO\n", 1); (storage only).
Attachment #287690 -
Attachment is obsolete: true
Attachment #288003 -
Attachment is obsolete: true
Comment 28•17 years ago
|
||
Using this statament: "SELECT * FROM cal_todos " + "WHERE " + " (("+floatingTodoDue+" > :range_start + :start_offset) OR " + " ("+nonFloatingTodoDue+" > :range_start) OR " + " (todo_due IS NULL) OR " + " ((("+floatingTodoDue+" = :range_start + :start_offset) OR " + " ("+nonFloatingTodoDue+" = :range_start)) AND " + " (("+floatingTodoEntry+" = :range_start + :start_offset) OR " + " ("+nonFloatingTodoEntry+" = :range_start) OR " + --> " (todo_entry IS NULL)))) " + " AND " + " (("+floatingTodoEntry+" < :range_end + :end_offset) OR " + " ("+nonFloatingTodoEntry+" < :range_end) OR " + " (todo_entry IS NULL)) " + " AND cal_id = :cal_id AND recurrence_id IS NULL" all unit tests are passed. (--> this line added)
Assignee | ||
Comment 29•17 years ago
|
||
final patch
Attachment #288862 -
Attachment is obsolete: true
Attachment #288989 -
Flags: review?(mvl)
Assignee | ||
Comment 30•17 years ago
|
||
Comment on attachment 288989 [details] [diff] [review] fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar Some more comments on the patch: >Index: mozilla/calendar/base/src/calDatetime.cpp >- buffer, sizeof(buffer), "%04d/%02d/%02d %02d:%02d:%02d %s", >- mYear, mMonth + 1, mDay, mHour, mMinute, mSecond, mTimezone.get()); >+ buffer, sizeof(buffer), "%04hd/%02hd/%02hd %02hd:%02hd:%02hd %s isDate=%01hd", >+ mYear, mMonth + 1, mDay, mHour, mMinute, mSecond, >+ mTimezone.get(), static_cast<PRInt16>(mIsDate)); adds isDate to calIDateTime::toString() and corrects format specifiers. >Index: mozilla/calendar/base/src/calRecurrenceInfo.js ... fixes searchStart (libical) hack to not include items before rangeStart >Index: mozilla/calendar/base/src/calRecurrenceRule.cpp fixes to handle all-day items correctly. ... >-#ifdef DEBUG_vlad >+#ifdef DEBUG will change to DEBUG_dbo as plain DEBUG is too verbose for everyday development >Index: mozilla/calendar/base/src/calUtils.js > var newDate = aDate.clone(); >- newDate.hour = 0; >- newDate.minute = 0; >- newDate.second = 0; > newDate.isDate = false; // newDate.isDate = false sets hour/minute/second to 0 >Index: mozilla/calendar/providers/memory/calMemoryCalendar.js rewrite due to lack of maintainability, using checkIfInRange now >Index: mozilla/calendar/providers/storage/calStorageCalendar.js fixes for all-day, zero-length and tasks without DUE.
Attachment #288989 -
Flags: review?(mvl) → review?(sebo.moz)
Comment 31•17 years ago
|
||
Comment on attachment 288989 [details] [diff] [review] fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar Sebo asked me to review the C++ part. Here we go: >+// static int PR_CALLBACK >+// calDateTimeComparator (calIDateTime *aElement1, >+// calIDateTime *aElement2, >+// void *aData) >+// { >+// PRInt32 result; >+// aElement1->Compare(aElement2, &result); >+// return result; >+// } If we decide to take out the sorting, remove this blokc >-#ifdef DEBUG_vlad >+#ifdef DEBUG > { > char* ss = icalrecurrencetype_as_string(mIcalRecur); > nsCAutoString tst, tend; > aRangeStart->ToString(tst); > aRangeEnd->ToString(tend); >- fprintf (stderr, "RULE: [%s -> %s, %d]: %s\n", tst.get(), tend.get(), mIcalRecur->count, ss); >+ printf("RULE: [%s -> %s, %d]: %s\n", tst.get(), tend.get(), mIcalRecur->count, ss); Why not to stderr? >+#ifdef DEBUG >+ { >+ nsCAutoString str; >+ cdt->ToString(str); >+ printf(" occ: %s\n", str.get()); >+ } >+#endif Should probably take this out, no valuable information. >- dates.Sort(calDateTimeComparator, nsnull); >+ // xxx todo: discuss/review! >+ // do we really need sorting here since datetimes have been added in increasing order? >+// dates.Sort(calDateTimeComparator, nsnull); I'll think about it. Need sleep for now. >Index: mozilla/calendar/base/src/calUtils.h >+#define CAL_ENSURE_MEMORY(p) NS_ENSURE_TRUE(p, NS_ERROR_OUT_OF_MEMORY) >+ Should this not be NS_ENSURE_FALSE ?
Comment 32•17 years ago
|
||
>/** calRecurrenceInfo.js **/ >+ >+ var index = 0; >+ const len = cur_dates.length; >+ >+ // skip items before rangeStart due to searchStart libical >hack: >+ if (rangeStart && baseDuration) { >+ for (; index < len; ++index) { Is this an style issue? You could use while() >+ var date = cur_dates[index].clone(); >+ date.addDuration(baseDuration); >+ if (rangeStart.compare(date) < 0) >+ break; brackets, please. Does compare() result in trouble if date is a DATE value? I think it doesn't, but.. >+ } >+ } >+ for (; index < len; ++index) { Like above, this could be changed to use while() >/** calMemoryCalendar.js **/ >+ var isEvent_ = isEvent(item); >+ if (isEvent_) { >+ if (!wantEvents) > continue; >- >- itemStartTime = (item.entryDate >- ? item.entryDate.nativeTime >- : START_OF_TIME); >- itemEndTime = (item.dueDate >- ? item.dueDate.nativeTime >- : END_OF_TIME); >- } else { >- // XXX unknown item type, wth do we do? >+ } else if (!wantTodos) > continue; Could this be changed to if ((isEvent_ && !wantEvents) || (!isEvent && !wantTodos)) { continue; } >+ if (itemReturnOccurrences && item.recurrenceInfo) { >+ var occurrences = item.recurrenceInfo.getOccurrences( >+ aRangeStart, aRangeEnd, aCount ? aCount - >itemsFound.length : 0, {}); >+ if (!isEvent_) >+ occurrences = occurrences.filter(checkCompleted); brackets >Index: mozilla/calendar/providers/storage/calStorageCalendar.js >=================================================================== < // The more readable version of the next where-clause is: >- // WHERE event_end >= :range_start AND event_start < :range_end >+ // WHERE ((event_end > :range_start OR >+ // (event_end = :range_start AND >+ // event_start = :range_start)) >+ // AND event_start < :range_end) >+ // I think we need a similar summary for the todos, since the query is now different The patch fixes the bug and a bit more. All unit tests are passed. r+ for the js part with the nits addressed.
Updated•17 years ago
|
Attachment #288989 -
Flags: review?(sebo.moz)
Attachment #288989 -
Flags: review?(philipp)
Attachment #288989 -
Flags: review+
Comment 33•17 years ago
|
||
> >Index: mozilla/calendar/base/src/calUtils.h
> >+#define CAL_ENSURE_MEMORY(p) NS_ENSURE_TRUE(p, NS_ERROR_OUT_OF_MEMORY)
> >+
> Should this not be NS_ENSURE_FALSE ?
I take that back, of course it should be NS_ENSURE_TRUE
Assignee | ||
Comment 34•17 years ago
|
||
(In reply to comment #31) > Why not to stderr? because it's DEBUG information, not error output > >+#ifdef DEBUG > >+ { > >+ nsCAutoString str; > >+ cdt->ToString(str); > >+ printf(" occ: %s\n", str.get()); > >+ } > >+#endif > Should probably take this out, no valuable information. I've used that for debugging purposes. Since I'll scope the code into DEBUG_dbo, nobody will suffer from it... ;) > >- dates.Sort(calDateTimeComparator, nsnull); > >+ // xxx todo: discuss/review! > >+ // do we really need sorting here since datetimes have been added in increasing order? > >+// dates.Sort(calDateTimeComparator, nsnull); > I'll think about it. Need sleep for now. After all IMO we should remove that code (even if it actually would resort the output), because no code should rely on sorted output. (In reply to comment #32) > >+ for (; index < len; ++index) { > > Is this an style issue? You could use while() Then I need to decouple the index modification into the block. My experience has shown that for-loops are better maintainable: Once somebody adds a continue statement to the block in the future, one has to keep care of the index, too. After all, I doubt that the style is bad or unreadable. > >+ var date = cur_dates[index].clone(); > >+ date.addDuration(baseDuration); > >+ if (rangeStart.compare(date) < 0) > >+ break; > > brackets, please. > Does compare() result in trouble if date is a DATE value? I think it doesn't, > but.. Good point. I think it doesn't, but it's worth to think about it: If date is a DATE, comparison will be performed on a DATE basis, i.e. rangeStart will eventually be cut by some hours. This IMO isn't a problem w.r.t. to the range check, because the item behind date is all-day (or more), thus falling into the range. > Could this be changed to > > if ((isEvent_ && !wantEvents) || (!isEvent && !wantTodos)) { > continue; > } I think that's ok, but IMO harder to read, and isEvent_ is eventually checked twice. > < // The more readable version of the next where-clause is: > >- // WHERE event_end >= :range_start AND event_start < :range_end > >+ // WHERE ((event_end > :range_start OR > >+ // (event_end = :range_start AND > >+ // event_start = :range_start)) > >+ // AND event_start < :range_end) > >+ // > > I think we need a similar summary for the todos, since the query is now > different yes.
Comment 35•17 years ago
|
||
Comment on attachment 288989 [details] [diff] [review] fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar > > >- dates.Sort(calDateTimeComparator, nsnull); > > >+ // xxx todo: discuss/review! > > >+ // do we really need sorting here since datetimes have been added in increasing order? > > >+// dates.Sort(calDateTimeComparator, nsnull); > > I'll think about it. Need sleep for now. > > After all IMO we should remove that code (even if it actually would resort the > output), because no code should rely on sorted output. I agree. Go ahead and take out the sorting and the Comparator.
Attachment #288989 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 36•17 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Updated•16 years ago
|
Flags: in-testsuite+
Comment 38•16 years ago
|
||
Checked in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•