942837 - [User Story] Show Alarm Time on Lock Screen

Status: RESOLVED FIXED

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

Description

[Adam Rogers (:arog)]

User Story:

As a user I want to view the time of my next alarm while my phone is locked - this enables me to verify that it is set without needing to unlock my phone and navigate to the clock app.

Acceptance Criteria:

1. When my phone is locked (but not in sleep mode) I can see that my alarm is set, and the time and date of my next alarm.
2. Only my next alarm is shown, for example if I have an alarm set for 8am every day I can only see tomorrows alarm.
3. If my alarm is removed, It is no longer shown on the lock screen.

Comment 2

[Prateek Jadhwani [:prateekjadhwani]]

I have started working on this user story. Can admin assign the bug to me please.

Thanks

Comment 3

[Joshua Smith [:joshua-s]]

(In reply to Prateek Jadhwani from comment #2)
> I have started working on this user story. Can admin assign the bug to me
> please.

Great!  I have assigned you :)

Comment 4

[Joshua Smith [:joshua-s]]

(In reply to Prateek Jadhwani from comment #2)
> I have started working on this user story. Can admin assign the bug to me
> please.

Great!  I have assigned you :)

Comment 5

[Joshua Smith [:joshua-s]]

Oops, double post.

Comment 6

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Joshua Smith [:joshua-s] from comment #4)

> Great!  I have assigned you :)

Thanks Joshua

Comment 7

[Edward Chen[:edchen]]

Hi,

I'm QA contact.
When feature has developed complete please let me know, I will start to test the feature. 

Thanks,
Edward

Comment 8

[Juwei Huang[:juwei]]

Attachment: [Clock] Next alarm info_1.0.pdf

Spec attached.

Comment 9

[Przemek Abratowski [:przemek] UX]

Attachment: Lockscreen_Alarm_480x854(flame).png

Location of alarm icon.

Comment 10

[Przemek Abratowski [:przemek] UX]

Attachment: alarm_icons.zip

Alarm icon file for all device resolutions.

Comment 11

[Prateek Jadhwani [:prateekjadhwani]]

One Question regarding the UX, how would the alarm information show if the alarm is set for 3 days from now? Or is it supposed to show only for the next 24 hours?

Comment 12

[Ed Morley [:emorley]]

I didn't file the bug, so not sure why you're asking me.

That said, this is one of the features that I'd love to see in FirefoxOS - so my answer would be "copy Android, because their UX for pretty much everything is better". Their behaviour is to only show it if the alarm is within 24 hours or the next calendar day or something like that.

Comment 13

[Prateek Jadhwani [:prateekjadhwani]]

Sorry, Ed. I wasn't able to get a hold of the Reporter or the Business Owner. So I thought may be someone from Mozilla might be able top convey the message.

Secondly, thanks for the answer. I am almost done setting up the DataStore to get alarm info from the clock along with a POC. It should be done by next week.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master

Comment 15

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8672186 [details] [review]
[gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master

I was wondering if someone from the Gaia:Clock and Gaia:System could review the code.
-Thanks

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Morphing a11y review flag into a Yura NI.

Comment 17

[Marcus Cavanaugh [:mcav]]

:preeti, is this feature still desired? and if so, am I correct in assuming that we'll want to wait to land until after 2.5 branches?

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to David Bolter [:davidb] from comment #16)
> Morphing a11y review flag into a Yura NI.

Yes it seems like there are some a11y related tweaks that need to happen:

* Vertical bar separator needs to be hidden from the screen reader (using aria-hidden="true" on that element). Otherwise it is pronounced.
* The alarm icon is not accessible to the screen reader which is fine IFF the time of the alarm has an additional label describing that it is the next alarm time. The best way to achieve that is to add an aria-label reading 'Next alarm' to the container that has the time text content via the l10n API.

Please feel free to flag me for final a11y check.

Comment 19

[Prateek Jadhwani [:prateekjadhwani]]

Thank you for the review. I will make  the changes, and add the flag again. :)

