973441 - [settings] refactor Date & Time panel with AMD pattern

Status: RESOLVED FIXED

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

Description

[[e-mail]]

Overview Description:

Refactor Date & Time panel with AMD pattern referring to
https://github.com/crh0716/gaia/tree/settings2_iterative

to make it modularize and more easier to maintain

Steps to Reproduce:
1) run make test-perf APP=settings
2) run make test-integration APP=settings


Expected Results:

pass all settings test and act the same as original implementation

Additional Information:

Comment 1

[[e-mail]]

take it as the pre-work for bug 1023735

Comment 2

[[e-mail]]

Attachment: pull request redirect to github

workable WIP, unit tests needed

Comment 3

[[e-mail]]

Comment on attachment 8450077 [details] [review]
pull request redirect to github

feedback request while adding test cases

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8450077 [details] [review]
pull request redirect to github

Thanks for the patch, Fred! I was thinking the module could be more general and separated from UI elements. Which means we may have a DateTime module that is a singleton and Observable providing functions for setting date, time, and time zone... etc. By doing this the module is easier to test because it is not involved with DOM elements.

Comment 5

[[e-mail]]

Attachment: PR2

add DateTime module to observe settings values

Comment 6

[[e-mail]]

Comment on attachment 8451532 [details] [review]
PR2

set for feedback again.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8451532 [details] [review]
PR2

The DateTime singleton should at least provide functions like `setDate`, `setTime`, and a function for toggling setting time automatically. And it should also be an observable at the same time providing current date and time for UI binding. Now we support creating an Observable with functions on it. Please refer to display/wallpaper.js.

Comment 8

[[e-mail]]

DateTimeUI removed, delegate its duty to panel.js and DateTime module

Comment 9

[[e-mail]]

Comment on attachment 8451532 [details] [review]
PR2

test cases added, please kindly review it

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8451532 [details] [review]
PR2

Thanks for the patch. I left some comments in github. Note that DateTime module is for users of the module being able to configure date, time, and timezone and getting the current date and time more easily. Manipulation to UI elements should not be done in the module.

And I noticed two bugs. The first one is the check box for automatically setting time is not shown even I used a sim card that supports it. The other is that the time no longer updates after I turn off and turn on the screen (this may also occurs on date).

Comment 11

[[e-mail]]

WIP: Update DateTime module, now it provide 'date', 'clock' for observe.

1. the check box for automatically setting time is not shown in neither Simulator nor debug device, but only shown on device with pvt build (no matter there's SIM card or not).

2. fixed by DateTime observer

Comment 12

[[e-mail]]

Comment on attachment 8451532 [details] [review]
PR2

test fine on frame with SIM card.

please kindly review it again

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8451532 [details] [review]
PR2

The architecture looks good to me, but there are a few calling sequence problems. Please check my comments in github. I'll continue to review the tests after the problems are addressed, thanks.

Comment 14

[[e-mail]]

Attachment: pull request redirect to github 3

comment addressed and add more jsdocs. Please kindly review it again

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8468272 [details] [review]
pull request redirect to github 3

I just did a few tests, there are still bugs:

- Enable "set automatically", timezone is not set correctly.

Please make sure all the behaviors are the same as before. Especially the cases like set a time zone manually -> enable auto time -> close app -> open app -> disable auto time -> the time zone should be the user selected one.

Comment 16

[[e-mail]]

Thanks for addressing more issues for refactor. For the "set automatically" bug, I can reproduce it in master either.

After checking the DateTime modification history, 
https://github.com/mozilla-b2g/gaia/commits/master/apps/settings/js/date_time.js

There's only Bug 1042774 modified the gaia code in master. I reverted it and that bug still occurred.

To further confirm the bug, set auto timezone and restart the phone, the statusbar clock shows manual timezone but not auto timezone.

I think the issue might be come from gecko part. let's track it separately in bug 1050091.

Comment 17

[[e-mail]]

Comment on attachment 8468272 [details] [review]
pull request redirect to github 3

besides above issue, all comments are addressed. Please kindly review it again

Comment 18

[[e-mail]]

The trace result shows tz_select change in bug 1026098 cause bug 1050091

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8468272 [details] [review]
pull request redirect to github 3

r=me, thank you!!

Comment 20

[[e-mail]]

merged to gaia-master https://github.com/mozilla-b2g/gaia/commit/c25177993e77aeae5c80528fee301c55b6d8d5f8

thanks!

Comment 21

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Unable to verify until the depends on ,bug 968686, is closed out and verified

Comment 22

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

It seems as though the user is able to toggle set Date and Time correctly, unable to verify as stated in comment 21

Comment 23

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

I added the [QAnalyst-verify-] tag because I am unsure that I can fully verify this bug, as some of the issues mentioned might be backend