983766 - [B2G][Dialer] Suggested phone numbers don't appear for Facebook contacts that have edited phone numbers

Status: RESOLVED FIXED

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

Description

[Tony Nguyen]

Description:
Suggested phone numbers aren't displayed for contacts that are imported from Facebook that don't initially have a number. When adding a number to the contact after importing from Facebook, the user will notice that no suggested phone number is being displayed when tapping on the keypad to dial. 

Repro Steps:
1) Updated Buri to BuildID: 20140314004002
2) Navigate to Contacts app
3) Import a contact from Facebook that doesn't have a phone number
4) Edit imported contact and add a phone number 
5) Navigate to Dialer app
6) Dial the number added to contact in step 4

Actual Result:
Suggested phone number for contact is not displayed 

Expected Result
Suggested phone number for contact is displayed 

Environmental Variables:
Device: Buri v1.3 mozRIL
BuildID: 20140314004002
Gaia: 932789749406a79c5630c27c724f1856bd3c3f10
Gecko: 0479f5480378
Version: 28.0
v1.2-device.cfg

Video: http://youtu.be/G4GjpAj0zRo

Comment 1

[Tony Nguyen]

This issue reproduces on Buri v1.2 mozRIL BuildID: 20140311004003.

Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 9008f899e5af
Version: 26.0
v1.2-device.cfg

Comment 2

[Jose Manuel Cantera]

Adrian,

Can you check if this is still a valid bug?

thanks

Comment 3

[Adrián de la Rosa]

Yes, I have been able to reproduce it on yesterday's build. Phone numbers for newly created contacts and new phone numbers for already existing contacts are appearing right away as suggestions in dialer, but this is not happening for facebook contacts.

Comment 4

[Johan Lorenzo [:jlorenzo]]

Flipping status-b2g-v2.1 as per comment #3.

Comment 5

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

Comment 6

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8493817 [details]
Github pull request

Thanks for the review Jose. Please feel free to add some peer here to review this patch too. Maybe you know who is the correct person.

Comment 7

[Jose Manuel Cantera]

Comment on attachment 8493817 [details]
Github pull request

Thanks Cristian. Now asking for a proper reviewer on dialer app

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8493817 [details]
Github pull request

redirecting...

Comment 9

[Anthony Ricaud (:rik)]

Comment on attachment 8493817 [details]
Github pull request

Thanks for splitting this method, it is more readable. I have some comments that I'd like to see addressed before r+ing this

Comment 10

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8493817 [details]
Github pull request

Hi Rik,

   I have addressed all suggestions (Promises, new name for the method,..) But after talking with Jose Manuel Cantera we want to keep the async mode because after doing this by default there are tons of unit tests in contacts/facebook failing. I think that it is out of the scope of this bug to change several tests in contacts and facebook. If you are not comfortable with the async mode I could remove the async test and keep the library as this was. Let me know what you think please.

Thanks a lot Rik

Comment 11

[Anthony Ricaud (:rik)]

Comment on attachment 8493817 [details]
Github pull request

I think we're not in any rush to land this. So I'd suggest fixing the tests to run with an asynchronous behaviour first (in another bug) and then fixing this bug. If you think we should land this first, let me know.

Outside of this, this would be a r+.

Comment 12

[Jose Manuel Cantera]

(In reply to Anthony Ricaud (:rik) from comment #11)
> Comment on attachment 8493817 [details]
> Github pull request
> 
> I think we're not in any rush to land this. So I'd suggest fixing the tests
> to run with an asynchronous behaviour first (in another bug) and then fixing
> this bug. If you think we should land this first, let me know.
> 
> Outside of this, this would be a r+.

Rik,

I think we need to land this in first place. The current behavior is totally wrong and buggy with FB Contacts and We need to have it fixed. Then, we can aim at refactoring tests but first things first. 

So, We are going to land this if you don't disagree

Please let us know

Comment 13

[Anthony Ricaud (:rik)]

It has been buggy for a long time and current master is not going to be branched anytime soon. Unless we want to uplift this to 2.1, I don't see why it's important to land this first.

Comment 14

[Cristian Rodriguez (:crdlc)]

I don't understand which the problem is here :) Sorry for my misunderstanding Rik. I fixed the bug which IMHO is a serious error and I modified a test in order to check what it is working right now. Obviously I had to make a asynchronous test just to confirm that the problem was fixed properly. AFAIK we don't want to have asynchronous unit test in Gaia so I wouldn't like to modify tons of unit tests in several places to introduce something that we wanted to avoid for a long time. What do you think? I don't think that a new variable to choose if this should be asynchronous/synchronous is wrong. In this special situation we want to check if this is working in an scenario when getData is asynchronous. The rest of cases where getData is used work fine thought. I don't see why we have to change them. This is only my point of view and I would like to explain to you my thoughts although obviously I don't have any weight in this module and you are the person who has the last decision. Thanks a lot for your time and support Rik.

(In reply to Anthony Ricaud (:rik) from comment #13)
> It has been buggy for a long time and current master is not going to be
> branched anytime soon. Unless we want to uplift this to 2.1, I don't see why
> it's important to land this first.

Comment 15

[Jose Manuel Cantera]

(In reply to Anthony Ricaud (:rik) from comment #13)
> It has been buggy for a long time and current master is not going to be
> branched anytime soon. Unless we want to uplift this to 2.1, I don't see why
> it's important to land this first.

Because we have a fix and you want to change tons of unit tests for the sake of nothing

Comment 16

[Anthony Ricaud (:rik)]

(In reply to Jose Manuel Cantera from comment #15)
> Because we have a fix and you want to change tons of unit tests for the sake
> of nothing
It's not for the sake of nothing. If a test passes with a synchronous mock but the real API is asynchronous, this can prevent us from detecting race conditions.

I also don't want to introduce a "mode" for this mock. This introduces complexity. The best way to deal with this nowadays is to use Sinon spies and use .yields to call the callback. That gives you complete control over the flow of the program.

Anyway, the tests passes without the mock changes. So you can land if you remove the mock changes (and setting the boolean in contacts_test.js)

Comment 17

[Anthony Ricaud (:rik)]

Comment on attachment 8493817 [details]
Github pull request

Thanks!

Comment 18

[Cristian Rodriguez (:crdlc)]

you're welcome Rik

Comment 19

[Cristian Rodriguez (:crdlc)]

merged in master:

https://github.com/mozilla-b2g/gaia/commit/488cfededa34b2b37bad9deed0b4c574d1d72deb