891730 - [User Story] Ringtone Runtime Customization by SIM

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Peter Dolanjski [:pdol]]

User Story:

As an OEM, of the superset of ringtones that I ship with the build, I want to be able to specify which ringtone should be used on the device based on the MNC/MCC setting from the SIM card inserted during the First Run Experience in order to target customizations to locales without needing to use separate builds.

Acceptance Criteria:

1. If a certain ringtone is specified to be used for an MNC/MCC combination, and a SIM card with that MNC/MCC combination is in the device during the First Run Experience, the ringtone used for the incoming phone call tone is that specified ringtone.
2. If no SIM card is inserted during the First Run Experience, the specified default ringtone is used for the incoming phone call tone.
3. If no SIM card is inserted during the First Run Experience, and no default ringtone is specified, the Mozilla defined default ringtone is used for the incoming phone call tone.

Comment 1

[Candice Serran (:cserran)]

Kyle Machulis changed story state to started in Pivotal Tracker

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Ringtone Customization based on Variant

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 804432 [details] [diff] [review]
Patch 1 (v1) - Ringtone Customization based on Variant

Salva can you r? this?

I know that you are not in the list of peers, but you did with Marina part of the implementation and I'm pretty sure you can do a good review here.

The reason I'm asking this is cause I'm actually in PTO till the 25 of October, and this cannot wait till that time.

Cheers!
F.

Comment 4

[Salvador de la Puente González [:salva]]

Comment on attachment 804432 [details] [diff] [review]
Patch 1 (v1) - Ringtone Customization based on Variant

Let me ask for :mai's feedback who build the architecture for customization events.

Anyway, from my point of view, all is ok. As soon as :mai give us the f+ I give you the r+.

Comment 5

[Salvador de la Puente González [:salva]]

Comment on attachment 804432 [details] [diff] [review]
Patch 1 (v1) - Ringtone Customization based on Variant

You have some not blocking nits. Please, if you think the same, fix them.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Ringtone Customization based on Variant

Updated pull request with nits addressed.

Comment 7

[Salvador de la Puente González [:salva]]

Comment on attachment 805504 [details] [review]
Patch 1 (v2) - Ringtone Customization based on Variant

Looks fair, thank you very much.

Comment 8

[Borja Salguero [:borjasalguero]]

Hi Kyle,
We have some discussion about the following https://bugzilla.mozilla.org/show_bug.cgi?id=916718#c5, could you take a look as well? I would like to know your opinion because it could affect to this code as well. Thanks!

Comment 9

[Borja Salguero [:borjasalguero]]

Hi Kyle,
I've added a dependency due to https://bugzilla.mozilla.org/show_bug.cgi?id=891730#c8 after talking with Reuben. Please could you avoid to merge this until having a clear solution about this? Because probably we need to add small modifications to your code. Furthermore, could you take a look to the bug? I would like to know if you agree as well! :) Thanks! Gracias!

Comment 10

[Borja Salguero [:borjasalguero]]

Comment on attachment 805504 [details] [review]
Patch 1 (v2) - Ringtone Customization based on Variant

Adding a new review due to the discussion related with https://bugzilla.mozilla.org/show_bug.cgi?id=917740

Comment 11

[Borja Salguero [:borjasalguero]]

Comment on attachment 805504 [details] [review]
Patch 1 (v2) - Ringtone Customization based on Variant

R+. Please rebase and merge!

Comment 12

[Borja Salguero [:borjasalguero]]

https://github.com/mozilla-b2g/gaia/pull/12220/files#r6482055 Could you take a look?

Comment 13

[Borja Salguero [:borjasalguero]]

Comment on attachment 805504 [details] [review]
Patch 1 (v2) - Ringtone Customization based on Variant

Going back to r? due to I have a doubt about how the ringtone is loaded in settings... Is a URL? a BLOB?

Comment 14

[Gregor Wagner [:gwagner]]

