335643 - Tasks in multiweek/month view are not displayed if start date is before first day in view

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Torsten]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060426 Mozilla Sunbird/0.3a1+

A task is not displayed in the multiweek or month view if the start date of the task is before the first day in the selected view. It is only shown if the start is visible in the view. An Event is always shown, regardless the visibilty of its start date. This also should be the case for tasks.

Reproducible: Always

Steps to Reproduce:
1. Create a task spanning two weeks (or months).
2. Select multiweek (month) view and "Tasks in view".
3. Configure the view such that the first week (month) of the task is not visible.

Actual Results:  
The task is not displayed in the view

Expected Results:  
The task should be displayed even if the first day of the task is before the first day of the actual view.

Comment 1

[Stefan Sitter [:ssitter]]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060429 Mozilla Sunbird/0.3a2

Comment 2

[Michael Büttner (no reviews TFN)]

reassigned.

Comment 3

[Thomas Benisch]

taking over this task

Comment 4

[Thomas Benisch]

Attachment: patch v1

This patch contains a fix for the SQL query for non-recurrent tasks
in the storage calendar.

A task has a date (todo_entry) and a due date (todo_due).
As the query looks quite complex, here's a short overview
(neglecting floating entries):

todo_entry  todo_due  condition
---------------------------------------------------------------------------
  -           -         -
  +           -         todo_entry >= range_start && todo_entry < range_end
  -           +         todo_due >= range_start && todo_due < range_end
  +           +         todo_due >= range_start && todo_entry < range_end

Comment 5

[Thomas Benisch]

In this context I think I also found a bug in the SQL query for
non-recurrent events (calStorageCalendar.js). The correct
query is

var floatingEventStart = "event_start_tz = 'floating' AND event_start"
var nonFloatingEventStart = "event_start_tz != 'floating' AND event_start"
var floatingEventEnd = "event_end_tz = 'floating' AND event_end"
var nonFloatingEventEnd = "event_end_tz != 'floating' AND event_end"
// The query needs to take both floating and non floating into account
this.mSelectEventsByRange = createStatement(
    this.mDB,
    "SELECT * FROM cal_events " +
    "WHERE " +
    " (("+floatingEventEnd+" >= :range_start + :start_offset) OR " +
    "  ("+nonFloatingEventEnd+" >= :range_start)) AND " +
    " (("+floatingEventStart+" < :range_end + :end_offset) OR " +
    "  ("+nonFloatingEventStart+" < :range_end)) " +
    "  AND cal_id = :cal_id AND recurrence_id IS NULL"
    );

For the case start_offset != end_offset, the original query is wrong.
If you confirm this, I will write a separate task.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #4)
> Created an attachment (id=223674) [edit]
> patch v1
> 
> This patch contains a fix for the SQL query for non-recurrent tasks
> in the storage calendar.
> 
> A task has a date (todo_entry) and a due date (todo_due).
> As the query looks quite complex, here's a short overview
> (neglecting floating entries):
> 
> todo_entry  todo_due  condition
> ---------------------------------------------------------------------------
>   -           -         -
>   +           -         todo_entry >= range_start && todo_entry < range_end


If I'm interpreting things correctly, the above line is not correct.  A task with a start date before range_start wouldn't show up, even if it hasn't been completed yet.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 223674 [details] [diff] [review]
patch v1

As you pointed out above, this code is pretty difficult to understand without a bunch extra documentation.  I think that's likely to hurt us the next time somebody needs to do maintenance on this code, because it's really tricky to verify that it does exactly what it ought to be doing.  I'd suggest using some more helper variables to further breakdown the code into more human-meaningful chunks.  In particular, I think it might be helpful to factor the code into "floating" and "non-floating" that can be ANDed together to form the WHERE clause.  There may be other additional ways to break this up as well.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #5)

> For the case start_offset != end_offset, the original query is wrong.
> If you confirm this, I will write a separate task.

Agreed; that looks like a bug too.  Nice catch!

Comment 9

[Thomas Benisch]

