Closed Bug 983766 Opened 9 years ago Closed 8 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.1 affected, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S5 (26sep)
Tracking Status
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: tnguyen, Assigned: crdlc)

References

()

Details

(Whiteboard: [planned-sprint])

Attachments

(1 file)

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
This issue reproduces on Buri v1.2 mozRIL BuildID: 20140311004003.

Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 9008f899e5af
Version: 26.0
v1.2-device.cfg
Summary: [B2G][Dialer] Suggested phone number don't appear for Facebook contacts that have edited phone numberss → [B2G][Dialer] Suggested phone numbers don't appear for Facebook contacts that have edited phone numbers
Adrian,

Can you check if this is still a valid bug?

thanks
Flags: needinfo?(hola)
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.
Flags: needinfo?(hola)
Assignee: nobody → hola
Target Milestone: --- → 2.1 S5 (26sep)
Whiteboard: [planned-sprint]
Assignee: hola → jmcf
Flipping status-b2g-v2.1 as per comment #3.
Assignee: jmcf → crdlc
Attached file Github pull request
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.
Attachment #8493817 - Flags: review?(jmcf)
Comment on attachment 8493817 [details]
Github pull request

Thanks Cristian. Now asking for a proper reviewer on dialer app
Attachment #8493817 - Flags: review?(jmcf) → review+
Attachment #8493817 - Flags: review?(etienne)
Comment on attachment 8493817 [details]
Github pull request

redirecting...
Attachment #8493817 - Flags: review?(etienne) → review?(anthony)
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
Attachment #8493817 - Flags: review?(anthony)
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
Attachment #8493817 - Flags: review?(anthony)
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+.
Attachment #8493817 - Flags: review?(anthony)
(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
Flags: needinfo?(anthony)
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.
Flags: needinfo?(anthony)
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.
Flags: needinfo?(anthony)
(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
(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)
Flags: needinfo?(anthony)
Attachment #8493817 - Flags: review?(anthony)
Comment on attachment 8493817 [details]
Github pull request

Thanks!
Attachment #8493817 - Flags: review?(anthony) → review+
you're welcome Rik
Status: NEW → ASSIGNED
merged in master:

https://github.com/mozilla-b2g/gaia/commit/488cfededa34b2b37bad9deed0b4c574d1d72deb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.