The default bug view has changed. See this FAQ.

Propagation of sequence in exceptions in calStorageCalendar

RESOLVED FIXED in 1.9

Status

Calendar
Provider: Local Storage
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Laurent Jouanneau, Assigned: Laurent Jouanneau)

Tracking

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
The OBM calendar provider uses the calStorageCalendar for local storage. It needs to have same sequence into exceptions, when an item is modified.
(Assignee)

Comment 1

5 years ago
Created attachment 627949 [details] [diff] [review]
The patch, v1

Here is a patch. I used a property to toggle the behavior.
Attachment #627949 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 627949 [details] [diff] [review]
The patch, v1

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

r-, but just to fix the below comments. I would also appreciate a unit test, since this is a feature not used by the main code.


Laurent, do you have time to refresh the patch and push this? Or should I take over?

::: calendar/providers/storage/calStorageCalendar.js
@@ +534,5 @@
> +        if (this.getProperty("propagate.sequence")) {
> +            // Ensure the exception, its parent and the other exceptions have the
> +            // same sequence number, to make sure we can send our changes to the
> +            // server if the event has been updated via the blue bar
> +            let newSequence = aNewItem.getProperty("SEQUENCE") ? aNewItem.getProperty("SEQUENCE") : "0";

The only concern I have here is that this will set SEQUENCE:0 number on all exceptions even if the item doesn't have a SEQUENCE number to start with. I would rather see:

let newSequence = aNewItem.getProperty("SEQUENCE");
this._propagateSequence(modifiedItem, newSequence);

and then...

@@ +2624,5 @@
> +            var exceptions = rec.getExceptionIds ({});
> +            if (exceptions.length > 0) {
> +                for each (exid in exceptions) {
> +                    let ex = rec.getExceptionFor(exid);
> +                    ex.setProperty("SEQUENCE", newSequence);

...

if (newSequence) {
  ex.setProperty("SEQUENCE", newSequence);
} else {
  ex.deleteProperty("SEQUENCE");
}
Attachment #627949 - Flags: review?(philipp) → review-
(Assignee)

Comment 3

5 years ago
I will refresh the patch this week. 

For unit test, I will try to provide one but it seems to be complex to test, since we have to create a calendar, some events etc.. Is there a framework to test calendars? (I don't talk about how xpcshell tests work, but how could we test calendars features)
(Assignee)

Comment 4

5 years ago
I think I found how to test this case. Patch soon.
Created attachment 654437 [details] [diff] [review]
Final Patch
Attachment #654437 - Flags: review+
Pushed to comm-central changeset 29fbdc5bfaad
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.9
You need to log in before you can comment on or make changes to this bug.