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

VERIFIED FIXED

Status

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: borjasalguero, Assigned: borjasalguero)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed)

Details

(Whiteboard: [u=commsapps-user c=contacts p=2])

Attachments

(1 attachment)

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.
Assignee: nobody → fbsc
Blocks: 900391
blocking-b2g: --- → leo?
Depends on: 900446
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.
blocking-b2g: leo? → koi?
ni? Borja for the complexity in v1.2 timeframe
Flags: needinfo?(fbsc)
Whiteboard: [comms-triaged]
blocking-b2g: koi? → koi+
Whiteboard: [comms-triaged] → [u=commsapps-user c=contacts p=0]
Hi Joe,
This would let us to avoid several issues with tel matching, so Im gonna try to fix this during this week. Thanks!
Flags: needinfo?(fbsc)
Whiteboard: [u=commsapps-user c=contacts p=0] → [u=commsapps-user c=contacts p=2]
Depends on: 908207
Summary: [SMS] Remove complexity related with 'getContactDisplayInfo' when retrieving a contact from 'Contacts' activity. → [SMS] Remove repeated request to Contacts API when retrieving a contact from 'Contacts' activity.
Posted file 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.
Attachment #798530 - Flags: feedback?(felash)
Flags: needinfo?(felash)
(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.
I'm gonna create a followup for fixing this issue. This could save headaches related with Contacts lookup in MMS App!
Blocks: 912399
(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!
Should be the same for when we send a SMS from the Contacts App. I've seen new bugs because of this.
Flags: needinfo?(felash)
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.
Attachment #798530 - Flags: feedback?(felash) → feedback-
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.
Attachment #798530 - Flags: feedback- → feedback+
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?
Attachment #798530 - Flags: review?(felash)
Attachment #798530 - Flags: review?(waldron.rick)
Attachment #798530 - Flags: review?(waldron.rick)
Comment on attachment 798530 [details]
Pull request

comments on the PR

thanks !
Attachment #798530 - Flags: review?(waldron.rick)
Attachment #798530 - Flags: review?(felash)
Attachment #798530 - Flags: review?(francisco.jordano)
Comment on attachment 798530 [details]
Pull request

Tests added to the Contacts App :)
Attachment #798530 - Flags: review?(felash)
Won't be able to r? this, @Julien can you take care?
Comment on attachment 798530 [details]
Pull request

I'll move tests review to Fernando Campo, due to he was implementing this part. Thanks Francisco!
Attachment #798530 - Flags: review?(francisco.jordano) → review?(fernando.campo)
Comment on attachment 798530 [details]
Pull request

review finished in my end

basically, I think the patch adds unnecessary complexity in the getContactDisplayInfo function.
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!
I perfectly agree with your comment 17 Borja, no worry, but I don't think this is opposed to my comments on github !
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 on attachment 798530 [details]
Pull request

Tests looks nice, thanks for fixing the mess I did with the ValueSelector!
Attachment #798530 - Flags: review?(fernando.campo) → review+
Comment on attachment 798530 [details]
Pull request

small comment on the PR
Comment on attachment 798530 [details]
Pull request

r=me with the last request change on the pull request
Attachment #798530 - Flags: review?(felash) → review+
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.
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.
Blocks: 918302
Uplifted 132dd57b5cffa7cc000447f8457892b92d7979fc to:
v1.2: 3f318a0c728a319d9eea927c0275d2e480d36e6f
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.
Status: RESOLVED → VERIFIED
Follow up for the open issue not covered in this bug but still failing, bug 918827
Existing bug 912399 will fix sending a message to a facebook contact from the Contacts app.
You need to log in before you can comment on or make changes to this bug.