(In reply to comment #6)
I think before I create a new patch, we should get in agreement about
how the SQL query should look like. I added some item numbers to the
overview table, which makes refering to each row easier:

     todo_entry  todo_due  condition
--------------------------------------------------------------------------------
(1)    -           -         -
(2)    +           -         todo_entry >= range_start && todo_entry < range_end
(3)    -           +         todo_due >= range_start && todo_due < range_end
(4)    +           +         todo_due >= range_start && todo_entry < range_end

> > todo_entry  todo_due  condition
> > ---------------------------------------------------------------------------
> >   -           -         -
> >   +           -         todo_entry >= range_start && todo_entry < range_end
> 
> 
> If I'm interpreting things correctly, the above line is not correct.  A task
> with a start date before range_start wouldn't show up, even if it hasn't been
> completed yet.

Your interpretation is correct, but that's actually the same behaviour as
before. Nevertheless this might be wrong, the question is how tasks with
missing date or due date are displayed. The only alternatives I see in this 
case are

(2a)   +           -         todo_entry < range_end
(2b)   +           -         -

I favour (2a). I think (1) is correct, all tasks with missing date and due date
are returned. (4) is also correct, this condition fixes the bug.
(3) is also discussable, tasks with a due date after range_end are not
returned, therefore an alternative might be

(3a)   -           +         todo_due >= range_start

What do you think?

Comment 10

[Thomas Benisch]

(In reply to comment #8)
> (In reply to comment #5)
> 
> > For the case start_offset != end_offset, the original query is wrong.
> > If you confirm this, I will write a separate task.
> 
> Agreed; that looks like a bug too.  Nice catch!

I filed Bug #339670.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

After spending a couple of hours today thinking about this, I believe that the use cases here are more varied and complex than I realized.  As a result, I'd like to suggest splitting this into two pieces.  

Specifically, this bug would just be used to address the obvious dataloss (non-matching start & end offsets; start is before the beginning of the range) here, while more or less preserving existing semantics: (only look at todos which have start dates; ignore due dates) both in storage and ICS providers.

I do agree with you that we need to take due dates as well as various permutations of undated items into account.  However, in pondering exactly how to do this, I can imagine various different use cases that would imply different behavior on the part of the app.  I think figuring out the best strategy there is going to require us to spend some time in UI-land enumerating these cases and figuring out how we think they're likely to apply to our target users.  So I'd suggest spinning off that work into another bug.

If that sounds like a reasonable strategy, that reduces the table entries to (1) and (2).  As far as (1), it looks to me like the current semantics is not to include undated entries in the views, so I'd propose sticking with that.  For (2), I agree that (2a) is the way to go there.

Comment 12

[Thomas Benisch]

(In reply to comment #11)
> After spending a couple of hours today thinking about this, I believe that the
> use cases here are more varied and complex than I realized.  As a result, I'd
> like to suggest splitting this into two pieces.  
> 
> Specifically, this bug would just be used to address the obvious dataloss
> (non-matching start & end offsets; start is before the beginning of the range)
> here, while more or less preserving existing semantics: (only look at todos
> which have start dates; ignore due dates) both in storage and ICS providers.
> 
> I do agree with you that we need to take due dates as well as various
> permutations of undated items into account.  However, in pondering exactly how
> to do this, I can imagine various different use cases that would imply
> different behavior on the part of the app.  I think figuring out the best
> strategy there is going to require us to spend some time in UI-land enumerating
> these cases and figuring out how we think they're likely to apply to our target
> users.  So I'd suggest spinning off that work into another bug.

I think this approach makes sense. There's not only the question, how tasks
with missing date or due date are displayed in the views, but also what
getItems() should return. This might be different, because getItems() is
also used by the calendar todo list, which calls getItems() with null,null
as parameters. In the storage calendar that means, that the start time
is set to the minimal possible value and the end time to the maximum
possible value. In this case, the SQL query should return all tasks.

> If that sounds like a reasonable strategy, that reduces the table entries to
> (1) and (2).  As far as (1), it looks to me like the current semantics is not
> to include undated entries in the views, so I'd propose sticking with that. 
> For (2), I agree that (2a) is the way to go there.

That means, the table will look like the following:

     todo_entry  condition
-----------------------------------------
(1)    -           -
(2)    +           todo_entry < range_end

I think your conclusion for (1) is wrong. Although the views don't display
tasks with missing date (todo_entry), getItems() should return ALL tasks with 
missing date (see my previous comment). Otherwise those tasks won't appear
in the calendar todo list. Therefore (todo_entry IS NULL) must be part of 
the SQL query.

Comment 13

[Thomas Benisch]

Attachment: patch v2 (checked in)

This patch fixes the dataloss problem only.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #12)
> I think this approach makes sense. There's not only the question, how tasks
> with missing date or due date are displayed in the views, but also what
> getItems() should return.

Agreed.  getItems is not the best API in the world and may well need some changes to accommodate todos in a reasonable way.

> I think your conclusion for (1) is wrong.

You're right; my mistake.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 223929 [details] [diff] [review]
patch v2 (checked in)

r=dmose; patch landed on both trunk and branch using cross-commit.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

If you could spin off another bug for "Investigate in-view task UI", then this bug can be resolved FIXED.  Thanks for the patch!

Comment 17

[Thomas Benisch]

(In reply to comment #16)
> If you could spin off another bug for "Investigate in-view task UI", then this
> bug can be resolved FIXED.  Thanks for the patch!

I filed Bug #339955.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Great; thanks!

Comment 19

[Torsten]

The bug is resolved for local calendars but I still can not see a task in a remote calendar with a start date before the start of the view in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060613 Sunbird/0.3a2+.

Comment 20

[Stefan Sitter [:ssitter]]

(In reply to comment #19)
Confirmed with Lightning 0.1+ (20060615). Works ok when task is shown in storage calendar but fails with task stored in ics file. -> REOPEN

Comment 21

[Stefan Sitter [:ssitter]]

Requesting blocking0.3 due to possible dataloss.

Comment 22

[Damian Szczepanik]

tip: easy way to verify this issue
subscribe this calendar https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&product=Calendar&version=Sunbird%200.3a1&version=Sunbird%200.3a2&version=Trunk&version=unspecified&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&emailtype1=exact&email1=&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=7D&chfieldto=Now&chfield=resolution&chfieldvalue=FIXED&query_based_on=fixedFor7Days&field0-0-0=noop&type0-0-0=noop&value0-0-0=&ctype=ics (bugs that have been resoved for 7 days)

tasks are visible in month/multiweek but don't in week and day

Comment 23

[Thomas Benisch]

(In reply to comment #22)
> tip: easy way to verify this issue
> subscribe this calendar
> https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&product=Calendar&version=Sunbird%200.3a1&version=Sunbird%200.3a2&version=Trunk&version=unspecified&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&emailtype1=exact&email1=&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=7D&chfieldto=Now&chfield=resolution&chfieldvalue=FIXED&query_based_on=fixedFor7Days&field0-0-0=noop&type0-0-0=noop&value0-0-0=&ctype=ics
> (bugs that have been resoved for 7 days)

If I subscribe a remote iCalendar with the URL from comment #22,
I don't see any tasks. If I store the ics file on my local disk
and subscribe to that file, I see tasks. Any ideas?

Comment 24

[Damian Szczepanik]

(In reply to comment #23)
> If I subscribe a remote iCalendar with the URL from comment #22,
> I don't see any tasks. If I store the ics file on my local disk
> and subscribe to that file, I see tasks. Any ideas?
which build do you have?
comment 22 works with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060907 Calendar/0.3a2+

I had this problem but opposite result, could subscribe but couldn't import from file

Comment 25

[Thomas Benisch]

(In reply to comment #24)
> which build do you have?
I used a Thunderbird debug build (20060817) and a Lightning debug
build (2006090817), both Windows.

Comment 26

[Thomas Benisch]

Attachment: memory calendar patch

With this patch also tasks in ics files are shown, if the entry date of
the task is before the first day in the view.

Comment 27

[Matthew (lilmatt) Willis]

Comment on attachment 237730 [details] [diff] [review]
memory calendar patch

r1=lilmatt

Yay for parens!

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 237730 [details] [diff] [review]
memory calendar patch

Looks good; thanks for the patch!

Comment 29

[Matthew (lilmatt) Willis]

"memory calendar patch" checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 30

[Stefan Sitter [:ssitter]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060917 Calendar/0.3a2+