The default bug view has changed. See this FAQ.

Some providers return an allday event on the day of the event, and the day after

VERIFIED FIXED in 0.8

Status

Calendar
Internal Components
--
major
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: dbo)

Tracking

({dataloss})

unspecified
dataloss
Dependency tree / graph
Bug Flags:
wanted-calendar0.8 +
in-testsuite +

Details

Attachments

(2 attachments, 8 obsolete attachments)

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

11 years ago
I think this could lead to dataloss issues if one tried to manipulate the bogus occurrence.
Keywords: dataloss

Comment 2

11 years ago
Marking as major, since this involves incorrect display of calendar data.
Severity: normal → major
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

11 years ago
Bug 330496 seems to be related to this one.

Updated

11 years ago
Flags: blocking0.3+
Whiteboard: [needs patch]
(Reporter)

Comment 5

11 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

11 years ago
Flags: blocking0.3+ → blocking0.3?

Comment 6

11 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

10 years ago
Whiteboard: [needs patch][cal relnote] → [needs patch]

Updated

10 years ago
Flags: blocking-calendar0.7?

Comment 7

10 years ago
0.7 is released- removing blocking flag and proposing wanted0.8
Flags: blocking-calendar0.7? → wanted-calendar0.8?
(Assignee)

Updated

10 years ago
Flags: wanted-calendar0.8? → wanted-calendar0.8+

Comment 8

10 years ago
A partial fix is provided in bug 354198.
(Assignee)

Comment 9

10 years ago
Created attachment 287690 [details] [diff] [review]
fix - calRecurrenceInfo, calMemoryCalendar

- 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

10 years ago
Created attachment 287692 [details] [diff] [review]
fix - calStorageCalendar

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

10 years ago
Since code in this area is very sensitive to changes, I'd want some additional testing of the attached patches; setting QAWANTED.
Keywords: qawanted
Whiteboard: [needs patch]
Version: Trunk → unspecified

Comment 12

10 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)

Updated

10 years ago
Blocks: 380060
(Assignee)

Comment 13

10 years ago
Created attachment 288003 [details] [diff] [review]
fix - calStorageCalendar

@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

10 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

10 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

10 years ago
Created attachment 288070 [details] [diff] [review]
preliminary unit Tests

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

10 years ago
Attachment #288070 - Attachment is patch: true
Attachment #288070 - Attachment mime type: application/octet-stream → text/plain

Comment 17

10 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

10 years ago
Created attachment 288584 [details] [diff] [review]
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.
Attachment #288070 - Attachment is obsolete: true

Comment 19

10 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

10 years ago
Created attachment 288831 [details] [diff] [review]
same patch for memoryCal and recurrenceInfo with small workaround
(Assignee)

Comment 21

10 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

10 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

10 years ago
Created attachment 288841 [details] [diff] [review]
new version of unit tests (storage and memoryCal)

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

10 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

10 years ago
Created attachment 288845 [details] [diff] [review]
unit tests, failing in memory and storage provider.

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

10 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

10 years ago
Created attachment 288862 [details] [diff] [review]
fix - calRecurrenceInfo, calMemoryCalendar, calRecurrenceRule

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

10 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

10 years ago
Created attachment 288989 [details] [diff] [review]
fix - calRecurrenceInfo, calRecurrenceRule, calMemoryCalendar, calStorageCalendar

final patch
Attachment #288862 - Attachment is obsolete: true
Attachment #288989 - Flags: review?(mvl)

Updated

10 years ago
Blocks: 353887
(Assignee)

Comment 30

10 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 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

10 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

10 years ago
Attachment #288989 - Flags: review?(sebo.moz)
Attachment #288989 - Flags: review?(philipp)
Attachment #288989 - Flags: review+
> >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

10 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 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+

Updated

10 years ago
Blocks: 405251
(Assignee)

Comment 36

10 years ago
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8

Comment 37

10 years ago
This checkin regressed Bug 405459.
Depends on: 405459
Keywords: qawanted
Flags: in-testsuite+

Comment 38

9 years ago
Checked in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED

Updated

9 years ago
Keywords: relnote
No longer blocks: 380060
You need to log in before you can comment on or make changes to this bug.