1029581 - [Dialer] Contact name is not shown in the call log after having two contacts with same number and the first one is removed

Status: VERIFIED FIXED

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

Description

[isabelrios]

Attachment: Call log showing the number

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.

Comment 1

[isabelrios]

Attachment: Attention screen after tapping on the number in the call log

Comment 2

[Preeti Raghunath(:Preeti)]

cert waiver

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20944

Requesting feedback on the fix.

Ignore the test, not ready yet - mock contacts module doesn't support what's needed here yet.

Comment 4

[Dietrich Ayala (:dietrich)]

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.

Comment 5

[Dietrich Ayala (:dietrich)]

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

Comment 6

[Dietrich Ayala (:dietrich)]

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)

Comment 7

[Dietrich Ayala (:dietrich)]

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 [1] 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.

Comment 8

[Dietrich Ayala (:dietrich)]

Requesting UX input on above. Let's figure out the expected behavior before sinking engineering time into this.

Comment 9

[Stephany Wilkes]

Flagging Carrie.

Comment 10

[Carrie Wang [New account: [e-mail]]]

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!

Comment 11

[Dietrich Ayala (:dietrich)]

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.

Comment 12

[Dietrich Ayala (:dietrich)]

Latest PR fixes original STR. Still need to fix alphabetical log stealing and multi delete fallback.

Comment 13

[David Scravaglieri [:scravag]]

Any update on this ?

Comment 14

[Doug Sherk (:drs) (inactive)]

Dietrich, would it help for someone else to take this and finish it?

Comment 15

[Anthony Ricaud (:rik)]

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.

Comment 16

[Anthony Ricaud (:rik)]

Eric: Could you look at comment 15, please ?

Comment 17

[Eric Chang [:ericcc] [:echang]]

(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.

Comment 18

[Anthony Ricaud (:rik)]

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.

Comment 19

[Eric Chang [:ericcc] [:echang]]

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 20

[Anthony Ricaud (:rik)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/21624

Comment 21

[Doug Sherk (:drs) (inactive)]

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.

Comment 22

[Anthony Ricaud (:rik)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/e5617480547edadf5674becee560fc09f62a3aa8

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Juanjo Iglesias]

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)

Comment 26

[Juanjo Iglesias]

Attachment: screenshot_callLogs_C2.png

Comment 27

[Beatriz Rodríguez [:brg]]

Per comment 25 bug marked as Verified. Thanks for the effort to fix this.

Comment 28

[Dietrich Ayala (:dietrich)]

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?

Comment 29

[Anthony Ricaud (:rik)]

I meant we don't have enough tests in our codebase, aka automated tests.

Comment 30

[Dietrich Ayala (:dietrich)]

Alphabetical call log entry theft still occurs. Filed bug 1043684.

Comment 31

[Coler]

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

Comment 32

[Coler]

Attachment: Verify_Flame_Callog.MP4