Closed Bug 891730 Opened 7 years ago Closed 6 years ago

[User Story] Ringtone Runtime Customization by SIM

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:-)

VERIFIED FIXED
1.3 Sprint 3 - 10/25
blocking-b2g -

People

(Reporter: pdol, Assigned: borjasalguero)

References

Details

(Keywords: feature, Whiteboard: [ucid:System18, FT:systems-fe, KOI:P2][systemsfe])

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [ucid:System18] → [ucid:System18, FT:systems-fe, KOI:P2]
Kyle Machulis changed story state to started in Pivotal Tracker
Assignee: nobody → kyle
Attachment #804432 - Flags: review?(mri) → review?(francisco.jordano)
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.
Attachment #804432 - Flags: review?(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+.
Attachment #804432 - Flags: feedback?(mri)
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.
Updated pull request with nits addressed.
Attachment #804432 - Attachment is obsolete: true
Attachment #804432 - Flags: review?(salva)
Attachment #804432 - Flags: review?(francisco.jordano)
Attachment #804432 - Flags: feedback?(mri)
Attachment #805504 - Flags: review?(salva)
Attachment #805504 - Flags: feedback?(mri)
Comment on attachment 805504 [details] [review]
Patch 1 (v2) - Ringtone Customization based on Variant

Looks fair, thank you very much.
Attachment #805504 - Flags: review?(salva) → review+
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!
Flags: needinfo?(kyle)
Depends on: 917740
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 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
Attachment #805504 - Flags: review+ → review?(fbsc)
Comment on attachment 805504 [details] [review]
Patch 1 (v2) - Ringtone Customization based on Variant

R+. Please rebase and merge!
Attachment #805504 - Flags: review?(fbsc) → review+
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?
Attachment #805504 - Flags: review+ → review?
(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.
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!
Assignee: kyle → fbsc
Attached file Pull Request
Attachment #811049 - Flags: review?(salva)
Attachment #811049 - Flags: review?(kyle)
Gregor, could you take a look as well? Thanks!
Flags: needinfo?(anygregor)
Attachment #805504 - Attachment is obsolete: true
Attachment #805504 - Flags: review?
Attachment #805504 - Flags: feedback?(mri)
(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 on attachment 811049 [details]
Pull Request

r=me with requested test addition
Attachment #811049 - Flags: review?(kyle) → review+
Flags: needinfo?(kyle)
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.
Attachment #811049 - Flags: review?(salva) → review+
Flags: needinfo?(anygregor)
Priority: -- → P1
Out of scope now for 1.2. Moved to 1.3.
No longer blocks: koi-system-fe
Attachment mime type: text/plain → text/x-github-pull-request
Peter Dolanjski deleted the linked story in Pivotal Tracker
https://github.com/mozilla-b2g/gaia/commit/8a89b7f2fafc49409994c80befe9e3833364645f
https://github.com/borjasalguero/gaia/commit/6e5f8ad8186288e5b5051c2ba77eba5256c5f522

Merged! Sorry for the delay :)
Status: NEW → RESOLVED
blocking-b2g: --- → 1.3?
Closed: 6 years ago
Resolution: --- → FIXED
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
Attachment #811049 - Flags: approval-gaia-v1.2?
Whiteboard: [ucid:System18, FT:systems-fe, KOI:P2] → [ucid:System18, FT:systems-fe, KOI:P2][systemsfe]
QA Contact: rafael.marquez
(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.
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.
(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 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.
Attachment #811049 - Flags: approval-gaia-v1.2? → approval-gaia-v1.2-
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Ok, I understand the motivation behind the approval. I agree.

Thank you :jsmith and :gwagner for the explanation.
Peter Dolanjski changed story state to finished in Pivotal Tracker
Peter Dolanjski changed story state to accepted in Pivotal Tracker
Peter Dolanjski changed story state to accepted in Pivotal Tracker
Blocks: 935411
No longer depends on: 935411
Depends on: 937096
Flags: in-moztrap?(rafael.marquez)
This is a targeted feature for 1.3, but not committed, which makes this a non-blocker.
blocking-b2g: 1.3? → -
Keywords: verifyme
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 959522
You need to log in before you can comment on or make changes to this bug.