isabelrios Reporter
	Description • 10 years ago

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.

	Preeti Raghunath(:Preeti)
	Comment 2 • 10 years ago

cert waiver

	Dietrich Ayala (:dietrich)
	Comment 3 • 10 years ago

Requesting feedback on the fix.

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

	Dietrich Ayala (:dietrich)
	Comment 4 • 10 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 5 • 10 years ago

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

	Dietrich Ayala (:dietrich)
	Comment 6 • 10 years ago

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)

	Dietrich Ayala (:dietrich)
	Comment 7 • 10 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 8 • 10 years ago

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

	Stephany Wilkes
	Comment 9 • 10 years ago

Flagging Carrie.

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 10 • 10 years ago

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!

	Dietrich Ayala (:dietrich)
	Comment 11 • 10 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 12 • 10 years ago

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

	David Scravaglieri [:scravag]
	Comment 13 • 10 years ago

Any update on this ?

	Doug Sherk (:drs) (inactive)
	Comment 14 • 10 years ago

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

	Anthony Ricaud (:rik) Assignee
	Comment 15 • 10 years ago

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.

	Anthony Ricaud (:rik) Assignee
	Comment 16 • 10 years ago

Eric: Could you look at comment 15, please ?

	Eric Chang [:ericcc] [:echang]
	Comment 17 • 10 years ago

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

	Anthony Ricaud (:rik) Assignee
	Comment 18 • 10 years ago

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.

	Eric Chang [:ericcc] [:echang]
	Comment 19 • 10 years ago

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.

	Doug Sherk (:drs) (inactive)
	Comment 21 • 10 years ago

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.

	Anthony Ricaud (:rik) Assignee
	Comment 22 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 10 years ago

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

	Juanjo Iglesias
	Comment 25 • 10 years ago

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)

	Beatriz Rodríguez [:brg]
	Comment 27 • 10 years ago

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

	Dietrich Ayala (:dietrich)
	Comment 28 • 10 years ago

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?

	Anthony Ricaud (:rik) Assignee
	Comment 29 • 10 years ago

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

	Dietrich Ayala (:dietrich)
	Comment 30 • 10 years ago

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

	Coler
	Comment 31 • 10 years ago

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