Closed Bug 1029581 Opened 7 years ago Closed 7 years ago
[Dialer] Contact name is not shown in the call log after having two contacts with same number and the first one is removed
23.54 KB, image/png
398.09 KB, image/png
46 bytes, text/x-github-pull-request
|Details | Review|
18.29 KB, image/png
5.80 MB, video/mp4
This bug has been seen on 1.3 build during cert process. It has also been reproducible with today's (24/06) master buri build: Gecko-fda21c3 Gaia-ed5231f STR 1) Enter a new contact in the phonebook C1 & "NUMBER 1" 2) Make a call to C1, and check that the call log shows "C1" 3) Enter a new contact in the phonebook C2 with "NUMBER 1" (same number) 4) Do not merge both contacts 5) From call logs, go to the phonebook (second tab) and check that both contacts are created 6) Delete C1, and check the call log 7) Call to "NUMBER 1" from call log ACTUAL When C1 is removed, call log shows the number but when tapping on it, the attention screen displays C2. After finishing the call, the call log is not updated with C2, instead it still shows the phone number. EXPECTED Call log should identify the number as a number of contact 2, C2.
blocking-b2g: 2.0? → 2.0+
Requesting feedback on the fix. Ignore the test, not ready yet - mock contacts module doesn't support what's needed here yet.
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
Comment on attachment 8445525 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20944 Bah, I had STR wrong, not ready for review yet.
Found another issue: STR: 1. create contact A with number N1 2. call it - hang up without waiting for ring 3. check call log - says A 4. create contact B with number N1 (do not merge) 5. open call log Expected: shows call to A Actual: shows call to B
Hrm, comment #5 is not correct - I cannot reproduce now. Would help to have a full set of testcases. QA, do you have testcases for "two contacts, one number" scenario besides this bug? Eg, some scenarios and actual/expected outcomes: * call a number, call log shows the number. (Correct) * create contact B with same number. call log shows B. (Correct) * create contact C with same number. call log shows B. (Correct) * create contact A with same number. call log now shows A. (<- new bug?) * delete contact A. call log now shows number. (<- this bug)
With my current PR, this bug is addressed, but only for the first contact. Eg, assuming the final step above works, and then: * delete contact B. call log still shows B. (<- new and different issue!) Sounds like the policy for this functionality should be: * For calls without a contact, show the number. * For calls to a contact, show the contact associated with the user action. * For calls whose contact was deleted, show the next matching contact  by some predictable order. Alphabetical? Contact creation time? * Names of deleted contacts should never be shown, only a number or another contact with the same number. * Numbers should never be shown if there's a contact with that number.
Requesting UX input on above. Let's figure out the expected behavior before sinking engineering time into this.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(cawang)
Hi, The rule should be like this: 1. Create a new contact C1 with number 1 2. Make a call to C1, and check that the call log shows "C1" 3. Enter a new contact C2 with number 1 (same number) 4. Do not merge both contacts 5. Delete C1, and check the call log 6. It should display C2 for the previous C1's call log ***If there are more than two contacts sharing the same number and one of them is deleted by user, show the next matching contact in call log according to the contact creation time. ***If the contact is deleted and there is no existing contact with the same number, then only show the number in call log. Thanks!
Ok, thanks Carrie. That validates what I wrote above. The only new bug then is that creating a new contact with name starting with letter prior in alphabetical order will "steal" the call log entry. Eg: with a single call log entry by B, create contact A with same number, and call log now says A. Will fix both issues here. Note to self: test should include fallback through multiple contact deletions. Eg, delete C1 -> fallback to C2 -> delete C2 -> fallback to C3.
Latest PR fixes original STR. Still need to fix alphabetical log stealing and multi delete fallback.
Any update on this ?
Dietrich, would it help for someone else to take this and finish it?
Target Milestone: --- → 2.0 S5 (4july)
I don't think we should fix more things than needed here. This is a patch that has to be uplifted to 2.0. Here's my take on it: https://github.com/Rik/gaia/tree/find-contacts-when-removing-1029581 . I'm asking qawanted to run the call log tests that we have against this branch. It is not well unit tested on our end so I'm not sure if this fix doesn't have side effects.
Assignee: dietrich → anthony
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Whiteboard: [dialer-in-sprint=v2.0-S5] → [in-sprint=v2.0-S5]
Whiteboard: [in-sprint=v2.0-S5] → [planned-sprint][in-sprint=v2.0-S5]
Eric: Could you look at comment 15, please ?
(In reply to Anthony Ricaud (:rik) from comment #16) > Eric: Could you look at comment 15, please ? Current v2.0 build has this problem, let me run some test cases to see if we have any more issues, will update tomorrow. (In reply to Dietrich Ayala (:dietrich) from comment #11) > The only new bug then is that creating a new contact with name starting with > letter prior in alphabetical order will "steal" the call log entry.
Eric: Of course 2.0 has this problem, this is why this bug is opened. I'm asking to test the branch https://github.com/Rik/gaia/tree/find-contacts-when-removing-1029581 that fixes this bug but may introduce other problems. I'm interested to know if my tentative patch breaks any other call log related test cases that you have.
I do some exploratory testing on the patch, it works okay, now a log entry sticks to a contact as long as it exists, I saw the contactId is added as a parameter there.
Comment on attachment 8454413 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21624 This patch is beautiful! It took me several hours to understand it, but it's very well done and the comments I left on the PR are minimal. I learned some stuff just by reading it.
Attachment #8454413 - Flags: review?(drs+bugzilla) → review+
Hey thanks :) Maybe ignoring whitespace changes, it would have been easier to review: https://github.com/mozilla-b2g/gaia/pull/21624/files?w=1 . Merged in https://github.com/mozilla-b2g/gaia/commit/534d16f011922ab0b06355887dbb3484e044be50
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I had to re-add value_selector.js and wallpaper.js to the jshint whitelist due to Linter failures. v2.0: https://github.com/mozilla-b2g/gaia/commit/8d89ca60aafcc0bb0ddc779c430b4d87205b6590 https://tbpl.mozilla.org/php/getParsedLog.php?id=43755595&tree=Mozilla-Aurora
I haver tested using: - Hamachi 2.0, gecko 5b0fd39, Gaia db0358b - Unagi master Gecko 73bd274, Gaia b3ed124 In both cases, following the steps to reproduce the bug, the contacts are properly identified in the call log. (see screenshot)
Per comment 25 bug marked as Verified. Thanks for the effort to fix this.
Status: RESOLVED → VERIFIED
I was out for the last three weeks. Thanks for finishing this up Rik! (In reply to Anthony Ricaud (:rik) from comment #15) > I don't think we should fix more things than needed here. This is a patch > that has to be uplifted to 2.0. I'll see if the other bugs I found still exist after your patch and file new bugs for them if so. (In reply to Anthony Ricaud (:rik) from comment #15) > I'm asking qawanted to run the call log tests that we have against this > branch. It is not well unit tested on our end so I'm not sure if this fix > doesn't have side effects. Since the call log is a touchy issue for certification, we should improve the regression tests if they're not covering enough. Rik, do you have more information about where the tests are lacking? Or who would I ask?
I meant we don't have enough tests in our codebase, aka automated tests.
Alphabetical call log entry theft still occurs. Filed bug 1043684.
QA Whiteboard: [fxosqa-auto-backlog?]
QA Whiteboard: [fxosqa-auto-backlog?] → [fxosqa-auto-backlog+]
This issue has been verified successfully on Flame2.1&2.0. Reproducing rate: 0/5 See attachment:Verify_Flame_Callog.mp4 Flame2.0 build version: Gaia-Rev 856863962362030174bae4e03d59c3ebbc182473 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1 Build-ID 20141208000206 Version 32.0 Flame2.1 build version: Gaia-Rev 38e17b0219cbc50a4ad6f51101898f89e513a552 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a Build-ID 20141205001201 Version 34.0
You need to log in before you can comment on or make changes to this bug.