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

Status: RESOLVED FIXED

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

Description

[sync-1]

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:

Comment 1

[sync-1]

Clone from brother

Comment 2

[sync-1]

Attachment: QXDM log

Clone from brother

Comment 3

[sync-1]

Clone from brother

Comment 4

[sync-1]

Attachment: ADB log

Clone from brother

Comment 5

[buri.blff]

This is GCF block bug;

Comment 6

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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.

Comment 7

[Carol Yang [:cyang]]

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.

Comment 8

[buri.blff]

Attachment: The log with debug in gaia true

the is the log with gaia debug true

Comment 9

[buri.blff]

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.

Comment 10

[buri.blff]

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.

Comment 11

[Carol Yang [: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?

Comment 12

[Fernando R. Sela (no CC, needinfo please) [: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?

Comment 13

[buri.blff]

This is GCF block case, Can we resolve it in the future??

Comment 14

[Alex Keybl [:akeybl]]

Let's resolve for v1.1

Comment 15

[Carol Yang [:cyang]]

(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.

Comment 16

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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?

Comment 17

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Adding German since dialer app is involved

Comment 18

[Carol Yang [:cyang]]

(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"

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking based on allowing it to be pushed out from v1.0.1 to v1.1 for GCF.

Comment 20

[Fernando R. Sela (no CC, needinfo please) [: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.

Comment 21

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 22

[Etienne Segonzac (: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.

Comment 23

[Germán Toro del Valle (:gtorodelvalle)]

100% agreed! ;-) Any suggestion regarding who to involve here from platform? Thanks!

Comment 24

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

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

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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?

Comment 26

[Etienne Segonzac (:etienne)]

(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)

Comment 27

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

(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)

Comment 28

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(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?

Comment 29

[Etienne Segonzac (:etienne)]

(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)

Comment 30

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(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.

Comment 31

[Joe Cheng [:jcheng] (please needinfo)]

ni? etienne for comment 30

Comment 32

[Etienne Segonzac (:etienne)]

This is a gecko decision, clearing ni.

Comment 33

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, Frsela
Can Settings app be able to send data(stk command) to Dialer App or wake Dailer app up?

Thanks

Comment 34

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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.

Comment 35

[Germán Toro del Valle (: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 :-(

Comment 36

[Alex Keybl [:akeybl]]

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.

Comment 37

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: PR11287.html

German's patch for dialer.

Can you test if the patch fix the issue?

Comment 38

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 784263 [details]
PR11287.html

Asking for review, mainly for the German's code in communications app

Comment 39

[Etienne Segonzac (:etienne)]

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.

Comment 40

[Germán Toro del Valle (:gtorodelvalle)]

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 [e-mail] 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!

Comment 41

[Fernando R. Sela (no CC, needinfo please) [: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 [e-mail] 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.

Comment 42

[Joe Cheng [:jcheng] (please needinfo)]

koi+ as it is added to comms team Sprint 3

Comment 43

[buri.blff]

(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

Comment 44

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(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

Comment 45

[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 [e-mail] 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.

Comment 46

[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.

Comment 47

[Marcelino Veiga Tuimil [:sonmarce]]

(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?

Comment 48

[Germán Toro del Valle (:gtorodelvalle)]

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!

Comment 49

[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.

Comment 50

[Germán Toro del Valle (:gtorodelvalle)]

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!

Comment 51

[Germán Toro del Valle (:gtorodelvalle)]

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).

Comment 52

[Germán Toro del Valle (:gtorodelvalle)]

Damn it! I forgot to enable the NEED-INFO flag for Etienne ;-) Sorry!

Comment 53

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 54

[Etienne Segonzac (: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 :)

Comment 55

[Germán Toro del Valle (:gtorodelvalle)]

Sounds good to me! ;-) I'll update the PR with Etienne's suggestion and ping you again :-) Thanks!

Comment 56

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 57

[Etienne Segonzac (: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

Comment 58

[Germán Toro del Valle (:gtorodelvalle)]

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

Comment 59

[Etienne Segonzac (: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 :)

Comment 60

[Germán Toro del Valle (:gtorodelvalle)]

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!

Comment 61

[Germán Toro del Valle (:gtorodelvalle)]

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 62

[Etienne Segonzac (:etienne)]

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');
      });
    });

```

Comment 63

[Germán Toro del Valle (:gtorodelvalle)]

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.

Comment 64

[Etienne Segonzac (: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)

Comment 65

[Germán Toro del Valle (:gtorodelvalle)]

merged in master: https://github.com/mozilla-b2g/gaia/commit/beba509d3b7d7c006c76fcf4dec20e37bbeefd88

Thank you very much, Etienne!

Comment 66

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted beba509d3b7d7c006c76fcf4dec20e37bbeefd88 to:
v1.2: 43865e778e5235bb633852938a118a67a14c7df4