910552 - [zffos1.1][P2][Call Setting]Call Barring menu not implemented

Status: RESOLVED FIXED

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

Description

[FangChen]

Built ID 20130823171628
Device: Ikura
QC RIL version: "ro.build.firmware_revision=V1.01.00.01.019.180"
gaia commit: 4916f82 Merge branch 'zte/ics_strawberry_cdr' of ssh://10.67.16.41:29418/quic/lf/releases/gaia into ics_strawberry_cdr
gecko commit: a1c2bb0 ZRL modify ACCEPT Http head for MMS
 

A.- Overview Description (technical background, concise explanation of the bug):  
    
    MMI for call barring not implemented
    ________________________________________________________________________________
    B.- Steps to Reproduce (initial conditions, required resources, step by step instructions to reproduce):
    
    
    ________________________________________________________________________________
    C.- Actual Result (current bad behaviour that is reported as a bug):
    
    
    ________________________________________________________________________________
    D.- Expected Result (correct behaviour wished):
    
    It sould be in available in call options menu

Comment 1

[Ivan Tsay (:ITsay)]

feature request

Comment 2

[Preeti Raghunath(:Preeti)]

Features cannot land after 9/16 unless a strong case has been presented for the same.

Comment 3

[Ivan Tsay (:ITsay)]

Preeti,

I don't have strong case for this one from partner. This is from one of the local OB test cases. I list is as koi? since it is the only flag I could pick up for the feature request at that time. I am ok if moving it to v1.3.

Comment 4

[Jason Smith [:jsmith]]

Not a committed feature for 1.3, so we don't need to block on this. Clearing nom.

Comment 5

[Noemí Freire (:noemi)]

Adding to backlog to be properly prioritized. Thanks!

Comment 7

[Beatriz Rodríguez [:brg]]

Include in Call Settings menu the ones related to Call Barring:

Barring of All Outgoing Calls supplementary service (BAOC)
Barring of Outgoing International Call supplementary service (BOIC)
Barring of Outgoing International Call except those made to the Home Country supplementary service (BOIC- exHC)
Barring of All Incoming Calls supplementary service (BAIC)
Barring of All Incoming Calls in Roaming supplementary service (BAIC-R)

Comment 8

[Maria Angeles Oteo (:oteo)]

Hi Omega, Telefonica is interested in developing this feature as it's a certification waiver, could you help us providing the corresponding UX?
Thanks a lot!

Comment 9

[Omega Feng [:Omega] [:馮於懋]]

I think we should ask for products comment first. Wilfred, are we going to do this?
ni? possible UX owner Carrie as well.

Comment 10

[Wilfred Mathanaraj [:WDM]]

yes - we will be taking this as a contribution from partner. partner will provide UX and development work here. We can review the UX work before we implement.

we have not added this to committed work as we can not control the development process of the partner but we are willing to support the work the partner is doing to make this change.

Comment 11

[Maria Angeles Oteo (:oteo)]

hi Rafa, could you please help us with the UX so Carrie/Omega can review you proposal?

Comment 12

[Fernando Campo (:fcampo)]

Assigning to myself to start working on it as soon as UX is defined.

Comment 13

[Rafael Rebolleda]

Hey guys, 

I don't think there is much to add here: it should work pretty much like Call Forwarding. What I would do is have each in their own menu, because each time the user lands in this screen a high number of network requests are made.

So my only comment would be to have additional "Call Forwarding" and "Call Barring" menu entries in this screen that would take the user to the corresponding set of options in a deeper level screen.

Comment 14

[Maria Angeles Oteo (:oteo)]

Including the Acceptance Criteria (thanks Beatriz) according to the Certification requirements so UX can start working on it

Comment 15

[Pau Masiá [:Pau]]

Attachment: UX Spec. Call Barring

Here I attach the UX proposal for the call barring feature.

Thanks!

Comment 16

[Maria Angeles Oteo (:oteo)]

Carrie, Omega, could you please review the UX proposal?

Let's know if you agree with it or you have any comment so Fernando can start with the implementation.

Comment 17

[Fernando Campo (:fcampo)]

I'd also like a review from l10n for wording (at least in english), to make clear that the proposal expresses correctly what it's shown in the user story.

Specifically, I'm afraid that use case for 'Barring of [Outgoing] International Call except those made to the Home Country supplementary service' is not well worded with 'Outgoing' > 'International roaming'

Comment 18

[Fernando Campo (:fcampo)]

Forgot to add people in the loop :)

Can you please give some feedback, Francesco?

Comment 19

[Francesco Lodolo [:flod]]

For copy you need someone like Matej (now CCed), not :l10n ;-)

I'd prefer "Please contact your carrier if you don't know" instead of "In case you don't know ...", but that's an Italian guy giving suggestions on English, not really reliable.

From a l10n point of view, that "International roaming" scares me in terms of truncation.

Comment 20

[Matej Novak [:matej]]

Can someone point me to the whole string that needs review? Thanks.

Comment 21

[Fernando Campo (:fcampo)]

(In reply to Matej Novak [:matej] from comment #20)
> Can someone point me to the whole string that needs review? Thanks.
It's not a specific string to review, but more like the general feeling to reflect the options. 
From the comment 7 (and user story), we are transforming them into design using sub-menus (second image of attachment 8460822 [details])
- "Barring of All Outgoing Calls supplementary service", transforms into 
- Call barring > Outgoing calls > All

- "Barring of Outgoing International Call supplementary service" is
- Call Barring > Outgoing calls > International

- "Barring of Outgoing International Call except those made to the Home Country supplementary service" is
- Call Barring > Outgoing calls > International roaming

- "Barring of All Incoming Calls supplementary service" is
- Call Barring > Incoming calls > All

- "Barring of All Incoming Calls in Roaming supplementary service" is
- Call Barring > Incoming calls > International roaming

I'm actually not sure if this should be approved by UX first and then reworded by L10n team, or both teams can work together (hence the feedback request instead of review)

Comment 22

[Francesco Lodolo [:flod]]

> I'm actually not sure if this should be approved by UX first and then
> reworded by L10n team, or both teams can work together (hence the feedback
> request instead of review)

It's not a task for l10n to review the English text. Ideally UX wireframes should be also copy reviewed.

I wonder if "Call Barring" is clear for English users, I definitely had to search on Google to find out what it is.

I don't see anything wrong in the approach, as long as we use different entities for menu and header, and possibly find a way to cope with long translations.

Comment 23

[Matej Novak [:matej]]

Everything in that mockup looks good to me, but I agree with flod about that one passcode string. Let's make it:

Please contact your service provider if you don't know your passcode.

Just two questions about that:
1) Is "passcode" what we use elsewhere in FxOS (as opposed to "password")?
2) I changed it to "service provider" because I don't think the average user will understand "carrier." Does that still sound right to everyone?

