21.32 KB, image/png
49.26 KB, image/png
9.08 KB, image/png
6.95 KB, image/png
7.88 KB, image/png
46 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
257.45 KB, image/png
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
Assignee: nobody → francisco.jordano
blocking-b2g: --- → leo?
Created attachment 761033 [details] Pointer to PR 10324
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
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>
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
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
uplift blocked by bug 882899
Depends on: 882899
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
uplifted master: 3c65d31e4e72fd2ee368ff3671f0c65e4f7eb956 to v1-train: 83783531ea22dbecc97f77cb387433d50c862b51
status-b2g18: --- → fixed
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.