[SMS/MMS] Search result for contacts from Messaging app is not showing the carrier

VERIFIED FIXED

Status

Firefox OS
Gaia::SMS
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: isabelrios, Assigned: rwaldron)

Tracking

unspecified
x86_64
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 verified, b2g-v1.1hd fixed)

Details

(Whiteboard: MMS_TEF)

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 760915 [details]
no carrier info

Unagi 06/11 build:
Gecko-68ad212
Gaia-ea18de8

PROCEDURE
1. Create some contacts with phone number, some with carrier field
2. Open Messaging app and create a new message
3. Tap into the 'To' field and type one of the contact's name which has phone number and carrier

EXPECTED
The contact is shown fine, with the number, phone type and carrier

ACTUAL
The carrier info does not appear when the contacts are shown in the search result panel
(Reporter)

Updated

4 years ago
Assignee: nobody → francisco.jordano
blocking-b2g: --- → leo?
Created attachment 761033 [details]
Pointer to PR 10324
Attachment #761033 - Flags: review?(waldron.rick)
status-b2g-v1.1hd: --- → affected
(Assignee)

Comment 2

4 years ago
Comment on attachment 761033 [details]
Pointer to PR 10324

Review comments on PR, the most important is the last: https://github.com/mozilla-b2g/gaia/pull/10324#discussion_r4640490
Attachment #761033 - Flags: review?(waldron.rick) → review-
Comment on attachment 761033 [details]
Pointer to PR 10324

Addressed the performance comment.

Regarding the two other comments already gave some replies on github, this are my arguments:

- In the case of using the number instead of the type is just cause based on the product definition, the type remains intact, is the phone field the one that alternates with the carrier name if this is present and match the logic that Ayman sent.

- Genarating the carrier information needed is done outside the |getCarrierNumberValue| caused we need to build this once, and calling |getCarrierNumberValue| happens in a loop for each phone.
We need to precalculate this cause of the point 3 of decision logic which says that if we have two phones with the same type and same carrier we show the phone number instead of the carrier.

Hope this is a bit clear, too many variants I know.

Anyway we can catch up on IRC if you want more info.

Cheers!
F.
Attachment #761033 - Flags: review- → review?
(Assignee)

Comment 4

4 years ago
Copying this back from the PR, because it's important that everyone involved read this:

- The changes in this PR should not affect contact search results or contact participant view—currently it does. Since a user may be typing the number, we must show the highlighted match.

- The changes in this PR only apply to the `carrierTag` or `contact-carrier` (they are the same thing) and that happens in `updateHeaderData`

The patch must absolutely not render a contact without a number.
(Assignee)

Comment 5

4 years ago
per IRC discussion, taking this ticket
Assignee: francisco.jordano → waldron.rick
(Assignee)

Comment 6

4 years ago
Created attachment 761582 [details]
Contact Search Results
(Assignee)

Comment 7

4 years ago
Created attachment 761584 [details]
Group View
(Assignee)

Comment 8

4 years ago
Created attachment 761585 [details]
Message thread with contact that has carrier
(Assignee)

Comment 9

4 years ago
Created attachment 761586 [details]
Message thread with contact with no carrier
(Assignee)

Comment 10

4 years ago
Created attachment 761587 [details] [review]
Fix for 881716
Attachment #761587 - Flags: review?(francisco.jordano)
Thanks a lot Rick for following the different ways of displaying the name, phone, type and carrier.

The patch code wise look incredible to me, the best part is the test :), and covers almost all the cases.

The only case that I still have doubts is the one related to searching for contacts.

In the specs and UI mocks that you sent me, here is the one for search:

http://gyazo.com/6e3a362318c6e76df3d2e2969b64d779

It does include the number and the carrier (both, so my patch was basically broken as well).

So I'll set the patch that I sent as obsolete, as yours solves almost the whole problem but would like to address the problem in the search.

Thanks a lot, impressive job!
Comment on attachment 761033 [details]
Pointer to PR 10324

><!DOCTYPE html><meta charset="utf-8"><meta http-equiv="refresh" content="5;https://github.com/mozilla-b2g/gaia/pull/10324"><title>Bugzilla Code Review</title><p>You can review this patch at <a href="https://github.com/mozilla-b2g/gaia/pull/10324">https://github.com/mozilla-b2g/gaia/pull/10324</a>, or wait 5 seconds to be redirected there automatically.</p>
Attachment #761033 - Attachment is obsolete: true
Attachment #761033 - Flags: review?
Created attachment 761598 [details]
PR 10348 carrier info not present in the contact search
(Assignee)

Comment 14

4 years ago
Ah! Good catch, I forgot to commit index.html!
(Assignee)

Comment 15

4 years ago
Fixed! http://gyazo.com/edd47e0d8157fbafba8d9a90d272c295

I've also added tests that include both a no-carrier and has-carrier expectation
Comment on attachment 761587 [details] [review]
Fix for 881716

r+ once solved a small comment on github.

Pretty good job, finally all contacts displaying cases seem solved \o/
Attachment #761587 - Flags: review?(francisco.jordano) → review+
(Assignee)

Comment 17

4 years ago
Landed https://github.com/mozilla-b2g/gaia/commit/3c65d31e4e72fd2ee368ff3671f0c65e4f7eb956
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
blocking-b2g: leo? → leo+

Updated

4 years ago
Depends on: 883138
I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  3c65d31e4e72fd2ee368ff3671f0c65e4f7eb956
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(waldron.rick)
(Assignee)

Comment 19

4 years ago
uplift blocked by bug 882899
Depends on: 882899
Flags: needinfo?(waldron.rick)
Whiteboard: MMS_TEF → MMS_TEF, NO_UPLIFT
(Assignee)

Comment 20

4 years ago
This bug should not be uplifted until bug 882899 has landed on master, as it fixes an issue that appeared in this patch.
(Assignee)

Comment 21

4 years ago
I will update this when the other two are ready: https://github.com/mozilla-b2g/gaia/pull/10402
removing NO_UPLIFT, let's land these bugs together on v1-train !
Whiteboard: MMS_TEF, NO_UPLIFT → MMS_TEF
Flags: needinfo?(jhford)
uplifted master: 3c65d31e4e72fd2ee368ff3671f0c65e4f7eb956
to v1-train: 83783531ea22dbecc97f77cb387433d50c862b51
status-b2g18: --- → fixed
Flags: needinfo?(jhford)
1.1hd: 83783531ea22dbecc97f77cb387433d50c862b51
status-b2g-v1.1hd: affected → fixed
(Reporter)

Comment 25

4 years ago
Carrier is now shown when searching for contacts from messaging app. Verified with unagi 07/07 v1-train build:
Gecko-e7708d4
Gaia-d336288
Ref ril
Status: RESOLVED → VERIFIED

Comment 26

4 years ago
Issue no longer reproduces on the Leo  
Build ID: 20130716070204
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/629020cf576b
Gaia: fb9362d34260771d4a00b9a0e10a6bbad397bd3b
Platform Version: 18.1 
contacts from Messaging app is showing the carrier when sending SMS/MMS.
status-b2g18: fixed → verified
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.