315051 - Alarms disappearing in remote calendars

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Torsten]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051103 Mozilla Sunbird/0.3a1

All alarms of events and tasks in remote calendars are not recognized in Sunbird 0.3a1 when subscribing to these remote calendars. The edit dialog box only reports "not specified" in the Alarm-field.

Even when I add an alarm it disappears after reloading the remote
clandar although the calendar file on the remote server still has the lines

BEGIN:VALARM
TRIGGER;VALUE=DURATION:-PT15M
END:VALARM

in the entry of the related event.

Reproducible: Always

Steps to Reproduce:
1. Specify an alarm for an event or task in a remote calendar
2. Reload remote calendars
3.

Actual Results:  
The alarm specified does not trigger and it has disappeared in the edit dialog box of the event/task although the remote calendar file still contains the BEGIN:VALARM
TRIGGER;VALUE=DURATION:-PT15M
END:VALARM
entry.

Expected Results:  
The alarm should trigger and show up correctly in the edit dialog box.

Could the described behaviour be a result of the suppressAlarmsByDefault attribute described in bug 257428?

Comment 1

[Torsten]

Alarms are also disappearing during imports. Steps to reproduce:

1. Create an event with an alarm in a local calendar
2. Select this event and export it to an ics-file
3. Delete the event in the local calendar
4. Import the file saved in step 2 into this calendar
5. The "Edit Event" dialog box shows "not specified" for alarms

Comment 2

[Joey Minta]

Can we get a testcase?

Comment 3

[Torsten]

Attachment: ics-file with one event with an alarm

The attachment is an ics-file with one event with an alam. 
Steps to reproduce the bug:

1. Create a remote calendar (WebDav, location: file:///C:/.../test.ics). 
2. Open the "Edit Event" dialog box.
3. Alarms are "not specified" although there is an alarm in test.ics.
4. Specify an alarm in the dialog box and click "OK".
5. Reopen the "Edit Event" dialog box, the alarm is still there.
6. Reload the remote calendar.
7. Reopen the "Edit Event" dialog box, the alarm has gone.

Alternatively:

1. Import the file test.ics into a local calendar. 
2. Open the "Edit Event" dialog box.
3. Alarms are "not specified" although there is an alarm in test.ics.

Comment 4

[Joey Minta]

Attachment: set alarmTime

Because we never set an alarmTime when getting the item's properties from ICS, this sort of thing gets ignored in the eventDialog.  The ugly part of this patch is that itemBase should try to remain independent of events/tasks, but alarmTime needs to be based off something, which requires getting a start/entry/end/due date.  I don't see any other way, but I'm open to better suggestions.

Passes the testcase for subscribing and for importing

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 202722 [details] [diff] [review]
set alarmTime

Won't the real fix be to make .alarmTime be generated in the getter? Then you know it's always correct and always set, instead of hoping that all code that sets anything alarm related also recalculates the alaram time.

Comment 6

[Joey Minta]

(In reply to comment #5)
> Won't the real fix be to make .alarmTime be generated in the getter? Then you
> know it's always correct and always set, instead of hoping that all code that
> sets anything alarm related also recalculates the alaram time.

This would make things pretty difficult.  To me, it doesn't make sense to create a getter that does all the computations, but not have a setter that doesn't compute everything (alarmUnits, alarmLength, alarmRelated) as well.  This, however, makes us much more fragile, if someone tries to set these properties without going through the setter.  (What's more, how does a simple set alarmTime(val) {} know what to relate the time to?)

I only see a few options here, none of which I really like.
1.) Force all callers to compute/recalculate alarm stuff.  In this case, we should change alarmTime to hasAlarm, so that callers don't try to read it as a time that might not have been updated.  The 'hasAlarm' attribute would be basically (alarmRelated && alarmUnits && alarmLength), assuring our callers that they can validly work with the alarm for computations.  This has the advantage that callers know whether they have an event or todo, and therefore it makes more sense for them to determine whether they should work off of entryDate or startDate.
2.) Much like recurrence-info, separate alarm-info out into its own separate implementation of a data-object, with its own component/idl.  This could nicely expose certain attributes as getters only, to avoid the problems of fragile code I mentioned above.  Also, Alarms may get more complicated, especially once we eventually try to set them as acknowledged or not, so this has the advantage of being more readily expandable.
3.) ??? I can't think of anything else, but there has to be a better way.  CC'ing some people for ideas.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

