Closed Bug 362496 Opened 13 years ago Closed 13 years ago

Importing .CSV causes Javascript error

Categories

(Calendar :: Import and Export, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Sunbird 0.5

People

(Reporter: halcyon1234, Assigned: jminta)

Details

Attachments

(2 files)

On a new install of 2006-11-30-04-trunk/, on both Windows 2000 and Windows XP, the following occurs:

1)  Create a new event
2)  Set event name, time, etc.
3)  Set an event Alarm
4)  Export as .csv
5)  Import the same .csv

Error is:

Unable to read form file:C:\temp\home.csv [Exception.."[JavaScript Error: "rd has no properties" {file:"file:///c:/Program%20Files/Calendar/js/calOutlookCSVImportExport.js" line: 354}]' when calling method: [callImporter::importFromStream]" nresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame::chrome://calendar/content/import-export.js :: loadEventsFromFile :: line 105" data:yes]
This is for Calender 2006-11-30-04-trunk/ Calendar/0.4a1
Summary: Importing CVS causes Javascript error → Importing .CSV causes Javascript error
Attached patch check for nullSplinter Review
This patch checks first to make sure our regex succeeded.  The calling code already does a nice job of handling a return of null here.  Note that I couldn't actually test this patch, because I can't seem to force a non-ics import on a Mac.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #247888 - Flags: second-review?(mvl)
Attachment #247888 - Flags: first-review?(lilmatt)
Comment on attachment 247888 [details] [diff] [review]
check for null

Moving this review to someone who has Outlook to test with.
Attachment #247888 - Flags: first-review?(lilmatt) → first-review?(ctalbert.moz)
Comment on attachment 247888 [details] [diff] [review]
check for null

I find only one case where the calling code can't handle a return of null. It is here:
 http://lxr.mozilla.org/seamonkey/source/calendar/import-export/calOutlookCSVImportExport.js#277

Please add an if(sDate) to protect this section and you have my r+. 

For now, as it stands, though I will r-. (Not exactly sure how to handle this).
Attachment #247888 - Flags: first-review?(ctalbert.moz) → first-review-
Comment on attachment 247888 [details] [diff] [review]
check for null

Changing my review flag. This is an r+ with the change at line 277 that I mentioned.
Attachment #247888 - Flags: first-review- → first-review+
(In reply to comment #6)
Did some testing with the patch as it stands. It can interpret the CSV file in question correctly, and it can also interpret a rather large recurrence intensive CSV file from outlook 2003.

Comment on attachment 247888 [details] [diff] [review]
check for null

r2=mvl
Attachment #247888 - Flags: second-review?(mvl) → second-review+
Hardware: PC → All
Whiteboard: [needs checkin]
Target Milestone: --- → Sunbird 0.5
Whiteboard: [needs checkin] → [needs checkin][litmus testcase wanted]
patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs checkin][litmus testcase wanted] → [litmus testcase wanted]
Whiteboard: [litmus testcase wanted]
Please review this patch https://bugzilla.mozilla.org/attachment.cgi?id=247888&action=diff

It tests if the date "rd" OR the time "rt" are invalid. In case of a valid date and an invalid time the whole function breaks. This contradicts the test just seven lines below where for events with missing times the "All Day Events" flag is set.

Pleace replace in line 334:    if (!rd || !rt) {
with                      :    if (!rd ) {
Hb, the patch is already reviewed twice and checked in. Please file a new bug report if you think there is an error in the code.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.