759324 - Propagation of sequence in exceptions in calStorageCalendar

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, enhancement)

Description

[Laurent Jouanneau]

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

Comment 1

[Laurent Jouanneau]

Attachment: The patch, v1

Here is a patch. I used a property to toggle the behavior.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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");
}

Comment 3

[Laurent Jouanneau]

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)

Comment 4

[Laurent Jouanneau]

I think I found how to test this case. Patch soon.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Final Patch

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 29fbdc5bfaad