[zffos1.1][P1][Voicemail Message]The notification of new voicemail message is deleted once is read

VERIFIED FIXED in Firefox OS v1.3

Status

Firefox OS
Gaia::System
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: FangChen, Assigned: gerard)

Tracking

unspecified
1.3 C1/1.4 S1(20dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [systemsfe], URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Built ID 20130823171628
Device: Ikura
QC RIL version: "ro.build.firmware_revision=V1.01.00.01.019.180"
gaia commit: 4916f82 Merge branch 'zte/ics_strawberry_cdr' of ssh://10.67.16.41:29418/quic/lf/releases/gaia into ics_strawberry_cdr
gecko commit: a1c2bb0 ZRL modify ACCEPT Http head for MMS

   A.- Overview Description (technical background, concise explanation of the bug):
    
    The notification of new voicemail message is deleted once is read

    ________________________________________________________________________________
    B.- Steps to Reproduce (initial conditions, required resources, step by step instructions to reproduce):
    
    
"1. Leave two voice messages on voice mailbox of the DuT
2. The notification isn磘 displayed on the screen"
    ________________________________________________________________________________
    C.- Actual Result (current bad behaviour that is reported as a bug):
    
    After reading the notification, it is automatically deleted

    ________________________________________________________________________________
    D.- Expected Result (correct behaviour wished):
    
    The notification should remain until the voice message is heard

Updated

4 years ago
blocking-b2g: --- → leo?
Julien,

Can you please confirm if this is a feature enhancement for 1.1 and is more doable in 1.2?
Flags: needinfo?(felash)
Sorry, I don't know anything about the voicemail notification, which is handled specially by the System app.
Flags: needinfo?(felash) → needinfo?(alive)
AFAIK this is the behavior existing from v1.0.1
Flags: needinfo?(alive)
Lucas,

Please assign for 1.2.
blocking-b2g: leo? → koi+
Flags: needinfo?(ladamski)
(Assignee)

Comment 5

4 years ago
Isn't it related to bug 768441 ? There's a TODO referencing this in dom/system/gonk/RadioInterfaceLayer.js
Vicamo, see last comment
It's true, there is no storage for voicemail notifications.  But bug 768441 is more about keeping voicemail status across system reboots.  I'm not sure whether the bug description means the same.
Component: Gaia::SMS → RIL
Gregor is lead for system f-e, please reach out to him directly for such questions.  I'm also cc'ing Peter who is product mgr for this team.  Is this feature targeted for 1.2?
Flags: needinfo?(ladamski) → needinfo?(anygregor)
Flags: needinfo?(anygregor)
Whiteboard: [systemsfe]

Comment 9

4 years ago
Do we really need this in v1.2? 
If yes, who can help this bug? Thanks. 

And, should we assign the component as RIL? It does not look like a RIL issue.
blocking-b2g: koi+ → koi?
Hi Kevin, 

(In reply to Kevin Hu [:khu] from comment #9)
> Do we really need this in v1.2? 

This is something that has been asked by all of Telefonica carriers from 1.0.1, it's not a blocking issue, but I think it'd nice to have it. Do you think it'd possible to address this?

> If yes, who can help this bug? Thanks. 
> 
> And, should we assign the component as RIL? It does not look like a RIL
> issue.

Yes, I think it's not RIL dependant... 

Thanks!
David

Comment 11

4 years ago
(In reply to David Palomino [:dpv] from comment #10)
> Hi Kevin, 
> 
> (In reply to Kevin Hu [:khu] from comment #9)
> > Do we really need this in v1.2? 
> 
> This is something that has been asked by all of Telefonica carriers from
> 1.0.1, it's not a blocking issue, but I think it'd nice to have it. Do you
> think it'd possible to address this?

Thanks, David. 
Since this bug looks like something in system front end team(because of the whiteboard). I would leave the decision to system front end team. 

> 
> > If yes, who can help this bug? Thanks. 
> > 
> > And, should we assign the component as RIL? It does not look like a RIL
> > issue.
> 
> Yes, I think it's not RIL dependant... 
> 

Thanks! 

> Thanks!
> David
Component: RIL → Gaia::System

Updated

4 years ago
Whiteboard: [systemsfe] → [systemsfe][koi-]

Updated

4 years ago
blocking-b2g: koi? → -
Whiteboard: [systemsfe][koi-] → [systemsfe]
Not blocking because we've already shipped a FxOS release with this bug.
Hi Jason, 

We get a waiver for this issue for both tef and leo, but this does not mean that we could have the waiver for all future versions. 

I think we'd be nice to implement this for koi.

Thnks, 
David
(In reply to Jason Smith [:jsmith] from comment #12)
> Not blocking because we've already shipped a FxOS release with this bug.

As David says, we persuade the operators to accept this bug, but this doesn't mean they will accept current behaviour forever. 

Having launched devices with a certain bug shouldn't be the reason not to fix that bug in future releases. The operator expects from new FFOS releases not only new features, but also some bugs previously detected fixed.

I really do think this, should be fixed in V1.2
blocking-b2g: - → koi?
Because this works as designed, it is late to accept changes in 1.2.  If this was the only remaining bug, we would not block the release because of this.  We will pull this into our backlog and try to fix it in 1.3/1.4, but would not block a release on it.
blocking-b2g: koi? → -
Hi, 

According to last comment, nominating to 1.3? for consideration... 

Thks!
David
blocking-b2g: - → 1.3?
After discussing offline, we need to block on this.  Waivers have been obtained to live with the existing functionality, but it will be difficult to get waivers for all OBs again for 1.3.
blocking-b2g: 1.3? → 1.3+
Let me copy here the description of this test case to improve the description in comment 0.
DESCRIPTION:
1. Activate the voicemail service 
2. Leave 5 voice messages in the voicemail account.
3. The terminal must display a notification of new voicemail message recorded (ex. Icon).
4. Access the voicemail account and listen to 5 voice messages recorded, deleting them one by one.

EXPECTED RESULT:
All the messges must be properly listened and the notification icon must be shown.  After listening to the voicemail messages and deleting them, the terminal must delete the voicemail message notification (icon).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → lissyx+mozillians
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
(Assignee)

Comment 19

4 years ago
(In reply to Beatriz Rodríguez [:brg] from comment #18)
> Let me copy here the description of this test case to improve the
> description in comment 0.
> DESCRIPTION:
> 1. Activate the voicemail service 
> 2. Leave 5 voice messages in the voicemail account.
> 3. The terminal must display a notification of new voicemail message
> recorded (ex. Icon).
> 4. Access the voicemail account and listen to 5 voice messages recorded,
> deleting them one by one.
> 
> EXPECTED RESULT:
> All the messges must be properly listened and the notification icon must be
> shown.  After listening to the voicemail messages and deleting them, the
> terminal must delete the voicemail message notification (icon).

Is there some trick to get the voicemail notification working ? Right now, on my Inari, I'm leaving a voice message from my Android phone, but I don't have the icon at all. I do, however, get a SMS from my provider stating that I have one voice message.

I will retry with a Keon and Peak to crosscheck.
(Assignee)

Comment 20

4 years ago
Etienne told me that it might be related to the carrier in fact. For now, no luck with: Orange, SFR and La Poste Mobile.
(Assignee)

Comment 21

4 years ago
\o/ I'm reproducing voicemail issue with Keon + Free Mobile SIM card.
(Assignee)

Comment 22

4 years ago
And also reproducing on Inari with Free Mobile SIM card. I've got everything ready and now can investigate more deeply what happens.
(Assignee)

Comment 23

4 years ago
I'm wondering how much this bug relates to bug 768441
(Assignee)

Comment 24

4 years ago
For now, from all what I can see, we only get a notification from the RIL via a special SMS that there are voice messages. In my case, the message count is -1 (that can happen).

So when we tap the notification, it gets removed (which is expected). Bug 768441 is about supporting storing this kind of messages in the SIM. I'm still investigating, but as far as I can read, we should be getting a new SMS indicating that we can now remove the notification.

So the correct way to handle this should be in fact to move to the new Notification API.
(Assignee)

Comment 25

4 years ago
I also notice that we setup multiple notification instead of updating current one.
(Assignee)

Comment 26

4 years ago
Okay I think I have the correct workflow now, thanks to this log:

1. Leave a first message:
I/Gecko   (  535): -*- RILContentHelper: Received message 'RIL:VoicemailNotification': {"clientId":0,"data":{"active":true,"discard":true,"msgCount":-1,"returnNumber":"666","returnMessage":"Messagerie vocale FreeMobile:\nVous avez 1 nouveau message."}}
I/Gecko   (  637): -*- RILContentHelper: Received message 'RIL:VoicemailNotification': {"clientId":0,"data":{"active":true,"discard":true,"msgCount":-1,"returnNumber":"666","returnMessage":"Messagerie vocale FreeMobile:\nVous avez 1 nouveau message."}}

2. Leave a second message:
I/Gecko   (  535): -*- RILContentHelper: Received message 'RIL:VoicemailNotification': {"clientId":0,"data":{"active":true,"discard":true,"msgCount":-1,"returnNumber":"666","returnMessage":"Messagerie vocale FreeMobile:\nVous avez 2 nouveaux messages."}}
I/Gecko   (  637): -*- RILContentHelper: Received message 'RIL:VoicemailNotification': {"clientId":0,"data":{"active":true,"discard":true,"msgCount":-1,"returnNumber":"666","returnMessage":"Messagerie vocale FreeMobile:\nVous avez 2 nouveaux messages."}}

3. Listen to one message, nothing happens.

4. Listen to the second message, we get:
I/Gecko   (  535): -*- RILContentHelper: Received message 'RIL:VoicemailNotification': {"clientId":0,"data":{"active":false,"discard":true,"msgCount":0,"returnNumber":"666","returnMessage":null}}
I/Gecko   (  637): -*- RILContentHelper: Received message 'RIL:VoicemailNotification': {"clientId":0,"data":{"active":false,"discard":true,"msgCount":0,"returnNumber":"666","returnMessage":null}}

Two things should be noted: the 'active' field is it to indicate whether we should or not display the voicemail indicator. We have msgCount -1 when some messages are left (this is a legit case) and it's becoming 0 when we have no notification to show.

So the current code in system app should be modified this way to handle properly this case:
- listen for 'statuschanged' event on mozVoicemail
- issue a first notification if status active payload is true
- upon further voicemailnotification events, we should *update* the notification
- when we receive a notification with a status active payload to false, then we can remove the notification.

Part of this work implies that we migrate this code to the new Notification API. As far as understand, persistence of the notification would rely on bug 768441 fixed.
Status: NEW → ASSIGNED
(Assignee)

Comment 27

4 years ago
I fear that the notification disappears on tap because of bug 890440.
Depends on: 890440
(In reply to Alexandre LISSY :gerard-majax from comment #26)
> Part of this work implies that we migrate this code to the new Notification
> API. As far as understand, persistence of the notification would rely on bug
> 768441 fixed.
Can you test with https://bugzilla.mozilla.org/show_bug.cgi?id=768441#c1 ? it means that the part needed from that bug is covered in Com RIL. Does it make sense?
(Assignee)

Comment 29

4 years ago
I have a patch that somehow works: I don't understand why tapping on the notification does not trigger the dialer. But the notification now disappears correctly when we receive the sms message.
(Assignee)

Comment 30

4 years ago
(In reply to Beatriz Rodríguez [:brg] from comment #28)
> (In reply to Alexandre LISSY :gerard-majax from comment #26)
> > Part of this work implies that we migrate this code to the new Notification
> > API. As far as understand, persistence of the notification would rely on bug
> > 768441 fixed.
> Can you test with https://bugzilla.mozilla.org/show_bug.cgi?id=768441#c1 ?
> it means that the part needed from that bug is covered in Com RIL. Does it
> make sense?

No, that was a blind guess while searching. I think we only need this patch for the persistence of those notification.
Hi,

Here is my understanding of the voicemail notification for your reference compared to other reference implementation:
(MWI means Message Waiting Indicator)
1. When the device receives MWI with flag set to ON, device has to display an voicemail indicator to allow user to access the voicemail.
2. The voicemail indicator shall still be displayed after user clicking it and shall not be clear manually by the user from the system tray.
3. Once there is no unread voicemail in the voicemail message box, the SMSC will send another MWI with  flag set to OFF for the device to clear the voicemail indicator from the system tray.
4. The bug addressed in bug#768441 is more related to how to store this MWI status into EF_MWIS in (U)SIM. Then, when user takes his/her (U)SIM to other devices, he/she can still be aware of his MWI status if other devices also support to read the EF_MWIS in (U)SIM.

Hence, the problem to be addressed here shall be more related to:
1. The voicemail indicator is not allowed to be removed manually by the user.
2. The voicemail indicator will be removed once there is no unread message in the network and the device receives a MWI with clear flag in it.
This is more relied on how Voicemail Indicator is implemented mentioned in comment#24, comment#26.

Hope this information is helpful to you to clarify the problem.

Regards,
Bevis Tseng
(Assignee)

Comment 32

4 years ago
I'm seeing this exception in logcat when the notification is sent/tapped:

I/Gecko   (  111): *************************
I/Gecko   (  111): A coding exception was thrown in a Promise resolution callback.
I/Gecko   (  111): 
I/Gecko   (  111): Full message: TypeError: this.byTag[origin] is undefined
I/Gecko   (  111): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
I/Gecko   (  111): Full stack: NotificationDB.taskSave@resource://gre/modules/NotificationDB.jsm:236
I/Gecko   (  111): NotificationDB.runNextTask/<@resource://gre/modules/NotificationDB.jsm:204
I/Gecko   (  111): onSuccess@resource://gre/modules/NotificationDB.jsm:61
I/Gecko   (  111): Handler.prototype.process@resource://gre/modules/Promise.jsm:767
I/Gecko   (  111): this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm:531
I/Gecko   (  111): 
I/Gecko   (  111): *************************
(Assignee)

Comment 33

4 years ago
Created attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

Current WIP pull request. Not yet ready.
(Assignee)

Comment 34

4 years ago
Updated PR: as far as I can say, we now have the expected behavior. When the carrier sends a SMS notifying of voicemail status, we have update it correctly:
 - if we have no notification, we add one
 - if we have a notification, we should be updating it correctly
 - if we have a notification and no more voicemail then we remove it

Travis is green for this PR, https://travis-ci.org/mozilla-b2g/gaia/builds/15399815 so it should be ready to be reviewed, although I'm not sure yet we might not need some more integration tests.
(Assignee)

Comment 35

4 years ago
Comment on attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

Tim, you have been chosen as a reviewer for this :)

Please note that I'm not sure where to handle properly the case of detecting whether a notification has been sent from the new API or from the old one ; I'm checking the format of the notification's id, as documented in bug 890440. But since bug 890440 is not a 1.3 blocker for now, and that it's scope is about *removing* the hacks, I'm not sure that this detection have to be done in this bug.

That's why, for now, I've included it on the present PR: we need this for the current bug.
Attachment #8347193 - Flags: review?(timdream)

Updated

4 years ago
Duplicate of this bug: 949951
Comment on attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

Alive would be the better reviewer for this bug.
Attachment #8347193 - Flags: review?(timdream) → review?(alive)
Comment on attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

Feedback from Julien since he is taking care of notification.
Attachment #8347193 - Flags: review?(alive)
Attachment #8347193 - Flags: review+
Attachment #8347193 - Flags: feedback?(felash)
Comment on attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

I like the general solution, so I'm giving a feedback+.

But:
* there is a bug
* you need unit tests for the notification code, that would have shown the bug
* I think the notification part should move to its own patch for bug 890440
Attachment #8347193 - Flags: feedback?(felash) → feedback+
Comment on attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

I take the liberty to add r- because of the remaining bug.

For the other comments, since alive gave r+, you can handle them or not, as you wish.
Attachment #8347193 - Flags: review-
(Assignee)

Comment 41

4 years ago
I'm really sorry to have let such a bad code slip through my hands, it looks like I sent an invalid branch :(

Thanks for catching this.

I've opened a PR for moving the old API to another bug, it's in https://github.com/mozilla-b2g/gaia/pull/14719
(Assignee)

Comment 42

4 years ago
I just merged the fix that disables workarounds for new notification in bug 890440. Retriggering travis for this PR.
(Assignee)

Comment 43

4 years ago
Green travis now https://travis-ci.org/mozilla-b2g/gaia/builds/15592843

Julien, do you mind having another look now ?
Flags: needinfo?(felash)
This is only voicemail, I know nothing about this piece of code, so I won't say anything... except I'm still sad there is no test for this ;) but maybe it's not necessary.
Flags: needinfo?(felash)
(Assignee)

Comment 45

4 years ago
(In reply to Julien Wajsberg [:julienw] from comment #44)
> This is only voicemail, I know nothing about this piece of code, so I won't
> say anything... except I'm still sad there is no test for this ;) but maybe
> it's not necessary.

Well, after thinking about, I came down to a unit test, not perfect but that should help. Maybe we would need some integration tests here too.
(Assignee)

Comment 46

4 years ago
Comment on attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

Julien, I added two unit tests, can you have a look ? Thanks :)
Attachment #8347193 - Flags: review?(felash)
Attachment #8347193 - Flags: review-
Attachment #8347193 - Flags: feedback+
Comment on attachment 8347193 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14648

r=me for the test parts with the changes that I asked on the pull request

Thanks a lot for taking the time to write tests!

please merge with a green Travis :)
Attachment #8347193 - Flags: review?(felash) → review+
(Assignee)

Comment 48

4 years ago
(In reply to Julien Wajsberg [:julienw] from comment #47)
> Comment on attachment 8347193 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/14648
> 
> r=me for the test parts with the changes that I asked on the pull request
> 
> Thanks a lot for taking the time to write tests!
> 
> please merge with a green Travis :)

Thanks, I'm merging your comments and waiting for travis :)
(Assignee)

Comment 49

4 years ago
Travis failing on one Persona gaia ui test ... https://travis-ci.org/mozilla-b2g/gaia/builds/15705407 everything else is green.
(Assignee)

Comment 50

4 years ago
Green travis at https://travis-ci.org/mozilla-b2g/gaia/builds/15705407, I'll merge it tomorrow :)
(Assignee)

Comment 51

4 years ago
https://github.com/mozilla-b2g/gaia/commit/024357ef445fc52f84283897203386430ba4dc0d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme

Comment 52

4 years ago
Bug is Verified fixed in Buri 1.4

Environmental Variables:
Device: Buri 1.4 Moz Ril
BuildID: 20140102040201
Gaia: 67a82f88da231969efa4d22df9fb946abf2cf4df
Gecko: 540d85f60c57
Version: 29.0a1
Keywords: verifyme

Updated

4 years ago
Status: RESOLVED → VERIFIED
John, could you please help with the uplift to branch 1.3?
Thanks in advance.
Beatriz.
Flags: needinfo?(jhford)
status-b2g-v1.3: --- → affected
[v1.3 eb40949] Merge pull request #14648 from lissyx/bug910999
status-b2g-v1.3: affected → fixed
Flags: needinfo?(jhford)
Depends on: 977593
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed

Updated

4 years ago
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.