Closed
Bug 1038701
Opened 10 years ago
Closed 9 years ago
[US] Support for ICE Contacts in Lockscreen/Emergency call
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Firefox OS Graveyard
Gaia::Dialer
Tracking
(feature-b2g:2.1)
People
(Reporter: rik, Assigned: drs)
References
Details
(Whiteboard: [planned-sprint c=3][in-sprint=v2.1-S2])
User Story
Attachments
(10 files, 6 obsolete files)
827.14 KB,
application/zip
|
Details | |
22.17 KB,
image/png
|
fang
:
ui-review+
|
Details |
46 bytes,
text/x-github-pull-request
|
rik
:
review-
|
Details | Review |
183.02 KB,
image/png
|
Details | |
27.48 KB,
image/png
|
fang
:
ui-review+
|
Details |
14.29 KB,
image/png
|
fang
:
ui-review+
|
Details |
29.47 KB,
image/png
|
fang
:
ui-review+
|
Details |
61.82 KB,
image/png
|
fang
:
ui-review+
|
Details |
47 bytes,
text/plain
|
drs
:
review+
|
Details |
5.40 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•10 years ago
|
Component: Gaia::Contacts → Gaia::Dialer
Updated•10 years ago
|
feature-b2g: --- → 2.1
Updated•10 years ago
|
QA Contact: echang
Reporter | ||
Comment 1•10 years ago
|
||
We now have the Datastore schema done in bug 1042586 so we can start on this.
Depends on: 1042586
Target Milestone: --- → 2.1 S2 (15aug)
Reporter | ||
Updated•10 years ago
|
User Story: (updated)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [planned-sprint c=]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gtorodelvalle
Whiteboard: [planned-sprint c=] → [planned-sprint c=3]
Updated•10 years ago
|
QA Whiteboard: [COM=Gaia::Dialer]
Comment 2•9 years ago
|
||
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!
Flags: needinfo?(chuang)
Flags: needinfo?(cawang)
Comment 3•9 years ago
|
||
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!
Flags: needinfo?(fshih)
Flags: needinfo?(chuang)
Flags: needinfo?(cawang)
Comment 4•9 years ago
|
||
I change QA Contact because it is asigned by German
QA Contact: echang → lolimartinezcr
Comment 5•9 years ago
|
||
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!
Flags: needinfo?(fshih)
Comment 6•9 years ago
|
||
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 :)
Flags: needinfo?(fshih)
Comment 7•9 years ago
|
||
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? :)
Updated•9 years ago
|
QA Contact: lolimartinezcr → jlorenzo
Updated•9 years ago
|
Flags: in-moztrap?(jlorenzo)
Comment 8•9 years ago
|
||
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! : )
Attachment #8472275 -
Attachment is obsolete: true
Flags: needinfo?(fshih)
Updated•9 years ago
|
Summary: Support for ICE Contacts in Lockscreen/Emergency call → [US] Support for ICE Contacts in Lockscreen/Emergency call
Comment 9•9 years ago
|
||
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.
Flags: needinfo?(fshih)
Comment 10•9 years ago
|
||
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 ;)
Attachment #8473099 -
Flags: ui-review?(fshih)
Comment 11•9 years ago
|
||
ICE contacts bar when there is at least one ICE contact with an associated phone number.
Attachment #8473100 -
Flags: ui-review?(fshih)
Comment 12•9 years ago
|
||
ICE contacts overlay for the case when there is 1 contacts with 1 associated phone number.
Attachment #8473104 -
Flags: ui-review?(fshih)
Comment 13•9 years ago
|
||
ICE contacts overlay for the case when there is 1 contacts with 3 associated phone numbers.
Attachment #8473105 -
Flags: ui-review?(fshih)
Comment 14•9 years ago
|
||
ICE contacts overlay for the case when there are 2 contacts with 3 and 5 associated phone number respectively.
Attachment #8473106 -
Flags: ui-review?(fshih)
Comment 15•9 years ago
|
||
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 ;)
Attachment #8473114 -
Flags: review?(anthony)
Updated•9 years ago
|
Attachment #8473099 -
Flags: ui-review?(fshih) → ui-review+
Flags: needinfo?(fshih)
Comment 16•9 years ago
|
||
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!
Attachment #8473100 -
Flags: ui-review?(fshih) → ui-review-
Comment 17•9 years ago
|
||
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!
Attachment #8473104 -
Flags: ui-review?(fshih) → ui-review-
Comment 18•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8473106 -
Flags: ui-review?(fshih) → ui-review-
Updated•9 years ago
|
Attachment #8473105 -
Flags: ui-review?(fshih) → ui-review-
Comment 19•9 years ago
|
||
(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!
Flags: needinfo?(fshih)
Comment 20•9 years ago
|
||
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!
Flags: needinfo?(anthony)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Whiteboard: [planned-sprint c=3] → [planned-sprint c=3][in-sprint=v2.1-S2]
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Assignee | ||
Updated•9 years ago
|
Assignee: gtorodelvalle → nobody
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → drs+bugzilla
Reporter | ||
Comment 21•9 years ago
|
||
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)
Flags: needinfo?(anthony)
Reporter | ||
Comment 22•9 years ago
|
||
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.
Attachment #8473114 -
Flags: review?(anthony) → review-
Comment 23•9 years ago
|
||
(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!
Flags: needinfo?(fshih)
Comment 24•9 years ago
|
||
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!
Flags: needinfo?(drs+bugzilla)
Comment 25•9 years ago
|
||
(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!
Flags: needinfo?(anthony)
Reporter | ||
Comment 26•9 years ago
|
||
(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.
Flags: needinfo?(anthony) → needinfo?(francisco)
Comment 27•9 years ago
|
||
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.
Flags: needinfo?(francisco)
Comment 28•9 years ago
|
||
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
Flags: needinfo?(anthony)
Comment 29•9 years ago
|
||
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 :)
Assignee | ||
Updated•9 years ago
|
Assignee: drs+bugzilla → gtorodelvalle
Flags: needinfo?(drs+bugzilla)
Reporter | ||
Comment 30•9 years ago
|
||
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.
Flags: needinfo?(anthony)
Comment 31•9 years ago
|
||
This feature has already been covered in the meta bug: bug 1026682 comment 2.
Flags: in-moztrap?(jlorenzo) → in-moztrap+
Comment 32•9 years ago
|
||
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.
Attachment #8473114 -
Flags: review- → review?(anthony)
Comment 33•9 years ago
|
||
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!
Flags: needinfo?(fshih)
Flags: needinfo?(cawang)
Flags: needinfo?(anthony)
Comment 34•9 years ago
|
||
Attachment #8473100 -
Attachment is obsolete: true
Attachment #8477444 -
Flags: ui-review?(fshih)
Comment 35•9 years ago
|
||
Attachment #8473104 -
Attachment is obsolete: true
Attachment #8477449 -
Flags: ui-review?(fshih)
Comment 36•9 years ago
|
||
Attachment #8473105 -
Attachment is obsolete: true
Attachment #8477453 -
Flags: ui-review?(fshih)
Comment 37•9 years ago
|
||
Attachment #8473106 -
Attachment is obsolete: true
Attachment #8477456 -
Flags: ui-review?(fshih)
Comment 39•9 years ago
|
||
Comment on attachment 8477444 [details]
emergency-call-ice-contacts-available.png
Looks good! Thanks!
Attachment #8477444 -
Flags: ui-review?(fshih) → ui-review+
Flags: needinfo?(fshih)
Comment 40•9 years ago
|
||
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!
Attachment #8477449 -
Flags: ui-review?(fshih) → ui-review-
Comment 41•9 years ago
|
||
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!
Attachment #8477453 -
Flags: ui-review?(fshih) → ui-review-
Updated•9 years ago
|
Attachment #8477456 -
Flags: ui-review?(fshih) → ui-review-
Updated•9 years ago
|
QA Contact: jlorenzo → lolimartinezcr
Comment 42•9 years ago
|
||
(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 :)
Flags: needinfo?(anthony)
Comment 43•9 years ago
|
||
(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•9 years ago
|
||
(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 :)
Updated•9 years ago
|
Attachment #8477449 -
Flags: ui-review- → ui-review?(fshih)
Updated•9 years ago
|
Attachment #8477453 -
Flags: ui-review- → ui-review?(fshih)
Comment 45•9 years ago
|
||
(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•9 years ago
|
||
Comment on attachment 8477449 [details]
emergency-call-ice-contacts-overlay-1-contact-1-number.png
Thanks!
Attachment #8477449 -
Flags: ui-review?(fshih) → ui-review+
Updated•9 years ago
|
Attachment #8477453 -
Flags: ui-review?(fshih) → ui-review+
Comment 47•9 years ago
|
||
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! ;)
Attachment #8477456 -
Flags: ui-review- → ui-review?(fshih)
Updated•9 years ago
|
Attachment #8477456 -
Flags: ui-review?(fshih) → ui-review+
Reporter | ||
Comment 48•9 years ago
|
||
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.
Attachment #8473114 -
Flags: review?(anthony) → review-
Flags: needinfo?(anthony)
Comment 49•9 years ago
|
||
Hi Germán, The interaction part looks fine. Just some small issues that Fang has already mentioned above. Great job! Thanks!
Flags: needinfo?(cawang)
Comment 50•9 years ago
|
||
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.
Attachment #8479131 -
Flags: review?(anthony)
Comment 51•9 years ago
|
||
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 ;)
Flags: needinfo?(fshih)
Reporter | ||
Comment 52•9 years ago
|
||
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.
Attachment #8479131 -
Flags: review?(anthony) → review-
Comment 53•9 years ago
|
||
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!
Flags: needinfo?(anthony)
Comment 54•9 years ago
|
||
(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 : )
Flags: needinfo?(fshih)
Comment 55•9 years ago
|
||
(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•9 years ago
|
||
Got it! Thanks! : )
Comment 57•9 years ago
|
||
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•9 years ago
|
||
Regarding comment 57 ;)
Updated•9 years ago
|
Attachment #8479131 -
Flags: review- → review?(anthony)
Reporter | ||
Comment 60•9 years ago
|
||
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.
Flags: needinfo?(gtorodelvalle)
Reporter | ||
Comment 61•9 years ago
|
||
(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.
Reporter | ||
Comment 62•9 years ago
|
||
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.
Attachment #8479131 -
Flags: review?(anthony)
Assignee | ||
Comment 63•9 years ago
|
||
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.
Assignee: gtorodelvalle → drs+bugzilla
Assignee | ||
Comment 64•9 years ago
|
||
PR: https://github.com/mozilla-b2g/gaia/pull/23491 Anthony gave me an over-IRC r+ on this.
Attachment #8479131 -
Attachment is obsolete: true
Attachment #8481662 -
Flags: review+
Flags: needinfo?(gtorodelvalle)
Assignee | ||
Comment 65•9 years ago
|
||
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.
Assignee | ||
Comment 66•9 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/d67090ed4146313843c5fb82caf41ffd4c76aedc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 67•9 years ago
|
||
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•9 years ago
|
||
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
Status: RESOLVED → VERIFIED
Comment 69•9 years ago
|
||
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
Assignee | ||
Comment 70•9 years ago
|
||
Loli, please set regressions or bugs with a feature as blockers.
Comment 71•9 years ago
|
||
(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.
Flags: needinfo?(lolimartinezcr)
Comment 72•9 years ago
|
||
(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
Reporter | ||
Comment 73•9 years ago
|
||
Please Loli… (In reply to Doug Sherk (:drs) from comment #70) > Loli, please set regressions or bugs with a feature as blockers.
Blocks: 1071500
You need to log in
before you can comment on or make changes to this bug.
Description
•