Closed Bug 873906 Opened 11 years ago Closed 11 years ago

[Buri][GCF][STK]case 27.22.4.13.2/1 SET UP CALL (second alpha identifier) failed

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Settings
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Milestone:
1.2 C2(Oct11)
Project Flags:
blocking-b2g koi+
Tracking Flags:
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: sync-1, Assigned: gtorodelvalle)

References

Details

(Whiteboard: [u=commsapps-user c=stk p=0] )

Attachments

(4 files)

QXDM log
1.35 MB, application/octet-stream
Details
ADB log
12.77 KB, application/octet-stream
Details
The log with debug in gaia true
219.72 KB, text/plain
Details
PR11287.html
363 bytes, text/html
etienne
: review+
Details 
SW12A
 AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.105
 Firefox os  v1.0.1
 Mozilla build ID:20130512070209
 
 +++ This bug was initially created as a clone of Bug #455602 +++
 
 DEFECT DESCRIPTION:
 [GCF][STK]case 27.22.4.13.2/1 SET UP CALL (second alpha identifier) failed
 
  REPRODUCING PROCEDURES:
 1 run GCF case 27.22.4.13.2/1 SET UP CALL (second alpha identifier) 
 2 second alpha ID will prompt before call establish--->KO1
 after end the call , ME will not return in idle mode--->KO2
 
  EXPECTED BEHAVIOUR:
 as spec 51.010-4 case 27.22.4.13.2.4.2 definition.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #455602 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:
Clone from brother
Attached file QXDM log — 
Clone from brother
Clone from brother
Attached file ADB log — 
Clone from brother
This is GCF block bug;
blocking-b2g: --- → tef?
Please, attach logcat with STK DEBUG enabled at Gaia level:

Files:

apps/system/js/icc_cache.js
apps/settings/js/utils.js

Both files change the DEBUG=false to DEBUG=true

Thanks in advance.
Flags: needinfo?(sync-1)
Can you please clarify this?

> second alpha ID will prompt before call establish--->KO1

Do you mean before the dialer even attempts to make a call? Perhaps a video recording while running this test would help us understand the issue.
the is the log with gaia debug true
the correct steps should be:

1 the confirmation alpha id prompt;
2 user click ok;
3 dialer connect screen promt.At the same time, the second alpha id(set up call) should promt in dialer connect screen for some seconds and disappear without any user click.
Current is as follow:
1 the confirmation alpha id prompt;
2 user click ok;
3 dialer connect screen promt;
4 the second alpha id(set up call) promt and setting app is launched and promt an alert(set up call), and will not disappear 
5 user click the alert, the screen keep in STK app screen not in dialer screen.
Flags: needinfo?(sync-1) → needinfo?(cyang)
Fernando has mentioned this before:

"Implement STK logic under settings was a decission taken at the first steps of STK implementation. Move to system some of the commands should be done in next releases."

So step 4 in Comment 10 is expected (Settings app is launched to prompt the second alphaId). I'm not sure if there's much that can be done about this part.

However, step 5 may be something that can be done by Gaia (bring dialer screen back to the foreground).

Fernando, thoughts?
Flags: needinfo?(cyang) → needinfo?(frsela)
Carol, agree. In next releases we can afford this bugs, but not now.

But now I don't understand the expected behaviour with on busy status.

In 873908 we discussed about in busy modes (in a call, for example) the STK message shouldn't be showed but in this one we talk about a second message during the call.

Now is showed opening settings (which is bad) but in the future, the message should or not be showed during the call?

Or this is a duplicate of 873908?
Flags: needinfo?(frsela)
This is GCF block case, Can we resolve it in the future??
Let's resolve for v1.1
blocking-b2g: tef? → leo?
(In reply to Fernando R. Sela [:frsela] from comment #12)
> Carol, agree. In next releases we can afford this bugs, but not now.
> 
> But now I don't understand the expected behaviour with on busy status.
> 
> In 873908 we discussed about in busy modes (in a call, for example) the STK
> message shouldn't be showed but in this one we talk about a second message
> during the call.

The issue in bug 873908 is for DISPLAY_TEXT. According to 3GPP TS 102 223, section 6.4.1 on DISPLAY_TEXT, the command qualifier specifies the priority of a text and when it should be displayed or rejected.

> 
> Now is showed opening settings (which is bad) but in the future, the message
> should or not be showed during the call?

The issue described by this current bug is for SET_UP_CALL. From the TS, I do not see any mention of priority for the alphaId display. The command qualifier here is just used to indicate whether a call is allowed to be setup or not based on whether the current status of the phone is busy or not.

> 
> Or this is a duplicate of 873908?

In a nutshell, no, this is not a duplicate of 873098.
Hi,

STK receives this JSON:

{"commandNumber":1,"typeOfCommand":16,"commandQualifier":0,"options":{"confirmMessage":"CONFIRMATION","callMessage":"CALL","address":"012340123456,1,2"}}

I supposse "Second alpha Id" is "callMessage", is that right?
Flags: needinfo?(sync-1)
Adding German since dialer app is involved
(In reply to Fernando R. Sela [:frsela] from comment #16)
> Hi,
> 
> STK receives this JSON:
> 
> {"commandNumber":1,"typeOfCommand":16,"commandQualifier":0,"options":
> {"confirmMessage":"CONFIRMATION","callMessage":"CALL","address":
> "012340123456,1,2"}}
> 
> I supposse "Second alpha Id" is "callMessage", is that right?

Yes, second alpha id is read and sent up as "callMessage"
Blocking based on allowing it to be pushed out from v1.0.1 to v1.1 for GCF.
blocking-b2g: leo? → leo+
Assignee: nobody → frsela
After a call with German we decide to use mozSettings to pass the second alpha identifier to the dialer app.

The settings parameter will be "icc.callmessage"

Dialer will check this param each time he receives an "call system message". If the param is set the string will be used instead looking in the contacts list; after that, dialer is responsible to remove the message setting this parameter to "null".

We'll work together in a same patch in system (icc_worker.js) and communications app.
Hi guys! There is a 'minor' hole with the approach mentioned by Fernando in comment 20 we just found. The problem is how to proceed if there is an error during the setting of the icc.callmessage to null. There would be no way to know which information to show in the call attention screen in a forthcoming call.

Does anyone envision any way to have this (and optionally additional information) at a call level (https://developer.mozilla.org/en-US/docs/Web/API/TelephonyCall)?

Inviting Etienne to comment on all this ;-) Thanks!

Just in case you want to have a look at the Communications app patch we prepared but which suffers of the aforementioned problem: https://github.com/gtorodelvalle/gaia/commit/8bb830a1dd469b8876086e95d8853f730119edcb
Flags: needinfo?(etienne)
(In reply to gtorodelvalle from comment #21)
> Hi guys! There is a 'minor' hole with the approach mentioned by Fernando in
> comment 20 we just found. The problem is how to proceed if there is an error
> during the setting of the icc.callmessage to null. There would be no way to
> know which information to show in the call attention screen in a forthcoming
> call.
> 
> Does anyone envision any way to have this (and optionally additional
> information) at a call level
> (https://developer.mozilla.org/en-US/docs/Web/API/TelephonyCall)?
> 
> Inviting Etienne to comment on all this ;-) Thanks!
> 
> Just in case you want to have a look at the Communications app patch we
> prepared but which suffers of the aforementioned problem:
> https://github.com/gtorodelvalle/gaia/commit/
> 8bb830a1dd469b8876086e95d8853f730119edcb

Well... this is a very critical code path, so I would prefer a gecko-supported solution rather than a fragile mozSettings hack.

We already have a |call.emergency| boolean affecting what we display on the call screen.
Maybe we could have a |call.iccmessage| string working in a similar way.
Flags: needinfo?(etienne)
100% agreed! ;-) Any suggestion regarding who to involve here from platform? Thanks!
Basically, I am hoping to have a clear module boundary between stk and telephony codes in gecko and not so excited about adding an additional attribute in a call object. However, after discussing with other developers about the inter-app communication mechanism, I realized that leaving gaia taking care of this issue seems a worse idea. So exposing a new attribute to a call object might be the way to go, at least at this moment. 

So, what would be done in gecko might look like:
in [1], once we got 'callMessage' (i.e. second alpha id), we search for the exact call from this.currentCalls by 'address' (i.e. call number). Then, send out some notification to Telephony to update nsIDOMTelephonyCall.iccMessage (Hm... we might need a better naming here).

ni? Yoshi as he is the expert of STK.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#9044
Flags: needinfo?(allstars.chh)
On Android it uses 'Toast' to display this kind of message without bringing the App for foreground and back forth.

If Gaia doesn't have this UI component, then why cannot Dial app listen STK command event by itself?
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #25)
> then why cannot Dial app listen STK
> command event by itself?

This is a really good point.

Can we add a listener a listener on mozIccManager in |oncall.js| and get the information we need? (string to display _and_ which call should it be displayed on)
(In reply to Etienne Segonzac (:etienne) from comment #26)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #25)
> > then why cannot Dial app listen STK
> > command event by itself?
> 
> This is a really good point.
> 

Yoshi's suggestion looks good to me as well! 

> Can we add a listener a listener on mozIccManager in |oncall.js| and get the
> information we need? (string to display _and_ which call should it be
> displayed on)
Whiteboard: [u=commsapps-user c=stk p=0]
(In reply to Etienne Segonzac (:etienne) from comment #26)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #25)
> > then why cannot Dial app listen STK
> > command event by itself?
> 
> This is a really good point.
> 
> Can we add a listener a listener on mozIccManager in |oncall.js| and get the
> information we need? (string to display _and_ which call should it be
> displayed on)

But if you listen to this system-message each STK message will open dialer ... we moved to system to avoid this effect on settings. Listening from dialer will be overkilling

Could be possible to create an alternative system message only for STK_CMD_SET_UP_CALL?

Also, calling Dialer from System when system receives the STK_CMD_SET_UP_CALL message using webintents.

WDYT?
(In reply to Fernando R. Sela [:frsela] from comment #28)
> (In reply to Etienne Segonzac (:etienne) from comment #26)
> > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #25)
> > > then why cannot Dial app listen STK
> > > command event by itself?
> > 
> > This is a really good point.
> > 
> > Can we add a listener a listener on mozIccManager in |oncall.js| and get the
> > information we need? (string to display _and_ which call should it be
> > displayed on)
> 
> But if you listen to this system-message each STK message will open dialer
> ... we moved to system to avoid this effect on settings. Listening from
> dialer will be overkilling
> 
> Could be possible to create an alternative system message only for
> STK_CMD_SET_UP_CALL?

Can we use a event listener instead of a system message? (if I understand correctly, in the scenario we want to tackle with this bug, we're guaranteed that the dialer is already open)
(In reply to Etienne Segonzac (:etienne) from comment #29)
> (In reply to Fernando R. Sela [:frsela] from comment #28)
> > (In reply to Etienne Segonzac (:etienne) from comment #26)
> > > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #25)
> > > > then why cannot Dial app listen STK
> > > > command event by itself?
> > > 
> > > This is a really good point.
> > > 
> > > Can we add a listener a listener on mozIccManager in |oncall.js| and get the
> > > information we need? (string to display _and_ which call should it be
> > > displayed on)
> > 
> > But if you listen to this system-message each STK message will open dialer
> > ... we moved to system to avoid this effect on settings. Listening from
> > dialer will be overkilling
> > 
> > Could be possible to create an alternative system message only for
> > STK_CMD_SET_UP_CALL?
> 
> Can we use a event listener instead of a system message? (if I understand
> correctly, in the scenario we want to tackle with this bug, we're guaranteed
> that the dialer is already open)

Not agree...

The STK_CMD_SET_UP_CALL message can be received at any time (with dialer closed too)

If the user accpets the call, the modem will start the establishment and will sent a System Message to open dialer.

At this point the Dialer shall receive the message about the second alpha id...

So the STK_CMD_SET_UP_CALL and the Dialer open proccess is async.

What I was suggesting is to manage this message (and only this) by the dialer or that system sent an intent to dialer if the user accepts the call so the dialer will receive both messages (async too) but easier to manage.
Assignee: frsela → gtorodelvalle
ni? etienne for comment 30
Flags: needinfo?(etienne)
This is a gecko decision, clearing ni.
Flags: needinfo?(etienne)
Hi, Frsela
Can Settings app be able to send data(stk command) to Dialer App or wake Dailer app up?

Thanks
Hi Yoshi,

Now is managed by system (settings only for menu navigation ;)

Anyway, it's possible through webactivities or mozsettings, but as German explained before, dialer team worries about async with this message since in a limit situation the name will be showed to others callers ...

In my side is possible but dialer team is responsible of this too.
Flags: needinfo?(gtorodelvalle)
Not much to add here to what we have already discussed, sorry :-(
The sending of the info to the Dialer app using a Setting presents the corner case I mention in comment 21.
Regarding the awaking of the Dialer app, I would say that would mean to include additional changes since currently the Dialer app is awaken by the telephony-new-call system message in this particular case.
Sorry for not being more useful :-(
Flags: needinfo?(gtorodelvalle)
Bumping this to 1.2 based upon the progress made thus far, and where we are in the cycle. Didn't block 1.0.1, so it's unclear why we'd block 1.1.
blocking-b2g: leo+ → koi?
Flags: needinfo?(sync-1)
Attached file PR11287.html — 
German's patch for dialer.

Can you test if the patch fix the issue?
Attachment #784263 - Flags: feedback?(sync-1)
Flags: needinfo?(sync-1)
Comment on attachment 784263 [details]
PR11287.html

Asking for review, mainly for the German's code in communications app
Attachment #784263 - Flags: review?(21)
Comment on attachment 784263 [details]
PR11287.html

This patch needs rebasing, but some quick notes:

* Please remove all the debugging statements
* handled_call.js is a critical and well tested piece of the dialer, you need to add unit tests for your changes (we already have mocks for the settings api).
* I'm not sure what should appear in the call log after the call here, but we may need additional changes to make it work.
Attachment #784263 - Flags: review?(etienne)
Blocks: 875679
No longer depends on: 875679
Retaking this bug... :-)

Regarding Etienne's comments, yeah, absolutely. This was kind of a proposal for the time being since you may be aware that the testing of these features is not quite straightforward (we need help by third parties for the testing). So I guess, what we need first is to have confirmation that the proposed patch (Fernando's side and my side) works as expected and then we can prepare a final version (including test cases, deleting debug traces and so on). In fact, I would appreciate some kind of confirmation that the final decision is to go for the original Gaia oriented solution based on passing the info using Settings, since out of comment 24 to comment 34 I am not that sure what the final decision is.

In case, the final decision is the Gaia one, Fernando, do we have confirmation that the patch works as expected and desired :) ? I didn't see any comment about it. Is it sync-1@bugzilla.tld the one who have to give us the final OK? I guess they need rebasing of the patches at least, right? I'll contact you also on Skype to speed this up. Thanks!
Flags: needinfo?(frsela)
(In reply to gtorodelvalle from comment #40)

> In case, the final decision is the Gaia one, Fernando, do we have
> confirmation that the patch works as expected and desired :) ? I didn't see
> any comment about it. Is it sync-1@bugzilla.tld the one who have to give us
> the final OK? I guess they need rebasing of the patches at least, right?
> I'll contact you also on Skype to speed this up. Thanks!

Thank you German !

I don't receive any confirmation through other channels.

I will add for more information to bury who uploaded the log traces.
Flags: needinfo?(frsela) → needinfo?(buri.blff)
koi+ as it is added to comms team Sprint 3
blocking-b2g: koi? → koi+
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #16)
> Hi,
> 
> STK receives this JSON:
> 
> {"commandNumber":1,"typeOfCommand":16,"commandQualifier":0,"options":
> {"confirmMessage":"CONFIRMATION","callMessage":"CALL","address":
> "012340123456,1,2"}}
> 
> I supposse "Second alpha Id" is "callMessage", is that right?

yes
Flags: needinfo?(sync-1)
Flags: needinfo?(buri.blff)
(In reply to buri.blff from comment #43)

Comment #16 had been answered in June (comment #18)

We need a response to comment #40 which refers to comment #37 (patch feedback) to assure this fix the issue. Thanks in advance
Flags: needinfo?(buri.blff)
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #41)
> (In reply to gtorodelvalle from comment #40)
> 
> > In case, the final decision is the Gaia one, Fernando, do we have
> > confirmation that the patch works as expected and desired :) ? I didn't see
> > any comment about it. Is it sync-1@bugzilla.tld the one who have to give us
> > the final OK? I guess they need rebasing of the patches at least, right?
> > I'll contact you also on Skype to speed this up. Thanks!
> 
> Thank you German !
> 
> I don't receive any confirmation through other channels.
> 
> I will add for more information to bury who uploaded the log traces.

I will give the test result tomorrow, thanks.
Flags: needinfo?(buri.blff)
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #37)
> Created attachment 784263 [details]
> PR11287.html
> 
> German's patch for dialer.
> 
> Can you test if the patch fix the issue?

it's ok, please close this pr, thanks.
(In reply to buri.blff from comment #46)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #37)
> > Created attachment 784263 [details]
> > PR11287.html
> > 
> > German's patch for dialer.
> > 
> > Can you test if the patch fix the issue?
> 
> it's ok, please close this pr, thanks.

Do you mean you have tested the patch an it is ok?
Regarding last comment by Marce, buri.blff, have you tested the patch provided by Fernando (https://github.com/mozilla-b2g/gaia/pull/11287) plus the updates I proposed for the handled_call.js file (https://github.com/gtorodelvalle/gaia/commit/8bb830a1dd469b8876086e95d8853f730119edcb) ?

If the answer to the previous questions is yes, are you suggesting us to create a new PR including Fernando and my patches so all the updates are collected in the same PR? You say "please close this pr"... Which PR? Fernando's (it misses my updates)? Do you mean closing without merging or merging and closing?

Thanks!
Flags: needinfo?(buri.blff)
we has check, the Bug 873906 - [Buri][GCF][STK]case 27.22.4.13.2/1 SET UP CALL (second alpha identifier)  is ok with the patch in attchement, please merge the patch and close this pr;, thanks.
Flags: needinfo?(buri.blff)
Hi Etienne, we need your review to the latest update to attachment 784263 [details].

I'll try to tell you the problems we found when solving this bug, why the solution was causing the unit tests to fail and the solution we found to make the tests pass.

As you can see at https://github.com/mozilla-b2g/gaia/pull/11287/files#L0L150 , we are substituting the asynchronous invocation to |Contacts.findByNumber(number, lookupContact);| with the code which solves this bug. This additional code is causing the update of the UI to take a little longer than before and, as a consequence, some of the test cases to fail since the assertions are run before the UI is ready to be asserted :-) In fact, let me tell you that the code which solves this bug has nothing to do with the test cases from failing and a simple change such as substituting:

Contacts.findByNumber(number, lookupContact);

by:

setTimeout(function() {
  Contacts.findByNumber(number, lookupContact);
}, 1000);

will cause the same effect. Or including some code in |findByNumber| to make it take longer to update the info about the contact.

To solve this race condition, we tried 2 options, both of which make the test cases to pass.

The first one is the one included in the PR (attachment 784263 [details]). As you can see, we use MutationObserver to check when the number element (|querySelector('.numberWrapper .number');|) is updated as a consequence of the invocation of |lookupContact|. This let us know when the UI is ready to be asserted and consequently the test cases pass.

Another solution I also tested and which works is to include a callback in |HandledCall| to be notified when the |lookupContact| execution ends so that the UI is ready to be asserted. The drawback of this second solution is the fact that we are modifying the (business) code to make the test cases pass. This is the reason why I opted for the MutationObserver solution which let us solve the issue with no effect on the (business) code.

I would appreciate your thoughts about it.

Thank you very much!
BTW, the errors Travis is complaining about in the PR have nothing to do with this update (they are related to the email app integration tests and the browser app unit tests).
Damn it! I forgot to enable the NEED-INFO flag for Etienne ;-) Sorry!
In fact I just noticed that in Google Chrome I have to check the "Need more information from ..." checkbox and it is not enough to just include the person I need info about in the input text field (in Firefox doing this automatically checks the checkbox) :-O I am using Google Chrome just for testing purposes, of course... :-)

I knew I had need-infoed Etienne or at least tried to... :-p
Flags: needinfo?(etienne)
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #20)
> The settings parameter will be "icc.callmessage"
> 
> Dialer will check this param each time he receives an "call system message".

Lets pause for a minute.
This solution makes *every single contact lookup* slower in order to fix an STK edge case.
We should at least try to avoid it.

What if we keep the main code path exactly the same (doing the contact lookup directly) and just override the phone number when (and if!) we find something in the settings? (we would still check the setting each time but without delaying anything else.)

We could add a small flag on the handledCall (_stkIdentified for example) to make sure that if we get the contact lookup result after the mozSettings check we wouldn't touch anything (and keep the information provided by the setting displayed).

What do you think?
Sounds like it would still fix the bug, keep the STK code decoupled from the main code path and prevent the test issue all at once :)
Flags: needinfo?(etienne)
Sounds good to me! ;-) I'll update the PR with Etienne's suggestion and ping you again :-) Thanks!
Etienne, I have updated the patch with your proposal or at least as far as I understood it. As you will see, I have also removed all the code I included to deal with the timing of the callbacks execution in the test cases. I checked all of them pass.

Any comment is more than welcome ;-) Thanks!

PR at https://github.com/mozilla-b2g/gaia/pull/11287
Flags: needinfo?(etienne)
Looking much better!

We should probably extract the icc code into a |checkICCMessage()| function to make the code clearer, and, of course, we need to add tests for this :)

We need to test:
* that if the setting has something for us we display it correctly
* that if we execute lookupContact _after_ we got an _iccCallMessage we're keeping the second alpha identifier displayed
Flags: needinfo?(etienne)
Hi Etienne, I have updated the PR including the new function as well as a couple of test cases. Regarding the test cases, I have tried to simplify and to document the code as much as possible. If you need further details, please do not hesitate to ask me ;-)

Any additional comment is more than welcome.

You can find the PR at https://github.com/mozilla-b2g/gaia/pull/11287
Flags: needinfo?(etienne)
Commented on github.

You're really going to like the sinon "yields" method :)
You'll be able to stub findByNumber _and_ choose when the given callback is triggered.

Which means:
* no need to modify the mock_contacts.js
* by controlling when the callback is triggered you know for sure that you can inspect the dom after that. No need for MutationObserver either!

BTW, when you want feedback on a patch you can use the feedback? flag :)
Flags: needinfo?(etienne)
Comment on attachment 784263 [details]
PR11287.html

Hi Etienne, I have updated the unit tests using Sinon.JS instead of MutationObserver and without modifying MockContacts. As you will see, I capture the 2 asynchronous "threads" of execution whose order we want to control (the contact lookup callback and the icc call message handler) and provoke their invocation in the desired order for each one of the 2 test cases.

Would you mind having a look at it, please? Thanks!
Attachment #784263 - Flags: feedback?(sync-1) → feedback?(etienne)
Maybe a clearer and better alternative would be to stub |navigator.mozSettings.createLock()| instead of |subject.replacePhoneNumber()| so we could control the order of the invocations of the |onsuccess| listener and the |contactLookup| callback. This way, if in the future the icc call message retrieval |onsuccess| code changes, we would be covering it and not only the invocation of the |replacePhoneNumber| function. What do you think? Do you want me to give it a try? :-)
Comment on attachment 784263 [details]
PR11287.html

This is not really using sinon's spy.yield ;)

I understand that this part of the sinon API can be tricky to grasp.
But I really want to take the time explain it because it works very well.

Below is what I had in mind, can you have a look at it and tell me if it's clearer?

Basically we're taking the control of when the various callbacks will be triggered:
* When we call reqStub.onsuccess()
* When we call contactLookupSpy.yield(...)

It takes a bit of setup but this way we don't have any async stuffs in the tests and the tests themselves are very readable.

Waiting for your feedback ;)

```
    suite('stk call second alpha identifier', function() {
      var reqStub, settingsSetSpy;
      var contactLookupSpy, tel, contact;

      setup(function() {
        reqStub = {
          onsuccess: null,
          result: {
            'icc.callmessage': 'stk second alpha identifier'
          }
        };

        settingsSetSpy = this.sinon.spy();
        this.sinon.stub(navigator.mozSettings, 'createLock', function() {
          return {
            get: function() {
              return reqStub;
            },
            set: settingsSetSpy
          };
        });

        tel = {
          value: '012345',
          carrier: 'roger',
          type: 'type'
        };
        contact = {
          id: 42,
          name: ['from contact lookup'],
          tel: [tel]
        };
        contactLookupSpy = this.sinon.spy(MockContacts, 'findByNumber');

        mockCall = new MockCall(String(phoneNumber), 'connected');
        subject = new HandledCall(mockCall);
      });

      test('should display the number from the setting', function() {
        contactLookupSpy.yield(null, null, 0);
        reqStub.onsuccess();
        assert.equal(subject.numberNode.textContent,
                     'stk second alpha identifier');
      });

      test('should clear the setting', function() {
        reqStub.onsuccess();
        assert.isTrue(settingsSetSpy.calledOnce);
      });

      test('should not let the contact lookup override the number',
      function() {
        reqStub.onsuccess();
        contactLookupSpy.yield(contact, tel, 0);
        assert.equal(subject.numberNode.textContent,
                     'stk second alpha identifier');
      });
    });

```
Attachment #784263 - Flags: feedback?(etienne)
Comment on attachment 784263 [details]
PR11287.html

Wow! That was just a "little" simpler that my solution :-D In fact, I had that feeling that "this should not be this complex" :-D Nothing better to learn a library and an API that see real code using it. Thank you very much!

I have updated the PR with your suggestions.
Attachment #784263 - Flags: review?(etienne)
Comment on attachment 784263 [details]
PR11287.html

r=me with the tiny nit addressed and a green travis :)

(the last failure doesn't look related, but it's worth waiting for a new travis run)
Attachment #784263 - Flags: review?(etienne) → review+
merged in master: https://github.com/mozilla-b2g/gaia/commit/beba509d3b7d7c006c76fcf4dec20e37bbeefd88

Thank you very much, Etienne!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 QE1(Oct11)
Uplifted beba509d3b7d7c006c76fcf4dec20e37bbeefd88 to:
v1.2: 43865e778e5235bb633852938a118a67a14c7df4
status-b2g-v1.2: --- → fixed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: