1200103 - Bluetooth address not shown in Settings App

Status: RESOLVED FIXED

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

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

The bluetooth mac address is supposed to be shown in the settings app under Settings-> Device Information -> More Information.

Brsun informed me that its not shown when bluetooth is disabled. Enabling bluetooth fixed this, and it shows the address.

Maybe when bluetooth is disabled we should add a string, "Bluetooth disabled" so its clear that the address doesn't show when bluetooth is turned off.

Comment 1

[[e-mail]]

looks like a good idea because we cant get address when BT is disabled.

Will set as good first bug if UX has no objection to it.

Comment 2

[Harly Hsu[:harly]]

Yes, I agree with Paul here. Thanks Paul for pointing this out :)

Comment 3

[Kalpesh Krishna [:martianwars]]

(In reply to Fred Lin [:gasolin] from comment #1)
> looks like a good idea because we cant get address when BT is disabled.
> 
> Will set as good first bug if UX has no objection to it.

Hi Fred, I was interested in solving this bug. I tried looking for the relevant code but couldn't do so. Which files should I look at?

Comment 4

[[e-mail]]

hint:

pages are organized in settings/elements, and `Device Information > More Information` would be as `about_more_info.html`

js code are organized in `settings/js/panels/about_more_info/`, you can check if bluetooth address is exist , or show a string instead.

You can use element.setAttribute('data-l10n-id', string_id to set a text string to certain html tag

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] martiansideofthemoon:my-code-fix > mozilla-b2g:master

Comment 6

[Kalpesh Krishna [:martianwars]]

Attachment: Adding Bluetooth disable message.

Not so sure about the workflow of the code. I guessed that address takes up a null value / empty string in the case of a disabled bluetooth.

Comment 7

[[e-mail]]

Comment on attachment 8656178 [details] [review]
Adding Bluetooth disable message.

Thanks for providing the patch!

Please address comments in github and set review again when you are ready.

Comment 8

[Kalpesh Krishna [:martianwars]]

Attachment: Bluetooth disable string patch

Not so sure about the if condition. Did the rest as you mentioned.

Comment 9

[[e-mail]]

Comment on attachment 8656379 [details] [review]
Bluetooth disable string patch

Kalpesh, thanks for update the patch, it works on device well now. 
But we have to correct the unit tests to land code into gaia.

Please address comments in github and set review again.



(little bugzilla tip) you can use same attachment to set review by click the `detail` hyperlink at right of the attachment.

Comment 10

[Kalpesh Krishna [:martianwars]]

@gasolin - I received an error on the following tests :- https://pastebin.mozilla.org/8845117

I see errors on the following tests here.
1. should show bluetooth address
2. should use correct bluetooth module bluetooth version 1
3. should use correct bluetooth module bluetooth version 2

What could I do?

Comment 11

[[e-mail]]

please open unit/panels/about_more_info/hardware_info_test.js , find related tests and fix them

Comment 12

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8656379 [details] [review]
Bluetooth disable string patch

It passed all tests. Do let me know the next steps

Comment 13

[[e-mail]]

Sorry the fix for unit test is not a proper fix, 
if test runs into `assert.isTrue(false);` statement, it means _loadBluetoothAddress is rejected (with promise). 

You could check the code and find out why _loadBluetoothAddress is rejected.

Comment 14

[[e-mail]]

(_loadBluetoothAddress supposed to return resolve as result) 
I'll check and give more concrete advice later.

Comment 15

[Kalpesh Krishna [:martianwars]]

@gasolin :-
I successfully set up the enviroment locally and noticed the following :-
The variable name in must be "fields" and nothing else. I managed to pass the tests locally. Will submit a review request once the tests pass treeherder

Comment 16

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8656379 [details] [review]
Bluetooth disable string patch

Passed the original unit tests successfully. :)

Comment 17

[[e-mail]]

Comment on attachment 8656379 [details] [review]
Bluetooth disable string patch

Thanks for keeping update the patch!

The patch fixed tests and works well, but for maintainability I think we should rename `fields` to something more meaningful as you did in previous change.

Please address the last comment in github and we could land this change soon.

Comment 18

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8656379 [details] [review]
Bluetooth disable string patch

Did the mentioned changed.

Comment 19

[[e-mail]]

Comment on attachment 8656379 [details] [review]
Bluetooth disable string patch

Looks good to me, thanks and congrats for the first gaia merge!

Comment 20

[[e-mail]]

The last thing is squash your commits into single commit, please check 

http://eragonj.me/blog/fxos-why-we-need-to-squash-commits.html

to squash commits

Comment 21

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8656379 [details] [review]
Bluetooth disable string patch

Done with the squashing. Could I add the checkin needed?

Comment 22

[[e-mail]]

Thanks! I'll help you merge the commit once gaia repo is open :)

Comment 23

[[e-mail]]

merged https://github.com/mozilla-b2g/gaia/commit/7939280f937e742043e5049e8cc7c566469d86fa

thanks!