405459 - Task without start and due date are not displayed anymore

Status: VERIFIED FIXED

(Calendar :: Provider: ICS/WebDAV, defect)

Description

[Mike Arena]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: 0.8 nightly 11/26/07

tasks disappeared after installing nightly. Applies to Calendar & Lightning both.

Reproducible: Always

Steps to Reproduce:
1.enter task 
2. restart calendar 
3.task disappeared


Expected Results:  
saved task

same bug in sunbird 0.8 pre & lightning 0.8 upon install of nightly 11/26

Comment 1

[Stefan Sitter [:ssitter]]

Do you see any Sunbird/Lightning related error messages in the Error Console?

Comment 2

[Mike Arena]

No. I just reproduced the bug in Lightning & Calendar to be sure.

Comment 3

[Mike Arena]

Possible cause: I just realized that if the task has no start date it disappears.

Comment 4

[Stefan Sitter [:ssitter]]

Please be more specific: 
After creating the task it's visible but after a restart it's gone. Correct? 
What location are you refering to: Task list, agenda, calendar view, ...
What type of calendar? Local, remote, ics, wcap, google, caldav, ...

Comment 5

[Mike Arena]

Correct. Unless I specify a date.
All places: list, cal view.
Local ics calendars. I do not share them or network them.

Comment 6

[Stefan Sitter [:ssitter]]

Confirmed as regression using ics calendar. Works fine using storage calendar. Only tasks without start and due are affected. Tasks with either start date or due date or both still work in ics calendar.

Regression Range:
  WORKS in Sunbird 0.8pre (2007-11-25-05)
  FAILS in Sunbird 0.8pre (2007-11-26-06)
  
Checkins during regression range: http://tinyurl.com/yq6bm3

Seems to be caused by the checkin for Bug 333363.

Comment 7

[Sebastian Schwieger]

From http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-07#section-3.3.5
page 57:

      A "VTODO" calendar component without the "DTSTART" and "DUE" (or
      "DURATION") properties specifies a to-do that will be associated
      with each successive calendar date, until it is completed.

That means this could be fixed in calITodo.js by assigning to its entryDate a DATE value for today? Problem would be that it needs to be changed on crossing midnight. Or checkIfInRange() could be adopted...

Comment 8

[Stefan Sitter [:ssitter]]

