Last Comment Bug 785733 - Move some properties to use icalString in database / Support extra parameters on attachments
: Move some properties to use icalString in database / Support extra parameters...
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 2.0
Assigned To: Philipp Kewisch [:Fallen]
:
Mentors:
Depends on: 785659 798783
Blocks: 498968 738078
  Show dependency treegraph
 
Reported: 2012-08-26 11:22 PDT by Philipp Kewisch [:Fallen]
Modified: 2012-10-06 06:39 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix - v1 (47.07 KB, patch)
2012-08-26 11:23 PDT, Philipp Kewisch [:Fallen]
matthew.mecca: review-
Details | Diff | Splinter Review
Fix - v2 (51.99 KB, patch)
2012-09-04 13:18 PDT, Philipp Kewisch [:Fallen]
matthew.mecca: review+
Details | Diff | Splinter Review

Description Philipp Kewisch [:Fallen] 2012-08-26 11:22:03 PDT
In bug 738078 we noticed that attachments in storage calendars don't have X-Props/IANA-Props preserved. Moving towards bug 498968 I have made these properties store their icalString instead of the single properties in DB fields.
Comment 1 Philipp Kewisch [:Fallen] 2012-08-26 11:23:42 PDT
Created attachment 655440 [details] [diff] [review]
Fix - v1
Comment 2 Philipp Kewisch [:Fallen] 2012-08-26 11:25:19 PDT
Requires the patch for bug 785659 applied.
Comment 3 Matthew Mecca [:mmecca] 2012-09-01 13:22:29 PDT
Comment on attachment 655440 [details] [diff] [review]
Fix - v1

Review of attachment 655440 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/test/unit/xpcshell.ini
@@ +21,4 @@
>  [test_freebusy.js]
>  [test_hashedarray.js]
>  [test_ics.js]
> +[test_ics_service.js]

Looks like this file is missing from the patch
Comment 4 Matthew Mecca [:mmecca] 2012-09-02 12:47:48 PDT
Comment on attachment 655440 [details] [diff] [review]
Fix - v1

Review of attachment 655440 [details] [diff] [review]:
-----------------------------------------------------------------

Aside from the missing test file it seems to work well.

::: calendar/base/src/calRecurrenceRule.cpp
@@ +565,5 @@
> +
> +    rv = prop->GetPropertyName(name);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (!name.EqualsLiteral("RRULE") && !name.EqualsLiteral("EXRULE")) {

If EXRULE is deprecated (and not fully supported anyway in http://mxr.mozilla.org/comm-central/source/calendar/base/src/calRecurrenceInfo.js#282 ) should we allow it here?

::: calendar/providers/storage/calStorageUpgrade.jsm
@@ +1497,5 @@
> +
> +                    let attendee = cal.createAttendee();
> +
> +                    attendee.id = aAttendeeId;
> +                    attendee.commonName = aCommonName;

The conversion throws errors with the corrupt organizer issue (see Bug 752163, the id and commonName properties are null) and results in a null icalString in the cal_attendees table. This seems to be no worse than it was before, but maybe we can use this opportunity to remove or correct the corrupt entries?
Comment 5 Philipp Kewisch [:Fallen] 2012-09-04 13:18:03 PDT
Created attachment 658200 [details] [diff] [review]
Fix - v2

(In reply to Matthew Mecca [:mmecca] from comment #4)

> Aside from the missing test file it seems to work well.
These damn test files! Added in v2.

> If EXRULE is deprecated (and not fully supported anyway in
> http://mxr.mozilla.org/comm-central/source/calendar/base/src/
> calRecurrenceInfo.js#282 ) should we allow it here?
I've taken it out, here and in the other place where EXRULE is specified. I don't think it will hurt.

> 
> The conversion throws errors with the corrupt organizer issue (see Bug
> 752163, the id and commonName properties are null) and results in a null
> icalString in the cal_attendees table. This seems to be no worse than it was
> before, but maybe we can use this opportunity to remove or correct the
> corrupt entries?

Tough call. Theoretically, yes. The only way to do I've come up with is to set the string to null if it throws and then DELETE the entries that are null. This will make other attendees go away if the block throws, but on the other hand if we can't convert the attendee the user is going to have to restore from a storage backup anyway (and we do make backups). See new patch for details.
Comment 6 Matthew Mecca [:mmecca] 2012-09-06 20:57:42 PDT
Comment on attachment 658200 [details] [diff] [review]
Fix - v2

r=mmecca
Comment 7 Stefan Sitter 2012-09-06 22:01:09 PDT
Comment on attachment 658200 [details] [diff] [review]
Fix - v2

You forgot to assign a new UUID to the changed interfaces. According to devmo "the uuid must be changed anytime any part of the interface or its ancestors are changed".
Comment 8 Philipp Kewisch [:Fallen] 2012-09-07 02:21:03 PDT
Indeed, good catch! I'll change the UUID before checkin.
Comment 9 Philipp Kewisch [:Fallen] 2012-09-17 11:10:34 PDT
Pushed to comm-central changeset e552b849d527

Note You need to log in before you can comment on or make changes to this bug.