379894 - "Show completed tasks" checkbox has no effect on CalDav calendar

Status: VERIFIED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Maxime Delorme]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.8.1.2pre) Gecko/20061023 SUSE/2.0.0.1-0.1 Firefox/2.0.0.2pre
Build Identifier: lightning 2007050103 for linux and thunderbird 2

the checkbox in the todo tab "Show completed tasks" do not have any consequences when it checked or not.
The bug is caused by the fact that ITEM_FILTER_COMPLETED_YES is not taken into account when sending report

Reproducible: Always

Steps to Reproduce:
1.select todo tab
2.uncheck the box
3.
Actual Results:  
all the todo completed are visible

Expected Results:  
should return only the pending To-Dos

Comment 1

[Maxime Delorme]

Attachment: a possible patch

This patch respect the caldav spec 
see : http://tools.ietf.org/html/rfc4791#section-7.8.9
The problem with this patch is that if the caldav server doesn't support  prop-filter, is-not-defined, text-match then the UI won't show off any todo
A better way, IMHO should have been to make apply the filter client side, but I'm not sure because if you want to use correctly CalDav capabilities, my patch is probably better

Your view point ?

Comment 2

[Bruno Browning]

First, thanks for the patch!

Your approach looks sound to me at first glance, but I'm not going to give it a thorough review right now, because:
* you haven't asked  :-)
* there's a patch already reviewed and waiting to be applied (after 0.5) in bug 378588 which will change this area of code. It probably would be easier for all concerned if you marked this bug as depending on 378588 and wrote the patch against a tree with that patch installed. Apologies if you already know this, but the patch should be against a Mozilla source tree (see http://developer.mozilla.org/en/docs/Creating_a_patch ) rather than against an installed Sunbird like this one is. Since it's a CalDAV patch, I'm probably the best person from whom to request review.
* As you point out, this patch depends on the server implementing particular parts of the CalDAV spec. It seems to work fine with the Apple CalDAV server, and with RSCDS, but in my testing it causes a regression against the current version of Cosmo (Cosmo can't parse the query and just returns events, which then show up in the Sunbird tasklist). That's not necessarily a showstopper, but it's good to mention such things along with a patch, if possible. If you're interested in Sunbird/CalDAV development and don't have access to these different server types, I may be able to help.

While it would be possible to apply the filter client-side, I think the correct approach is to insist that CalDAV servers follow the spec, as your patch does. I'll file a bug against Cosmo (which at any rate does not yet attempt to support CalDAV VTODOs and should be simply ignoring these queries) unless you'd prefer to do so yourself.

Thanks again for the patch. I hope you will revise, resubmit, and request review.

Comment 3

[Maxime Delorme]

(In reply to comment #2)
First I'm sorry because I haven't seen bug 378588, even if I looked at similar bug before filling this one:-/
At the moment this patch won't work with RSCDS because it don't support at the  prop-filter, is-not-defined, text-match.
But I could implemented it in RSCDS, it shouldn't be that hard.
I couldn't patch COSMO, I've never look at its code, and have no JAVA skills only PHP. 
I will look at your patch, and wait for it applied in the tree, then as you say 
>  I think the correct
>  approach is to insist that CalDAV servers follow the spec, as your patch does.
we should then try to implement more of the spec in caldav server.

Comment 4

[Maxime Delorme]

Now that bug 378588 is fixed, does someone can review this small patch ?
RSCDS (SVN version) support now the prop-filter, is-not-defined, text-match element, 
It should be interesting to have my patch REVIEW and if ok Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 5

[Stefan Sitter [:ssitter]]

(In reply to comment #4)
You need to request review from someone to get your patch reviewed. Check http://wiki.mozilla.org/Calendar:Module_Ownership to find the correct person to ask for review. But according to Comment #2 it seems that a new patch is required.

Comment 6

[Bruno Browning]

Following up on comment #5, just to make sure everything is clear:
* you need to submit a new version of the patch since the current one will no longer apply cleanly
* you need to request review using the form on the "Add an attachment" page, not via a bug comment. Set the review dropdown to ? and the Requestee to the appropriate module owner. In this case that's [e-mail]; for other patches to other modules please consult the page Stefan cites in comment #5.
* Thanks for the patch!

Comment 7

[Maxime Delorme]

OK I will try tonight 

Comment 8

[Maxime Delorme]

Attachment: ask correct REPORT request to the server according to http://tools.ietf.org/html/rfc4791#section-7.8.9

I've tested this patch, by checking the query send to server, but unfortunately the server (RSCDS) doesn't work properly against this REPORT

Comment 9

[Bruno Browning]

This patch also does not work for me against my (admittedly somewhat dated) instance of the Apple CalDAV server - though according to my comment above the previous version of this patch did work for me against that same instance back in May. Since a review is usually a last step before code being committed, and since we are unlikely to commit non-working code, I think we ought to figure out why this isn't work before reviewing it.

I'll post some wiretrace and apachelog data showing what happens with this patch against an RSCDS server, and am cc'ing the RSCDS author on the bug. Will also update and retest wrt Apple. It's possible that the patch is good (as far as logic goes) and that we just need to wait for server implementations to grow up. 

As far as the non-logic parts of the patch go, you'll want to do some style cleanups: there's some badly-aligned code in there, and possibly a long line or two. Basically, you want to follow the style guide at http://developer.mozilla.org/en/docs/JavaScript_style_guide except that indents should be four spaces, not two.

Comment 10

[Bruno Browning]

Attachment: XML query and response from wiretrace, RSCDS server

Comment 11

[Bruno Browning]

Comment on attachment 273642 [details] [diff] [review]
ask correct REPORT request to the server according to http://tools.ietf.org/html/rfc4791#section-7.8.9

r- for now based on comment #9

Comment 12

[Bruno Browning]

patch works on current Apple CalendarServer if (and only if) the "and if not cancelled" part of the query is removed. I think we can live with this since our checkbox reads "Hide Completed Tasks", not "Hide Completed and Cancelled Tasks". 

RSCDS and Bedework both ignore the COMPLETED part of the query and return tasks regardless of the COMPLETED status. This at least allows users to use Tasks.

Cosmo, unfortunately, still (0.6.1) returns events to the query and thus spams the task list into unusability. Bug filed at OSAF.

Comment 13

[Maxime Delorme]

I am sorry but what I need to do for my patch being applied ?

Comment 14

[Martin Schröder [:mschroeder]]

Maxime, you should update your patch according comment#9 (last paragraph) and comment#12, attach it to this bug again and ask for review from Bruno Browning.

Comment 15

[Maxime Delorme]

Attachment: ask correct REPORT request to the server according to http://tools.ietf.org/html/rfc4791#section-7.8.9

fix mis-aligned code, line should not so long, remove

Comment 16

[Bruno Browning]

Comment on attachment 280798 [details] [diff] [review]
 ask correct REPORT request to the server according to http://tools.ietf.org/html/rfc4791#section-7.8.9 

looks good. This patch still causes "Hide Completed Tasks" to hide all tasks on RSCDS, but I think we can live with that and I understand it will be fixed soon with 0.8.1. Thanks for the patch!
r=browning

Comment 17

[Maxime Delorme]

Does I need to do something for the patch to be applied ?
Regards

Comment 18

[Martin Schröder [:mschroeder]]

Bruno, should this patch be checked in for 0.7 or post-0.7?

Comment 19

[Bruno Browning]

I've been intending to check it in all week but haven't found a moment. Will try to do so tomorrow morning - but if someone else can check it in first that's fine with me.

Comment 20

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 21

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.8pre) Gecko/20071002 Sunbird/0.7