976990 - [Sora][Call Log] Contact's name still display in call log after delete the contact from phonebook.

Status: VERIFIED DUPLICATE of bug 1112577

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

Description

[sync-1]

Firefox OS v1.3
 Mozilla build ID: 20140208004002
 
 Created an attachment (id=648761)
 Logcat for this PR
 
 DEFECT DESCRIPTION:
  Contact's name still display in call log after delete the contact from phonebook.
 
  REPRODUCING PROCEDURES:
  Precondition: There is a number in call log which not associated to any contact.
  1. Idle -> Dialer -> Enter call log interface -> Tap the number in call log -> Create new contact -> Input name -> done
  2. Delete the contact -> Back to call log interface -> Contact's name still display in call log(K.O)
 
  EXPECTED BEHAVIOUR:
  It should diplay the phone number in call log.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE: 100%
 
  For FT PR, PleaseInsert list reference mobile's behavior:
  It is OK on Beetle Lite FF.
  
  If you have any questions,please dial 7526.

Comment 1

[sync-1]

Attachment: Logcat for this PR

Comment 2

[sync-1]

Attachment: Screenshots for this PR

Comment 3

[Jason Smith [:jsmith]]

Minor regression.

Comment 5

[Johan Lorenzo [:jlorenzo]]

Per bug 1083124 comment 14

Comment 6

[promise09th]

Dear

I think below code occur some errors

gaia/apps/communication/dialer/js/call_log.js
===================================================================
this._updateContact(log, logInfo.phoneNumber, contactId, i === 0);
===================================================================

Because of "i === 0" condition, Dialer doesn't save updated contact information.

Please check this code

Comment 7

[bhavana bajaj [:bajaj]]

Tamara, will be be helping resolve this given the feedback in https://bugzilla.mozilla.org/show_bug.cgi?id=1083124 ?

Comment 8

[Doug Sherk (:drs) (inactive)]

(In reply to bhavana bajaj [:bajaj] from comment #7)
> Tamara, will be be helping resolve this given the feedback in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1083124 ?

Yes, it is targeted to be done in this sprint. I updated the tracking flags of this bug once bug 1083124 was marked as a duplicate.

Comment 11

[Tamara Hills [:thills]]

Attachment: WIP for feedback

Hi Doug,

This is a WIP.  It does not address bug 903345.  I believe there is something different going on there which I will continue to look into.  

Thanks,

-tamara

Comment 12

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8518250 [details] [diff] [review]
WIP for feedback

Review of attachment 8518250 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know off the top of my head what the correct fix is here, and I'd have to do more investigation myself to say. But I don't think that this is it. If your findings are different than mine, please let me know and we can talk about it more. At the very least, we should understand better why this fixes the issue.

::: apps/communications/dialer/js/call_log.js
@@ +891,5 @@
> +        // multiple call log entries with the same TN in list.  The first
> +        // call in removeGroupContactInfo will return the number of entries with
> +        // the contacts id, but the subsequent calls will return 0 and hence
> +        // don't get their headers updated by the following function.
> +        self.updateContactInfo(log);

This callback is only called once with the correct number of results. I'm not sure I understand why we need this change. Could you explain how you were able to get it to fire more than once?

@@ +948,5 @@
>  
> +      //This change seems to fix the issue reported in 976990.  If this is set
> +      //to 0, then the updates only happen for the first item in the list.
> +      this._updateContact(log, logInfo.phoneNumber, contactId,
> +        (reason === 'create' || reason === 'remove'));

I don't see how this could fix the issue. This actually makes the criteria for calling `CallLog.updateContactInfo()` more restrictive.

Comment 13

[Tamara Hills [:thills]]

Attachment: 976990v2.diff

Hi Doug,

The key change is that on a create, it updates the database once only in the case where there are multiple call groups with the same phoneNumber.  The initial code was updating only the first record in the DOM (hence it didn't work for anything after the first log item element).

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8524277 [details] [diff] [review]
976990v2.diff

Review of attachment 8524277 [details] [diff] [review]:
-----------------------------------------------------------------

The general approach seems reasonable to me, but this doesn't work in all cases. For example, a contact with a non-exact-matching phone number doesn't get added/removed from the call log correctly. An example of this is 663-911-7347 in the call log, 6639117347 on the contact.

One idea that I have right now is to separate out the DB deletion logic and the rest of the call log presentation code. This code is very confusing because of this strange flow. I will think more about this and come back here.

Comment 15

[Doug Sherk (:drs) (inactive)]

For comment 14.

Comment 16

[Tamara Hills [:thills]]

Comment on attachment 8524277 [details] [diff] [review]
976990v2.diff

Hi Etienne,

Would you be able to feedback this patch?  As Doug points out, this is not solving some of the existing matching problems, but just handling the case mentioned where a contact is created, and later deleted and the change is not reflected properly.  

Let me know if you have any questions.

Thanks,

-tamara

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8524277 [details] [diff] [review]
976990v2.diff

Review of attachment 8524277 [details] [diff] [review]:
-----------------------------------------------------------------

Talked to Fernando, his feedback will be better than mine :)

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8524277 [details] [diff] [review]
976990v2.diff

Review of attachment 8524277 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Tamara. Nice catch. The approach looks good to me as well.

::: apps/communications/dialer/js/call_log.js
@@ +10,5 @@
>    _headersInterval: null,
>    _empty: true,
>    _dbupgrading: false,
>    _contactCache: true,
> +  _updateSet:null,

Does this need to be global?

Comment 20

[Tamara Hills [:thills]]

Attachment: PR for fix

Comment 21

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8553953 [details] [review]
PR for fix

Pretty good. I left some comments on the PR.

Comment 22

[Gabriele Svelto [:gsvelto]]

Note that this is probably fixed now that bug 1112577 landed.

Comment 23

[Gabriele Svelto [:gsvelto]]

Tamara, I've tried reproducing this after landing bug 1112577 but couldn't. Could you double-check if this has been truly solved and close the bug if it has?

Comment 24

[Tamara Hills [:thills]]

I tested and this is fixed by bug 1112577.

Comment 25

[Johan Lorenzo [:jlorenzo]]

Per comment 24.

Comment 26

[Johan Lorenzo [:jlorenzo]]

Verified against master:
Build ID               20150219160225
Gaia Revision          0d9a825dd27ed63f877d5140a08327cc8f91ec74
Gaia Date              2015-02-19 19:55:26
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/51458a066fda
Gecko Version          38.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150219.193017
Firmware Date          Thu Feb 19 19:30:28 EST 2015
Bootloader             L1TC000118D0