I think that 1 comes closest to the current situation. The event dialog set the alarm-related properties, and also calculates the alarmtime.
What i'm suggesting is that |get alarmTime() {}| caculates that alarmtime. It can do that, because it is part of the event, and knows the startTime, endTime, what to relate to etc.
Then the evetndialog just sets what to relate the alrm to, the duration before or after etc, and the alarmService can get alarmTime, and will get the right results.

As an alternative, iirc only the alarmService needs to know the alarmTime. We could also do the calculation in there.

Comment 8

[Simon Paquet [:sipaq]]

*** Bug 317117 has been marked as a duplicate of this bug. ***

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

*** Bug 323884 has been marked as a duplicate of this bug. ***

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 202722 [details] [diff] [review]
set alarmTime

You could move the event and task code into the corrosponding .js files, in  the icalComponent setter

Comment 11

[Joey Minta]

So what I'd like to do here is change the idl to only report the duration (offset) of the alarm.  Anyone who wants the time should do the computation themselves.  This will indirectly solve bug 325143.  It will, however, require a schema upgrade for the storage calendar to change from storing a time to storing a duration.  Given that bug 324970 will also require a schema upgrade I'd further like to propose that I either handle these bugs together, or at least create patches that are designed to land simultaneously.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #11)
> So what I'd like to do here is change the idl to only report the duration
> (offset) of the alarm.  Anyone who wants the time should do the computation
> themselves.

Is the intent for this to be a permanent change, or just temporary while waiting for the larger alarms overhaul?

Comment 13

[Joey Minta]

(In reply to comment #12)
> (In reply to comment #11)
> > So what I'd like to do here is change the idl to only report the duration
> > (offset) of the alarm.  Anyone who wants the time should do the computation
> > themselves.
> 
> Is the intent for this to be a permanent change, or just temporary while
> waiting for the larger alarms overhaul?
> 
Eventually I think we're going to need to offer both, since ics calendars can offer either (and if the cache uses storage, it will need to persist these).  In the code-drop of the alarm overhaul, I consolidated these with a getFiringTimes() function that would allow consumers to get times regardless of how the alarm was constructed, but the duration or time was also stored.  So, I wouldn't call this temporary, but merely subject to subsequent revision.

Comment 14

[Joey Minta]

-> me

Comment 15

[Joey Minta]

Attachment: proposed idl changes

Proposed changes to the alarm idl needed to fix both this and bug 324970.  The alarm service, and anyone else who wants the actual alarm time, will be responsible for actually computing the correct time.

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 210772 [details] [diff] [review]
proposed idl changes

>+  attribute calIDuration alarmOffset;
>+  attribute AUTF8String alarmRelated;
>+  attribute calIDateTime lastAlarmAck;

I think this interface looks good, but there's one thing i'm wondering: how will snooze work? Will that just set a new alarmOffset? In that case, lastAlarmAck is only to check for missed alarms? (nit: can we rename it to alarmLastAck, so that the 'alarm' part is some kind of semi-namespace?)

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

How about making alarmRelated be an |unsigned long| with a couple of constants defined for START and END so that if there are ever any C++ callers, they get compiler type-checking goodness.

Comment 18

[Joey Minta]

Attachment: patch v1

Phew... so here it is.  This patch ought to fix this bug, bug 324970, and bug 325143.  It'll also put us in a position where I finally feel our alarm system is at a high enough quality that we can close most of the 0.2 alarm bugs.  Changes included here:

-Start storing alarm offsets, rather than alarm times.
-Start storing the time the alarm was acknowledged.  At startup, we check up to 1 month in the past for alarms that may not have been fired, because the program was closed.
-Upgrade the storage provider to handle these new fields.  As we discussed in IRC, this will cause some issues if you go back to older builds once your profile has been upgraded.  Forward upgrading should happen silently and smoothly.
-Check for alarms on events up to 1 month in the future.  Previously, we only got events in a 6 hours span, which meant that alarms set for the day before an event were never found.  1 month seems a reasonable time length in the future to look.  We may want to rel-note that alarms more than 30days before an event will not be found.
-Contains a fix to libical for a bug in adding negative durations to datetimes.
-Makes the icalString of a datetime read-write rather than read-only.

I'd like to ask that, as much as possible, we keep the number of iterations before this patch goes in to a minimum.  Trying to keep track of which trees and which profiles of mine are working with which storage versions is a pain.  I've given it some decent testing and everything seems fine, but I added some extra debug code to hopefully get wider QA on it prior to 0.1 release.  In my opinion, that's really the only way we can truely test this stuff.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 211510 [details] [diff] [review]
patch v1

In general this looks good.  Some misc thoughts:

* can you hide the snooze button for 0.1 as we discussed on IRC yesterday?  The snooze-then-move behavior is weird enough that I think we want to avoid it.  Also, file a new bug to do the better thing for snooze in the future: make snooze alarms separate VALARM objects.

* can you add a comment in the relnotes bug that we should relnote the dismiss-then-move behavior?

* do we want makeMemberAttr calls in calItemBase.js for the three new alarm attributes?

* should we bump the producer ID in our ICS for this change?  I kinda think we want to.


>Index: calendar/base/content/calendar-event-dialog.js
>===================================================================
>
> [...]
>
>+        if (document.getElementById("alarm-trigger-relation").selectedItem.value == "START") {
>+            item.alarmRelated = item.CAL_ALARM_RELATED_START;
>+        } else {
>+            item.alarmRelated = item.CAL_ALARM_RELATED_END;
>         }
>+        var duration = Components.classes["@mozilla.org/calendar/duration;1"]
>+                                 .createInstance(Components.interfaces.calIDuration);
>+        duration.isNegative = true;
>+        duration[alarmUnits] = alarmLength;

The UI exposes alarm-trigger-relation as "before" and "after", which is (arguably, at least) a reasonable thing to do.  However, that means that for the "after" case, duration.isNegative should be false.

>-  attribute calIDateTime alarmTime;
>+  const unsigned long CAL_ALARM_RELATED_START = 0;
>+  const unsigned long CAL_ALARM_RELATED_END = 1;

I'd suggest dropping the CAL_ prefix here, since we're already inside of a "cal" interface.  Also, the convention with consts is to list them directly after the attribute/method for which they are intended.

>     /* calIAlarmService APIs */
>     snoozeEvent: function(event, duration) {
>         /* modify the event for a new alarm time */
>+        var newEvent = event.clone();
>         var alarmTime = jsDateToDateTime((new Date())).getInTimezone("UTC");
>+
>+        // Set the last acknowledged time to now.
>+        newEvent.alarmLastAck = alarmTime;
>         alarmTime.addDuration(duration);
>-        newEvent = event.clone();
>-        newEvent.alarmTime = alarmTime;
>+
>+        var datetime;
>+        if (newEvent.alarmRelated == "START") {

This wants to be one of the constants, not a string, I think.

>@@ -230,16 +225,21 @@ calAlarmService.prototype = {
> 
>         var observerSvc = Components.classes["@mozilla.org/observer-service;1"]
>                           .getService
>                           (Components.interfaces.nsIObserverService);
> 
>         observerSvc.addObserver(this, "profile-after-change", false);
>         observerSvc.addObserver(this, "xpcom-shutdown", false);
> 
>+        /* tell people that we're alive so they can start monitoring alarms */
>+        this.notifier = Components.classes["@mozilla.org/embedcomp/appstartup-notifier;1"].getService(Components.interfaces.nsIObserver);
>+        var notifier = this.notifier;
>+        notifier.observe(null, "alarm-service-startup", null);
>+

Why move this clause here?  (I don't see anything wrong with it, I'm just curious what the motivation is).

>-        // figure out the 'now' and 6 hours from now and look for events 
>-        // between then with alarms
>-        this.mRangeStart = jsDateToDateTime((new Date())).getInTimezone("UTC");
>+        var now = jsDateToDateTime((new Date())).getInTimezone("UTC");
> 
>-        var until = this.mRangeStart.clone();
>-        until.hour += kHoursBetweenUpdates;
>+        var start;
>+        if (!this.mRangeEnd) {
>+            // This is our first search for alarms.  We're going to look for
>+            // alarms +/- 1 month from now.  If someone sets an alarm more than
>+            // a month ahead of an event, or doesn't start Sunbird/Lightning
>+            // for a month, they'll miss some, but that's a slim chance

Some providers (i.e. CalDAV) will be able to search for alarms in a given time period automagically, so I think that in the future, this code may want to be per-provider.  Can you file a bug for calICalendar.getItems to provide an API for alarm-searching?

>@@ -204,16 +202,20 @@ calItemBase.prototype = {
> 
>             m.mProperties.setProperty (prop.name, val);
>         }
> 
>         m.mDirty = false;
> 
>         // these need fixing
>         m.mAttachments = this.mAttachments;
>+
>+        m.alarmOffset = this.alarmOffset;
>+        m.alarmRelated = this.alarmRelated;
>+        m.alarmLastAck = this.alarmLastAck;

Don't alarmLastAck and alarmOffset need to have makeImmutable() called on them?  Similarly, shouldn't they also be cloned in cloneItemBaseInto()?

>@@ -429,17 +431,16 @@ calItemBase.prototype = {
>         "DTSTAMP": true,
>         "X-MOZILLA-GENERATION": true,
>         "RRULE": true,
>         "EXDATE": true,
>         "RDATE": true,
>         "ATTENDEE": true,
>         "ORGANIZER": true,
>         "RECURRENCE-ID": true,
>-        "ALARMTIME": true,
>     },

All three new properties added are promoted to top-level properties and have special serialization code.  Since they're not listed here, I think the storage provider (the only caller of isPropertyPromoted) may try and serialize them an extra time.  So I _think_ they need to be listed here.  Might be worth testing to see if my analysis is correct.
 
>+            if (this.alarmLastAck) {
>+                var lastAck = icssvc.createIcalProperty("X-MOX-LASTACK");

This wants to be "X-MOZ-LASTACK", I bet.

>+                lastAck.valueAsIcalString = this.alarmLastAck.icalString;
>+                alarmComp.addProperty(lastAck);
>+            }
>+
>+            // We don't use this, but the ics-spec requires it
>+            var descProp = icssvc.createIcalProperty("DESCRIPTION");
>+            descProp.value = "Mozilla Alarm: "+ this.title;
>+            alarmComp.addProperty(descProp);

Can you file a separate bug on figuring out a better way to deal with "DESCRIPTION" in the future?  I think it may make sense to display it in the UI.

I still haven't reviewed the calStorageCalendar.js changes; I'll do that soon.  In the meantime, I think most of the comments I made above won't effect that code much, so feel free to either produce another patch iteration now, or wait until I finish reviewing.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 211510 [details] [diff] [review]
patch v1

>+                addColumn(this.mDB, "cal_events", "alarm_last_ack", "VARCHAR");
>
> [...]
>
>+                addColumn(this.mDB, "cal_todos", "alarm_last_ack", "VARCHAR");

alarm_last_ack wants to be of type INTEGER in both tables, and also in the schema lines at the end of the file.

>+                this.mDB.executeSimpleSQL("DELETE FROM cal_calendar_schema_version; INSERT INTO cal_calendar_schema_version VALUES (5);");

How about using UPDATE instead of DELETE;INSERT here?

>@@ -1184,18 +1218,52 @@ calStorageCalendar.prototype = {
>             item.title = row.title;
>         if (row.priority)
>             item.priority = row.priority;
>         if (row.privacy)
>             item.privacy = row.privacy;
>         if (row.ical_status)
>             item.status = row.ical_status;
> 
>-        if (row.alarm_time)
>-            item.alarmTime = newDateTime(row.alarm_time, row.alarm_time_tz);
>+        if (row.alarm_time) {
>+            // Old (schema version 4) data, need to convert this nicely to the
>+            // new alarm interface

Can you add more info in this comment saying that we will eventually be supporting alarm_time, which makes this clause temporary, and that this is why it's reasonable for this code to live here, rather than in upgradeDB, where it otherwise would.

>+        if (row.alarm_offset) {

RFC 2445 seems to imply that a trigger of zero is disallowed, but it doesn't actually state this.  But I can imagine a user (or user-agent) wanting to put a zero here.   If we accept that an alarm_offset of 0 should be allowed, this if statement won't trigger, even though it should.

>-        if (tmp = item.getUnproxiedProperty("ALARMTIME"))
>-            this.setDateParamHelper(ip, "alarm_time", tmp);
>+        if (item.alarmOffset) {

Same problem with an alarmOffset of zero as before.

I still have a few more things I need to go over before I'm done reviewing, and Gecko's crummy selection code is making it impossible for me to copy the review comments out of this text box for later editing, so I'm posting them now.  Hope to get to the rest later this evening.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 211510 [details] [diff] [review]
patch v1

>+        if (row.alarm_time) {
>+            // Old (schema version 4) data, need to convert this nicely to the
>+            // new alarm interface
>+            var alarmTime = newDateTime(row.alarm_time, row.alarm_time_tz);
>+            var time;
>+            var related = item.CAL_ALARM_RELATED_START;
>+            if (row.event_start) {
>+                time = newDateTime(row.event_start, row.event_start_tz);
>+            } else if (row.todo_entry) {
>+                time = newDateTime(row.todo_entry, row.todo_entry_tz);
>+            } else if (row.todo_due) {
>+                related = item.CAL_ALARM_RELATED_END;
>+                time = newDateTime(row.todo_due, row.todo_due_tz);
>+            }
>+            if (time) {
>+                var duration = time.subtractDate(alarmTime);
>+                item.alarmOffset = duration;
>+                item.alarmRelated = related;

The subtraction wants to be the other way around, as discussed on IRC.  Also, be sure to test how a converted todo that hits the ALARM_RELATED_END clause loads in the dialog after conversion, given the begin|end UI semantics versus the RELATED_* semantics clash mentioned earlier in the review.

>+            } else {
>+                dump("WARNING! Couldn't do alarm conversion!\n");

Can make this error go to the JS console instead (or in addition), maybe even with a bit of info about the item in question?

In general, this looks good.

Comment 22

[Joey Minta]

Attachment: patch v2

Patch updated to review comments as follows:  (Thanks again for all the time put into that review!)

(comment #19)
-Snooze button hidden
-Relnote comment added regarding dismiss+move
-We do not want makeMemberAttr calls, as discussed on irc
-ICS provider id version has been bumped
-isNegative fixed
-CAL_ prefix removed from related constants, and they have been appropriately moved in the idl
-"START"->ALARM_RELATED_START change made
-I moved the notification clause because it needs to be called before findAlarms().  I added a comment stating so.
-Bug 298350 already exists for better alarm querying
-alarmLastAck and alarmOffset are now made immutable and cloned where appropriate
-Since the properties don't make it into the property bag, the storage provider ought not add them.
-X-MOX typo fixed
-Bug 327992 filed for using DESCRIPTION

(Comment #20)
-I've changed the VARCHAR entries to INTEGER
-switched to using UPDATE instead of DELETE+INSERT
-Comment added about future uses of alarmTime
-row.alarm_offset now compares with null, so the 0 case should work.  item.alarmOffset will still return true if the duration is 0, because it's an xpcom object, not just a '0' value

(Comment #21)
-subtraction revered.  Also, I did some more testing with task alarms that required some minor tweaks to the upgrade code.  Specifically, even checking for |if (row.event_start)| throws NS_ERROR_FAILURE for tasks, so we now do instanceof on the item to figure out if it's an event or task
-We now add an error to the console if upgrading fails.  The error includes the item's title and id.

I'm a bit scared by the race issue I was seeing in mozStorage during early testing.  It's disappeared for now.  I don't think there's a lot we can do about it, but when I publicize this landing, I'll make sure the warning is a bit stronger than I was previously planning.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 212891 [details] [diff] [review]
patch v2

>+        if (item.alarmRelated == item.ALARM_RELATED_START) {
>+            duration.isNegative = true;
>         }
>+        duration[alarmUnits] = alarmLength;

Would it be worth normalizing the duration here, in case the user has done a custom offset of, say, 90 minutes?

>Index: calendar/base/public/calIItemBase.idl
>===================================================================

Please use doxygen-style comments for your additions to this file.

>+  // One of the ALARM_RELATED constants below.  START corresponds to the
>+  // start or entry date of the item, END to the end or due date.

This comment can probably reasonably be split up into three comments, one for each line of IDL below:

>+  attribute unsigned long alarmRelated;
>+
>+  const unsigned long ALARM_RELATED_START = 0;
>+  const unsigned long ALARM_RELATED_END = 1;

r=dmose with these nits addressed.  Thanks for the hard work!

Comment 24

[Joey Minta]

Review nits addressed.  Patch checked in.  Please file separate follow-up bugs for any regressions.