889245 - [Call log] When user make a voicemail or emergency call, call log shows only number

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : When user make a emergency call or voicemail, call log shows only number
2. Precondition : Set voicemail number
3. Tester's Action : make a emergency call or voicemail
4. Detailed Symptom : call log shows only number
5. Expected : call log shows "Emergency call" or "Voicemail"
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train : Reproduce
8. Gaia Revision : f2d6ed54a236e6e3b94f0abad9f0dacb8a1cc7b3
9. Personal email id : [e-mail]

Comment 1

[Anthony Ricaud (:rik)]

Splitting this bug into two different because the changes involved are very different.

Comment 2

[Anthony Ricaud (:rik)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10792

Pointer to Github pull-request

Comment 3

[Anthony Ricaud (:rik)]

Unassigning myself as I'm on PTO. I'll email around to find someone who could finish this.

Comment 4

[:gerard-majax]

I'll handle review changes.

Comment 5

[:gerard-majax]

With the current patch, the Voicemail string is added only when we open the call log from the database, i.e., there is no Voicemail label when you check the call log right after making the call and not killing the application.

Are you fine with that ? I'd doubt so, but I want to make sure.

Comment 6

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> With the current patch, the Voicemail string is added only when we open the
> call log from the database, i.e., there is no Voicemail label when you check
> the call log right after making the call and not killing the application.
> 
> Are you fine with that ? I'd doubt so, but I want to make sure.

I found a way to handle this case.

Comment 7

[:gerard-majax]

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

Please find attached a link to the Github pull request https://github.com/mozilla-b2g/gaia/pull/10865

Main change of this pull request upon the previous one is that we enforce the isVoicemailNumber when adding the call to the live call log, thus ensuring that the correct display is performed without needing to reload the database.

Comment 9

[:gerard-majax]

After discussing with etienne, we decided to put both voicemail and emergency booleans in the database to simplify the code.

However, I'm currently unsure whether we can make an upgrade path that can handle detecting that a number is a voicemail number: we need to load voicemail.js for this, and LazyLoad gets in the way of indexedDB and makes the transaction failing.

Comment 10

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #9)
> After discussing with etienne, we decided to put both voicemail and
> emergency booleans in the database to simplify the code.
> 
> However, I'm currently unsure whether we can make an upgrade path that can
> handle detecting that a number is a voicemail number: we need to load
> voicemail.js for this, and LazyLoad gets in the way of indexedDB and makes
> the transaction failing.

Please note that this means we would not be able to have a consistent display (i.e. show « Voicemail » label in the call log) until the next call to this number is placed.

Comment 11

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #10)
> (In reply to Alexandre LISSY :gerard-majax from comment #9)
> > After discussing with etienne, we decided to put both voicemail and
> > emergency booleans in the database to simplify the code.
> > 
> > However, I'm currently unsure whether we can make an upgrade path that can
> > handle detecting that a number is a voicemail number: we need to load
> > voicemail.js for this, and LazyLoad gets in the way of indexedDB and makes
> > the transaction failing.
> 
> Please note that this means we would not be able to have a consistent
> display (i.e. show « Voicemail » label in the call log) until the next call
> to this number is placed.

Updated pull request, this is getting mitigated, but I still have the issue that on the first load, when doing database upgrade, the display is not correct for "Voicemail".

Killing then restarting the app and getting to call log again gives a correct voicemail label.

Comment 12

[:gerard-majax]

Finally we go back to a simpler migration. While detecting on the fly that the number is a Voicemail one could be good, it is not possible to be performed during the upgradeneeded transaction (Voicemail.check() callback makes us losing the transaction).

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 772635 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

Commented on github.

Main issues are:
- the breakage of the tests
- the async code issue (LazyL10n related)

Cheers!

Comment 14

[:gerard-majax]

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

Comment 15

[:gerard-majax]

Comment on attachment 777824 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

The link in the attachment was pointing to the wrong pull request.

Comment 16

[Anthony Ricaud (:rik)]

Comment on attachment 777824 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

Stealing the review as Etienne is on PTO.

One small detail left to fix, commented on the pull request.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

I've also added some drive-by review comments.

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Alexandre LISSY :gerard-majax from comment #12)
> Finally we go back to a simpler migration. While detecting on the fly that
> the number is a Voicemail one could be good, it is not possible to be
> performed during the upgradeneeded transaction (Voicemail.check() callback
> makes us losing the transaction).

You can open a new transaction within the Voicemail.check() handler

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: DB upgrade checking voicemail

This patch contains the DB upgrade code that checks if a group contains voicemail calls or not. It still needs to do the same with the recents store (we probably don't need to block the rendering of the call log for this object store update as we are not actually using it now) and for the emergency call use case.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Oh, and I included your patch for bug 892520, as now it's needed :)

Comment 21

[:gerard-majax]

(In reply to Fernando Jiménez Moreno [:ferjm] (PTO from 30/7 to 26/8) from comment #20)
> Oh, and I included your patch for bug 892520, as now it's needed :)

Once again I predicted a bug :)

Comment 22

[:gerard-majax]

Sorry for the delay, I thought that fernando was taking this bug while I was away. I'll work on this one right now.

Comment 23

[Julien Wajsberg [:julienw]]

Alexandre, do you still feel like working on this bug or should we reassign ?

Comment 24

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment #23)
> Alexandre, do you still feel like working on this bug or should we reassign ?

Well it's still blocked by 895517, and since I'm off this week, feel free to reassign. When I replied previously, I secretly hoped that bug 895517 would be fixed soon :(

Comment 25

[Julien Wajsberg [:julienw]]

From discussions in bug 895517 it looks like we don't mind about migrating the existing call log database. At least we don't want to expose a new API for a one-time operation on an ephemeral list.

Ferjm, could we move forward here, then ?

Comment 26

[:gerard-majax]

I've updated my pull request. As far as I could test, it's updating things correctly.

Comment 27

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Julien Wajsberg [:julienw] from comment #25)
> From discussions in bug 895517 it looks like we don't mind about migrating
> the existing call log database. At least we don't want to expose a new API
> for a one-time operation on an ephemeral list.
> 

Julien, from my understanding of the discussion there, starting from bug 895517 comment 25, migrating the call log db to find previously made emergency calls is NOT advisable, especially because of the short-lived life of calls in the log.

Comment 28

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 777824 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

Thanks for the patch Alexandre, while it does work on the phone, I have a few comments on GitHub regarding some refactoring that would optimize performance and maintainability of the code.

Feel free to request the review again once those suggestions are addressed.

Comment 29

[:gerard-majax]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #28)
> Comment on attachment 777824 [details]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/10865
> 
> Thanks for the patch Alexandre, while it does work on the phone, I have a
> few comments on GitHub regarding some refactoring that would optimize
> performance and maintainability of the code.
> 
> Feel free to request the review again once those suggestions are addressed.

Thanks for the comments, I've addressed most of those. For the two remaining that I did not addressed, I'm not sure of the way to deal with, see comments on github.

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #27)

> 
> Julien, from my understanding of the discussion there, starting from bug
> 895517 comment 25, migrating the call log db to find previously made
> emergency calls is NOT advisable, especially because of the short-lived life
> of calls in the log.

Yep, that's what I meant :) sorry for the misunderstanding

Comment 31

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 777824 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

Thanks Alexandre, we're getting there! I have left you suggestions on GitHub on how to go about the remaining comments -- request the review again once those are in :)

Comment 32

[:gerard-majax]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #31)
> Comment on attachment 777824 [details]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/10865
> 
> Thanks Alexandre, we're getting there! I have left you suggestions on GitHub
> on how to go about the remaining comments -- request the review again once
> those are in :)

I've updated the latest part. I can't more the latest if addition in createGroup() because it depends on primInfo.parentNode which is undefined until the |main.appendChild(primInfo);| call

Comment 33

[Etienne Segonzac (:etienne)]

Comment on attachment 777824 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

Commented on github.

I have to r- because the migration code as it is will leave the call log stuck on the "database migration screen" the first time it launches.
The details are on github but the fix should be pretty straightforward.

Other than that we're good :)

Comment 34

[:gerard-majax]

Comment on attachment 777824 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

I've addressed the comments. Migration is now working well and notifies the user correctly. This also adds tests.

Please note I'm still working on making this bug dependant on 892520, which should be straigh forward.

Comment 35

[:gerard-majax]

Pull request is updated, now this depends on bug 892520.

Comment 36

[Etienne Segonzac (:etienne)]

Comment on attachment 777824 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10865

r=me (7b0c70b)

Comment 37

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/ae1905a5bf10f064a86731e2bff195c2bccf620f

Comment 38

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted ae1905a5bf10f064a86731e2bff195c2bccf620f to:
v1-train: 0e8e28d549d3f1e8fafe4d7b4be459ce7640d779

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 0e8e28d549d3f1e8fafe4d7b4be459ce7640d779

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Sorry for jumping on this again so late, but I've been on PTO for a big while.

I might be missing something obvious, so forgive me in advance if I am wrong about this comment. But I'm just curious about why did we end up bumping the DB version to 5 only to set two booleans to false for each group of calls?

We are not adding/removing new indexes or modifying the value of the data stored with real voicemail/emergency information (other than a default 'false' value for each group of calls) as we are ignoring the already stored information, so we could just leave the db version and the data as it was and just add the new fields in CallLogDBManager.add as needed. Correct me if I am wrong, please, but the condition at [1] would still be evaluated as false if both fields ('voicemail' and 'emergency') are undefined.

IMHO CallLogDBManager._upgradeSchemaVersion5 is not needed the way it is. Adding a 'false' value in a DB field is like not having that field in this case.

[1] https://github.com/lissyx/gaia/blob/7b0c70b0b9a2e26e9c5ba0cd34e46e02cb4c4d1d/apps/communications/dialer/js/call_log.js#L479

Comment 41

[Fernando Jiménez Moreno [:ferjm]]

Since this patch already landed, we cannot decrease the DB version, but we can just clear the CallLogDBManager._upgradeSchemaVersion5 function. I'll file a bug to do that if you agree that the upgrade code is not needed.

Comment 42

[Etienne Segonzac (:etienne)]

(In reply to Fernando Jiménez Moreno [:ferjm] (PTO from 30/7 to 26/8) from comment #41)
> Since this patch already landed, we cannot decrease the DB version, but we
> can just clear the CallLogDBManager._upgradeSchemaVersion5 function. I'll
> file a bug to do that if you agree that the upgrade code is not needed.

Agreed.
You're painfully right, old SQL reflexes...

Comment 43

[Julien Wajsberg [:julienw]]

We discussed this with Alexandre, and we decided to do this even if it was not mandatory, because explicit is better than implicit.

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Julien Wajsberg [:julienw] from comment #43)
> because explicit is better than implicit.

Unless is useless and decreases the performance of the upgrade process :)