900443 - [SMS] Remove repeated request to Contacts API when retrieving a contact from 'Contacts' activity.

Status: VERIFIED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

Due to the complexity added by the activity, we have to request several times to the API for retrieving the whole contact info. However, if the Contact Activity could grab this info for us, we are going to save several request to Gecko, and we are going to reduce all bugs generated by this issue.

Comment 1

[Wayne Chang [:wchang]]

Triage - Seems to be non-trivial improvement, as we're tight on leo schedule and there is not direct user impact with this, renoming to koi.

Comment 2

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

ni? Borja for the complexity in v1.2 timeframe

Comment 3

[Borja Salguero [:borjasalguero]]

Hi Joe,
This would let us to avoid several issues with tel matching, so Im gonna try to fix this during this week. Thanks!

Comment 4

[Borja Salguero [:borjasalguero]]

Attachment: Pull request

Julien, I was thinking that probably we could open the possibility of receiving a contact as param in the activity instead of the number only. In the case of sending a SMS from contacts, Contacts App made the effort for retrieving all the info, and we are not using this info from SMS App (because we receive only the phone number, and then we have to request Contacts API again :S). Wdyt about adding this to the activity, and try to use {Contact} instead of a phone number? {Contact} object would send filtered by *phone number*, so we can save effort there as well.

Comment 5

[Rick Waldron [:rwaldron]]

(In reply to Borja Salguero [:borjasalguero] from comment #4)
> Created attachment 798530 [details]
> Pull request
> 
> Julien, I was thinking that probably we could open the possibility of
> receiving a contact as param in the activity instead of the number only. In
> the case of sending a SMS from contacts, Contacts App made the effort for
> retrieving all the info, and we are not using this info from SMS App
> (because we receive only the phone number, and then we have to request
> Contacts API again :S). Wdyt about adding this to the activity, and try to
> use {Contact} instead of a phone number? {Contact} object would send
> filtered by *phone number*, so we can save effort there as well.

This would be ideal.

Comment 6

[Borja Salguero [:borjasalguero]]

I'm gonna create a followup for fixing this issue. This could save headaches related with Contacts lookup in MMS App!

Comment 7

[Borja Salguero [:borjasalguero]]

(In reply to Rick Waldron from comment #5)

> This would be ideal.

I've created the followup here https://bugzilla.mozilla.org/show_bug.cgi?id=912399. Once I have a patch I will ask you to be a reviewer there. I hope to avoid some issues with Contacts matching that we were suffering in the past!

Comment 8

[Julien Wajsberg [:julienw]]

Should be the same for when we send a SMS from the Contacts App. I've seen new bugs because of this.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 798530 [details]
Pull request

some comments on github.

But I wonder how is security here: could someone send as an activity or pick from a malicious contact app with eg : "Barack Obama" as name and his number as number, and the user wouldn't know he would send his SMS to the malicious number ?

To be fair, I'm not sure we should do this. When we'll have the datastore we'll have a local contact db in the Messages app anyway, so no more request to Gecko...

So I'd say it's more feedback- for me, for these reasons.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 798530 [details]
Pull request

ok, discussed with Borja, and it's more feedback+ to support the facebook use case, and third-party contacts app use cases too.

Comment 11

[Borja Salguero [:borjasalguero]]

In this case the goal is to 'play' with real {contact} object, not with the 'view' made by Contacts App (actually we received only name & phone number :S ). This patch allows us to receive the entire object filtered by phone number, so even a Facebook contact could be used in the right manner. Furthermore, when DataStore or IAC will be available, we will retrieve the {contact} from different providers.
This patch is *only* for the case of *pick* a contact from SMS/MMS App. There is other bug, for adding {contact} as a param to the activity.
Rick, Julien, r?

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 798530 [details]
Pull request

comments on the PR

thanks !

Comment 13

[Borja Salguero [:borjasalguero]]

Comment on attachment 798530 [details]
Pull request

Tests added to the Contacts App :)

Comment 14

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

Won't be able to r? this, @Julien can you take care?

Comment 15

[Borja Salguero [:borjasalguero]]

Comment on attachment 798530 [details]
Pull request

I'll move tests review to Fernando Campo, due to he was implementing this part. Thanks Francisco!

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 798530 [details]
Pull request

review finished in my end

basically, I think the patch adds unnecessary complexity in the getContactDisplayInfo function.

Comment 17

[Borja Salguero [:borjasalguero]]

As we were talking in Oslo, this patch is needed for avoiding Apps to retrieve from Contacts a 'fake' Contact object (with this patch SMS is going to work with real {Contact} filtered by tel, which is the goal of the activity, not with a 'fake' object with 'name' and 'tel' only). 
Furthermore, this patch let us to retrieve the info from contacts imported from Facebook, which is a great win (datastore will be available in v1.3, so we need a way for adding this feature in v1.2, and this was the way agreed in Paris&Oslo).
On the other hand *this patch is fixing the activity* from Contacts, which was not working as expected, and all tests related.
I will add your comments to my code, and I will ask you to review again. Thanks!

Comment 18

[Julien Wajsberg [:julienw]]

I perfectly agree with your comment 17 Borja, no worry, but I don't think this is opposed to my comments on github !

Comment 19

[Borja Salguero [:borjasalguero]]

I agree with all your comments in Github as well! So I think we agree at the end (probably It was a misunderstanding related with https://bugzilla.mozilla.org/show_bug.cgi?id=900443#c16, I thought that you had doubts about the goal of the patch, sorry for that!) Updating my code... :)

Comment 20

[Fernando Campo (:fcampo)]

Comment on attachment 798530 [details]
Pull request

Tests looks nice, thanks for fixing the mess I did with the ValueSelector!

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 798530 [details]
Pull request

small comment on the PR

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 798530 [details]
Pull request

r=me with the last request change on the pull request

Comment 23

[Borja Salguero [:borjasalguero]]

Thanks Julien!
https://github.com/mozilla-b2g/gaia/commit/87e0df1bdcca5c2b57f8408b1baa84c50e999b6b
https://github.com/borjasalguero/gaia/commit/d0f09b7167d9c2b52e966c532cbb7e1a5a7202c5
R+. Merged!

Comment 24

[Julien Wajsberg [:julienw]]

In comment 22, I said "r=me with the last requested change", but you landed the pull request without that change.

Please commit a follow-up asap or I'll back out this patch.

Comment 25

[Borja Salguero [:borjasalguero]]

I've just seen the comment, I did not refresh the PR link. Opening a followup and I will add this line. Sorry for the noise.

Comment 26

[Borja Salguero [:borjasalguero]]

Patch for the followup (bug https://bugzilla.mozilla.org/show_bug.cgi?id=918302) is here:
https://github.com/mozilla-b2g/gaia/pull/12312

Comment 27

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

Uplifted 132dd57b5cffa7cc000447f8457892b92d7979fc to:
v1.2: 3f318a0c728a319d9eea927c0275d2e480d36e6f

Comment 28

[isabelrios]

Verified with master 09/24 build: 
Gecko-b95f5c3
Gaia-eaae9cd

Now when selecting a FB contact with the same number than a contact in the contact list, the FB's name is added into the 'To' field.

A new bug will be open to follow what happens next. When writing text and tapping on Send key, then the other contact's name is displayed in the header and in the messages list.

Comment 29

[isabelrios]

Follow up for the open issue not covered in this bug but still failing, bug 918827

Comment 30

[Julien Wajsberg [:julienw]]

Existing bug 912399 will fix sending a message to a facebook contact from the Contacts app.