336175 - Export calendar to CSV format fails if a task exists in calendar

Status: RESOLVED FIXED

(Calendar :: Import and Export, defect)

Description

[Blair Sutton]

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

When exporting a calendar that has iCal subcomponents that do not contain a DTSTART value, Sunbird and Lightning both respond with unableToWrite. This appears due to the sort function in calHtmlExport.js (line 100). Similar problems also occur for exporting to CSV. 

Reproducible: Always

Steps to Reproduce:
1. Subscribe to a remote calendar using sample ICS provided (i.e. file://c:/calendars/sample.ics. 
2. In lightning or sunbird right click calendar and choose "Export Calendar"
3. Select either HTML or CSV format to export and a file name.

Actual Results:  
Dialog appears "Error getting calendar" stating "Unable to write to file: <path as provided in steps-to-reproduce>".
An incomplete file is produced.

Expected Results:  
A file should be produced without an error message. The file should be complete.

Comment 1

[Blair Sutton]

Attachment: ICS sample that exhibits failure

Comment 2

[Michael Büttner (no reviews TFN)]

confirmed on latest trunk version.

Comment 3

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

tasks were generally ignored while exporting to HTML/CVS, so i simply added support for them.

Comment 4

[Joey Minta]

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

+    aItems.sort(
+        function (a, b) {
I don't think this is the sorting algorithm we want here.  For this layout, I would expect events to be first.  Also, tasks can be sorted the same way as events.  First check entryDates, if those match or aren't present, then check dueDates.

There also seems to be a fair bit of code duplication between the event and task cases.  Can we parametrize those to avoid that?

Lastly, import/export is definitely mvl's code, please ask for review from him.

Comment 5

[Robert "Unlogic" Olofsson]

This bug is still present in Lightning 0.4a1 build 2006103107. Exporting both local and remote calendars to HTML produce the message described in this bug.

Comment 6

[Stefan Sitter [:ssitter]]

*** Bug 360347 has been marked as a duplicate of this bug. ***

Comment 9

[Paul Ivanov]

confirming bug on Linux as well

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2pre) Gecko/20070130 Calendar/0.4a1

Comment 11

[Daniel Boelzle [:dbo]]

status update: bug 368066 has fixed the foremost bugs, thus the html export doesn't choke anymore on tasks.
still open: sorting order UI review, maybe a different/better presentation of tasks, and of course the csv export.

Comment 12

[Stefan Sitter [:ssitter]]

Updating summary to reflect the remaining issue with export to CSV format.
Currently Sunbird/Lightning fails with the following error on every task:

Error: aDateTime has no properties
Source File: file:///D:/sunbird/js/calOutlookCSVImportExport.js
Line: 453

Comment 13

[:Hb]

The patch for bug 359083 allows to export complete calendars which contain todo items to CSV format too. 

It changes calOutlookCSVImportExport.js to exports only event items. Todo items ares skipped in the function csv_exportToStream(...) with "if (item
instanceof Components.interfaces.calIEvent) {...}".

A complete file is available at bug 359083 attach 299692 The status of this bug could be changed to WORKSFORME.

Further decision is necessary how calendars with events and todos should be exported: 
1. Into one csv file with two dozen columns (easy to code)
2. Using two files (Two file open dialogs? Extend the first file name with "_tasks"?)
3. Leaving todos unexportable to csv (proposed patch)

Comment 14

[Bas van den Bosch]

outlook 2000 only exports events to csv so no problem with just the events for me, how do other versions do this?

Comment 15

[:Hb]

Attachment: Patch for calOutlookCSVImportExport.js

Bug resolution:

> +        if (isEvent(item)) {

Check if it's an event and not a todo item before filling the output line. 


Resulting changes:

Following lines more intended. Two (sloppy ? if : else ) lines replaced with regular if (cond) { } else { } constructs.


Enhancements:

> -    dateFormat      : "%m/%d/%y",
> +    dateFormat      : "%m/%d/%Y",

Output four digit years in English locale as default.

> +            line.push(noNullString(item.getProperty("CATEGORIES")));
> +            line.push(noNullString(item.getProperty("DESCRIPTION")));
> +            line.push(noNullString(item.getProperty("LOCATION")));
> -        line.push(item.getProperty("CATEGORIES"));
> -        line.push(item.getProperty("DESCRIPTION"));
> -        line.push(item.getProperty("LOCATION"));
> +
> +function noNullString(tstVar) {
> +    if (tstVar) {
> +        return tstVar;
> +    } else {
> +        return "";
> +    }
> +}

Output "" and not "null" for empty categories, descriptions or locations.

Comment 16

[:Hb]

Attachment: Patch_v2 for calOutlookCSVImportExport.js limited to bug resolution

Small patch for an quick interim solution for the CSV part of this bug. Tasks are simply skipped in CSV output.

Note to reviewer: Indentation is broken in the enclosed part for not criss crossing the patch for bug 301750.

If it is wanted to export events and tasks this couldn't be done into one CSV file because Outlook is unable to distinguish them. After version 0.8 the array with the to-be-exported items has to be searched. If it contains events and tasks the user has to be asked which type he wants to export.

Comment 17

[:Hb]

Attachment: Patch_v3 now against

Comment 18

[:Hb]

Attachment: Patch_v3 now against current tree

New Diff with Bug resolution:

> +        if (isEvent(item)) {

Check if it's an event and not a todo item before filling the output line. 

Resulting changes:

Following lines more intended. Two sloppy (cond ? if : else ) lines replaced with
regular if (cond) { } else { } constructs. The one-liners disturb venkman.

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 306489 [details] [diff] [review]
Patch_v3 now against current tree

>+        if (isEvent(item)) {
How about this instead:
if (!isEvent(item))
  continue;

That would not only reduce the size of the patch, it will also don't over-indent the source code. And, I think it improves readabitiliy, because it's clear that only events are supported. You don't need to go searching for an else block.

>-        line.push((item.privacy=="PRIVATE") ? localeEn.valueTrue : localeEn.valueFalse);

I don't see what's wrong with it. I like that line, it's short and readable. I don't think it is sloppy in any way. What is the problem with venkman on this line? And isn't that a venkman bug?

Comment 20

[:Hb]

Attachment: Patch_v4 minimalistic

Do not set to RESOLVED after checking this in. We still want a warning message.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 306802 [details] [diff] [review]
Patch_v4 minimalistic

r=mvl.
bonus points if you add a code comment like:
// XXX todo: warn the user (once) that tasks are not supported (bug 336175)

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Comment added, bonuspoints++ :)

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED