1038701 - [US] Support for ICE Contacts in Lockscreen/Emergency call

Status: VERIFIED FIXED

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

Comment 1

[Anthony Ricaud (:rik)]

We now have the Datastore schema done in bug 1042586 so we can start on this.

Comment 2

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

Hi Carol, are the specs for the emergency call dialer page available? Although I guess you are currently aware of them, I would like to share some thoughts with you to be considered and included in that spec, please:
 1. According to page 6 of https://bug1026682.bugzilla.mozilla.org/attachment.cgi?id=8459474 , the "ICE Contacts - In case of emergency" should be shown if there are ICE contacts set, so would need the specs for both cases: 1) there are and 2) there are not ICE contacts set :)
 2. In the case of the emergency call dialer, the user is not suggested contacts when a phone number is typed (as it is in the standard dialer as you can see at https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8415279), so maybe you would like to reuse this contact suggestion area for the "ICE Contacts - In case of emergency" one since in the Hamachi device it may shorten the keypad too much :) But as I said, just for your consideration since I am sure you may end up with a much better and, no doubt, nicer solution ;)

Need-infoing Carrie to please confirm point 2 above (I mean, no contacts should be suggested when typing numbers in the emergency call dialer). Thanks!

Comment 3

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

Hi  Germán, 

Yes we don't suggest contacts on Emergency call page. 
Actually Fang is the one who is doing Contacts design. So ni? Fang here. ;)
Thanks!

Comment 4

[Loli (:lolimartinezcr)]

I change QA Contact because it is asigned by German

Comment 5

[Fang Shih [:fang]]

Attachment: ICE Contacts_Visual Spec.zip

Hi Germán,

I've attached the visual spec for ICE contact on dialer. 
And If there are not ICE contacts set, we'll just use the original emergency call dialer layout, I didn't move around the layout, only added the ICE contact section on the top of it. So no need to change anything if there is no ICE contacts set : ) And since we are in the emergency call dialer page, there will be no tab bar. Much more space, will be no problem for the Hamachi device : )  Thanks!

Comment 6

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

Hi Fang, just a couple of minor remarks (please, consider them just as suggestions ;) ):
  1. As you can see at https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8415279 , the height of the suggestion bar in the standard dialer is 4.5rem whereas you have set the height for the ICE contacts bar to 4 rem. For the sake of uniformity I would suggest also setting the height for the ICE Contacts bar to 4.5rem.
  2. Would you be so kind to attach the ">" icon which appears on the right hand side of the ICE Contacts bar? To this regard, it would be great if you could also include in the specs the margins/paddings/etc. of this icon to precisely position where you really want it to be ;)

You can have a look at the current status of the implementation at http://youtu.be/0q0rkp8T4-s . As you can see, the ">" icon is missing and I am currently using the 4rem height I mentioned in point 1 above :)

Comment 7

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

BTW, last but not least, I saw in the specs that in the second raw of the ICE contacts overlay you only included the telephone number whereas in page 8 of https://bug1026682.bugzilla.mozilla.org/attachment.cgi?id=8459474 and also in my video ( http://youtu.be/0q0rkp8T4-s ), you'll see that the type of phone is also included. In my opinion, this may be of value to make the decision about which of the available phone numbers to call. What do you think? :)

Comment 8

[Fang Shih [:fang]]

Attachment: ICE Contacts_Visual Spec_0814.zip

Hi Germán,

Thanks for the demo video. I tried the ICE contacts bar to 4.5 rem, it looks fine to me as well. Let's make the height to 4.5 rem for the sake of uniformity. I've updated the spec with new height as well, you can refer to the file for more detail of the margins,paddings and ">" assets. For the telephone number included phone type, Yes! I agree with you on this. Thanks for pointing out on this! Spec updated also! : )

Comment 9

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

Thanks Fang! Looking great :)

BTW, I am going to ask ui-review from you instead of from Carol since you have provided the specs ;) If I should ask the ui-review from Carol, please just let me know.

Comment 10

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

Attachment: emergency-call-no-ice-contacts.png

No visual change in the case where there are no ICE contacts with at least 1 associated phone number but just for you to check it ;)

Comment 11

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

Attachment: emergency-call-ice-contacts-available.png

ICE contacts bar when there is at least one ICE contact with an associated phone number.

Comment 12

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

Attachment: emergency-call-ice-contacts-overlay-1-contact-1-number.png

ICE contacts overlay for the case when there is 1 contacts with 1 associated phone number.

Comment 13

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

Attachment: emergency-call-ice-contacts-overlay-1-contact-3-number.png

ICE contacts overlay for the case when there is 1 contacts with 3 associated phone numbers.

Comment 14

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

Attachment: emergency-call-ice-contacts-overlay-2-contacts-3-and-5-number-scrolling.png

ICE contacts overlay for the case when there are 2 contacts with 3 and 5 associated phone number respectively.

Comment 15

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

Attachment: Pull request 22894

Hi Anthony, let me share with you some comments to ease the review process:
  1. All the needed information to try the patch can be found at the beginning of the new ice_contacts_bar.js file (this is, https://github.com/mozilla-b2g/gaia/pull/22894/files#diff-c7f19e57e6602e14b9238c25487cd5daR1).
  2. Apart from the updates needed for the bug itself, you'll see that I moved the mock_mozContacts.js file from communications to shared since now it is being also used by emergency-call. You'll see some updated test files for this shifting. I also updated that concrete file (mock_mozContacts.js) since I needed the find() function to retrieve results based on the (ICE) contacts ids. Until now, the mocked find() function was just returning all the contacts with no filtering. Of course I kept that already existent behaviour and all the (already existent and new) tests are passing :)
Comments are more than welcome ;)

Comment 16

[Fang Shih [:fang]]

Comment on attachment 8473100 [details]
emergency-call-ice-contacts-available.png

Hi Germán, looks great!! I was wondering, do you think we can move the ">" icon to left 15px ( 1.5 rem)? So it can look like has the same amount​ of space to the edge on both sides. Thanks!

Comment 17

[Fang Shih [:fang]]

Comment on attachment 8473104 [details]
emergency-call-ice-contacts-overlay-1-contact-1-number.png

Currently we are using " / " instead of " , " in our match result. I would like to keep the same for ICE contacts as well. Thanks!

Comment 18

[Fang Shih [:fang]]

Attachment: Contact_Match_result.png

Hi Germán,
Attached the sample of the match result. Currently we use " / " for divider, and also not showing (1) for the different home number. I think we should use the same design on here to be consistent. What do you think?

Comment 19

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

(In reply to Fang Shih from comment #17)
> Comment on attachment 8473104 [details]
> emergency-call-ice-contacts-overlay-1-contact-1-number.png
> 
> Currently we are using " / " instead of " , " in our match result. I would
> like to keep the same for ICE contacts as well. Thanks!

Sure, I'll update it to use "/" instead. I reused a library which gives us that information for a contact and it is currently using "," as the separator as you can see in the Call Screen (https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8415255) and also in the Call Log. BTW, I just noticed that in the suggestion bar we are currently using "|" as you can see at https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8415182 But if you prefer a "/" a "/" it is :) Thanks!

Comment 20

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

BTW, Anthony, you'll notice when trying the patch that the calls are not currently made since navigator.mozTelephony.dialEmergency() (see https://github.com/gtorodelvalle/gaia/blob/emergency-call-bug-1038701-support-for-ice-contacts/apps/emergency-call/js/dialer.js#L22) is rejecting the call since the selected number is not a valid emergency number. Do you know if this will be solved at a Settings app level or does the Telephony API implementation need to be udpated? Thanks!

Comment 21

[Anthony Ricaud (:rik)]

We just need to change the code to use telephony.dial() if we tap on a ICE contact instead of typing a number. For security, we should probably do something like:
if (number is in ICE datastore) telephony.dial(number)
else telephony.dialEmergency(number)

Comment 22

[Anthony Ricaud (:rik)]

Comment on attachment 8473114 [details] [review]
Pull request 22894

I left some comments on the PR. The code is doing too much when loading the emergency-call app. It should only read the ICE datastore to know if it should display the ICE contacts bar.

Only when we display the overlay should we query for Contacts information.

Given that Germán is on PTO, Doug will take it from there this week.

Comment 23

[Fang Shih [:fang]]

(In reply to Germán Toro del Valle from comment #19)
> (In reply to Fang Shih from comment #17)
> > Comment on attachment 8473104 [details]
> > emergency-call-ice-contacts-overlay-1-contact-1-number.png
> > 
> > Currently we are using " / " instead of " , " in our match result. I would
> > like to keep the same for ICE contacts as well. Thanks!
> 
> Sure, I'll update it to use "/" instead. I reused a library which gives us
> that information for a contact and it is currently using "," as the
> separator as you can see in the Call Screen
> (https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8415255) and also
> in the Call Log. BTW, I just noticed that in the suggestion bar we are
> currently using "|" as you can see at
> https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8415182 But if you
> prefer a "/" a "/" it is :) Thanks!

Hi Germán, Thank you for pointing out this. I'll talk to dialer designer see if we can change all of them to the same symbol. But for now we'll use the "/" so that we use the same one as match contact action menu. And also I've asked Carrie, I think we are not showing the (1) next to Home here. Thanks!

Comment 24

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

Hi Doug, are you working on this? I will be around until next Tuesday when I start my summer holidays :) in case you want me to work on it and to include the received comments. Thanks!

Comment 25

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

(In reply to Anthony Ricaud (:rik) from comment #22)
> Comment on attachment 8473114 [details] [review]
> Pull request 22894
> 
> I left some comments on the PR. The code is doing too much when loading the
> emergency-call app. It should only read the ICE datastore to know if it
> should display the ICE contacts bar.
> 
> Only when we display the overlay should we query for Contacts information.
> 
> Given that Germán is on PTO, Doug will take it from there this week.

I implemented it this way for 2 main reasons (mainly for reason 2 in fact :p):
 1. To take advantage of the asynchronous nature of the data store and Contacts APIs. So the loading of the data gets started as soon as possible. The "ICE contacts area" is also shown as soon as possible, even before feeding the overlay with the concrete contacts.
 2. As suggested in the wireframe, the "ICE Contact bar" should only be shown if there is 1 ICE contact with at least 1 associated phone number. It would have no sense to show the bar and then show a blank overlay when tapped or including a contact with no associated phone number. Since the ICE contacts data store only store contacts' ids, we have to get the contacts information from the data base to know if at least one of them has an associated phone number. As far as my understanding and from the wireframe, it is possible to have ICE contacts with no associated phone numbers.

What do you think about this, Anthony? Thanks!

Comment 26

[Anthony Ricaud (:rik)]

(In reply to Germán Toro del Valle from comment #25)
> I implemented it this way for 2 main reasons (mainly for reason 2 in fact
> :p):
>  1. To take advantage of the asynchronous nature of the data store and
> Contacts APIs. So the loading of the data gets started as soon as possible.
> The "ICE contacts area" is also shown as soon as possible, even before
> feeding the overlay with the concrete contacts.
Loading data is asynchronous but still performing more I/O than necessary. So it slows down devices even if the code is not blocked.x
>  2. As suggested in the wireframe, the "ICE Contact bar" should only be
> shown if there is 1 ICE contact with at least 1 associated phone number. It
> would have no sense to show the bar and then show a blank overlay when
> tapped or including a contact with no associated phone number. Since the ICE
> contacts data store only store contacts' ids, we have to get the contacts
> information from the data base to know if at least one of them has an
> associated phone number. As far as my understanding and from the wireframe,
> it is possible to have ICE contacts with no associated phone numbers.
It is not possible because when a contact has no phone number, it will be removed from the ICE list. We discussed this with the Contacts team. So if it is in the Datastore, it has at least a phone number. Needinfo-ing Francisco to confirm this behaviour.

Comment 27

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

Hi,

Anthony is right, we decided to add this checking in contacts. But as long as mozContacts is a privileged API, 3rd party apps still can modify contacts api, so a checking will be needed anyway.

Comment 28

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

Sooo... Francisco, you seem Galician (yesss, but no... :p) Kidding! ;)

So, Anthony, since the (phone number existence) checking is needed, do we want to take advantage of that checking and start loading the ICE contacts data when the Emergency Call app is loaded or do we want just to check that there's a phone number associated to some ICE contact to show the ICE contacts bar and once it is tapped do the real data loading (of the second contact if any)? :) I don't know if it would be really worth it but you decide :p

Comment 29

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

No need to say that we could combine both approaches loading the ICE contacts details on the Emergency Call app loading (to be able to decide if the bar should be shown) but including them in the overlay only when the bar is tapped :)

Comment 30

[Anthony Ricaud (:rik)]

Comment on attachment 8473114 [details] [review]
Pull request 22894

Re-reviewed this after Francisco clarified that we need to check for contacts existence.

There is no need to name functions anymore. The only reason was for debugging tools but they are better now.

I suggest looking at the suggestion-item.html since it is basically the same widget (just without highlighting parts of the phone number).

Also, take into account comment 21.

Comment 31

[Johan Lorenzo [:jlorenzo]]

This feature has already been covered in the meta bug: bug 1026682 comment 2.

Comment 32

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

Comment on attachment 8473114 [details] [review]
Pull request 22894

Hi Anthony,

I have included all your suggestions as well as additional ones I found :-) The more noticeable ones are:
  1. I moved telephony_helper.js from communications to shared since the change suggested in comment 21 was not as simple as that :) We have to cover all the possible scenarios such as when the device is on airplane mode, when there is no network available, etc. All this is currently supported.
  2. The previous change increase the number of files included in the patch since many of them were affected by that shifting.
  3. As suggested, I used HTML imports to include the code. In the new test cases too of course.
  4. I removed the callback from updateICEContacts().
  4. Now we use " / " as the separator for the type and phone numbers as requested by Fang.

I will include a comment with videos covering all the possible scenarios in a sec.

Comment 33

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

Hi guys,

You may want to have a look at the videos I recorded, just to verify I covered any possible scenario:
  1. No ICE contacts or associated phone numbers: https://www.youtube.com/watch?v=0aymhcoCt-c
  2. 1 ICE contact with 3 phone numbers: https://www.youtube.com/watch?v=J7r25xwUiMw
  3. Scrolling of 2 ICE contacts with many phone numbers: https://www.youtube.com/watch?v=D8G5q7IrK3A
  4. ICE contacts with airplane on: https://www.youtube.com/watch?v=hy7LGuhVUv0
  5. ICE contacts with SIM PIN not inserted (no network): https://www.youtube.com/watch?v=ogQ2Pc6IGWY
  6. 1 ICE contact with 1 valid phone number: https://www.youtube.com/watch?v=PE7jvY3j4EA

Any comments are more than welcome! ;)

Enjoy!

Comment 34

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

Attachment: emergency-call-ice-contacts-available.png

Comment 35

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

Attachment: emergency-call-ice-contacts-overlay-1-contact-1-number.png

Comment 36

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

Attachment: emergency-call-ice-contacts-overlay-1-contact-3-number.png

Comment 37

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

Attachment: emergency-call-ice-contacts-overlay-2-contacts-3-and-5-number-scrolling.png

Comment 38

[Anthony Ricaud (:rik)]

Germán: All your videos are private :(

Comment 39

[Fang Shih [:fang]]

Comment on attachment 8477444 [details]
emergency-call-ice-contacts-available.png

Looks good! Thanks!

Comment 40

[Fang Shih [:fang]]

Comment on attachment 8477449 [details]
emergency-call-ice-contacts-overlay-1-contact-1-number.png

Hi Germán, As I mentioned in comment 23, Can we remove the "(1)" here. Thanks!

Comment 41

[Fang Shih [:fang]]

Comment on attachment 8477453 [details]
emergency-call-ice-contacts-overlay-1-contact-3-number.png

Everything looks good, just need to remove the " (1) (2) (3) ". Thanks!

Comment 42

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

(In reply to Anthony Ricaud (:rik) from comment #38)
> Germán: All your videos are private :(

Ups! Sorry about that ;) I also doubt between "private" and "hidden" :-S All should be "hidden" now and accesible via URL :)

Comment 43

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

(In reply to Fang Shih from comment #40)
> Comment on attachment 8477449 [details]
> emergency-call-ice-contacts-overlay-1-contact-1-number.png
> 
> Hi Germán, As I mentioned in comment 23, Can we remove the "(1)" here.
> Thanks!

Hi Fang,

No, no, the "(1)" is part of the phone number I programmatically create (to distinguish amongst them and check they are properly shown) :-) It is not included referencing the phone number position or whatever :) I'll ask for ui-review again ;)

Thanks!

Comment 44

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

(In reply to Fang Shih from comment #41)
> Comment on attachment 8477453 [details]
> emergency-call-ice-contacts-overlay-1-contact-3-number.png
> 
> Everything looks good, just need to remove the " (1) (2) (3) ". Thanks!

As mentioned in comment 43, the "(N)" is part of the phone number I programmatically assign to the contact. Not included to reference the phone number position in the contact or whatever :)

Comment 45

[Fang Shih [:fang]]

(In reply to Germán Toro del Valle from comment #43)
> (In reply to Fang Shih from comment #40)
> > Comment on attachment 8477449 [details]
> > emergency-call-ice-contacts-overlay-1-contact-1-number.png
> > 
> > Hi Germán, As I mentioned in comment 23, Can we remove the "(1)" here.
> > Thanks!
> 
> Hi Fang,
> 
> No, no, the "(1)" is part of the phone number I programmatically create (to
> distinguish amongst them and check they are properly shown) :-) It is not
> included referencing the phone number position or whatever :) I'll ask for
> ui-review again ;)
> 
> Thanks!

Oh! I see! Sorry about that! :P

Comment 46

[Fang Shih [:fang]]

Comment on attachment 8477449 [details]
emergency-call-ice-contacts-overlay-1-contact-1-number.png

Thanks!

Comment 47

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

Comment on attachment 8477456 [details]
emergency-call-ice-contacts-overlay-2-contacts-3-and-5-number-scrolling.png

Ups, forgot to ask for a new ui-review also for this one since I guess the problem was also the "(N)" as part of the programmatically created phone numbers :) Sorry for the confusion! ;)

Comment 48

[Anthony Ricaud (:rik)]

Comment on attachment 8473114 [details] [review]
Pull request 22894

Let's not move telephony_helper.js. I'm afraid of security issues that could arise from this. Let's just have a simpler version that has only one dialog in case it doesn't work. Something like "We couldn't call this number".

This also broke the error dialog when you compose a non emergency number.

We're missing tests to verify that taping a number calls it. And that taping cancel dismisses the overlay.

Comment 49

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

Hi Germán,

The interaction part looks fine. Just some small issues that Fang has already mentioned above. Great job!
Thanks!

Comment 50

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

Attachment: Pull request 23322

I created a new pull request and closed the previous one since I preferred to apply the changes on master than removing updates from the latest version of the patch. I thought it would be safer not to leave anything undeleted :)

The main changes in this new pull request are:
  1. Not moving telephony_helper.js to shared so we basically invoke navigator.mozTelephony's dial() and dialEmergency() depending on if the number to be called is from an ICE contact or a emergency phone number, covering of course the possible erroneous scenarios (airplane mode, etc.).
  2. Using promises which simplifies the included tests not needing using MutationObservers.

Regarding Anthony's comments, I didn't include the following suggestions in the current version of the patch (I'll discuss them with Anthony via IRC):
  1. Refactoring the code for the contacts suggestion overlay used in Dialer and ICE contacts list overlay used in Emergency Call since they are very similar in shape, although there are some distinctions. Maybe we could deal with this refactoring in a follow up bug.
  2. Refactoring /shared/test/mocks/mock_mozContacts.js. I had to include support for finding contacts via id which was not supported. This has included some complexity in this file which may recommend some refactoring and rethinking since it is widely used in many tests. Once again, we could deal with this refactoring in a follow up bug. No need to say that the current version make all the test pass :)
  
The rest of the comments should be included so I'll ask for a new review by Anthony ;) Thanks!

I am not making the previous pull request as obsolete to keep its reference in the bug in case it is needed for further reference.

Comment 51

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

BTW, Fang, Anthony asked me to keep the ", " as the separator between the phone type and the phone number for the time being since there are other changes in other bugs from which we may inherit the final text to be shown using the correct separator ;)

Comment 52

[Anthony Ricaud (:rik)]

Comment on attachment 8479131 [details] [review]
Pull request 23322

We're still missing the tests I suggested in comment 48.

I have other comments in the pull request.

Let's keep shared/test/mocks/mock_mozContacts.js in this form. This is test code so we can always come back to it later.
But I think moving suggestion_item into a contact_in_overlay shared element would be valuable.

Comment 53

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

Hi Anthony, I have just updated the pull request (attachment 8479131 [details] [review]) including the first of the changes we discussed via IRC: the refactoring of the contact list overlays in Dialer and Emergency Call. As you can see, both apps now use the generic contact list overlay code available in shared. I left those changes in its own commit for easier reference.

BTW, still 3 pending commits I'll make tomorrow:
 1. To unify the error message shown in case a call to an ICE contact cannot be made.
 2. To check that the phone number to be called is from and existent ICE contact.
 3. To include the tests cases you suggest in comment 48.

Regarding the tests you mention in comment 48. Do you refer to unit or integration test? Thanks!

Comment 54

[Fang Shih [:fang]]

(In reply to Germán Toro del Valle from comment #51)
> BTW, Fang, Anthony asked me to keep the ", " as the separator between the
> phone type and the phone number for the time being since there are other
> changes in other bugs from which we may inherit the final text to be shown
> using the correct separator ;)

Hey Germán, 
I see, As long as we all using the same separator. Do you know what bug number is it? Is it from dialer? Thanks for letting me know : )

Comment 55

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

(In reply to Fang Shih from comment #54)
> (In reply to Germán Toro del Valle from comment #51)
> > BTW, Fang, Anthony asked me to keep the ", " as the separator between the
> > phone type and the phone number for the time being since there are other
> > changes in other bugs from which we may inherit the final text to be shown
> > using the correct separator ;)
> 
> Hey Germán, 
> I see, As long as we all using the same separator. Do you know what bug
> number is it? Is it from dialer? Thanks for letting me know : )

Hi Fang,

Forget what I said :) The update to the patch mentioned in comment 53 properly deals with the separator and the "/" is shown as you requested. So we are good here ;)

Thanks!

Comment 56

[Fang Shih [:fang]]

Got it! Thanks! : )

Comment 57

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

Hi Anthony, I have just updated the pull request (attachment 8479131 [details] [review]) again. The release notes :p are (each entry included in its own commit):
 1. Refactoring of the contact list overlays used in Dialer (suggested contacts) and Emergency Call (ICE contacts).
 2. Checking if a number belongs to an ICE contact before calling (no boolean isICEContact passing).
 3. Deletion of the hideICEContactsBar() function in ice_contacts.js.

Still pending:
 1. Clarification if the requested additional tests are unit or integration ones (comment 48).
 2. Rethinking of the error messages we would like to send to the user. I included some comments on Github.

Thanks!

Comment 58

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

Regarding comment 57 ;)

Comment 59

[Anthony Ricaud (:rik)]

Looking at this.

Comment 60

[Anthony Ricaud (:rik)]

Because bug 1022767 landed, we need to rebase this. Germán: Could you do this without addressing my review comments? That will help me test this patch.

Comment 61

[Anthony Ricaud (:rik)]

(In reply to Germán Toro del Valle from comment #57)
> Still pending:
>  1. Clarification if the requested additional tests are unit or integration
> ones (comment 48).
Unit tests.
>  2. Rethinking of the error messages we would like to send to the user. I
> included some comments on Github.
I think you have a valid point. I don't like sharing all of telephony_helper nor duplicating so much code so let's figure out how to deal with this in a follow-up bug. For now, let's keep the duplicated code.

Comment 62

[Anthony Ricaud (:rik)]

Comment on attachment 8479131 [details] [review]
Pull request 23322

We need some tests for isFromIceContact. We also need some tests to check the content of the DOM for a contact item (does it have the correct content). 
Open a followup for error messages that are now duplicated. I'm not asking for new tests for the dialer.js changes because it's mostly a copy of telephony_helper.js. But we need to make sure in that followup that we have good test coverage after choosing a solution.
Open a followup for calling when default SIM is set on "Always asked".

I still have a few comments on Github but this is close.
I'll need to test it rebased on today's code though.

Comment 63

[Doug Sherk (:drs) (inactive)]

I'm taking this for the final touches. Germán, please let me know if you've made any progress or you have time to finish it up instead.

Comment 64

[Doug Sherk (:drs) (inactive)]

Attachment: Pull request 23491

PR: https://github.com/mozilla-b2g/gaia/pull/23491

Anthony gave me an over-IRC r+ on this.

Comment 65

[Doug Sherk (:drs) (inactive)]

Attachment: Add testing code for ICE contacts within the emergency-call app.

Here's the code that Germán wrote to allow us to test ICE contacts from within the emergency-call app without the ICE contacts patches landed on the contacts app.

Comment 66

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/d67090ed4146313843c5fb82caf41ffd4c76aedc

Comment 67

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

Thank you very much, guys! The time difference and the connection availability made this more complicated than expected but I was planning on implementing the final comments by Anthony during the weekend ;) Thank you very much once again!

Comment 68

[Eric Chang [:ericcc] [:echang]]

Verified User story is fixed. ICE can be accessed lockscreen.

Gaia      2be78d83a760fa3b9638fe51c266b442d14597f1
Gecko     https://hg.mozilla.org/mozilla-central/rev/1db35d2c9a2f
BuildID   20140831160203
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230

Comment 69

[Loli (:lolimartinezcr)]

Tested and with new bugs
Flame
2.1
Gecko-8a9db5d
Gaia-a8e4d26

https://bugzilla.mozilla.org/show_bug.cgi?id=1064239
https://bugzilla.mozilla.org/show_bug.cgi?id=1064234

Comment 70

[Doug Sherk (:drs) (inactive)]

Loli, please set regressions or bugs with a feature as blockers.

Comment 71

[Loli (:lolimartinezcr)]

(In reply to Doug Sherk (:drs) from comment #70)
> Loli, please set regressions or bugs with a feature as blockers.

Updated: 1064234, 1064239. Not regression because it is new feature.

Comment 72

[Loli (:lolimartinezcr)]

(In reply to Loli (:lolimartinezcr) from comment #71)
> (In reply to Doug Sherk (:drs) from comment #70)
> > Loli, please set regressions or bugs with a feature as blockers.
> 
> Updated: 1064234, 1064239. Not regression because it is new feature.

Other bug related wiht this US

https://bugzilla.mozilla.org/show_bug.cgi?id=1071500

Comment 73

[Anthony Ricaud (:rik)]

Please Loli…

(In reply to Doug Sherk (:drs) from comment #70)
> Loli, please set regressions or bugs with a feature as blockers.