Disabling alarm of a snoozed task/events causes an error, menu items are disabled

VERIFIED FIXED in 0.7

Status

VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: omarb.public, Assigned: ssitter)

Tracking

unspecified
x86
Windows XP

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060923 BonEcho/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060923 BonEcho/2.0

When you snooze alarm of a task and then change the option to 'none' it causes an java error:

Error: alarmTime has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js
Line: 378
Error: alarmTime has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js
Line: 378
Error: alarmTime has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js
Line: 378
Error: uncaught exception: [Exception... "'[JavaScript Error: "alarmTime has no properties" {file: "file:///C:/Program%20Files/Mozilla%20Sunbird/js/calAlarmService.js" line: 378}]' when calling method: [calIOperationListener::onGetResult]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Sunbird/components/calStorageCalendar.js :: queueItems :: line 621"  data: yes]

Reproducible: Always

Steps to Reproduce:
1.Set up a task with alarm and wait until it is fired
2.Snooze alarm for 5 minutes
3.Edit the task and change alarm option to 'none'
4.Restart Sunbird
Actual Results:  
1.Error in console
2.'Edit selected event' and 'Delete Selected Event' in context menu in calendar view is disabled


Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060925 Calendar/0.3a2+
(Reporter)

Updated

12 years ago
Summary: Disabling alarm of a snoozed task caused an error → Disabling alarm of a snoozed task causes an error
Status: UNCONFIRMED → NEW
Component: General → Alarms
Ever confirmed: true
QA Contact: general → alarms
Summary: Disabling alarm of a snoozed task causes an error → Disabling alarm of a snoozed task causes an error
(Reporter)

Comment 1

12 years ago
I can still reproduce it using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061227 Calendar/0.4a1 but I can see different errors in console:
Error: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [calIDateTime.addDuration]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: file:///C:/Documents%20and%20Settings/Omar/Pulpit/sunbird/js/calAlarmService.js :: anonymous :: line 395"  data: no]
Source File: file:///C:/Documents%20and%20Settings/Omar/Pulpit/sunbird/js/calAlarmService.js
Line: 395

And I can also reproduce it for events. Menu items are still disabled
(Reporter)

Updated

12 years ago
Summary: Disabling alarm of a snoozed task causes an error → Disabling alarm of a snoozed task/events causes an error
(Assignee)

Comment 2

11 years ago
Issue still exists using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.7pre) Gecko/20070824 Calendar/0.7pre

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [calIDateTime.addDuration]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: file:///D:/Programme/sunbird/js/calAlarmService.js :: anonymous :: line 428"  data: no]
(Assignee)

Updated

11 years ago
Summary: Disabling alarm of a snoozed task/events causes an error → Disabling alarm of a snoozed task/events causes an error, menu items are disabled
(Assignee)

Comment 3

11 years ago
Created attachment 278183 [details] [diff] [review]
rev0 - add error check, remove snooze time if setting alarm to none

Added an error check in addAlarm() to not fail on existing event/tasks created with previous releases. If the alarm is reset to None in the dialog also remove the obsolete snooze property. I also took opportunity to remove the anonymous functions.
Assignee: nobody → ssitter
Status: NEW → ASSIGNED
Attachment #278183 - Flags: review?(daniel.boelzle)
(Assignee)

Comment 4

11 years ago
Created attachment 278188 [details]
ics testcase from sunbird 0.5

Comment 5

11 years ago
Comment on attachment 278183 [details] [diff] [review]
rev0 - add error check, remove snooze time if setting alarm to none

>Index: mozilla/calendar/base/src/calAlarmService.js
>         var offset = aItem.alarmOffset || aItem.parentItem.alarmOffset;
>-
>-        alarmTime.addDuration(offset);
>+        if (offset) {
>+            alarmTime.addDuration(offset);
>+        }
Although this check makes the code more robust, it is strictly not needed if hasAlarm() works as expected, returning true iff the item actually has an alarm specified. Thus IMO the core problem is that hasAlarm (from line 255) may return true if only a snooze X-props is present, which I doubt is sensible. I'd suggest to use hasAlarm as per line 88:

            function hasAlarm(a) {
                return a.alarmOffset || a.parentItem.alarmOffset;
            }

Maybe the snooze part from the second definition is a leftover; someone knows why it's in?

>Index: mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog.js
> function saveReminder(item) {
>     var reminderPopup = document.getElementById("item-alarm");
>     if (reminderPopup.value == 'none') {
>         item.alarmOffset = null;
>         item.alarmLastAck = null;
>         item.alarmRelated = null;
>+        item.deleteProperty("X-MOZ-SNOOZE-TIME");
This won't cover snoozing of occurrences, which are "X-MOZ-SNOOZE-TIME-" + aItem.recurrenceId.nativeTime, and even then I'd prefer that resetting the alarm offset would implicitly also reset alarmLastAck and any snoozes which would be an issue of calItemBase.js then.
Attachment #278183 - Flags: review?(daniel.boelzle) → review-
(Assignee)

Comment 6

11 years ago
(In reply to comment #5)
> Thus IMO the core problem is that hasAlarm (from line 255) may
> return true if only a snooze X-props is present, which I doubt is sensible.

The check for X-MOZ-SNOOZE-TIME in hasAlarm was added in Bug 298358. 
Jminta, do you remember the reasons for this?

Comment 7

11 years ago
(In reply to comment #6)
I don't recall the exact reasoning behind the inclusion of the snooze check.  I think it may have been in order to address this case, where an alarm is deleted while a snooze is pending.  In that instance, the check would be designed to still fire the alarm.

However, I'm not convinced that firing the snooze alarm is the most intuitive response to that set of user-actions.  If someone decides that it would be better to cancel the snooze as well, I *think* it would be ok to remove the line.  Still, the alarm code has started getting swapped out my brain, so I'm not as familiar with it as I once was.
(Assignee)

Comment 8

11 years ago
Created attachment 279951 [details] [diff] [review]
rev1 - change hasAlarm() as suggested

This patch changes hasAlarm() as suggested in Comment #5 to prevent the error. During testing I saw no issues with snoozed alarms. In addition onAddItem() uses the same logic: <http://lxr.mozilla.org/mozilla1.8/source/calendar/base/src/calAlarmService.js#87>.

This patch doesn't take care of removing the "X-MOZ-SNOOZE-TIME" property upon removing the alarm from the item / parent item. This needs to be done in a followup patch or bug.
Attachment #278183 - Attachment is obsolete: true
Attachment #279951 - Flags: review?(daniel.boelzle)

Comment 9

11 years ago
Comment on attachment 279951 [details] [diff] [review]
rev1 - change hasAlarm() as suggested

r=dbo, thanks for the patch.
Attachment #279951 - Flags: review?(daniel.boelzle) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
(Reporter)

Comment 11

11 years ago
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070908 Calendar/0.7pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.