898666 - [clock] Clock refactor alarm object and add tests

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Clock, enhancement)

Description

[Eric O'Connor [:oconnore]]

Currently, alarms are tracked using object literals which have no constructors.

We should use an Alarm object with a real constructor and prototype, and use that API consistently across the application.

Comment 1

[Eric O'Connor [:oconnore]]

Attachment: Pull request on github

My pull request also includes Mike's patches, because those haven't been merged yet.

I marked this bug as dependent on Mike's bugs.

Comment 2

[[e-mail], [e-mail]]

(In reply to Eric O'Connor [:oconnore] from comment #1)
> Created attachment 781997 [details] [review]
> Pull request on github
> 
> My pull request also includes Mike's patches, because those haven't been
> merged yet.
> 
> I marked this bug as dependent on Mike's bugs.
Hi Eric,

Thanks for the point out:) Mike has landed these patches.
Could you please rebase and reopen the pull request on Github?
I will comment for the pr.

Comment 3

[[e-mail], [e-mail]]

Comment on attachment 781997 [details] [review]
Pull request on github

Eric,

I have reviewed the pr quickly. Basically, I agree with the majority modification. Since the pr is at close state, I would like to leave my comment on it. In order to mange the code base accurately, I prefer to land one major change(module) via one pull request(bug). It's also helpful for reviewing effort. We can check each functionality during reviewing stage.

So, could you please separate the request with one major change to one pull request, if it's possible to be subdivided. Thank you so much.

Comment 4

[Eric O'Connor [:oconnore]]

Attachment: pull request on master

Hi Ian,

I just made a new pull request with additional testing and fixes for the informal review that Rick gave me. 

I am not sure that it's possible to split this into multiple commits, since many of the changes are co-dependent.

Comment 5

[[e-mail], [e-mail]]

Comment on attachment 786379 [details] [review]
pull request on master

This is a big refactor. I have addressed some comment on Github. Beside of these comment, I'm care about the functionality normally or not. Most of the broken feature are happened while alarm goes off. You have to make sure the alarm state for each case(repeat/non-repeat with snoozed/non-snoozed). The main problem are as following which be found out quickly.

1. The snoozed alarm is not canceled while user modify a alarm which is be scheduled with snooze.
2. Don't close(handle) the multiple alarms case while these alarms goes off at the same time.
3. The alarm status of alarm list is not correct while user closed the repeat alarm.

Comment 6

[Eric O'Connor [:oconnore]]

Hi Ian,

I updated the PR addressing your comments on github and rebased onto master. 

1. This is fixed here: https://github.com/oconnore/gaia/commit/d8e1fba78b08153153b27c1fad857b7cd06b6e38#L3R354

2. I don't quite understand this comment. The existing behavior on  https://github.com/mozilla-b2g/gaia/blob/935b9b4849353568535d5063f49c14484df9efa2/apps/clock/js/active_alarm.js#L85 closes previous alarms. How are you proposing we change that? 

3. I'm not quite sure what you are referring to here. I believe this should be correct (after addressing some comments you made on the PR). 

Thanks for the feedback! I am flagging you for review again.

Comment 7

[[e-mail], [e-mail]]

(In reply to Eric O'Connor [:oconnore] from comment #6)
> 2. I don't quite understand this comment. The existing behavior on 
> https://github.com/mozilla-b2g/gaia/blob/
> 935b9b4849353568535d5063f49c14484df9efa2/apps/clock/js/active_alarm.js#L85
> closes previous alarms. How are you proposing we change that? 
Looks like it does not work as before with the refactor.
Step:
a. Set multiple alarms with the same alarm time(Alarm A --> am8:00, Alarm B --> am8:00, Alarm C --> am9:00, given different label is helpful to identify them)
b. Wait the time is up
c. Will see only one attention screen while the alarm time we set
d. Make sure that other alarm is turned off without attention screen
> 
> 3. I'm not quite sure what you are referring to here. I believe this should
> be correct (after addressing some comments you made on the PR). 
Step:
a. Set a repeat alarm
b. Wait alarm goes off
c. Close the alarm
d. Make sure the alarm status:
   Indicator on status bar: alarm icon is enable
   The alarm in alarm list: is disable(It should be enable since it's a repeat alarm.)
> 
> Thanks for the feedback! I am flagging you for review again.
Thanks.

Comment 8

[[e-mail], [e-mail]]

(In reply to Ian Liu [:ianliu] from comment #7)
> (In reply to Eric O'Connor [:oconnore] from comment #6)
> > 2. I don't quite understand this comment. The existing behavior on 
> > https://github.com/mozilla-b2g/gaia/blob/
> > 935b9b4849353568535d5063f49c14484df9efa2/apps/clock/js/active_alarm.js#L85
> > closes previous alarms. How are you proposing we change that? 
> Looks like it does not work as before with the refactor.
> Step:
> a. Set multiple alarms with the same alarm time(Alarm A --> am8:00, Alarm B
> --> am8:00, Alarm C --> am9:00, given different label is helpful to identify
> them)
> b. Wait the time is up
> c. Will see only one attention screen while the alarm time we set
> d. Make sure that other alarm is turned off without attention screen
> > 
It still does not work. The indicator and status of alarm list are not correct.(Alarmdb is right, but UI layout is out of updated.) Looks like parallel callback function is not be run in this case.
https://github.com/mozilla-b2g/gaia/pull/11383/files#L1R40
> > 3. I'm not quite sure what you are referring to here. I believe this should
> > be correct (after addressing some comments you made on the PR). 
> Step:
> a. Set a repeat alarm
> b. Wait alarm goes off
> c. Close the alarm
> d. Make sure the alarm status:
>    Indicator on status bar: alarm icon is enable
>    The alarm in alarm list: is disable(It should be enable since it's a
> repeat alarm.)
> > 
For the section, it works fine after applied your latest patch. Thanks.
> > Thanks for the feedback! I am flagging you for review again.
> Thanks.

Comment 9

[[e-mail], [e-mail]]

Comment on attachment 786379 [details] [review]
pull request on master

Eric,

Could you actually test the code before submitting for review? It will help you to find out the major problem and decrease our reviewing time between different timezone. Please fix the above issue and Rick's comment. And set review again for me. Thanks.

Comment 10

[Mike Pennisi [:jugglinmike]]

Eric asked me to check out his patch and test out these features. They seem to
be working for me. I want to be sure that I am not making any incorrect
assumptions about the intended behavior, so I will document my experience in
detail.

Eric and Ian: please let me know where your experience is different or if some
part of my report reflects a bug.

---

## 12:36PM

1. I created a new alarm for 12:39PM. I named it "a", changed the "Sound"
   setting to "No Sound", and ensured that the "Repeat" setting was "Never".
2. I created a new alarm for 12:39PM. I named it "b", changed the "Sound"
   setting to "No Sound", and ensured that the "Repeat" setting was "Never".
3. I created a new alarm for 12:43PM. I named it "c", changed the "Sound"
   setting to "No Sound", and ensured that the "Repeat" setting was "Never".
   Here is a text-based representation of the main Clock view as it appeared on
   my phone after taking these steps:
   
   > 12:39 PM   a      [x]
   > 12:39 PM   b      [x]
   > 12:43 PM   c      [x]

## 12:38 PM

4. The phone began to vibrate and the "attention screen" was rendered. It
   described alarm "b".
5. I tapped the button labeled "Close".
6. The phone stopped vibrating and the main Clock view was rendered.  Alarm "c"
   was still active and rendered at the top of the list. Alarms "a" and "b"
   were inactive and rendered below (in reverse order). Here is a text-based
   representation:
   
   > 12:43 PM   c      [x]
   > 12:39 PM   b      [ ]
   > 12:39 PM   a      [ ]

## 12:43 PM

7. The phone began to vibrate and the "attention screen" was rendered. It
   described alarm "c".
8. I tapped the button labeled "Close".
9. The phone stopped vibrating and the main Clock view was rendered. All three
   alarms were inactive. The alarms were rendered in the same order.  Here is a
   text-based representation:
   
   > 12:43 PM   c      [ ]
   > 12:39 PM   b      [ ]
   > 12:39 PM   a      [ ]

Comment 11

[Rick Waldron [:rwaldron]]

Attachment: refactor-full-pass.png

This screenshot illustrates the current green status of this patch

Comment 12

[[e-mail], [e-mail]]

Hi Mike, 

Your experience is also working fine for me. And I find out that I wrote an incorrect alarm time in comment 7. It should be "Alarm C --> am9:00" ==> "Alarm C --> am8:00". 

It means that we set three alarms or more with same alarm time. The callback could not be run for updating indicator and alarm status while alarm goes off.(https://github.com/mozilla-b2g/gaia/pull/11383/files#L1R40) My reproduced rate is 60%. It is not hard to repro it. But it's an edge case for a user. Is it possible to have unit test for this case? Thanks.

Comment 13

[[e-mail], [e-mail]]

(In reply to Rick Waldron from comment #11)
> Created attachment 789125 [details]
> refactor-full-pass.png
> 
> This screenshot illustrates the current green status of this patch

Unit test is very important and meaningful for verifying a pull request. But some of the user stories are not be covered via it. That is why Productivity team is implementing integration test for these case. So, I would like to say that unit test is a good preliminary reference for expected function input/output. But we still need to check the functionality carefully while landing a patch. Especially this is a big refactor patch.

Comment 14

[Eric O'Connor [:oconnore]]

Comment on attachment 786379 [details] [review]
pull request on master

Resolved issue with 3 alarms, where the onload event was not firing.

Added comments.

Added property hiding for id, repeat, enabled, and registeredAlarms.

Comment 15

[Mike Pennisi [:jugglinmike]]

Comment on attachment 786379 [details] [review]
pull request on master

Alrighty, Eric: I've finished reviewing your patch on GitHub. You can find my comments there.

This feedback didn't fit as an in-line comment, so I'll leave it here: can you change the subject on the commit to "Bug 898666 - [clock] Clock refactor alarm object and add test"?

Comment 16

[Eric O'Connor [:oconnore]]

Comment on attachment 786379 [details] [review]
pull request on master

Pushed review changes

Comment 17

[Mike Pennisi [:jugglinmike]]

Comment on attachment 786379 [details] [review]
pull request on master

Great work, Eric! I'll land this for ya

Comment 18

[Mike Pennisi [:jugglinmike]]

master: https://github.com/mozilla-b2g/gaia/commit/3746b1cdc0e344460c58cd52f85564b0c4f5940a

Comment 19

[Mike Pennisi [:jugglinmike]]

backed out: https://github.com/mozilla-b2g/gaia/commit/3746b1cdc0e344460c58cd52f85564b0c4f5940a

This patch caused a UI regression--new Alarm list items were appended to the alarms list (rather than inserted at the beginning).

Comment 20

[Mike Pennisi [:jugglinmike]]

master: https://github.com/mozilla-b2g/gaia/commit/46dcd65c2075f0872a4597169ad9c9f7bdba0335

Thanks for working so quickly to resolve the regression, Eric