(In reply to Borja Salguero [:borjasalguero] from comment #13)
> Comment on attachment 805504 [details] [review]
> Patch 1 (v2) - Ringtone Customization based on Variant
> 
> Going back to r? due to I have a doubt about how the ringtone is loaded in
> settings... Is a URL? a BLOB?

When we want to set the ringtone/wallpaper via the API or the default settings.json file we can handle both but in the actual SettingsDB we always try to convert data uris into blobs and store blobs.

Comment 15

[Borja Salguero [:borjasalguero]]

Im gonna reassign this because after landing the other patch, this is quite easy to fix! Kyle feel free to reassign again, and if you have free cycles to review this I'll appreciate it!

Comment 16

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 17

[Borja Salguero [:borjasalguero]]

Gregor, could you take a look as well? Thanks!

Comment 18

[Jason Smith [:jsmith]]

(In reply to Borja Salguero [:borjasalguero] from comment #17)
> Gregor, could you take a look as well? Thanks!

We're past feature complete + 1 week, which means this should not get an uplift. This should ride the trains the 1.3.

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 811049 [details]
Pull Request

r=me with requested test addition

Comment 20

[Salvador de la Puente González [:salva]]

Comment on attachment 811049 [details]
Pull Request

Only a little comment on GitHub. Not blocking on that but if you feel the same, please improve it before merging. Thank you very much.

Comment 21

[Jason Smith [:jsmith]]

Out of scope now for 1.2. Moved to 1.3.

Comment 22

[Candice Serran (:cserran)]

Peter Dolanjski deleted the linked story in Pivotal Tracker

Comment 23

[Borja Salguero [:borjasalguero]]

https://github.com/mozilla-b2g/gaia/commit/8a89b7f2fafc49409994c80befe9e3833364645f
https://github.com/borjasalguero/gaia/commit/6e5f8ad8186288e5b5051c2ba77eba5256c5f522

Merged! Sorry for the delay :)

Comment 24

[Salvador de la Puente González [:salva]]

Comment on attachment 811049 [details]
Pull Request

[Approval Request Comment]

Since bug 917740 finally landed on v1.2, this bug should land as well in order to fulfill the mission of the former one.

[Bug caused by] (feature/regressing bug #): No one in particular.
[User impact] if declined:

From the point of view of the customization. Ringtone is one of the most powerful ways of branding as the audio used is commonly recognized. Despite it is true the final user is not very impacted by this bug, the provider can be *greatly* impacted if denied. So, in my opinion the impact is high.

[Testing completed]: yes, note [1]

[Risk to taking this patch] (and alternatives if risky): very low

[String changes made]: no l10n strings have changed

[1] https://github.com/mozilla-b2g/gaia/pull/12497/files#diff-22d1d9cc3eb25cb5ae6c21f2c83ca7ccR26

Comment 25

[Jason Smith [:jsmith]]

(In reply to Salvador de la Puente González [:salva] from comment #24)
> Comment on attachment 811049 [details]
> Pull Request
> 
> [Approval Request Comment]
> 
> Since bug 917740 finally landed on v1.2, this bug should land as well in
> order to fulfill the mission of the former one.
> 
> [Bug caused by] (feature/regressing bug #): No one in particular.
> [User impact] if declined:
> 
> From the point of view of the customization. Ringtone is one of the most
> powerful ways of branding as the audio used is commonly recognized. Despite
> it is true the final user is not very impacted by this bug, the provider can
> be *greatly* impacted if denied. So, in my opinion the impact is high.
> 
> [Testing completed]: yes, note [1]
> 
> [Risk to taking this patch] (and alternatives if risky): very low
> 
> [String changes made]: no l10n strings have changed
> 
> [1]
> https://github.com/mozilla-b2g/gaia/pull/12497/files#diff-
> 22d1d9cc3eb25cb5ae6c21f2c83ca7ccR26

I don't see why an approval request is being asked here. We're past feature complete + 1 week at this point, which doesn't allow us to take any features at this point. This needs to ride the trains to 1.3.

Comment 26

[Salvador de la Puente González [:salva]]

Simply because the (ratio user (OEM) impact / risk). A great gain firstly scheduled for 1.2 with low risk no causing regressions in master at the current moment. We have the chance of re-schedule it and recover an important feature for the OEM.

Keeping the approval.

Comment 27

[Jason Smith [:jsmith]]

(In reply to Salvador de la Puente González [:salva] from comment #26)
> Simply because the (ratio user (OEM) impact / risk). A great gain firstly
> scheduled for 1.2 with low risk no causing regressions in master at the
> current moment. We have the chance of re-schedule it and recover an
> important feature for the OEM.
> 
> Keeping the approval.

I don't think that matters. Taking new features late in the game isn't just an evaluation of risk vs. value - other factors, so as increased testing scope, messaging related to the feature, docs, etc play factors into why we have to be conscious against taking features late in the game. Additionally, that goes against the train model - the train model argues against taking any features on Aurora. There's many problems that already exist within the testing scope of the customization distribution directory anyways - especially around apps. We should not be focusing on increasing the feature scope in that area, but rather figuring out how to get what already exists stable. On that regard, this isn't getting approval as it stands from the QA side.

Comment 28

[Gregor Wagner [:gwagner]]

Comment on attachment 811049 [details]
Pull Request

If we follow our rules then it's pretty clear that this feature will be in 1.3 and not in 1.2. Sorry.

Comment 29

[Salvador de la Puente González [:salva]]

Ok, I understand the motivation behind the approval. I agree.

Thank you :jsmith and :gwagner for the explanation.

Comment 30

[Candice Serran (:cserran)]

Peter Dolanjski changed story state to finished in Pivotal Tracker

Comment 31

[Candice Serran (:cserran)]

Peter Dolanjski changed story state to accepted in Pivotal Tracker

Comment 32

[Candice Serran (:cserran)]

Peter Dolanjski changed story state to accepted in Pivotal Tracker

Comment 33

[Jason Smith [:jsmith]]

This is a targeted feature for 1.3, but not committed, which makes this a non-blocker.