Closed Bug 390492 Opened 15 years ago Closed 14 years ago

events with DURATION get serialized with DURATION and DTEND

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: randy, Assigned: dbo)

References

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: 2007062404

I subscribed to a remote calendar usiing CalDAV and an event with a DURATION appears correctly on the calendar.  When I used used Lightning to update the event by dragging the end time down a couple hours, the event gets sent to the CalDAV server with DTEND and DURATION, resulting in an error because that is invalid icalendar (only one of those properties is allowed).

Reproducible: Always

Steps to Reproduce:
1. subscribe to caldav calendar that contains an event with a DURATION
2. update with lightning to be +2 hours

Actual Results:  
.ics sent contains both DURATION and DTEND

Expected Results:  
.ics should have either DURATION or DTEND
This either sounds like a regression or a CalDAV only issue. Bug 317786 states that DURATION will be replaced with DTEND and DURATION is lost.
Confirming, and changing component to "Internal Components" as I can reproduce this on WebDAV/ICS as well as CalDAV. Cosmo refuses to accept the malformed .ics, so the drag does not work. WebDAV accepts the malformed .ics, and the drag appears to work, but on the next reload the views ignore the changed DTEND in favor of the unchanged DURATION, causing the item to (apparently) revert.
Status: UNCONFIRMED → NEW
Component: Provider: CalDav → Internal Components
Ever confirmed: true
Hi.  My name is Jared Rhine; I run the free CalDAV service Chandler Hub,  http://hub.chandlerproject.org (an instance of OSAF's Cosmo).  Interoperability with Mozilla Calendar products is very important to us; we want to actively encourage and welcome people to use Lightning/Sunbird with Chandler Hub.

This bug is currently a blocker to people using Lightning against Chandler Hub (and other sources like WebDAV as Bruno Browning mentions).  You can load an existing calendar into Lightning, but not make any changes.  In an upcoming blog post about interoperability, I think I'm going to have to say that Lightning/Sunbird is unfortunately read-only for now.

It seems like it might qualify for "blocking" status under the guidelines that Daniel Boelzle outlined here:

  http://article.gmane.org/gmane.comp.mozilla.devel.calendar/14117

under "bugs that prevent users from using the app".  But I don't want to futz with your bugzilla flags without some feedback.  Given Mozilla development priorities, is this issue worthy of a flag?  I'm not sure what the most popular providers in Lightning real-world use are, but this one is a big deal for us over at OSAF.  Our view is that sending both a DTEND and DURATION is pretty broken under the iCalendar specs and aren't sure there's a reasonable thing to do on the server side. 

Feedback welcome.  For reference, the OSAF bug is:

  https://bugzilla.osafoundation.org/show_bug.cgi?id=11147

Thanks for reviewing.
Flags: wanted-calendar0.8?
I agree that this bug needs to be fixed, but I think that "read-only" might be overstating things a little bit. I test Sunbird<-->Chandler on an almost daily basis, and had almost forgotten this bug, because Sunbird/Lightning can:
* create items in a Chandler Server calendar collection
* delete items from a Chandler Server calendar collection
* modify items in a Chandler Server calendar collection IF the item was created by Mozilla calendar code.

It can't modify items created on the Chandler side of the house. This is bad and the bug needs to be squashed, granted. But one can use CS purely as a CalDAV server for Sunbird/Lightning quite happily without ever seeing this bug - I do. That being said, I would like to see this fixed in the 0.8 timeframe.

Thanks for prompting me to update my CS to double-check on this bug. 
OS: Windows XP → All
Hardware: PC → All
Bruno wrote:
> I think that "read-only" might be overstating things a little bit.

Fair enough; when reviewing/documenting/blogging Lightning interop, I'll make sure to note that "pure" Lightning calendars should work properly and are well-tested.

> It can't modify items created on the Chandler side of the house.