Thanks.

Comment 24

[Omega Feng [:Omega] [:馮於懋]]

@Carrie, could you review the spec, thanks!
@Maria, I left Comms team since May. Now the UX owners are Carrie and Jenny. :P

Comment 25

[Pau Masiá [:Pau]]

Attachment: UX Spec. Call Barring

Comment 26

[Fernando Campo (:fcampo)]

(In reply to Omega Feng [:Omega] [:馮於懋] from comment #24)
> @Maria, I left Comms team since May. Now the UX owners are Carrie and Jenny.
> :P
I thought Settings app was part of SystemFE, but maybe the Comms-related settings are managed by Comms team?

Comment 27

[Fernando Campo (:fcampo)]

(In reply to Pau Masiá from comment #25)
> Created attachment 8461348 [details]
> UX Spec. Call Barring
Some comments about the specs.

When we activate/deactivate call barring services, it's necessary to provide a password to complete the action. When exactly is the user prompted to write this password? I actually don't even know how to get it in the first place, to be honest. Is it provided by the carrier?

Comment 28

[Pau Masiá [:Pau]]

(In reply to Fernando Campo (:fcampo) from comment #27)
> (In reply to Pau Masiá from comment #25)
> > Created attachment 8461348 [details]
> > UX Spec. Call Barring
> Some comments about the specs.
> 
> When we activate/deactivate call barring services, it's necessary to provide
> a password to complete the action. When exactly is the user prompted to
> write this password? I actually don't even know how to get it in the first
> place, to be honest. Is it provided by the carrier?

The user is prompted to write the password immediately after activating/deactivating any of the call barring services. To be more precise, once the switch has finished it's animation, the prompt appears.

Regarding how to get the password, as far as I know, it should be provided by the carrier but Beatriz can answer better to this question.

Comment 29

[Beatriz Rodríguez [:brg]]

(In reply to Fernando Campo (:fcampo) from comment #27)
> When we activate/deactivate call barring services, it's necessary to provide
> a password to complete the action. When exactly is the user prompted to
> write this password? I actually don't even know how to get it in the first
> place, to be honest. Is it provided by the carrier?
Yes, the password is checked under carrier infraestructure. If it is ok, the suplementary service is activated or deactivated. The first time the service is use in our premises, there is a default value for this password (0000) but user can modify it.

Comment 30

[Fernando Campo (:fcampo)]

I've been trying to test the feature with different SIMs (thanks Juanjo!), and everytime I try I get a request error, but with no details of what the error might be.

Tests on this commit (over master) - https://github.com/fcampo/gaia/commit/5b3678950bb379e85e3240737bea1c0bfda4dc2a
Repro after install the patch:
Settings > Call settings > Call barring > click on the first option (Outgoing > All)
That should trigger the request and create some logs.

Log:
E/GeckoConsole(  849): Content JS LOG at app://settings.gaiamobile.org/js/call.js:934 in cs_baocInputchange: option: {"program":0,"enabled":true,"password":"0000","serviceClass":1}

E/GeckoConsole(  849): Content JS LOG at app://settings.gaiamobile.org/js/call.js:941 in cs_initCallBarring/cs_baocInputchange/request.onerror: ERROR
E/GeckoConsole(  849): Content JS LOG at app://settings.gaiamobile.org/js/call.js:942 in cs_initCallBarring/cs_baocInputchange/request.onerror: request: {}
E/GeckoConsole(  849): Content JS LOG at app://settings.gaiamobile.org/js/call.js:943 in cs_initCallBarring/cs_baocInputchange/request.onerror: RESULT: undefined
E/GeckoConsole(  849): Content JS LOG at app://settings.gaiamobile.org/js/call.js:944 in cs_initCallBarring/cs_baocInputchange/request.onerror: e =  {}

I've also been talking with Jose Antonio, who pointed me to bug 833754 comment 31, but even if this is the case, the error should be more specific. In my case, error comes empty, so I don't know what's wrong.

Any insights, Szu-Yu Chen?

Comment 31

[Carrie Wang [New account: [e-mail]]]

Hi, 

Some comments here.

1.As I know, currently the settings of Call forwarding is not grouped in one entry. It does not look like Screen1 in this spec. ni? Jenny to double-confirm it.

2.Will users know this passcode should be provided by carriers? How do they distinguish it from their PIN code or Passcode for lockscreen? And what if they input wrong numbers, what will happen?

3.Are these switches related to each others? For example, if user wants to change the setting from "All" to "International", the switch of "All" will be automatically turned off when user switches on "International" with the correct passcode?

Thanks.

Comment 32

[Maria Angeles Oteo (:oteo)]

(In reply to Carrie Wang [:carrie] from comment #31)
Hi Carrie,
I can answer to some of your questions
> 
> 1.As I know, currently the settings of Call forwarding is not grouped in one
> entry. It does not look like Screen1 in this spec. ni? Jenny to
> double-confirm it.

I included in the ACs of the US "As UX suggested, we should try to avoid sharing the same screen with Call Forwarding to avoid too many network requests at the same time so the idea would be including the corresponding set of options for "Call Forwarding" and "Call Barring” in a deeper level and in different screens"
> 

> 3.Are these switches related to each others? For example, if user wants to
> change the setting from "All" to "International", the switch of "All" will
> be automatically turned off when user switches on "International" with the
> correct passcode?

I included these ACs in the User Story field too

AC3: If BAOC is activated, the BOIC and BOIC- exHC Supplementary Services can not be changed directly by the user and will appear shaded. Once the BAOC is deactivated, BOIC and BOIC-exHC will recover their values and can be modified again.
AC4: If BAIC is activated, the BAIC-R Supplementary Service can not be changed directly by the user and will appear shaded. Once the BAIC is deactivated,  BAIC-R will recover its value and can be modified again.
> 

Hope it helps
Maria

Comment 33

[Beatriz Rodríguez [:brg]]

(In reply to Carrie Wang [:carrie] from comment #31)
> 
> 2.Will users know this passcode should be provided by carriers? How do they
> distinguish it from their PIN code or Passcode for lockscreen? And what if
> they input wrong numbers, what will happen?
If user types a wrong password, the service is not activated.
This service is not widely used and if a user want to activate it... they will know the password in advance. The behaviour after inserting the wrong password may depend on the carrier decission(block it or not). We just need to show the answer received from carrier.

Comment 34

[Jenny Lee]

Hello all, I'm ok with grouping call forwarding and call barring into two different entries. A few things to clarify about the passcode:

1) Is passcode requirement a one time thing? Does user have to enter the code everytime he tries to enable a setting? Does user have to re-enter the passcode after restart phone? this is something that needs to be specify on the spec.
2) In the case of screen passcode lock, we give user infinite chances to enter wrong passcode. In the case of SIM PIN, we only give user 3 chances before locking the SIM. For call barring, we need to define what's going to happen and again, this needs to be specify on spec too.
3) Is the correct passcode stored on SIM card?
4) The header on last screen (change passcode) should be "Change passcode" instead of "New passcode". The first entry should be "Current passcode" instead of "passcode".    

Thanks!

Comment 35

[Beatriz Rodríguez [:brg]]

(In reply to Jenny Lee from comment #34)
> Hello all, I'm ok with grouping call forwarding and call barring into two
> different entries. 
Great, thanks.
>A few things to clarify about the passcode:
> 1) Is passcode requirement a one time thing? Does user have to enter the
> code everytime he tries to enable a setting? Does user have to re-enter the
> passcode after restart phone? this is something that needs to be specify on
> the spec.
User had to enter the passcode in each activation or deactivation of any option. No need of passcode when cheking status.
> 2) In the case of screen passcode lock, we give user infinite chances to
> enter wrong passcode. In the case of SIM PIN, we only give user 3 chances
> before locking the SIM. For call barring, we need to define what's going to
> happen and again, this needs to be specify on spec too.
That may depend on carrier decission and type of user. In some cases the service may be blocked till user calls service center and reactivates it.
> 3) Is the correct passcode stored on SIM card?
NO.
> 4) The header on last screen (change passcode) should be "Change passcode"
> instead of "New passcode". The first entry should be "Current passcode"
> instead of "passcode".    
> 
> Thanks!

Comment 36

[Pau Masiá [:Pau]]

Attachment: UX Spec. Call Barring

I've made a change regarding the "change passcode" scenario. I've added the same pattern that is applied in the "passcode lock" within the Screen lock menu in Settings.

Thanks.

Comment 37

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Fernando Campo (:fcampo) from comment #30)
> I've been trying to test the feature with different SIMs (thanks Juanjo!),
> and everytime I try I get a request error, but with no details of what the
> error might be.
> 
> Tests on this commit (over master) -
> https://github.com/fcampo/gaia/commit/
> 5b3678950bb379e85e3240737bea1c0bfda4dc2a
> Repro after install the patch:
> Settings > Call settings > Call barring > click on the first option
> (Outgoing > All)
> That should trigger the request and create some logs.
> 
> Log:
> E/GeckoConsole(  849): Content JS LOG at
> app://settings.gaiamobile.org/js/call.js:934 in cs_baocInputchange: option:
> {"program":0,"enabled":true,"password":"0000","serviceClass":1}
> 
> E/GeckoConsole(  849): Content JS LOG at
> app://settings.gaiamobile.org/js/call.js:941 in
> cs_initCallBarring/cs_baocInputchange/request.onerror: ERROR
> E/GeckoConsole(  849): Content JS LOG at
> app://settings.gaiamobile.org/js/call.js:942 in
> cs_initCallBarring/cs_baocInputchange/request.onerror: request: {}
> E/GeckoConsole(  849): Content JS LOG at
> app://settings.gaiamobile.org/js/call.js:943 in
> cs_initCallBarring/cs_baocInputchange/request.onerror: RESULT: undefined
> E/GeckoConsole(  849): Content JS LOG at
> app://settings.gaiamobile.org/js/call.js:944 in
> cs_initCallBarring/cs_baocInputchange/request.onerror: e =  {}
> 
> I've also been talking with Jose Antonio, who pointed me to bug 833754
> comment 31, but even if this is the case, the error should be more specific.
> In my case, error comes empty, so I don't know what's wrong.
> 
> Any insights, Szu-Yu Chen?

It's known and we don't know why |console.log('e = ' + JSON.stringify(request.error));| doesn't work on gaia, i.e. the json properties not printed out.

Could you please try |console.log('e = ' + request.error && request.error.name);|?

Comment 38

[Maria Angeles Oteo (:oteo)]

Setting ni to Jenny and Carrie (already with ni) to confirm if the answers already given and the modified spec provided by Pau are enough or there are more suggestions/doubts. Thanks a lot!

Comment 39

[Jenny Lee]

(In reply to Beatriz Rodríguez [:brg] from comment #35)

> User had to enter the passcode in each activation or deactivation of any
> option. No need of passcode when cheking status.

Is this carrier request? Having to enter passcode for each activation/deactivation would be quite annoying.

> > 3) Is the correct passcode stored on SIM card?
> NO.

How do we verify the passcode?

Thanks!

Comment 40

[Beatriz Rodríguez [:brg]]

(In reply to Jenny Lee from comment #39)
> (In reply to Beatriz Rodríguez [:brg] from comment #35)
> 
> > User had to enter the passcode in each activation or deactivation of any
> > option. No need of passcode when cheking status.
> 
> Is this carrier request? Having to enter passcode for each
> activation/deactivation would be quite annoying.
Not carrier request. It is how the service was defined and specified. 
> 
> > > 3) Is the correct passcode stored on SIM card?
> > NO.
> 
> How do we verify the passcode?
Passcode is not verified by us. It is verified by carrier.
> 
> Thanks!

Comment 41

[Jenny Lee]

Hello, overall I think the spec is in good shape, just need a detail flow on this case: "After user enters the wrong passcode, what happens next?" For example, if it results in blocking the service, how many changes do user get to enter wrong passcode before it locks up? What sort of warning message should be displayed? Is it possible for us to list out all the possible cases?
Thanks!

Comment 42

[Beatriz Rodríguez [:brg]]

According to the specs (24.010 at 3gpp):

"If the served mobile subscriber enters a wrong call barring "password" three consecutive times, the subscription option "control of services" is set to "by the service provider" in the network: thus the network makes the use of password impossible for any subscriber operation. The password check procedure returns to the parent procedure an indication of Password Attempts Violation. The password can be made valid by the service provider only."

Comment 43

[Fernando Campo (:fcampo)]

(In reply to Beatriz Rodríguez [:brg] OoO till 8/25 from comment #42)
> According to the specs (24.010 at 3gpp):
> 
> "If the served mobile subscriber enters a wrong call barring "password"
> three consecutive times, the subscription option "control of services" is
> set to "by the service provider" in the network: thus the network makes the
> use of password impossible for any subscriber operation. The password check
> procedure returns to the parent procedure an indication of Password Attempts
> Violation. The password can be made valid by the service provider only."
So basically, after 3 failed attempts the provider blocks the service until the user calls the provider to unblock it. Similar to the PIN-code case.

(In reply to Jenny Lee from comment #41)
> [...] how many changes does the user get to enter wrong passcode before it locks up? 
> What sort of warning message should be displayed?
> Is it possible for us to list out all the possible cases?
> Thanks!
AFAIK the provider doesn't send any warnings, or number of attempts left before blocking, only an error message.
I'll make some tests tomorrow to check the kind of messages that we are getting back from the provider when:
- successful password is used
- wrong password is used
- we try to activate a service that is already activated
- the provider doesn't support the service
If you can think about some other cases, please add them and we'll try to test them.

(In reply to Maria Oteo from comment #32)
> I included these ACs in the User Story field too
>
> AC3: If BAOC is activated, the BOIC and BOIC- exHC Supplementary Services can not be changed 
> directly by the user and will appear shaded. Once the BAOC is deactivated, BOIC and BOIC-exHC 
> will recover their values and can be modified again.
> AC4: If BAIC is activated, the BAIC-R Supplementary Service can not be changed directly 
> by the user and will appear shaded. Once the BAIC is deactivated,  BAIC-R will recover 
> its value and can be modified again.
This is only specified for the full restriction (ALL), but I understand that this behaviour is extendable to the rest of the options? I mean, if I activate ANY service, the rest of the related options (under the same label, Outgoing/Incoming) should be disabled, being the services mutually exclusive (you can't have BOIC and BOIC-exHC active at the same time)? Or is the service state overwritten by the most restrictive service? 
e.g. Barring All Outgoing is off, but Barring Outgoing International is on. Should BAOC be enabled for clicking because it overwrites the less restrictive, or should we disable it until the user deactivates BOIC before?

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #37)
> It's known and we don't know why |console.log('e = ' +
> JSON.stringify(request.error));| doesn't work on gaia, i.e. the json
> properties not printed out.
> 
> Could you please try |console.log('e = ' + request.error &&
> request.error.name);|?
Thanks for the info! I've managed to get all the fields (error.name & error.message), apparently neither JSON.stringify or Object.keys() show the fields from the prototype object.

Comment 44

[Szu-Yu Chen [:aknow] (inactive)]

> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #37)
> > It's known and we don't know why |console.log('e = ' +
> > JSON.stringify(request.error));| doesn't work on gaia, i.e. the json
> > properties not printed out.
> > 
> > Could you please try |console.log('e = ' + request.error &&
> > request.error.name);|?
> Thanks for the info! I've managed to get all the fields (error.name &
> error.message), apparently neither JSON.stringify or Object.keys() show the
> fields from the prototype object.

Yes, JSON.stringify will not show the fields from prototype objects. But I think the exact problem here is that |e| is an instance of a webidl interface which could not be serialized.

If we want it to be serializable, we could add 'jsonifier' in DOMError.webidl. It indeed works. But I don't know why we didn't do this before (any concern?)

+++ b/dom/webidl/DOMError.webidl
@@ -20,4 +20,6 @@ interface DOMError {
 
   [ChromeOnly]
   void init(DOMString name, optional DOMString message = "");
+
+  jsonifier;
 };

Comment 45

[Maria Angeles Oteo (:oteo)]

(In reply to Fernando Campo (:fcampo) from comment #43)
> (In reply to Maria Oteo from comment #32)
> > I included these ACs in the User Story field too
> >
> > AC3: If BAOC is activated, the BOIC and BOIC- exHC Supplementary Services can not be changed 
> > directly by the user and will appear shaded. Once the BAOC is deactivated, BOIC and BOIC-exHC 
> > will recover their values and can be modified again.
> > AC4: If BAIC is activated, the BAIC-R Supplementary Service can not be changed directly 
> > by the user and will appear shaded. Once the BAIC is deactivated,  BAIC-R will recover 
> > its value and can be modified again.
> This is only specified for the full restriction (ALL), but I understand that
> this behaviour is extendable to the rest of the options? I mean, if I
> activate ANY service, the rest of the related options (under the same label,
> Outgoing/Incoming) should be disabled, being the services mutually exclusive
> (you can't have BOIC and BOIC-exHC active at the same time)? Or is the
> service state overwritten by the most restrictive service? 
> e.g. Barring All Outgoing is off, but Barring Outgoing International is on.
> Should BAOC be enabled for clicking because it overwrites the less
> restrictive, or should we disable it until the user deactivates BOIC before?
> 

I don't think we need to prevent user from modifying an option more restrictive that the current active one. E.g. in your example, I would let the user click on BAOC and restrict in one click all the calls (after that BOIC won't be enabled for clicking)

Comment 46

[Fernando Campo (:fcampo)]

Attachment: WIP

After some time away from this due to more urgent bugs, I got back to it, and we are finding some troubles when testing it.

Even when using a SIM card that allows CallBarring (tested it before with MMI codes), and after being able to activate it through the new menu (receive a success response from the RIL), I'm still receiving GenericFailure when trying to get the current status of the service [getCallBarringOption(option)]. I'm using the option = {
  'program': 0,
  'serviceClass': 1
}

Is there something wrong on my code, or is the RIL not answering correctly?

STR:
- Install the WIP patch over latest master.
- Go to Settings > Call Settings > Call Barring
- When opening the panel, the request is made to the RIL, always failing (see the logs)
- Activate / Deactivate any option, and see that the answer is a success (not reverting state)

Comment 47

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Fernando Campo (:fcampo) from comment #46)
> Created attachment 8477380 [details] [review]
> WIP
> 
> After some time away from this due to more urgent bugs, I got back to it,
> and we are finding some troubles when testing it.
> 
> Even when using a SIM card that allows CallBarring (tested it before with
> MMI codes), and after being able to activate it through the new menu
> (receive a success response from the RIL), I'm still receiving
> GenericFailure when trying to get the current status of the service
> [getCallBarringOption(option)]. I'm using the option = {
>   'program': 0,
>   'serviceClass': 1
> }
> 
> Is there something wrong on my code, or is the RIL not answering correctly?
> 
> STR:
> - Install the WIP patch over latest master.
> - Go to Settings > Call Settings > Call Barring
> - When opening the panel, the request is made to the RIL, always failing
> (see the logs)
> - Activate / Deactivate any option, and see that the answer is a success
> (not reverting state)

Debugging now. I will update my finding here.

Comment 48

[Szu-Yu Chen [:aknow] (inactive)]

Well, I couldn't find the problem. I tried -- getCallBarringOption({'program':0, 'serviceClass':1}) and got the successful result.

Comment 49

[Fernando Campo (:fcampo)]

Attachment: log_RILdebugging_get.txt

Maybe this might help? RIL DEBUGGING activated, with the following steps:
1. Activate BAOC with MMI codes
2. Check with MMI that BAOC is indeed activated
3. Open Call Barring menu (triggers a getCallBarringOption for each service)

log shows the answer GenericFailure

Comment 50

[Fernando Campo (:fcampo)]

Attachment: CallBarring_RIL-ON_12092014.txt

Updated LOG with the RIL Worker debug on, hope this one helps.

Comment 51

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: use password for query call barring

Comment 52

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to [PTO 28/8 - 9/9] Fernando Campo (:fcampo) from comment #50)
> Created attachment 8488991 [details]
> CallBarring_RIL-ON_12092014.txt
> 
> Updated LOG with the RIL Worker debug on, hope this one helps.

Hi Fernando,

I noticed some differences between performing the query by API and by MMI code. When working by MMI, you use "*#33*0000#". That means the password ("0000") is specified. However, if we use getCallBarringOption, the password will be set to empty ("") in gecko because we thought the password is not required.

Well, maybe it's not the case and the password is needed for the operator you're testing on. I have two suggestions.

1. Can you try MMI code -- *#33#
   Let's see what will the result be if we don't specify the password.

2. Apply the gecko patch ( attachment 8489190 [details] [diff] [review] ). Then set the password in option
   Ex: getCallBarringOption({'program':0, 'serviceClass':1, 'password': "0000"}).

Comment 53

[Fernando Campo (:fcampo)]

Attachment: CallBarring_get_with_password.txt

Hey Szu-Yu, sorry for the delay with the tests.

New log with RIL debug on and your patch applied, testing the getCallBarringOption with a password ('0000'). The test made the following:

- Activate BAOC with MMI - *33*0000*11#
- Check that it was indeed activated, not using password - *#33# - result is OK
- Check that is active, now with password - *#33*0000# - result is OK
- Go to the app to make the check through the API, using password - getCallBarringOptions({"program":0,"password":"0000","serviceClass":1}) - result ERROR

Hope it helps, because frankly, I don't have any idea of what's going on.

Comment 54

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Fernando Campo (:fcampo) from comment #53)
> Created attachment 8490807 [details]
> CallBarring_get_with_password.txt
> 
> Hey Szu-Yu, sorry for the delay with the tests.
> 
> New log with RIL debug on and your patch applied, testing the
> getCallBarringOption with a password ('0000'). The test made the following:
> 
> - Activate BAOC with MMI - *33*0000*11#
> - Check that it was indeed activated, not using password - *#33# - result is
> OK
> - Check that is active, now with password - *#33*0000# - result is OK
> - Go to the app to make the check through the API, using password -
> getCallBarringOptions({"program":0,"password":"0000","serviceClass":1}) -
> result ERROR
> 
> Hope it helps, because frankly, I don't have any idea of what's going on.

Looks like the password is not the problem.

I checked the log and noticed the only difference is that
  1. sendMMI "*#33#" => it use serviceClass = 0
  2. getCallBarringOptions({"program":0,"password":"0000","serviceClass":1}) => serviceClass = 1
All the other parameters that gecko sent to modem are the same.

How about trying getCallBarringOptions({"program":0,"serviceClass":0})
I don't have other ideas.

Comment 55

[Fernando Campo (:fcampo)]

Attachment: CB_RIL_ON_1809v2_serviceClass0_get.txt

RIL DEBUG LOG testing getCallBarringOption with {"program":0,"serviceClass":0}
Result SUCCESS

Comment 56

[Fernando Campo (:fcampo)]

Attachment: CB_RIL_ON_1809v2_2_serviceClass1_set.txt

RIL DEBUG LOG testing setCallBarringOption with {"program":0,"enabled":false,"password":"0000","serviceClass":1}
Result SUCCESS

Comment 57

[Fernando Campo (:fcampo)]

Attachment: CB_MMI_test.txt

As :aknow requested, tested different MMI codes

*#33# --> OK
*#33*0000# --> ok
*#33*0000*11# -- KO!

Also tested from an Android device, with same result. Apparently using the service 11 on the MMI works in Taiwan, but not on spanish Telefonica SIM nor UK O2 ones.

Probably Szu-Yu can give more details about this operator specific issue.

Comment 58

[Fernando Campo (:fcampo)]

Attachment: Link to PR - https://github.com/mozilla-b2g/gaia/pull/25064

Comment 59

[Fernando Campo (:fcampo)]

Looking at the size of the PR (1500 lines), and to try to keep the mental health of the reviewers at an acceptable level, I'm trying to split it in smaller chunks, but being this a whole new functionality the only way I see of doing it is to make a first PR with a dumb menu (no interaction), and then adding functionality in subsequent pull requests. My proposal is:
- 1. Move 'Call Forwarding' to a submenu. Add item to 'Call Settings' list that opens new screen.
- 2. Add 'Call barring' item in Call Settings list, and create new screen with dumb options (not clickable).
- 3. Add functionality to CallBarring screen. Get new values at startup, Set when clicking on item (use '0000' as default password).
- 4. Ask for password when trying to set a new value
- 5. Add 'Change Password' functionality.

Asking for ni? to JoseAntonio as he'll be the reviewer, and to Arthur as the owner of Settings, to see if it is acceptable to merge dumb screens without full functionality (for a short time, at least).

Comment 60

[Anshul]

Do we really have a requirement to have a UI for this feature given that users can already exercise the call barring feature using MMI codes? This is a feature that is not widely used and adding a lot of code for something that is not widely used or may not be used at all doesn't seem to make much sense. I might be completely wrong here so apologize in advance.

Comment 61

[Fernando Campo (:fcampo)]

Attachment: [Patch1] - Move Call Forwarding to submenu

Comment 62

[Fernando Campo (:fcampo)]

Attachment: [Patch2] - Adding Call Barring screen (no interaction)

Comment 63

[Beatriz Rodríguez [:brg]]

(In reply to Anshul from comment #60)
> Do we really have a requirement to have a UI for this feature given that
> users can already exercise the call barring feature using MMI codes? This is
> a feature that is not widely used and adding a lot of code for something
> that is not widely used or may not be used at all doesn't seem to make much
> sense. I might be completely wrong here so apologize in advance.
Each of our operating business units had asked for it. This is our most wanted feature.

Comment 64

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Fernando Campo (:fcampo) from comment #59)

> - 1. Move 'Call Forwarding' to a submenu. Add item to 'Call Settings' list
> that opens new screen.
> - 2. Add 'Call barring' item in Call Settings list, and create new screen
> with dumb options (not clickable).
> - 3. Add functionality to CallBarring screen. Get new values at startup, Set
> when clicking on item (use '0000' as default password).
> - 4. Ask for password when trying to set a new value
> - 5. Add 'Change Password' functionality.

Add this five patches as five commits on the same PR please. Once all of them get reviewed we land the whole feature. I hope Arthur agrees with that. IMHO we should not land anything that is not fully functional.

Comment 65

[Anshul]

(In reply to Beatriz Rodríguez [:brg] from comment #63)
> (In reply to Anshul from comment #60)
> > Do we really have a requirement to have a UI for this feature given that
> > users can already exercise the call barring feature using MMI codes? This is
> > a feature that is not widely used and adding a lot of code for something
> > that is not widely used or may not be used at all doesn't seem to make much
> > sense. I might be completely wrong here so apologize in advance.
> Each of our operating business units had asked for it. This is our most
> wanted feature.
Thanks for the confirmation Beatriz.

Comment 66

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

I agree with Jose that the five patches should be in the same PR but they could be in five commits.

Comment 67

[Fernando Campo (:fcampo)]

Attachment: 4 parts Pull Request

I ended up doing only 4 commits, hope they are small enough for a not so hard review :)

Comment 68

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

Comment on attachment 8506225 [details] [review]
4 parts Pull Request

Newly added panels should be wrapped as AMD modules. Please refer to this page[1] for details. And it seems call barring is a quite standalone function, newly added code is supposed not being referred in |call.js|.

[1]: https://github.com/mozilla-b2g/gaia/tree/master/apps/settings

Comment 69

[Fernando Campo (:fcampo)]

Attachment: 5 commit PR (panel adapted)

converted into AMD modules, please arthur give some feedback as it's the first time I deal with this pattern

Comment 70

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

Comment on attachment 8516359 [details] [review]
5 commit PR (panel adapted)

Sorry for the late feedback. I left some comments in github. The general guideline would be keeping the UI logic and business logic separated. And try not to include many functionality in a single module, which might cause side effects easily when modifying the code.

I understand that the direction seems vague, so feel free to pin me when you need. :)

Comment 71

[Fernando Campo (:fcampo)]

Attachment: panel test

Hi Arthur. Following our talk, I've updated the panels as you advised, separating UI and logic, and creating a specific panel for the passcode change.
Could you give some more feedback before I ask for final review?

I created a separated branch for this, to not mess much with the original work.

Many thanks.

Comment 72

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

Comment on attachment 8525988 [details]
panel test

The CallBarring object does not only wrap the API. In addition, it should provide properties that are to be displayed on UI or related to UI states.

For example, the UI displays the states of the five types of the call barring settings. And in some cases the check boxes corresponding to certain types for settings are disabled. Therefore, the CallBarring should at least have five properties representing the settings and another X (which I am not so sure) properties controlling the disabled state of the check boxes.

CallBarring should also provide methods for manipulating the settings or querying the settings from the carrier. The results are exposed via the properties mentioned earlier.

The code for the UI only changes settings using the exposed methods. It also observes the changes of the properties and updates the UI accordingly.

Comment 73

[Fernando Campo (:fcampo)]

Attachment: Observer pattern

Followed your advice and created an observer for the data model where UI changes depends.

Please give some more feedback, hopefully we're closer.

Comment 74

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

Comment on attachment 8529843 [details]
Observer pattern

It looks good! f=me. Now the CallBarring module works as what we discussed.

One last point would be the creation of the CallBarring instance. I think it would be better to have only one CallBarring instance for each mobile connection. Consider this case: users enter the call barring panel and leave the panel while querying the call barring status. When users enter the panel again, it is not likely creating another CallBarring instance and do the querying again. Let's address this in the following review process.

Comment 75

[Fernando Campo (:fcampo)]

(In reply to Arthur Chen [:arthurcc] (PTO 12/8 - 12/9) from comment #74)
> Comment on attachment 8529843 [details]
> Observer pattern
> 
> It looks good! f=me. Now the CallBarring module works as what we discussed.
\o/

> One last point would be the creation of the CallBarring instance. I think it
> would be better to have only one CallBarring instance for each mobile
> connection. Consider this case: users enter the call barring panel and leave
> the panel while querying the call barring status. When users enter the panel
> again, it is not likely creating another CallBarring instance and do the
> querying again. Let's address this in the following review process.
I've updated the patch with your request, and added a check for updating in process to address your concerns. Now if user triggers a request, exit the screen and comes back to trigger it again, we keep waiting for the results of the first request without creating a second request.

If your feedback is still positive with this, I'm gonna start adding the tests to request a full review.

Comment 76

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

I did not see the latest changes you made in the PR[1]. Are they in a separate one?

[1]: https://github.com/fcampo/gaia/compare/mozilla-b2g:master...call-barring-panels-2

Comment 77

[Fernando Campo (:fcampo)]

(In reply to Arthur Chen [:arthurcc] from comment #76)
> I did not see the latest changes you made in the PR[1]. Are they in a
> separate one?
> 
> [1]:
> https://github.com/fcampo/gaia/compare/mozilla-b2g:master...call-barring-
> panels-2

Argh, I was uploading in automatic pilot, and didn't make a new commit but amend the existing one :([https://github.com/fcampo/gaia/commit/d7a6b31a61579ef2603e8c78da795f50b87a7341]

Changes are there but are difficult to see. I'll try to update again later today.

Comment 78

[Fernando Campo (:fcampo)]

Ok, updated, you can see latest changes on new commit https://github.com/fcampo/gaia/commit/178002d8625313867aa1e3329ce3e678d52d6b78

Comment 79

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

If you return an object from the module definition function, requirejs guarantees that you get the same object every time you require the module. Let's just make CallBarring an object instead of a function.

In addition to that, we should remove the use of DsdsSettings.getIccCardIndexForCallSettings() in CallBarring as it is considered as UI logic. Instead, the code of UI should get the corresponding instance of CallBarring of a certain icc card and performs operations using it.

Comment 80

[Fernando Campo (:fcampo)]

Updated again, over the same commit. callbarring is now an object, and we set mobileConnection from the UI.

I'd rather do the assignment during the init process instead of checking it on every API call, as I don't see a reason why it would change during the process, but I've not a strong opinion about it.

I also added a toast when we get an error from the API, it wasn't on the specs but it feels weird when user clicks and nothing changes without a reason.

Please give new feedback.

P.S. I'm having some problems creating integration tests for this, as we need a SIM card and no way of mocking it, I'm trying to use a device, but getting some errors during the process. If you have experience on this, I'd be glad to talk about it.

Comment 81

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

 (In reply to Fernando Campo (:fcampo) from comment #80)
> Updated again, over the same commit. callbarring is now an object, and we
> set mobileConnection from the UI.
> 
> I'd rather do the assignment during the init process instead of checking it
> on every API call, as I don't see a reason why it would change during the
> process, but I've not a strong opinion about it.

This way the call barring states of two sim cards are mixed together.

Consider this case: users navigate to the call barring of one sim card and do a configuration. Before the configuration is completed, they navigate to the call barring panel of the other sim card. I'm not sure if it is allowed by RIL but if yes, they should be able to do the configuration on this sim card. This can only be done by creating a call barring instance for each mobile connection object. That means we will have two call barring instances on devices with two sim slots like the flame.

> 
> I also added a toast when we get an error from the API, it wasn't on the
> specs but it feels weird when user clicks and nothing changes without a
> reason.

Sounds good!

> 
> Please give new feedback.
> 
> P.S. I'm having some problems creating integration tests for this, as we
> need a SIM card and no way of mocking it, I'm trying to use a device, but
> getting some errors during the process. If you have experience on this, I'd
> be glad to talk about it.

I don't have such experience, either. How about writing unit tests?

Comment 82

[Fernando Campo (:fcampo)]

Attachment: Final patch

So as a new year's present, here's the final patch after all the rounds of feedback. I believe all concerns are addressed now, so ready to review :)

Comment 83

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

Comment on attachment 8542971 [details] [review]
Final patch

Thanks for the effort! Basically it looks good and works well. Although I would prefer using separate observable for each mobile connection so the call barring states won't be overwritten while making queries on different sim cards, but this can be improved with a follow-up bug.

I left some comments in github mainly focusing on the tests. The code could be greatly reduced by using loops as most of the test cases are only slightly different.

Comment 84

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

Why?

Comment 85

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

You're asking about the feature nomination?
This is the requirement from partner.
I think the conclusion is to allow landings which is in good state.

Comment 86

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

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #85)
> I think the conclusion is to allow landings which is in good state.

You don't need a feature-b2g flag to say the patch is in a good state. qa-approval and release management rules are in place to determine that.

Given the fact there has been a few rounds of reviews already, I don't think this will miss FL anyway. Why are we abusing the flag?

Comment 87

[Fernando Campo (:fcampo)]

Comment on attachment 8542971 [details] [review]
Final patch

tests reduced and nits addressed, thanks for this first round.

I also added some comments on github to explain some things I left unchanged, and agree with you that it would be a good idea to add a followup for the edge cases (change SIM in the middle of the process).

Please take another look at the code :)

Comment 88

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

Comment on attachment 8542971 [details] [review]
Final patch

We are almost there. Only Two items to be addressed before merging.

- Set the check boxes to unchecked before showing the panel.
- Use MockMobileConnection directly without setting it to navigator.

Details please refer to github, thanks!

Comment 89

[Fernando Campo (:fcampo)]

Comment on attachment 8542971 [details] [review]
Final patch

All comments addressed, hopefully for the last round :)

Comment 90

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

Comment on attachment 8542971 [details] [review]
Final patch

r=me, thanks!

Comment 91

[Anshul]

Is this bug intended for 2.2?

Comment 92

[Maria Angeles Oteo (:oteo)]

(In reply to Anshul from comment #91)
> Is this bug intended for 2.2?

Yes, it is

Comment 93

[Fernando Campo (:fcampo)]

\o/ great!! 

waiting for the last manual tests and intermittent linter failures to be fixed before squashing commits and merge.

Comment 94

[Loli (:lolimartinezcr)]

flame
eng
master
Gecko-3e13a1e
Gaia-e0cc056.tgz

Tested Fernando's patch:

- "Call barring" menu, doesn't load with predefined configuration by user.
- Change password option doesn't shown "actual password" screen, second time user try to change password.

Improvement:
Try to change the error message ("Generic Failure") when user enables an Call barring and inserts an invalid password.

Comment 95

[Fernando Campo (:fcampo)]

Attachment: patch with extra logs

Attached a patch with extra logs to see what's going on during the api requests.

I also think I fixed the password change issue on the second access to the screen, please re-test

Comment 96

[Loli (:lolimartinezcr)]

(In reply to Fernando Campo (:fcampo) from comment #95)
> Created attachment 8551399 [details] [diff] [review]
> patch with extra logs
> 
> Attached a patch with extra logs to see what's going on during the api
> requests.
> 
> I also think I fixed the password change issue on the second access to the
> screen, please re-test

I have applied new patch and "Change password option doesn't shown "actual password" screen, second time user try to change password." is resolved.

But in some scenarios (I don't know steps exactly yet) when i try to change password, SIM card is blocked. 

I hope to SIM available for further testing.

Comment 97

[Loli (:lolimartinezcr)]

Tested the new patch. 

I always have tested with Spain SIM card and some scenarios haven't been possible to test:
-  Barring of Outgoing International Call except those made to the Home Country supplementary service (BOIC- exHC)
-  Barring of All Incoming Calls in Roaming supplementary service (BAIC-R).

Comment 98

[Fernando Campo (:fcampo)]

so all green (finally), gaia open, and me happy = merged on master https://github.com/mozilla-b2g/gaia/commit/e4c69e3f5149f7fb450016807f2a309e9363184f

Many thanks to jaoo and Arthur that helped me a lot understanding the new settings structure

Comment 99

[Fernando Campo (:fcampo)]

Attachment: Approval-gaia-2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: user won't be able to to set call barrings from menu
[Testing completed]: manual testing, unit testing
[Risk to taking this patch] (and alternatives if risky): low risk, changes on call forwarding, but tests done are ok
[String changes made]: multiple strings added, but no changes on existing ones

Comment 100

[Francesco Lodolo [:flod]]

How is it possible to test this bug? I tried flashing Gaia on my flame but the menu is disabled.

One small nit for the future: strings should use curly quote ’, not straight quotes '

Comment 101

[Fernando Campo (:fcampo)]

(In reply to Francesco Lodolo [:flod] from comment #100)
> How is it possible to test this bug? I tried flashing Gaia on my flame but
> the menu is disabled.
For testing this you need a device with a SIM card, plus call barring provisioned on that sim (operator does that). 
Due to the current implementation of Call Settings, you need to wait a little for the different menus to be enabled (until the update of the operator provided settings is finished).
You can also use MMI codes to check that the changes made on the menu are indeed passed to the operator.

> 
> One small nit for the future: strings should use curly quote ’, not straight
> quotes '
Totally my fault, I got so focused on finishing the functionality and merging asap that I forgot to ask for l10n feedback for the new strings :(

Comment 102

[bhavana bajaj [:bajaj]]

(In reply to Fernando Campo (:fcampo) from comment #101)
> (In reply to Francesco Lodolo [:flod] from comment #100)
> > How is it possible to test this bug? I tried flashing Gaia on my flame but
> > the menu is disabled.
> For testing this you need a device with a SIM card, plus call barring
> provisioned on that sim (operator does that). 
> Due to the current implementation of Call Settings, you need to wait a
> little for the different menus to be enabled (until the update of the
> operator provided settings is finished).
> You can also use MMI codes to check that the changes made on the menu are
> indeed passed to the operator.

Can you please help test this on-device once it lands on 2.2 or verify it on master even ? Thanks!
> 
> > 
> > One small nit for the future: strings should use curly quote ’, not straight
> > quotes '
> Totally my fault, I got so focused on finishing the functionality and
> merging asap that I forgot to ask for l10n feedback for the new strings :(

Comment 103

[Maria Angeles Oteo (:oteo)]

> 
> Can you please help test this on-device once it lands on 2.2 or verify it on
> master even ? Thanks!

The feature has already been tested by Telefónica QA team in master branch (see comment 97), as soon as it uplifts to 2.2 it will be verified also in that branch.

Comment 104

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/d7d8ddecc39226d05f87579d1afd3d582aa083af

Comment 105

[Loli (:lolimartinezcr)]

Attachment: CallBarring_v1.docx

Comment 106

[Loli (:lolimartinezcr)]

Tested with build master (02/03/2015)
Eng
Gecko-769dd38
Gaia-321c698
All scenarios tested you can see it in attached file: CallBarring_v1.docx. I haven't changed the status of this bug to "verified" because some scenarios, I can't test them.