857284 - When multiple alarms are available to fire for a calendar event, fire only one notification, not multiple notifications

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 4/2/2013
Device: Unagi

STR

1. Import a calendar with an event that has two or more alarm reminders that would fire on import

Example - Time is 8:58pm, Event is 9pm - 10pm, reminders is 5 minutes and 15 minutes before.

Expected

We should fire only one alarm notification in this case, even though both alarms are valid here.

Actual

We fire an alarm per alarm reminder. Sort of annoying, cause your phone with sound on will make the notification noise ever single time that alarm fires. So five alarm reminders means 5 sounds in a row.

Comment 1

[Jason Smith [:jsmith]]

Can be quite annoying when this happens. Definitely not a blocker, but we should probably get this fixed eventually.

Comment 2

[Gareth Aye [:gaye] (back from PTO)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17171

So I think these tests prove that this issue is already resolved? I have been unable to reproduce the bug.

:jsmith - Would you mind confirming whether the bug reproduces for you on master?

:millermedeiros - r?

Comment 3

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8390927 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17171

one small nit on github but looks good to me. - nice to see the integration test coverage grow "organically".

Comment 4

[Gareth Aye [:gaye] (back from PTO)]

https://github.com/mozilla-b2g/gaia/commit/f09ec7d9d0bb7c40998ddb6b5bf397e578add541

Comment 5

[Dietrich Ayala (:dietrich)]

Master (1.5), Nexus 4, built 3/24

Multiple notifications from a single event on a Zimbra-synced calendar.

I got 4 notifications for a meeting at 5pm. They were all within a fairly short period before the meeting.

I cleared those notifications, and then got two more 35 minutes after 5pm.

Comment 6

[Gareth Aye [:gaye] (back from PTO)]

Also reported by Lucas and work happening for 2.0 in 1018834

Comment 7

[Gareth Aye [:gaye] (back from PTO)]

Not duplicate. Lucas' bug is that the reminders get persisted to the server. Yikes!

Comment 8

[Dylan Oliver [:doliver]]

[priority] --> tracking-b2g:+ conversion

Comment 9

[Miller Medeiros [:millermedeiros]]

maybe :dietrich bug is something different? see Bug 865394

Comment 10

[Gareth Aye [:gaye] (back from PTO)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25673

Before we were streaming our alarms straight to the notifications controller if they were meant to trigger in the past. This patch pulls all of the alarms we're going to issue into memory first, groups the ones we're sending straight to the notifications controller by event and then only issues a single notification. Still need to add tests :)

Comment 11

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8514592 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25673

LGTM

Comment 12

[Gareth Aye [:gaye] (back from PTO)]

fixed, landed on master https://github.com/mozilla-b2g/gaia/commit/80fd4b51714ec936ae3230e76b5b773b5a79c64c

Comment 13

[Edward Chen[:edchen]]

[Environment]
Gaia-Rev        4c159e75a1568afbbf0c83c1235ec56facfbe87d
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b9849b3c6aaa
Build-ID        20141111161207
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  39
FW-Date         Thu Oct 16 18:19:14 CST 2014
Bootloader      L1TC00011880

[Result]
PASS