Closed Bug 881716 Opened 11 years ago Closed 11 years ago

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

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
VERIFIED FIXED
Project Flags:
blocking-b2g leo+
Tracking Flags:
Tracking Status
b2g18 --- verified
b2g-v1.1hd --- fixed

People

(Reporter: isabelrios, Assigned: rwaldron)

References

Details

(Whiteboard: MMS_TEF)

Attachments

(7 files, 1 obsolete file)

no carrier info
21.32 KB, image/png
Details
Contact Search Results
49.26 KB, image/png
Details
Group View
9.08 KB, image/png
Details
Message thread with contact that has carrier
6.95 KB, image/png
Details
Message thread with contact with no carrier
7.88 KB, image/png
Details
Fix for 881716
46 bytes, text/x-github-pull-request
arcturus
: review+
Details | Review
PR 10348 carrier info not present in the contact search
257.45 KB, image/png
Details
Attached image 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
Assignee: nobody → francisco.jordano
blocking-b2g: --- → leo?
Attached file Pointer to PR 10324 (obsolete) — 

        Attachment #761033 -
        Flags: review?(waldron.rick)

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
per IRC discussion, taking this ticket
Assignee: francisco.jordano → waldron.rick

    
    

    
  

      
      
      
      

        Attached image
          
        Contact Search Results
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached image
          
        Group View
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        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?

    
    

    
  
Ah! Good catch, I forgot to commit index.html!

    
    

    
  
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+

    
  
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

    
  
blocking-b2g: leo? → leo+
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)

    
    

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

    
    

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

    
    

    
  
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

    
    

    
  
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

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: