934354 - [WAP push][CP] Pressing home key or opening other apps will close client provision message directly without any protections like pressing "x" button does(warning dialog shows).

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Enpei]

Attachment: screen shot

Based on current design, user will get a warning message(screen shot attached) when press "x" button of a client provision message. Since pressing home key will exit the message as well, I thing the behavior should sync, which mean when users press home key, they shall get the same warning message as well.

* Build Number  
Buri              
Gaia:     00cd7534afb8d727531c7a939e54e382b3be713a
Gecko:    http://hg.mozilla.org/mozilla-central/rev/47c8e9b16918
BuildID   20131103040200
Version   28.0a1

* Reproduce Steps
1. Send a WAP Client Provision message to DUT
2. Open the message.
3. Press home key.

* Expected Result
A warning message (see attached pic) should pop up.

* Actual Result
Exit the message directly and won't be able to open it again. Unlike press "x" button behavior.

* Occurrence rate
100%

Comment 1

[Enpei]

Not just home key, any way that can exit(home key, screen time out and unlock screen, incoming call, etc.) the message is not protected by the warning message. Users need to request the CP message again which is not convenient.

Comment 2

[Enpei]

Change block flag to koi? since WAP CP (bug 917312, koi+) is for v1.2.

Comment 3

[Kai-Chih Hu [:khu]]

Joe, can Comms team work on it and land it to v1.3? Thanks.

Comment 4

[David Palomino [:dpalomino]]

Hi, 

jaoo, do you think this is a blocking feature for koi? It sounds like something that we should have implemented for koi... wdyt?

Thanks!
David

Comment 5

[David Palomino [:dpalomino]]

Hi, 

After some internal discussions, we think it could be a bit late to include this change for koi now. As it is an important issue, we suggest to keep the focus on it, and try at least to have this for 1.3. 

Changing nominating accordingly to 1.3? and clearing ni from jaoo. 

Thanks!
David

Comment 6

[Jason Smith [:jsmith]]

Not a blocker - minor UX interaction bug.

Comment 7

[Enpei]

Hi Jason,

The reason I mark it as 1.3? is because since there is a warning mechanism to inform user while press "x" button, other ways of exit the CP message should also be considered. Or it does not mean much to have this function. So do you still think it's just minor UX bug?

Comment 8

[Jason Smith [:jsmith]]

(In reply to Enpei from comment #7)
> Hi Jason,
> 
> The reason I mark it as 1.3? is because since there is a warning mechanism
> to inform user while press "x" button, other ways of exit the CP message
> should also be considered. Or it does not mean much to have this function.
> So do you still think it's just minor UX bug?

I'm going to send this over to UX to see what they think.

I originally thought this was minor because 1) this can be easily worked around by getting another CP message, 2) this follows in alignment with existing design of how the home button works with modal prompts.

Comment 9

[Stephany Wilkes]

Flagging Francis on System Front-end.

Comment 10

[Enpei]

I met a real case which makes me want to re-nominate this bug again.

1. I got 2 messages from local carrier after I inserted TWN SIM. First one is text message, second one is CP message.
2. I dropped down utility try, the top most message is the CP message because it arrived later than text message.
3. I selected CP first just because it's the top most message. The CP require USERPIN which is in text message.
4. So I went to utility try again and opened text message to read the PIN code. 
5. When I pressed back key and wanted to enter the PIN in CP message, the message is closed due to step 4. 

This is a bad user experience but easily to be caught by users.

Comment 11

[Francis Djabri [:djabber]]

Personally, I don't feel we should throw up a warning message when the user tasks away. Home should take the user home - this a core principle and we shouldn't mess with that. Showing the warning message won't help for the timeout case, nor the incoming call, and in any case, the warning message will not help the user complete the task that you mention in Comment #10. 

So I think a better approach would be to give the user some means to get back to the message after they have tasked away. Much of this will be solved as we move to a sheets model, but until then, our options are limited. We could keep the message as an open task, but getting access to the task manager is fairly hidden. 

My suggestion would be to keep the notification for the CP message in the notification drawer open until the user completes the action or dismisses the message.

Comment 12

[Jason Smith [:jsmith]]

Not a system bug - moving into the Gaia component, until we get a bugzilla component created for WAP Push.

Comment 13

[Enpei]

update summary and expectation of this bug.

* Expected Result
Either a warning dialog shows or keep CP messages in utility tray as long as user does not press "x" key.

Comment 14

[Kai-Chih Hu [:khu]]

Joe, can you help to triage?

Comment 15

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ to work on this in convergence

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

Go for it.

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

Gabriele, this WIP patch implements the idea that Francis commented in comment #11. It basically stores the message into the database again and keeps the notification (well, the notification is sent again). Would you mind to take a look at it and give me some feedback please? Thanks!

Note: the patch is still a WIP patch so you will see code I've used to test the development. You can use it if you need. If you are prompted for a PIN type '0000' as the PIN.

Comment 18

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8360418 [details] [diff] [review]
v1

This is a good starting point but I'd like to do things a little bit differently if it's possible: Alexandre is about to land bug 938540 where he converted the app to use the new notification API. This gives us greater control over the notifications so I'd like you to explore the option described in comment 11 of leaving the notification there until the message has been processed:

- When opening a CP message do not remove the notification, leave it there, this is fortunately possible with the new API. Also do not remove the message from the database, we can modify the retrieve method to special-case those messages.

- Change the visibility handler to not close the application when going back to the homescreen if a CP message is being interacted with. This way the user can either tap the notification again to get back into the application (we'll have to modify the application onclick event to accomodate for this) or get at it from the cards view (which shouldn't require any changes).

- Once the message has been fully processed remove it from the database and remove the notification.

All the while if the user taps another message notification I'd leave the flow as-is: the other message is shown and tapping again the CP message notification will start the interaction again from the start. 

Does this sound reasonable?

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Gabriele Svelto [:gsvelto] from comment #18)
> Comment on attachment 8360418 [details] [diff] [review]
> v1

Thanks for the feedback Gabriele.

> This is a good starting point but I'd like to do things a little bit
> differently if it's possible: Alexandre is about to land bug 938540 where he
> converted the app to use the new notification API.

The thing is that this bug is 1.3+ and I don't know the ETA of bug 938540 and even it will be flagged as 1.3+ as It will be a dependency for this bug. Should we go ahead with the changes on top of bug 938540 then?

Comment 20

[Gabriele Svelto [:gsvelto]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #19)
> The thing is that this bug is 1.3+ and I don't know the ETA of bug 938540
> and even it will be flagged as 1.3+ as It will be a dependency for this bug.

Nasty, I hadn't paid attention to the flag.

> Should we go ahead with the changes on top of bug 938540 then?

No, that wouldn't work as I don't think we're going to uplift any patch for the new notification API in 1.3 as they've got Gecko dependencies too so we'd have to uplift them too. Let's do it this way then. Go ahead with your patch but let's make it a 1.3-only patch and then we open a follow up to do a cleaner fix in master on top of the new notification API changes.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

This is the 1.3-only patch we commented about in comment #20. I've test it and the unit tests pass. Gabriele, could you take a look at it please? Thanks.

Just a comment for the review. I've seen the logic in the wappush.js file for storing the messages wait until all the messages have been stored before closing the app. Since I store here the message should we wait until the message has been stored before actually close the app as well? Please, let me know. Thanks.

Comment 22

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8361129 [details] [diff] [review]
v2

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

LGTM with a small addition to make 100% sure we don't accidentally lose the message we're saving while closing the app. In the cpsh_onVisibilityChange() function before saving the message in the database increment the WapPushManager._pendingMessages field. Later on, after you've sent the new notification, call WapPushManager.finish() which will decrement that counter and ensure that the application closes correctly. This way we should be 100% sure that we don't drop any messages. Feel free to land with this change; thanks for your work on this and for testing it on a real device.

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

Attachment: [1.3-only] Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/15206

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

Landed the v1.3-only patch at https://github.com/mozilla-b2g/gaia/commit/3da35b2c08646b77e27e7998bbc263d6971ef0d5

There was one UI test failing but already present and totally unrelated with this patch.

Comment 25

[Enpei]

I've verified the bug on Buri
Gaia      744fb691c2b2a25a07c5d19fabf5748ae9aba4d9
Gecko     http://hg.mozilla.org/releases/mozilla-aurora/rev/71a8786c3815
BuildID   20140122004001
Version   28.0a2

And current behavior is that if user presses home key, the notification will be displayed on status bar again with audio indication which is like a new message is arrived, yet it says "Configuration message not processed...".

I am okay with the fix, but not sure if this needs UX to review it as well. 

Hi Joe, do you think this bug should be reviewed by UX?

Comment 26

[Joe Cheng [:jcheng] (please needinfo)]

sure, lets have UX to have a quick look
thanks

Comment 27

[Wei Gao (Spreadtrum)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #21)
> Created attachment 8361129 [details] [diff] [review]
> v2
> 
> This is the 1.3-only patch we commented about in comment #20. I've test it
> and the unit tests pass. Gabriele, could you take a look at it please?
> Thanks.
> 
> Just a comment for the review. I've seen the logic in the wappush.js file
> for storing the messages wait until all the messages have been stored before
> closing the app. Since I store here the message should we wait until the
> message has been stored before actually close the app as well? Please, let
> me know. Thanks.

Hi Jaoo

Can your patch be merged into v1.4?
I think v1.4 needs your patch also.

Thanks a lot.

Comment 28

[Gabriele Svelto [:gsvelto]]

(In reply to Wei Gao (Spreadtrum) from comment #27)
> Can your patch be merged into v1.4?
> I think v1.4 needs your patch also.

The patch here was meant for v1.3t only (see comment 20). For a proper fix see bug 1022552, you might want to nom that one for 1.4+ as that patch should apply cleanly there.

Comment 29

[Wei Gao (Spreadtrum)]

(In reply to Gabriele Svelto [:gsvelto] from comment #28)
> The patch here was meant for v1.3t only (see comment 20). For a proper fix
> see bug 1022552, you might want to nom that one for 1.4+ as that patch
> should apply cleanly there.

Oh, yes, I get it.
Thanks for your suggestion.