874043 - [sms] When the first message in a thread has an error due to flight mode, we don't display the "flight mode" alert dialog

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

STR:
* turn on flight mode
* send a SMS to a new number

Expected:
* the message is in error and the "flight mode" alert dialog should be displayed.

Actual:
* the messages is in error, but the "flight mode" alert dialog is not displayed.
Note that it is when you send a SMS to a number that already has a thread.

I think this is a regression, but I'm not sure. The bug happens in v1.1 for sure.

Comment 1

[Michał Budzyński (:michalbe)]

On the first try to send an SMS in airplane mode we receive an error:
at app://sms.gaiamobile.org/js/message_manager.js:339 in onerror: Reading the database. Error: InternalError

and this._mozMobileMessage never fires 'failed' event. Looks like a platform issue.

Comment 2

[Julien Wajsberg [:julienw]]

(I know I've seen the same error in another bug but I can't find it)

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> (I know I've seen the same error in another bug but I can't find it)

Do you mean bug 874186?  However bug 874186 requires we have no thread for the recipient yet.

Comment 4

[Julien Wajsberg [:julienw]]

Vicamo> this bug needs a new recipient as well :)

Yep, that was bug 874186, but here we still reproduce it afaik.

Comment 5

[Julien Wajsberg [:julienw]]

Just tried it again with a recent b2g18:

* we don't have the logcat error anymore
* but we still have the bug

Michal, could you check with a newer b2g18 if we get the 'failed' event ?

Comment 6

[Michał Budzyński (:michalbe)]

Nothing changed - the bug still occurs, the event doesn't fire and the error is still there SOMETIMES.

Comment 7

[Michał Budzyński (:michalbe)]

Attachment: patch

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 755612 [details]
patch

reviewed on github, mostly nits

Comment 9

[Borja Salguero [:borjasalguero]]

Before merging, I would like to confirm with Ayman. 

@Ayman: Do we need this alert in airplane mode? Thanks!

Comment 10

[Julien Wajsberg [:julienw]]

If we don't need the alert anymore, I think part of the patch is still necessary: we can't add the class on a dom node which doesn't exist :)

Comment 11

[Julien Wajsberg [:julienw]]

To be very clear, we have this alert at least since I started to work on the Sms app :) I'd be happy to remove this code (I love removing code ;) ) but this is here since ages. It was failing in this specific case only.

Comment 12

[ayman maat :maat]

(In reply to Borja Salguero [:borjasalguero] from comment #9)
> Before merging, I would like to confirm with Ayman. 
> 
> @Ayman: Do we need this alert in airplane mode? Thanks!

Hey borja, yes we do need this alert... original plan was to provide the end user a direct rout from the alter to turn flight mode off... but that apparently is technically not possible at the moment.. but we still keep the alert and improve its functionality when we can.

Comment 13

[ayman maat :maat]

...in my personal opinion, the alerts articulation is a little clumsy. We dont need to block the user from continuing on their path by using a full screen message to inform them that they are in flight mode they must cancel in order to proceed. We do enough notification within in the message thread itself with the error state of the message module itself. We should have a temporary message that informs but does not intrude like what we display when the users moves between SMS packets... However the full screen message is in line with building blocks, and we were blocked from implementing a temporary message at the time the flight mode message was added because there was no such building block.

Comment 14

[Julien Wajsberg [:julienw]]

I agree that the full screen message is most useful if we can do some action on it. Maybe we should check for the flight mode setting earlier and display a banner message asap in the Sms app.

But let's do that in another bug if you will ?

Comment 15

[ayman maat :maat]

> But let's do that in another bug if you will ?

Totally! ...comment 13 was just a stream of consciousness as i was looking at the UX ;)

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 755612 [details]
patch

r=me

cheers !

Comment 17

[Michał Budzyński (:michalbe)]

Thanks!
Merged: https://github.com/mozilla-b2g/gaia/commit/2c7fc2127478cd881f3a9f948dfa02d0f7684d29

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

If this is intented for uplift to v1.1 please nominate and fill out the approval form with risk evaluation, testing done, regressing bug, etc.

Comment 19

[Maria Angeles Oteo (:oteo)]

It's strange, I am not reproducing the bug in v1-train but the patch is not uplifted in that branch. Requesting QA to confirm it.

Comment 20

[Jeni]

Unable to repro issue on

Unagi Build ID: 20130605070207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/09dc1ae3b1b5
Gaia: 92a6e36957145cdb2ac8867e5cdba8ecf12308fc 

When sending a sms message with airplane mode enabled message is now displayed saying 'Airplane Mode Activated: To send a message; first disable airplane mode"

Comment 21

[bov]

Unagi Build ID:Gecko-363855a.Gaia-92a6e36 (20130605180132)
Gaia: 92a6e36957145cdb2ac8867e5cdba8ecf12308fc 

STEPS (Fisrt SMS)
1.Turn on flight mode
2.Send a SMS to a new number (not in contacts). It must be the first SMS, there is no previous thread.
Expected:
* The message is in error and the "flight mode" alert dialog screen should be displayed.
Actual:
* The SMS is in error, but the "flight mode" alert dialog screen is not displayed. No message informs about what's happening.
Note that first time I tried it, an alert message "Airplane mode on" appeared in the thread view (see attachment 2013 [details]-06-05-20-57-34), but did not appear in the following "First SMS" tests (see attachment 2013 [details]-06-05-21-38-52), .

Then...

STEPS (Existing SMS thread)
1.Turn on flight mode
2.Send a SMS to a new number (not in contacts)that already has a SMS thread.
Expected:
* The message is in error and the "flight mode" alert dialog screen should be displayed.
Actual:
* The SMS is in error, and the "flight mode" alert dialog is displayed showing 'Airplane Mode Activated: To send a message; first disable airplane mode".

Comment 22

[bov]

Attachment: First SMS (Test 1)

Comment 23

[bov]

Attachment: First SMS (Other tests)

Comment 24

[Fernando Campo (:fcampo)]

Attachment: Approval request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: 
The user doesn't receive a warning when trying to send a SMS on airplane mode to a new number
Testing completed: 
Risk to taking this patch (and alternatives if risky): In my opinion, risk is low-medium, as we mess with the thread ui and warning messages, but I'd like to ask Michael for his opinion, as he made the code changes and knows it better
String or UUID changes made by this patch:

Comment 25

[Corey Frang [:gnarf]]

this is blocking a blocker from merging clean so I'm picking it onto the train:

uplifted master: 747f412b370039569e37161e43185427ad20fbb8
to v1-train: 8eb11d8de3fa994e6db893a688df4b4d1dabb66d

Comment 26

[Fernando Campo (:fcampo)]

Cleaning flags as it's solved and uplifted

Comment 27

[Fernando Campo (:fcampo)]

Comment on attachment 759026 [details]
Approval request

This has been already uplifted

Comment 28

[Julien Wajsberg [:julienw]]

Note for next time: Corey, you're supposed to wait for approval before uplifting, even if it's blocking another uplift. Also, having conflicts is not a good reason to uplift, as you can resolve conflict by hand.

(but if you're confident that the conflicting bug will get approved, then you can just wait of course).