Closed Bug 405459 Opened 14 years ago Closed 14 years ago

Task without start and due date are not displayed anymore

Categories

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

defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: arenam, Assigned: dbo)

References

Details

(Keywords: dataloss, regression)

Attachments

(3 files, 3 obsolete files)

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
Do you see any Sunbird/Lightning related error messages in the Error Console?
No. I just reproduced the bug in Lightning & Calendar to be sure.
Possible cause: I just realized that if the task has no start date it disappears.
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, ...
Correct. Unless I specify a date.
All places: list, cal view.
Local ics calendars. I do not share them or network them.
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.
Status: UNCONFIRMED → NEW
Component: Tasks → Provider: ICS/Webdav
Ever confirmed: true
Keywords: dataloss, regression
OS: Windows XP → All
QA Contact: tasks → ics-provider
Hardware: PC → All
Summary: tasks disappeared after installing nightly. Applies to Calendar & Lightning both. → Task without start and due date are not displayed anymore
Version: unspecified → Mozilla 1.8 Branch
Blocks: 333363
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...
(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.
(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.
Attached patch fixing memory/ics (obsolete) — Splinter Review
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.
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #290384 - Flags: review?(sebo.moz)
Attached patch alternative fix (obsolete) — Splinter Review
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).
Attachment #290391 - Flags: review?(sebo.moz)
Flags: blocking-calendar0.8+
Version: Mozilla 1.8 Branch → unspecified
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.
(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?
(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.
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.

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 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.
Attachment #290384 - Flags: review?(sebo.moz) → review+
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.
Attachment #290391 - Flags: review?(sebo.moz) → review+
(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 on attachment 290384 [details] [diff] [review]
fixing memory/ics

We prefer fixing this in checkIfInRange.
Attachment #290384 - Attachment is obsolete: true
incorporating completionDate
Attachment #290671 - Flags: review?(sebo.moz)
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.
Attachment #290671 - Flags: review?(sebo.moz) → review+
Attached patch fix storageCalendar (obsolete) — Splinter Review
patch adds the concept of treating completedDate as dueDate to the storageCalendar.
Attachment #290759 - Flags: review?(daniel.boelzle)
Attachment #290391 - Attachment is obsolete: true
(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.
Target Milestone: --- → 0.8
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?
patch changes the select statement. It passes all unit tests. I hope I didn't miss anything.
Attachment #290759 - Attachment is obsolete: true
Attachment #290874 - Flags: review?(daniel.boelzle)
Attachment #290759 - Flags: review?(daniel.boelzle)
I think you "nailed" it...Thanks!
Blocks: 405251
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]?
My alternative doesn't work: returns todos for DTSTART >= rangeEnd.
(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 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) ?
(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 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.
Attachment #290874 - Flags: review?(daniel.boelzle) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
I can confirm that this is working in lightning build (2007121104)
VERIFIED based on comment 35
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.