Last Comment Bug 759324 - Propagation of sequence in exceptions in calStorageCalendar
: Propagation of sequence in exceptions in calStorageCalendar
Product: Calendar
Classification: Client Software
Component: Provider: Local Storage (show other bugs)
: Trunk
: All All
-- enhancement (vote)
: 1.9
Assigned To: Laurent Jouanneau
Depends on:
  Show dependency treegraph
Reported: 2012-05-29 07:02 PDT by Laurent Jouanneau
Modified: 2012-08-22 17:09 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

The patch, v1 (2.48 KB, patch)
2012-05-29 07:05 PDT, Laurent Jouanneau
philipp: review-
Details | Diff | Splinter Review
Final Patch (5.28 KB, patch)
2012-08-22 17:08 PDT, Philipp Kewisch [:Fallen]
philipp: review+
Details | Diff | Splinter Review

Description User image Laurent Jouanneau 2012-05-29 07:02:30 PDT
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 User image Laurent Jouanneau 2012-05-29 07:05:35 PDT
Created attachment 627949 [details] [diff] [review]
The patch, v1

Here is a patch. I used a property to toggle the behavior.
Comment 2 User image Philipp Kewisch [:Fallen] 2012-06-07 05:23:51 PDT
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 {
Comment 3 User image Laurent Jouanneau 2012-06-10 15:05:55 PDT
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 User image Laurent Jouanneau 2012-06-14 03:34:24 PDT
I think I found how to test this case. Patch soon.
Comment 5 User image Philipp Kewisch [:Fallen] 2012-08-22 17:08:31 PDT
Created attachment 654437 [details] [diff] [review]
Final Patch
Comment 6 User image Philipp Kewisch [:Fallen] 2012-08-22 17:09:15 PDT
Pushed to comm-central changeset 29fbdc5bfaad

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