1062799 - Call information is not updated on the fly after creating a contact

Status: RESOLVED WONTFIX

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

Description

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

### STR
1. Have a call in call log.
2. Long tap to menu, choose "Call Information".
3. Tap the dial button right next to the number.

### Actual 
Need to tap back, than go into the call information to see the change
http://youtu.be/kPb-gKlNmZM

### Version
Gaia      af04d8bc2111d4ea146239a89ff602206b85eaf5
Gecko     https://hg.mozilla.org/mozilla-central/rev/acbdce59da2f
BuildID   20140903160205
Version   35.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230

Comment 1

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

1. Get 'Call information' from an unknown number.
2. 'Create a new contact' with this.
3. The Call Information page is not updated with contact name(to replace the number shown)

Comment 2

[Anthony Ricaud (:rik)]

We already have bug 1060723 for updating the calls list after calling someone. So let's keep this bug focused on comment 1.

Eric: When testing a feature, please mark new bugs as blocking the original feature. That helps us track the remaining work.

Comment 3

[Jorge Prudencio [:jorgep]]

Attachment: 24327.html

Hello Doug, here is the patch and test for this bug.

Thanks!

Comment 4

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8493576 [details]
24327.html

This should be using the 'contactchange'/oncontactchange event. You can find an example here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/call_log.js#L1027

The reason is that if the contact changes for some other reason, such as opening the contacts app itself and creating it there, it will be caught by this method but not the one in the PR.

Sorry for not suggesting this sooner. We appreciate the work you put into this anyways and you'll definitely be able to reuse some of the unit test code.

Comment 5

[Jorge Prudencio [:jorgep]]

Hello Doug,

To make my test pass, I need support for navigator.mozContacts.find() in mock_contacts.js in shared which currently hasn't. There are 2 options here: 1) I can include this support for the find() function in mock_contacts.js and 2) I could just include the support for that find() function locally in my test. I guess the first one is the general and best one but any preference?

You can see my actual code on PR

Thanks!

Comment 6

[Doug Sherk (:drs) (inactive)]

(In reply to Jorge Prudencio [:jorgep] from comment #5)
> Hello Doug,
> 
> To make my test pass, I need support for navigator.mozContacts.find() in
> mock_contacts.js in shared which currently hasn't. There are 2 options here:
> 1) I can include this support for the find() function in mock_contacts.js
> and 2) I could just include the support for that find() function locally in
> my test. I guess the first one is the general and best one but any
> preference?
> 
> You can see my actual code on PR
> 
> Thanks!

Yes, in general, the first is preferred and is what I would suggest doing here. Just remember to keep it as simple as possible and to only solve your problem, even though it's shared (and thus more generalized) code.

There are exceptions to this. For example, we may want to stub a function in a class to do something other than what the shared mock does. In that case, we would stub it locally.

Comment 7

[Jorge Prudencio [:jorgep]]

Comment on attachment 8493576 [details]
24327.html

Hello Doug,

I have updated the code. I have finally selected the local solution because I think that it works better in this case.

Can you review it?

Thanks!

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8493576 [details]
24327.html

Thanks, the structure here is better. I left some comments on the PR.

One thing in particular that I must draw attention to is that this patch doesn't distinguish between contacts being updated. I can update any contact, and the call info page will also be updated to show their name and change the buttons displayed. It should only update the call info page if its number now corresponds with a contact.

This raises another point. If the contact associated with a call info page is deleted or their number is changed, the call info page should just show the phone number. This will require added functionality and tests. We can do this in a followup if you prefer, as this is still a useful patch that I don't want to hold back.

Comment 9

[Doug Sherk (:drs) (inactive)]

No action required, but ccing Carrie for comment 8.

Comment 10

[Jorge Prudencio [:jorgep]]

Comment on attachment 8493576 [details]
24327.html

Hello Doug,

I have updated the code with your comments and I have added a difference between create and update a contact. Can you review the changes?

Thanks a lot!

Comment 11

[Anthony Ricaud (:rik)]

I think it would be simpler here to just reload the view like it was done in bug 1060723.

Comment 12

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8493576 [details]
24327.html

Anthony makes a good point. Doing it this way would also be more future-proof, in case we add more fields from the contact to this page.

Comment 13

[Jorge Prudencio [:jorgep]]

Attachment: 24694.html

Hello Doug,

I have updated the code with Anthony suggestions. Can you review it?

Thanks!

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498809 [details]
24694.html