Comment 20

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Yura Zenevich [:yzen] from comment #18)
> (In reply to David Bolter [:davidb] from comment #16)
> > Morphing a11y review flag into a Yura NI.
> 
> Yes it seems like there are some a11y related tweaks that need to happen:
> 
> * Vertical bar separator needs to be hidden from the screen reader (using
> aria-hidden="true" on that element). Otherwise it is pronounced.
> * The alarm icon is not accessible to the screen reader which is fine IFF
> the time of the alarm has an additional label describing that it is the next
> alarm time. The best way to achieve that is to add an aria-label reading
> 'Next alarm' to the container that has the time text content via the l10n
> API.
> 
> Please feel free to flag me for final a11y check.

Yura, I added the aria-label for the required sections. I was wondering if you could take a look at it again and let me know if this is expected.

Thanks

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

Ah this works really well, thanks for helping out on the a11y front.

Comment 22

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8672186 [details] [review]
[gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master

Thanks for tackling this feature. As mentioned on GitHub, I think we can get rid of most of the date/time logic by using `alarm.getNextAlarmFireTime()` to determine which alarm needs to be included. That logic can probably be simplified to only a few lines.

I haven't gone over the lockscreen parts; that's probably more in the purview of a Gaia::Lockscreen peer.

Comment 23

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Marcus Cavanaugh [:mcav] (Mozilla SF) from comment #22)
> Comment on attachment 8672186 [details] [review]
> [gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master
> 
> Thanks for tackling this feature. As mentioned on GitHub, I think we can get
> rid of most of the date/time logic by using `alarm.getNextAlarmFireTime()`
> to determine which alarm needs to be included. That logic can probably be
> simplified to only a few lines.
> 
> I haven't gone over the lockscreen parts; that's probably more in the
> purview of a Gaia::Lockscreen peer.

Thanks for the reviews Marcus. I will updated the code by tomorrow. :)

Comment 24

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8672186 [details] [review]
[gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master

Hi Marcus, I made the changes as per the comments. Please let me know if I need to make any more changes,
Thanks

Comment 25

[Prateek Jadhwani [:prateekjadhwani]]

Greg, I was wondering of you could take a look at the lockscreen part of the pull request. 

Thanks

Comment 26

[Greg Weng [:snowmantw][:gweng][:λ]]

Please set the review (when you done your work) or feedback (when you're still working on that) since it's part of the workflow.

Thanks for patching, but I'm afraid there are some nested code need to be cleared up which I've commented on the PR page. After you address them I think we can take a look again.

Comment 27

[Prateek Jadhwani [:prateekjadhwani]]

Hi Greg, I made the changes as per the comments. I am still new to the concept of Promises, so if I am doing something wrong. please let me know.

Thanks

Comment 28

[Greg Weng [:snowmantw][:gweng][:λ]]

Commented. You're almost there. But please, set feedback or review for the patch instead of just NI.

Comment 29

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8672186 [details] [review]
[gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master

Greg, I made the changes as per the comments. I was wondering if you could take a look at it. I wasnt able to move one block into its own then() block, I did not have access to the object that was required due to scoping. But, please let me know if this not a good solution.

Thanks

Comment 30

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8672186 [details] [review]
[gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master

Hi, I've addressed the issue. Maybe you can take a look and to see if it's reasonable for you. If so, you may fix other parts and rebase the patch. Thanks.

Comment 31

[GitHub Autolander Bot]

Attachment: [gaia] prateekjadhwani:942837 > mozilla-b2g:master

Comment 32

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8672186 [details] [review]
[gaia] prateekjadhwani:Bug-942837 > mozilla-b2g:master

Created a new PR for this

Comment 33

[Prateek Jadhwani [:prateekjadhwani]]

Greg, I cleaned out the PR. And updated the Promise where it was required. Please let me know if I need to update anything else.

Thanks

Comment 34

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

I think there are some issues I've addressed in the first PR. And although I don't know the policy of Clock app, in System app you need to attach an unit test for your patch before setting the review (for feedback it's not necessary). So please fix the PR, write an unit test and send the review again.

Comment 35

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

Greg, I made the changes as per the comments. I am not sure if I am adding the assert conditions for the tests correctly. Since the fetchAlarm() returns a Promise, i had to wrap it around the then() method. 
Also, I wasnt able to use (fetchedData = null) as per the comments, because the dataStore returns an empty object instead of null. 
If I am doing something wrong, please let me know.

Fabrice, I was wondering if you could code review the alarm part for this story.

Thanks

Comment 36

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

I'll go ahead and grab this review; I know bugzilla lists him as a suggested reviewer but he has a lot on his plate and I'm the clock component owner.

Comment 37

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Marcus Cavanaugh [:mcav] (PTO 11/2-11/3) from comment #36)
> Comment on attachment 8682236 [details] [review]
> [gaia] prateekjadhwani:942837 > mozilla-b2g:master
> 
> I'll go ahead and grab this review; I know bugzilla lists him as a suggested
> reviewer but he has a lot on his plate and I'm the clock component owner.

Thanks Marcus. Let me know if i need to change any thing.

Comment 38

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

It's fine for me to measure a empty object instead of null. And there are only two issues in the patch now. Good work.

Comment 39

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

Hi Greg,

I updated the PR as per the comments. Took me a while to understand that .then(done).catch(done) thing. 

Please let me know if I need to make any more changes.

Thanks

Comment 40

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

There is only one issue. Unfortunately, it relates to error handling so I can't just f+ it. Please address it or discuss with me on the PR page if you have other concerns, thanks.

Comment 41

[Prateek Jadhwani [:prateekjadhwani]]

Hi Greg, do i need to catch the error on line 80 as well and again throw it again after I add the throw code on line 101?
Sorry, i misunderstood the statement.
Thanks

Comment 42

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

Greg, I added the throw code in the catch block. Please let me know if it needs more changes. 

Thanks

Comment 43

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

Okay, I think it's good now.
Let's see if it's good in testing now.

Thanks for patching.

Comment 44

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

This looks mostly good to me now, thanks for the revisions. Treeherder is still complaining about a failing test ("TypeError: this.component.updateAlarm is undefined"). You'll need to fix that before it can be landed. Please flag me for review once more when that is fixed, and I'll be quick on the r+. (I was on PTO some of last week, causing the slow response.)

Comment 45

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Marcus Cavanaugh [:mcav] from comment #44)
> Comment on attachment 8682236 [details] [review]
> [gaia] prateekjadhwani:942837 > mozilla-b2g:master
> 
> This looks mostly good to me now, thanks for the revisions. Treeherder is
> still complaining about a failing test ("TypeError:
> this.component.updateAlarm is undefined"). You'll need to fix that before it
> can be landed. Please flag me for review once more when that is fixed, and
> I'll be quick on the r+. (I was on PTO some of last week, causing the slow
> response.)

Hi Marcus. Thanks for the quick response. I ran the tests using bin/gaia-test command. And all the unit tests related to alarm are passing. Could you point me in the correct direction as in what tests are you running besides the unit tests.

Thanks

Comment 46

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

Hi Marcus, I found the problem. And fixed the unit test. I forgot to add the methods in one of the test. Sorry :(

Let me know if I need to add anything else. Thanks

Comment 47

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

Treeherder run looks green, thanks.

Greg, did you want to take a formal review pass on this?

Comment 48

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8682236 [details] [review]
[gaia] prateekjadhwani:942837 > mozilla-b2g:master

Well it looks fine as the last time I reviewed, but with one weird indention here:

https://github.com/mozilla-b2g/gaia/pull/32955/files#diff-460a1c0f44fb12b2fbf9dd1d3be82a20R56

Although it's only a nit.

Comment 49

[Prateek Jadhwani [:prateekjadhwani]]

Sorry Greg, i dont know how it got past the jslist. My bad.

Thanks for noticing though, I fixed the indentation.

Comment 50

[Prateek Jadhwani [:prateekjadhwani]]

Marcus, the review had been done :)

Comment 51

[Marcus Cavanaugh [:mcav]]

Landed. Thanks!

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

Comment 52

[Ed Morley [:emorley]]

Many thanks for fixing this :-)