Yes.  I went digging to see if we had any data about how many in-the-wild DURATIONs we see (outside Chandler Desktop/Web UI) but couldn't find much; I'm without an Evolution to check right now too.

Thanks for the various responses and flag adjustments.  I only wish I had some code to offer for the issue.
I should point out that my remarks in comment #4 were about the release version of Sunbird. Due to a recent checkin (Mozilla bug 373370), Sunbird now triggers OSAF bug 11140, and current Sunbird nightlies cannot PUT a new or modified item to Chandler Server. Presumably this will be fixed by the time of the next Sunbird release.
It would be good to ahve a solution for this in 0.8
Flags: wanted-calendar0.8? → wanted-calendar0.8+
When we get an item with DURATION instead of DTEND (such as created by Chandler) we're adding a DTEND property; in order to serialize to valid .ics one of those two props needs to be deleted. It's not clear, though, that every serialization needs to produce valid .ics (e.g. clipboard), nor that DURATION should in every case be the property preserved on serialization. So this patch adds a function to calUtils.js that can be used to ensure that an item has DTEND || DURATION, and uses it in the CalDAV and ICS providers to persist DURATION.

With this patch we can modify Chandler-created items.
Assignee: nobody → browning
Status: NEW → ASSIGNED
Attachment #290171 - Flags: review?(daniel.boelzle)
As written in Comment #1 this seems to be a regression. Can you elaborate the regression range? Maybe this helps to fix the real issue instead of adding a workaround.
Comment on attachment 290171 [details] [diff] [review]
new func for calUtils; fix for caldav, ics

