1032675 - [Ringtones] [OTA] Handle the migration for the new tones in 2.0

Status: VERIFIED FIXED

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

Description

[Dominic Kuo [:dkuo]]

Visual team has a major 2.0-visual-refresh for the new system tones, besides to land those resources(bug 1014009) and set the default tones for 2.0, we should also handle the OTA case for the users that upgraded from some previous version, and this bug is for discussion and solution.

Comment 1

[Jim Porter (:squib)]

This is actually something we'll be doing in 2.1, I think. It's very confusing.

Comment 2

[Dominic Kuo [:dkuo]]

I think there should be three cases that the OTA users might have, which are:

1. The user never changed the default tones.
2. The user has changed the default tones, but the tones are from the preloaded/system tones.
3. The user has set his/her own tones from the media app, like Music.

And if visual wants to let the users notice the preloaded tones has changed after OTA to 2.0(Bug 1014009 comment 25), I am proposing we should:

Case 1: Change the old default tones to the new default tones.
Case 2: Change the old default tones to the new default tones.(Same as above)
Case 3: Keep the user's custom tones. The new preloaded tones will be listed in the ringtones app so the user is still able to choose them if he/she wants.

Patryk and Tiffanie, does these make sense to visual and ux? just what I have for now and please advice, thanks!

Comment 3

[Tiffanie Shakespeare [:tif]]

Hi Dominic! Patryk and I talked this over...

Your first two cases are dead on - if the user is using built-in tones, they've now disappeared in 2.1 so we should update them to the new default (which I believe is Firefox).

The third actually has two parts, one is creating a ringtone in 2.0 vs creating a ringtone in 1.4.

So if a 2.0 user has created custom ringtones, they are saved and should be kept when upgrading to 2.1. If the ringer so happens to use one of these custom ringtones, this choice should be retained (pretty much what you said above).

For this and the first two cases, I would like the 2.0 behaviour to be fixed so that when the user opens the ringer value selector the selected sound is visually selected, if not already. I could see a 1.4 user upgrading to 2.0, making no changes (and thus nothing is visibly selected) and upgrading again to 2.1.

When the 1.4 user creates a custom ringtone, which is not saved anywhere, and updates to 2.0 they will still hear their custom ringtone but it would not appear in the ringer value selector. From here we have two possible paths:
1. The user picks something else and upgrading to 2.1 falls into one of the three cases mentioned already.
2. The user does nothing (custom ringtone is still heard) and upgrading to 2.1 should just revert to the default Firefox tone.

Yes, this kind of sucks, but I think it's a pretty edge edge case and I think the alternatives could cause more confusion. At some point we're going to have to get the user to move away from this unsaved custom ringtone.

Clearing NI for Patryk as well...

Comment 4

[Tiffanie Shakespeare [:tif]]

See above comment.

Do you have everything else you need? I believe Patryk got the raw files to Jim(?). Do you have all the strings needed for the new tones?

Please let me know if there's anything further needed from UX. Thanks!!

Comment 5

[Jim Porter (:squib)]

I think we need to decide on an improved string for ringer_firefox (and notifier_firefox), but we can do that over in bug 1014009.

The migration work here should be pretty straightforward though.

Comment 6

[Dominic Kuo [:dkuo]]

I think for the migration part it should be enough for us to implement it, thanks Tiff!

Comment 7

[Dominic Kuo [:dkuo]]

Attachment: patch

Tim,

This wip is basically implemented base on comment 2 and comment 3, I didn't go for the approach in bug 1014009 comment 47 because, to have a new settings key for the OTA migration needs to modify the apps that use the previous key, like findmydevice, operatorvariant, ringtones, settins and system app, which we didn't expect to make such many modifications, so I turned to Arthur to know about how they handle the similar case on voice mail migration.

What they did is to record the version to identify the difference for OTA, then handle the migration while the system is booting up so they don't have to touch the other code blocks. I used the same idea and implemented it in the system's dialer agent, would you please take a look on it and give us some feedback? thanks.

Comment 8

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

We encounter similar problem when migrating the time format. The difference is that there is no existing module responsible for time formatting in system app so we may turn to add a generic module for migration in bug 1055424. The patch there is still under discussion but could be considered as another option.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8476553 [details] [review]
patch

There is a shared/js/version_helper.js -- would it help you get things done in a better way? Also, you might want to put the migration code in LazyLoader as we should only load it when we need it.

Other than that I have no opinion since this is what we did to fit the requirement.

Comment 10

[Dominic Kuo [:dkuo]]

Thanks Tim, I have took a quick look on shared/js/version_helper.js, and it seems like a helper to check the os version, so probably we could also use it on the ringtone migration. LazyLoader sounds like a good idea because we should only run the migration code for once after OTA, I will see if the migration part can be separated as an individual module then only load it when we need it.

Comment 11

[Kai-Chih Hu [:khu]]

Dominic, do we have confidence to land this bug before FL? Thanks.

Comment 12

[Dominic Kuo [:dkuo]]

(In reply to Kevin Hu [:khu] from comment #11)
> Dominic, do we have confidence to land this bug before FL? Thanks.

Yes, I think so, because I have addressed the issues that Tim mentioned in comment 9 and wrapping my wip, then add some unit tests shouldn't take too long. I will keep updating the progress to let people know this is on going, thanks.

Comment 13

[Dominic Kuo [:dkuo]]

Comment on attachment 8476553 [details] [review]
patch

Okay, I think I have wrapped the patch with the necessary unit tests, Tim, would you please review this? thanks! (or redirect to someone you think is qualified to do it)

Comment 14

[Jim Porter (:squib)]

Dominic: I commented over on Github about a change to built-in ringtone IDs that's happening in my SD card work. Whichever one of us lands our work last will have to make the change (but it's pretty simple). Thanks for taking this on!

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8476553 [details] [review]
patch

Looks good except we the module format of ToneUpgrader. Actually, I am not really sure if ToneUpgrader should follow constructor pattern if it's lazily loaded to do an one-time job. Alive, could you give us some guidance here?

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

The "load-on-updating" pattern is not covered in my framework yet; we assume any submodule will be loaded and started in the start stage of its parent module.

However, I will recommend to do constructor/start stuff for every singleton/non-singleton modules in system app so in the future baseModule could support load-on-updating easily.

Comment 17

[Gerry Chang [:gchang]]

Add the test cases.
https://moztrap.mozilla.org/manage/cases/?filter-suite=758

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8476553 [details] [review]
patch

Commented though I think it's too late to change, could we have followup? Thanks.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #18)
> Comment on attachment 8476553 [details] [review]
> patch
> 
> Commented though I think it's too late to change, could we have followup?
> Thanks.

Sounds good!

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8476553 [details] [review]
patch

r+ based on Alive's comment.

Comment 21

[Dominic Kuo [:dkuo]]

Thanks Tim and Alive, I think it won't take too long to change my patch to follow constructor pattern, and I am making the changes then will land it later.

But I have noticed bug 1027995(the one Jim mentioned in comment 14) was reopened, so probably I should wait for it to be re-landed, or Jim's patch also needs to include some changes on the migration part. If it takes too long, I will just land this before the FL.

Comment 22

[Dominic Kuo [:dkuo]]

Update: all issues are addressed but waiting for the gaia-try to be passed.

Comment 23

[Dominic Kuo [:dkuo]]

Gaia-try seems closed for now so I was unable to trigger the tests on tpbl. But I have passed the tests locally and this is a 2.1 feature-b2g bug, so I have landed it first.

master: afd2a980a447b4d0fa19ad6bd40fad38f5b5d271

Comment 24

[Ed Morley [:emorley]]

Reverted for gaia-linter failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=47172193&tree=B2g-Inbound

https://github.com/mozilla-b2g/gaia/commit/44bf2e3bc5ddea9db9a8c851bd353cb234aa883c

Comment 25

[Dominic Kuo [:dkuo]]

re-landed on master: 2b5808daa17be5d852dff2fd777413e6c38c76cc

Comment 26

[Gerry Chang [:gchang]]

Verified @ 
Gaia      e731a63484c532168f4de6dc1eeb4c6612ad73a9
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/868cc9e5e32a
BuildID   20140915160206
Version   34.0a2