(In reply to Doug Sherk (:drs) from comment #8)
> One thing in particular that I must draw attention to is that this patch
> doesn't distinguish between contacts being updated. I can update any
> contact, and the call info page will also be updated to show their name and
> change the buttons displayed. It should only update the call info page if
> its number now corresponds with a contact.

This is still a problem.

Also, regarding Anthony's point: I think what makes more sense here is to just get the entry from the call log DB again using the previous search parameters. The approach in this patch is problematic as it relies on the structure of the schema staying the same, and it only supports 1 phone number. Also, group.number is unset when we call `updateGroupInformation(group)`.

Comment 15

[Jorge Prudencio [:jorgep]]

Hello Doug,

I have updated the PR with a solution based in your comment. Can you revise it? If you like it, I will modify the tests.

Thanks!

Comment 16

[Doug Sherk (:drs) (inactive)]

Thanks, I commented on the PR. In the future, you can use the feedback? flag.

Comment 17

[Anthony Ricaud (:rik)]

I don't think we should listen to mozcontactchange events. We need to wait for the call log database to be updated before reloading the view. I'd use the same event mechanism than bug 1060723. CallLogDBManager.updateGroupContactInfo and CallLogDBManager.removeGroupContactInfo should dispatch an event when the data has been updated.

Also, I don't think there's a strong need to filter out updates that don't concern the current view. If it's easy to do then we can do it but if isn't then that's ok. Cotnacts DB update will only happen as fast as the user can do the changes so unnecessary reloading of our view are not that bad.

Comment 18

[Jorge Prudencio [:jorgep]]

Hello Anthony,

I have been seeing the code and I think that we can't apply the same solution. In your patch 'call_log.js' update the call_log_db with the onchange event:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/call_log.js#L1035
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/call_log.js#L871

I have done the same on 'call_info.js' because we don't call to CallLogDBManager.updateGroupContactInfo when we update the call info with a contact.

Am I wrong?

Thanks!

Comment 19

[Anthony Ricaud (:rik)]

oncontactchange will call CallLog.updateListWithContactInfo. [1][2][3]
CallLog.updateListWithContactInfo will call CallLog._updateContact. [4]
CallLog._updateContact will call CallLogDBManager.updateGroupContactInfo [5] or CallLog._removeContact [6]
CallLog._removeContact will call CallLogDBManager.removeGroupContactInfo.

CallLogDBManager.removeGroupContactInfo and CallLogDBManager.updateGroupContactInfo will update the CallLogDB. Once it is done, the patch to fix should emit an event saying that contacts have been updated in the CallLogDB. It could be called CallLogDbUpdatedContacts for example. And then, CallInfo should listen for that event and reload when receiving it.

[1] https://github.com/mozilla-b2g/gaia/blob/5b1a35436cd6f1776b907a6f60270e00635bf371/apps/communications/dialer/js/call_log.js#L1045
[2] https://github.com/mozilla-b2g/gaia/blob/5b1a35436cd6f1776b907a6f60270e00635bf371/apps/communications/dialer/js/call_log.js#L1048
[3] https://github.com/mozilla-b2g/gaia/blob/5b1a35436cd6f1776b907a6f60270e00635bf371/apps/communications/dialer/js/call_log.js#L1062
[4] https://github.com/mozilla-b2g/gaia/blob/5b1a35436cd6f1776b907a6f60270e00635bf371/apps/communications/dialer/js/call_log.js#L946
[5] https://github.com/mozilla-b2g/gaia/blob/5b1a35436cd6f1776b907a6f60270e00635bf371/apps/communications/dialer/js/call_log.js#L871
[6] https://github.com/mozilla-b2g/gaia/blob/5b1a35436cd6f1776b907a6f60270e00635bf371/apps/communications/dialer/js/call_log.js#L865
[7] https://github.com/mozilla-b2g/gaia/blob/5b1a35436cd6f1776b907a6f60270e00635bf371/apps/communications/dialer/js/call_log.js#L888

Comment 20

[Jorge Prudencio [:jorgep]]

Comment on attachment 8498809 [details]
24694.html

Hi Anthony,

I have updated the patch with the events. Can you give me feedback from the changes?

If the patch is ok, I will modify the tests.

Thanks!

Comment 21

[Anthony Ricaud (:rik)]

Comment on attachment 8498809 [details]
24694.html

This code can't work at all. We need a new kind of event as I suggested in comment 19. Then, you shouldn't add the self._asyncReturn. This will break updating all instances of calls to a number in the call log.

And all the code in call_info.js should be removed. It should be as simple as just listening to a new event.

Comment 22

[Jorge Prudencio [:jorgep]]

Hello Anthony

I think that if I remove navigator.mozContacts.oncontactchange (from call_info.js) we don't have any function that trigger updateGroupContactInfo() on call_log_db.js, and later the event that update the call_info view.

What do you think?

Thanks!

Comment 23

[Anthony Ricaud (:rik)]

See my very detailed explanations in comment 19… We have code already in place that calls updateGroupContactInfo.

Comment 24

[Germán Toro del Valle (:gtorodelvalle)]

Setting it as unassigned since Jorge won't be working on them for the time being ;)

Comment 26

[BMO Automation]

Firefox OS is not being worked on