998428 - Start button should be disabled when the Timer time-picker is set to 0:00

Status: RESOLVED FIXED

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

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. launch clock
2. go to timer

Expected: start button should be disabled at 0:00
Actual: it's clickable

Gaia      f0872318d46781bb59d0a13a2e1fffb115b8ca2b
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/32bb713e6d0d
BuildID   20140418014001
Version   28.1
ro.build.version.incremental=eng.cltbld.20140418.052257
ro.build.date=Fri Apr 18 05:23:05 EDT 2014
Tarako

Happens on 1.3 buri as well.

Comment 1

[Vishnu Dangi]

i am trying to fix this issue.can you help what should be our approach to resolve this issue?

Comment 2

[Vishnu Dangi]

Hi Wayne,
please help us to resolving this issue.

Comment 3

[Wayne Chang [:wchang]]

This is minor, leaving for the functional team to take care.

Comment 4

[lana]

Attachment: patch to disable start button when time is 00:00

I fixed this bug.
I'm beginning to write the tests while I'm waiting for your feedback.

Comment 5

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8490297 [details] [diff] [review]
patch to disable start button when time is 00:00

Thanks for the patch, Lana!

Ordinarily I would have suggested that we avoid using MutationObserver, because it's a pretty heavyweight construct. But in this case, as you probably discovered while looking at this bug, the Picker and Spinner classes make it pretty difficult to get notified when they change. (Ideally, they'd just emit a "change" event, and we'd listen to that event to decide what to do with the start button.)

But given the way things are, I think your patch is a reasonable alternative, and potentially even cleaner than an alternate solution, so let's run with it.

Feel free to ask if you have questions about writing the test, but so far, so good. Thanks!

Comment 6

[lana]

Attachment: new patch to disable start button when time is 00:00 with test

As I was tying to write the test, I made the code cleaner.

What you suggested on IRC for the test didn't work. I finally found a way to use an eventListener instead  of a mutationObserver and it works fine.

Comment 7

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8503626 [details] [review]
new patch to disable start button when time is 00:00 with test

Looks good apart from one small thing -- the automated Linter has tests which check for code formatting and other nitpicky things, and it requires us to use single-quoted strings. If you click the link in the comment from "try-server-hook" in your pull-request, it takes you to a complicated page:

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=7505409a6039

and that page has a red "Li" square, indicating that the linter's tests didn't pass. If you click that "Li" square, it shows the errors listed below.

You should also be able to see those errors by running "make lint". If you could fix those errors to make the linter happy, I think we'll be all set!

----

TEST-UNEXPECTED-FAIL | apps/clock/js/panels/timer/main.js | line 101, col 44, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | apps/clock/js/panels/timer/main.js | line 101, col 52, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | apps/clock/js/panels/timer/main.js | line 108, col 37, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | apps/clock/js/panels/timer/main.js | line 108, col 45, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | apps/clock/js/panels/timer/main.js | line 110, col 40, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | apps/clock/js/panels/timer/main.js | line 115, col 56, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | apps/clock/js/panels/timer/main.js | line 116, col 54, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | apps/clock/test/unit/panels/timer/timer_panel_test.js | line 159, col 118, Line is too long.

TEST-UNEXPECTED-FAIL | apps/clock/test/unit/panels/timer/timer_panel_test.js | line 174, col 32, Strings must use singlequote.

TEST-UNEXPECTED-FAIL | make lint | make[1]: *** [hint] Error 1

Comment 8

[lana]

I always forget the single-quotes.
I changed it and the linter seems happy now.

Comment 9

[Marcus Cavanaugh [:mcav]]

Real close! I only see one error now (found by looking at the latest comment from the try server robot):

TEST-UNEXPECTED-FAIL | apps/clock/test/unit/panels/timer/timer_panel_test.js | line 159, col 108, Line is too long.

I think the limit for Gaia is 80 characters. Fix that and we're good to go.

Comment 10

[lana]

Is it okay like this ?

Comment 11

[lana]

Sorry I had pushed the modifications but "try-server-hook" wouldn't start the tests.
I did it again and the linter is green this time but some other tests (made by "try-server-hook") seem to have failed (Gij and Gu).
Do we care ? If yes, I have no idea about what to do.

Comment 12

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8503626 [details] [review]
new patch to disable start button when time is 00:00 with test

Sorry for the delay. I double-checked the test run and it looks okay, so I think we're good to go.

Comment 13

[lana]

That's okay. I just rebased the patch so you can merge the pull request.
Thanks.

Comment 14

[Marcus Cavanaugh [:mcav]]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/0004cde45a76a2e508738646525a5b1ebf77cd8c

Thanks again for your contribution! And sorry (again) for the delay, Lana; I had forgotten that you didn't have the account permissions to merge the patch.

Also, if you ever wonder what's happening with a bug (like here, when it has been a few days and it seemed like I had forgotten), don't be afraid to set the needinfo flag for the person you're waiting for -- sometimes bugs get lost in the shuffle, and a "needinfo" flag shows up in the Bugzilla Dashboard until the person dismisses it, so they're more likely to remember to pay attention. In fact, you'll see a lot of people with the words "please needinfo" in their bugzilla display name because it's such a good way to track things that people need to respond to. :)

Comment 15

[lana]

No problem and thanks for the tip.