(In reply to comment #7)
A task without any date is valid. The error introduced by Bug 333363 should not be worked around by setting a date on the task but should be fixed I'd say.

Comment 9

[Sebastian Schwieger]

(In reply to comment #8)
I'm not saying that the date should be serialized, but internally we could treat it as a task that is due today. At least, thats what the rfc suggests, if I understand it correctly.

The regression is - probably - caused by removing code from calMemoryCalendar that assigned non existing start/due dates arbitrary times in the far past/future. Thus, these todos have always been included in a getItems() call.

IMO, the question is rather what is the better workaround..

The decision would be easier if the UI-relevant pieces would be clear:
- Should an uncompleted task with no dates assigned show up in the agenda?
- Should an uncompleted task with start date in the past show up in the agenda?
- Should an uncompleted task with due date in the future show up in the agenda?

The same questions with possibly different answers can be asked for the views and for the task list.

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: fixing memory/ics

Despite this fix, I can imagine we fix this right in checkIfInRange, i.e. VTODO without DTSTART/DUE will fit into every range (as sebo mentioned). That way this patch would be obsolete.

Comment 11

[Daniel Boelzle [:dbo]]

Attachment: alternative fix

Suggestion to respecify checkIfInRange to be used in a boolean manner (by default, because that's its common use) and add a fourth parameter whether DTSTART/DUE should be returned (currently only a use case for calRecurrenceInfo).

Comment 12

[Sebastian Schwieger]

Just to clarify this: We do want to return zero-date todos on every getItems() call (no matter what range)? Every caller but one (the task list) has no use for these items.
Possible alternatives might be an additional filter or just to return those items on unbounded queries.

Comment 13

[Daniel Boelzle [:dbo]]

(In reply to comment #12)
> Just to clarify this: We do want to return zero-date todos on every getItems()
> call (no matter what range)? Every caller but one (the task list) has no use
> for these items.
Yes, I think that's correct. If callers have no use for todos, they shouldn't specify ITEM_FILTER_TYPE_TODO.

> Possible alternatives might be an additional filter or just to return those
> items on unbounded queries.
Why should that be dependent on the query range?

Comment 14

[Sebastian Schwieger]

(In reply to comment #13)
> Yes, I think that's correct. If callers have no use for todos, they shouldn't
> specify ITEM_FILTER_TYPE_TODO.
I meant specifically todos with neither start nor due date set. The views can consume other todos, but the date-less todos are of no use for them.
But if thats the wanted behaviour, I can live with it...

> Why should that be dependent on the query range?
Thats indeed a bit hackyish. Its just the case that the task list issues an unbounded query while the views probably don't.

Comment 15

[Stefan Sitter [:ssitter]]

The task list displays just all tasks and not only tasks within a certain range. Therefore I assume there is no query date range. Also consider that getting such tasks still works for e.g. storage provider - only the ics provider was broken. The calendars views on the other hand should keep the current behavior. This means only tasks in the selected day/week/months are displayed.

Comment 16

[Pete Riley]

Sorry if this doesn't apply to your discussion, but I'm under the impression that the task list will eventually be based on date ranges (as an option).  Christian's design spec currently has the following sections:  "Today", "Tomorrow", "Next Week", and "Next two weeks".

http://wiki.mozilla.org/index.php?title=Calendar:Mail_View_Integration&section=12#Customize_Tasks_List

Comment 17

[Sebastian Schwieger]

Comment on attachment 290384 [details] [diff] [review]
fixing memory/ics

fixes the bug. r=sebo
It might still be more appealing to fix this in a more central place like checkIfInRange(). You decide.

Comment 18

[Sebastian Schwieger]

Comment on attachment 290391 [details] [diff] [review]
alternative fix


>+    var startDate;
>+    var endDate;
>+    if (isEvent(item)) {
>+        startDate = item.startDate;
>+        if (!startDate) { // DTSTART mandatory
>+            return null;
>+        }
>+        endDate = (item.endDate || startDate);
>+    } else {
>+        var dueDate = item.dueDate;
>+        startDate = (item.entryDate || dueDate);
>+        if (!startDate) {
>+            return (returnDtstartOrDue || item.isCompleted ? null : true);
Why check for isCompleted here? This should be decided by the calling function, I think.


The rest works as advertised. r=sebo with the question addressed.

Comment 19

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> It might still be more appealing to fix this in a more central place like
> checkIfInRange(). You decide.
I agree, we should make this in checkIfInRange as this is likely to be used from more client code, e.g. agenda code.

(In reply to comment #18)
> >+        if (!startDate) {
> >+            return (returnDtstartOrDue || item.isCompleted ? null : true);
> Why check for isCompleted here? This should be decided by the calling function,
> I think.
Hmm, in case of a calling provider this is valid, because the provider code (e.g. memory) checks completion up front. But w.r.t. other code (e.g. agenda), it makes sense to handle this task in checkIfInRange, too. But reading your RFC comment #7 again, IMO we need to use the completion date, too, instead of only isCompleted.

Comment 20

[Daniel Boelzle [:dbo]]

Comment on attachment 290384 [details] [diff] [review]
fixing memory/ics

We prefer fixing this in checkIfInRange.

Comment 21

[Daniel Boelzle [:dbo]]

Attachment: alternative fix - v2

incorporating completionDate

Comment 22

[Sebastian Schwieger]

Comment on attachment 290671 [details] [diff] [review]
alternative fix - v2

I like the idea of checking completedDate.

>+        if (!startDate) {
>+            if (returnDtstartOrDue) { // DTSTART or DUE mandatory
>+                return null;
>+            }
>+            // 3.6.2. To-do Component
>+            // A "VTODO" calendar component without the "DTSTART" and "DUE" (or
>+            // "DURATION") properties specifies a to-do that will be associated
>+            // with each successive calendar date, until it is completed.
>+            var completedDate = item.completedDate;
>+            if (completedDate) {
>+                var queryStart = ensureDateTime(rangeStart);
>+                var completedDate = ensureDateTime(completedDate);
>+                return (!queryStart || completedDate.compare(queryStart) > 0);
>+            }
>+            return !item.isCompleted;
I would prefer
  else { return true; } 
for the following reason: todos can have isCompleted == true without a completedDate set. There would be no way for the tasklist to get these todos. I think we must return them to stay with the current model.

r=sebo with that discussed.

I added some unit tests to bug 405251 and it showed that after this patch memory and storage are still not in sync. I will attach a patch for storage calendar.

Comment 23

[Sebastian Schwieger]

Attachment: fix storageCalendar

patch adds the concept of treating completedDate as dueDate to the storageCalendar.

Comment 24

[Daniel Boelzle [:dbo]]

(In reply to comment #22)
> >+            return !item.isCompleted;
> I would prefer
>   else { return true; } 
> for the following reason: todos can have isCompleted == true without a
> completedDate set. There would be no way for the tasklist to get these todos. I
> think we must return them to stay with the current model.
Yes, make sense.

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 25

[Daniel Boelzle [:dbo]]

Comment on attachment 290759 [details] [diff] [review]
fix storageCalendar

>+                var completedDate = item.completedDate;
>+                if (!item.entryDate && !item.dueDate && completedDate) {
>+                    var queryStart = ensureDateTime(aRangeStart);
>+                    completedDate = ensureDateTime(completedDate);
>+                    if (queryStart && completedDate.compare(queryStart) <= 0) {
>+                        continue;
>+                    }
Why don't you enhance the select statement?

Comment 26

[Sebastian Schwieger]

Attachment: enhancing select statement

patch changes the select statement. It passes all unit tests. I hope I didn't miss anything.

Comment 27

[Mike Arena]

I think you "nailed" it...Thanks!

Comment 28

[Daniel Boelzle [:dbo]]

Comment on attachment 290874 [details] [diff] [review]
enhancing select statement

hard query ;)
works as advertised w.r.t. your unit tests

>+       /**
>+        * WHERE (due > rangeStart AND start < rangeEnd) OR
>+        *       (due = rangeStart AND start = rangeStart) OR

>+        *       (due IS NULL AND ((start >= rangeStart AND start < rangeEnd) OR
>+        *                         (start IS NULL AND 
>+        *                          (completed > rangeStart OR completed IS NULL))) OR

I can imagine you don't select todos like e.g.
- rangeStart specified, but open rangeEnd (null)
- todo has a DTSTART before rangeStart, but no DUE nor COMPLETED.

I haven't tested that, but why not just

...
(due IS NULL AND
 (start IS NULL OR start >= rangeStart) AND
 (completed IS NULL OR completed > rangeStart)) OR ...

[the part in the middle]?

Comment 29

[Daniel Boelzle [:dbo]]

My alternative doesn't work: returns todos for DTSTART >= rangeEnd.

Comment 30

[Sebastian Schwieger]

(In reply to comment #28)
> 
> I can imagine you don't select todos like e.g.
> - rangeStart specified, but open rangeEnd (null)

rangeEnd is never null, it gets assigned the max time possible.

> - todo has a DTSTART before rangeStart, but no DUE nor COMPLETED.

I don't think this should be selected. We treat it as zero-length todo in other parts of the code (checkIfInRange)

I'm not sure if I understood your comment correctly. Please rephrase if my answer doesn't make sense..

Comment 31

[Daniel Boelzle [:dbo]]

Comment on attachment 290874 [details] [diff] [review]
enhancing select statement

Yes, you're right.

>+            "   ((todo_entry IS NULL) AND " +
>+            "    ((("+floatingCompleted+" > :range_start + :start_offset) OR " +
>+            "      ("+nonFloatingCompleted+" < :range_start)) OR " +
Isn't this  typo? Shouldn't it be 
                     ("+nonFloatingCompleted+" > :range_start) ?

Comment 32

[Sebastian Schwieger]

(In reply to comment #31)
> >+            "    ((("+floatingCompleted+" > :range_start + :start_offset) OR " +
> >+            "      ("+nonFloatingCompleted+" < :range_start)) OR " +
> Isn't this  typo? Shouldn't it be 
>                      ("+nonFloatingCompleted+" > :range_start) ?
> 
Yes, that looks like a very bad typo. Thanks for spotting it. It shows that we need unit tests with items that have timezones.;-)

Comment 33

[Daniel Boelzle [:dbo]]

Comment on attachment 290874 [details] [diff] [review]
enhancing select statement

Despite the typo, the patch is ok; r=dbo.

Corrected the typo and checked in on HEAD and MOZILLA_1_8_BRANCH.

Would be great if sebo could refine the unit tests.

Comment 34

[Stefan Sitter [:ssitter]]

Attachment: ics testcase with task date combinations

For reference: This is an .ics file I created for testing. It contains 
a task without any date ("Task 00"), a task with only start date ("Task 10"), a task with only due date ("Task 01"), a task with start and due date ("Task 11"). Each task with dates is available with timezone Europe/Berlin, with timezone UTC and floating.

Comment 35

[seshu.parvataneni]

I can confirm that this is working in lightning build (2007121104)

Comment 36

[Sebastian Schwieger]

VERIFIED based on comment 35