I don't like this approach as it works around the inherent problem that a DTEND is enforced on reading. This IMO causes bug 317786, too.
Attachment #290171 - Flags: review?(daniel.boelzle) → review-
Attached patch fixes calEvent and calTodo (obsolete) — Splinter Review
I'd rather go for preserving DURATION unless the item's endDate/dueDate is explicitly set.
- Reading bug 317786, this patch should fix it, too.
- This patch fixes also that a todo's DURATION has not been minded (implying it's DUE); don't know if there's an existing bug.
- We need to keep care that all routines stick to calIEvent::endDate resp. calITodo::dueDate (not getProperty("DTEND"), because DTEND isn't enforced any longer.
Assignee: browning → daniel.boelzle
Attachment #290216 - Flags: review?(sebo.moz)
Comment on attachment 290216 [details] [diff] [review]
fixes calEvent and calTodo

The patch fails on Test 3 of the unit tests attached to bug 405251.
Thats an event without DTEND:

BEGIN:VEVENT                DTSTART:20020402T000000Z
END:VEVENT

minusing based on that.
Attachment #290216 - Flags: review?(sebo.moz) → review-
(In reply to comment #12)

> The patch fails on Test 3 of the unit tests attached to bug 405251.
> Thats an event without DTEND:

I wonder if this doesn't reflect a problem with the unit tests rather than (necessarily) the patch. RFC2445 (4.6.1) says that VEVENTs can have DTEND or DURATION but not both. The patch ensures the "not both" part. It looks to me like the unit tests are conflating two distinct cases: 1) VEVENTs with neither DTEND nor DURATION and 2) VEVENTs with DURATION but not DTEND. The first case should be an error; the second should not.

This patch additionally fixes calStorageCalendar to use start/end/entry/dueDate.

It strikes me that we need to fix the currently wrong exception/overridden item handling (e.g. bug 368976) soon, getting rid of the "unproxied" API.
What's IMHO deeply going wrong is that an exception/overridden item consults its parent item in case a property isn't present (calItemBase.mIsProxy is set on those). I suspect we already have exported wrong/incomplete ICS this way. I think the core reason for that decision was cheap occurrence (proxy) creation (which is honourable), but those proxies need to be COW (copy on write) wrappers. That way they would emerge into a deep copy once something writes on them, and enables those objects to actually undefine properties (bug 368976).
Attachment #290216 - Attachment is obsolete: true
Attachment #291010 - Flags: review?(sebo.moz)
I tested memory calendar and storage calendar with some events that have the DURATION property. For storageCalendar there is still a problem. Adding and then getting the event via getItems() returns an icalString with both DURATION and DTEND set. Memory Calendar works fine. Interestingly, using getItem() on storage the icalString looks fine. This might be because its cached.
Comment on attachment 291010 [details] [diff] [review]
fixes calEvent and calTodo, calStorageCalendar

Confirmed using the UI. Importing and subsequent exporting produces invalid ics (having both DTEND and DURATION).
minusing based on that.
Attachment #291010 - Flags: review?(sebo.moz) → review-
It's (like your previous tests) only a problem of storage calendar. We need to ignore either the stored endDate or DURATION. Problem is users may already have storage calendars with DURATION property imported. Because the previous code has always enforced DTEND (computed out of DURATION), I'd go and let endDate have precedence. However we've to admit that any DURATION property will be lost forever then.
(In reply to comment #18)
> It's (like your previous tests) only a problem of storage calendar. 
Yes, while your previous patch introduced a regression in storageCalendar, the second one does - as far as tested - not. It just doesn't fix the bug wrt storage. I'm fine with moving that to another bug, since this bug originally is about CalDAV.
> We need to
> ignore either the stored endDate or DURATION. Problem is users may already have
> storage calendars with DURATION property imported. Because the previous code
> has always enforced DTEND (computed out of DURATION), I'd go and let endDate
> have precedence. However we've to admit that any DURATION property will be lost
> forever then.
> 
Since we loose the duration property on modification of the duration/endDate anyway, that doesn't sound too bad.
(In reply to comment #19)
> (In reply to comment #18)
> > It's (like your previous tests) only a problem of storage calendar. 
> Yes, while your previous patch introduced a regression in storageCalendar, the
> second one does - as far as tested - not. It just doesn't fix the bug wrt
> storage. I'm fine with moving that to another bug, since this bug originally is
> about CalDAV.
Even though storage is not mentioned in the bug, it makes sense to fix it here, too. BTW: Why do you think the previous patch regressed calStorageCalendar? IIRC it just changed calTodo and calEvent.
(In reply to comment #20)
> BTW: Why do you think the previous patch regressed calStorageCalendar?
> IIRC it just changed calTodo and calEvent.
Because unit test #3 failed with the patch applied while it worked without the patch.
(In reply to comment #21)
Even today, if you import an event with DURATION (into storage calendar) and export it, you'll get both DTEND and DURATION; regardless of that patch.
(In reply to comment #22)
There must be a misunderstanding going on. Let me try to summarize:

* storageCal suffers from the same issue as mentioned in this bug (and bug 317786)
* attachement 290216 fixed this for memoryCal but regressed storageCal in a rather unrelated area (see comment 12)
* attachement 291100 fixed this for memoryCal and removed the regression in storageCal. StorageCal still suffers from the issue mentioned in this bug.

Hope that clarifies things.
Blocks: 405251
new version, depends on fixes for bug 405459
Attachment #290171 - Attachment is obsolete: true
Attachment #291010 - Attachment is obsolete: true
Attachment #291657 - Flags: review?(sebo.moz)
Comment on attachment 291657 [details] [diff] [review]
fixes calEvent and calTodo, calStorageCalendar

Works as advertised. r=sebo
Should we also bother to remove all duration properties from the database?
Attachment #291657 - Flags: review?(sebo.moz) → review+
(In reply to comment #25)
> Should we also bother to remove all duration properties from the database?
Hmm, I am not sure about that. At the end, I'd like to have a storage provider that could both store events with just DTEND and events without DTEND but DURATION (and properly roundtrip those).
However, IMO this would require a major rewrite.


Checked in on HEAD and MOZILLA_1_8_BRANCH and FIXED.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
checked in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.