Move some properties to use icalString in database / Support extra parameters on attachments

RESOLVED FIXED in 2.0

Status

Calendar
Internal Components
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 655440 [details] [diff] [review]
Fix - v1
Attachment #655440 - Flags: review?(matthew.mecca)
(Assignee)

Comment 2

5 years ago
Requires the patch for bug 785659 applied.
Blocks: 498968, 738078
Depends on: 785659

Updated

5 years ago
Summary: Move some properties to use icalString in database / Support extra aramters on attachments → Move some properties to use icalString in database / Support extra parameters on attachments
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
Attachment #655440 - Flags: review?(matthew.mecca) → review-
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?
(Assignee)

Comment 5

5 years ago
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.
Attachment #655440 - Attachment is obsolete: true
Attachment #658200 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #658200 - Flags: review? → review?(matthew.mecca)
Comment on attachment 658200 [details] [diff] [review]
Fix - v2

r=mmecca
Attachment #658200 - Flags: review?(matthew.mecca) → review+

Comment 7

5 years ago
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".
(Assignee)

Comment 8

5 years ago
Indeed, good catch! I'll change the UUID before checkin.
(Assignee)

Comment 9

5 years ago
Pushed to comm-central changeset e552b849d527
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0

Updated

5 years ago
Depends on: 798783
You need to log in before you can comment on or make changes to this bug.