Closed Bug 336175 Opened 18 years ago Closed 16 years ago

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

Categories

(Calendar :: Import and Export, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: blair, Assigned: Hb)

References

()

Details

Attachments

(3 files, 4 obsolete files)

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.
confirmed on latest trunk version.
Assignee: base → michael.buettner
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Attached patch patch v1Splinter Review
tasks were generally ignored while exporting to HTML/CVS, so i simply added support for them.
Attachment #226149 - Flags: first-review?(jminta)
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.
Attachment #226149 - Flags: first-review?(jminta) → first-review-
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.
*** Bug 360347 has been marked as a duplicate of this bug. ***
Summary: Sunbird/Lightning HTML/CSV "Export Calendar" function fails when iCal subcomponet does not contain DTSTART → Sunbird/Lightning HTML/CSV "Export Calendar" function fails when iCal subcomponent does not contain DTSTART
Blocks: 359083
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
OS: Windows XP → All
Hardware: PC → All
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.
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
Summary: Sunbird/Lightning HTML/CSV "Export Calendar" function fails when iCal subcomponent does not contain DTSTART → Export calendar to CSV format fails if a task exists in calendar
Assignee: michael.buettner → nobody
Status: ASSIGNED → NEW
No longer blocks: 359083
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)

outlook 2000 only exports events to csv so no problem with just the events for me, how do other versions do this?
Depends on: 359083
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.
Assignee: nobody → hb
Status: NEW → ASSIGNED
Attachment #304851 - Flags: review?(mvl)
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.
Attachment #304851 - Attachment is obsolete: true
Attachment #305140 - Flags: review?(mvl)
Attachment #304851 - Flags: review?(mvl)
Component: Internal Components → Import and Export
Flags: wanted-calendar0.8?
Flags: wanted-calendar0.8? → wanted-calendar0.8+
QA Contact: base → import-export
No longer depends on: 359083
Attached patch Patch_v3 now against (obsolete) — Splinter Review
Attachment #305140 - Attachment is obsolete: true
Attachment #306408 - Flags: review?(mvl)
Attachment #305140 - Flags: review?(mvl)
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.
Attachment #306408 - Attachment is obsolete: true
Attachment #306489 - Flags: review?(mvl)
Attachment #306408 - Flags: review?(mvl)
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?
Attachment #306489 - Flags: review?(mvl)
Do not set to RESOLVED after checking this in. We still want a warning message.
Attachment #306489 - Attachment is obsolete: true
Attachment #306802 - Flags: review?(mvl)
Attachment #306489 - Flags: review?(mvl)
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)
Attachment #306802 - Flags: review?(mvl) → review+
Keywords: checkin-needed
Comment added, bonuspoints++ :)

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 0.8
You need to log in before you can comment on or make changes to this bug.