917954 - [B2G][Buri][SMS] Service currently unvailable error message is prompted when sending a sms to an invalid number

Status: RESOLVED DUPLICATE of bug 928330

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

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: Screenshot of the error message

Description:
The user is prompted "Service currently unavailable" when sending a text to an invalid number.

Repro Steps:
1) Updated to Buri Build ID: 20130916040205
2) Open the "Messaging" app.
3) Tap the "Compose New Message" icon.
4) In the "To" text field, enter in an invalid number. Such as (465665676567).
5) Type text into the "Message" text field and tap the "Send" button.

Actual:
The user is prompted with the error message "Service currently unavailable Message will automatically be sent once service is available." when sending a sms to an invalid number.

Expected:
The user is prompted to "Please check the number and try again." for example.

Environmental Variables
Gecko: http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Platform Version: 26.0a1

Notes:
Repro frequency: (100%)
Test Suite Name: (SMS)
See attached: (screenshot)

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: Logcat

Comment 2

[Lucas Aliaga]

Found a window, but we're seeing some problems from past builds that didn't exist before. Currently 6/24 Buri 1.2 was when it was working, then SMS was broken through to 7/3. 7/4 build is able to "send" a text and the issue repros.

- 6/24 Buri 1.2 Working -
Build ID: 20130624031223
Gecko: http://hg.mozilla.org/mozilla-central/rev/76820c6dff7b
Gaia: 59c7f4a59157a0bc1c45d705294395f988c509b2
Platform Version: 24.0a1

- 7/4 Buri 1.2 Broken -
Build ID: 20130704030830
Gecko: http://hg.mozilla.org/mozilla-central/rev/85d75ed04851
Gaia: 3f51aa0913ad86b50abd97d2b25e07b59677708c
Platform Version: 25.0a1

Comment 3

[Julien Wajsberg [:julienw]]

This is not a regression, this is basically like this since the start.

I proposed something in bug 823392 comment 13, and was accepted by Ayman in bug 823392 comment 14, but was somewhat rejected later in the discussion. There was a general agreement to not validate the number before sending though.

My plan was:
* send the recipient
* if there is a general error (service currently unavailable), then we do a sanity check on the recipients and display a non-generic error.

If we're ok with this plan, at least as a temporary solution that is better than a statu quo, then we can move forward. Ayman, what do you think ?

Comment 4

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> This is not a regression, this is basically like this since the start.
> 
> I proposed something in bug 823392 comment 13, and was accepted by Ayman in
> bug 823392 comment 14, but was somewhat rejected later in the discussion.
> There was a general agreement to not validate the number before sending
> though.
> 
> My plan was:
> * send the recipient
> * if there is a general error (service currently unavailable), then we do a
> sanity check on the recipients and display a non-generic error.
> 
> If we're ok with this plan, at least as a temporary solution that is better
> than a statu quo, then we can move forward. Ayman, what do you think ?

I think that is fine for a temporary solution and certainly improves the users understanding about what the actual problem is.

Comment 5

[Julien Wajsberg [:julienw]]

BTW it occurs to me that "Message will automatically be sent once service is available." is displayed in situations that it's not true.

Comment 6

[Julien Wajsberg [:julienw]]

Not a blocker for 1.2 after all, this is not a regression, and we don't want to add new strings at this point.

Let's make it a blocker for 1.3 though, as the message is definitely not right.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Not a blocker for 1.2 after all, this is not a regression, and we don't want
> to add new strings at this point.
> 
> Let's make it a blocker for 1.3 though, as the message is definitely not
> right.

No. comment 2 shows this working on a 6/24 build, which implies that this was working previously.

Comment 8

[Julien Wajsberg [:julienw]]

comment 2 is not clear to me.

please test in 1.1.

Comment 9

[Julien Wajsberg [:julienw]]

Maybe in 1.1 we didn't show any error message at all, but rather we were only showing the error icon.

Then is it really a regression ?

Moreover, do we want new strings in 1.2 at this point ?

Comment 10

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Maybe in 1.1 we didn't show any error message at all, but rather we were
> only showing the error icon.
> 
> Then is it really a regression ?
> 
> Moreover, do we want new strings in 1.2 at this point ?

I need actual proof that this isn't a regression, otherwise, I tend to think the arguments presented here are not right.

Comment 11

[Lucas Aliaga]

Issue occurs on Buri and Leo 1.1. The "service unavailable" message still appears after sending an SMS to an invalid number. 

Not a regression it seems.

- Buri 1.1 repros -
Build ID: 20130925041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/18175c7fdaa6
Gaia: 6d9eec501a209bea945c6f841400ec0a75fac11d
Platform Version: 18.1

- Leo 1.1 repros -
Build ID: 20130925041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/18175c7fdaa6
Gaia: 6d9eec501a209bea945c6f841400ec0a75fac11d
Platform Version: 18.1

Comment 12

[Jason Smith [:jsmith]]

Okay. That gives us concrete evidence this isn't a regression. Clearing flags.

Comment 13

[Kodanda rami reddy V]

hi julien,

Please assign this issue to me and provide info to proced furthur.

Comment 14

[Kodanda rami reddy V]

My observations:

Errors like 'NoSignalError', 'NotFoundError', 'UnknownError', 'InternalError','InvalidAddressError' were grouped to show common error message. My Idea is to have different error messages for each type. With log information an "UnknowError" is shown even for "InvalidAddressError" type.

Comment 15

[Julien Wajsberg [:julienw]]

Hi,

first you need to make sure that "InvalidAddressError" is actually sent in that case.

If that's what's happen, then I think this is good.

Otherwise, please see my comment 3.

Thanks !

Comment 16

[Kodanda rami reddy V]

hi,

Errors like 'NoSignalError', 'NotFoundError', 'UnknownError', 'InternalError','InvalidAddressError' were grouped to show common error message.
Now the general error message "Service currently unavailable" is used for error type 'NoSignalError' and new error messages are defined for each error type.

When an address is like "&%&", gives an 'InvalidAddressError' and when an address is of random number or unknown name say (112345676543 or gfds) gives 'UnknownError' which are grouped to shown the error message "Message not sent successfully" as shown in attachemnet.

When a file is removed while a mms is still in sending mode, gives 'NotFoundError', for this a seperate error message is used.

Please review the strings used for this error messages.

Comment 17

[Kodanda rami reddy V]

Attachment: Invalid_address.png

Comment 18

[Kodanda rami reddy V]

Attachment: NotFoundError.png

Comment 19

[Kodanda rami reddy V]

Attachment: pull request 13064

Comment 21

[Julien Wajsberg [:julienw]]

Gene, here is the list of errors that we know in Gaia:

NoSimCardError
RadioDisabledError
FdnCheckError
NoSignalError
NotFoundError
InternalError
UnknownError
InvalidAddressError

Could you please explicit the causes of all these errors ? This will be useful to display a correct error message to the user.

Also, the current generic message is "Message will automatically be sent once service is available." which I think is wrong: we won't send automatically the message when the service is available. Can you tell us if this message is wrong as I think, or if _I_ am wrong here ? :)

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 821637 [details] [review]
pull request 13064

please request review again once you've updated your pull request

thanks !

Comment 23

[Gene Lian (I already quit Mozilla)]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Gene, here is the list of errors that we know in Gaia:
> 
> NoSimCardError
> RadioDisabledError
> FdnCheckError
> NoSignalError
> NotFoundError
> InternalError
> UnknownError
> InvalidAddressError
> 
> Could you please explicit the causes of all these errors ? This will be
> useful to display a correct error message to the user.

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=891855#c1.

> 
> Also, the current generic message is "Message will automatically be sent
> once service is available." which I think is wrong: we won't send
> automatically the message when the service is available. Can you tell us if
> this message is wrong as I think, or if _I_ am wrong here ? :)

Yes! That message looks very weird. We won't do the resend unless users send them manually.

Comment 24

[Julien Wajsberg [:julienw]]

Thanks. FYI this message is here since 1.0 ;-)

Thanks also for the pointer to bug 891855, I completely forgot about this, and it seems we didn't filed a bug to make it correct as bug 891855 didn't introduce the new string.


Kodanda, please see then the following informations for the correct strings:

    case "InvalidAddressError":
      messageTitle = 'Invalid number';
      messageBody = 'Make sure the mobile number you're sending to is valid.';
      buttonLabel = 'OK';
      break;
    case "NoSimCardError":
      messageTitle = 'Missing SIM card';
      messageBody = 'Insert a valid SIM card to continue.';
      buttonLabel = 'OK';
      break;
    case "RadioDisabledError":
      messageTitle = 'Airplane mode activated';
      messageBody = 'Turn off airplane mode to send messages.';
      buttonLabel = 'OK';
      break;
    case "NotFoundError":
      messageTitle = 'Message not found';
      messageBody = 'The message you're looking for is no longer available.';
      buttonLabel = 'OK';
      break;
    case "NoSignalError":
      messageTitle = 'No network coverage';
      messageBody = 'Make sure you have a mobile signal and try again.';
      buttonLabel = 'OK';
      break;
    case "UnknownError":
    case "InternalError":
      /* falls through */
    default:
      messageTitle = 'Message not sent';
      messageBody = 'There was a problem sending the message. Please try again.';
      buttonLabel = 'OK';

+ keep the FdnCheckError strings as it was before.

Thanks to all !

Comment 25

[Kodanda rami reddy V]

Attachment: pull request 13064

Comment 26

[Kodanda rami reddy V]

hi julien,

Please find the pull request 13064, modified as per the comment 24.

Regards,
Kodanda.

Comment 27

[Kodanda rami reddy V]

This pull request includes the changes in the test cases.

Comment 28

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823878 [details] [review]
pull request 13064

Looks nice to me. However, I'm not neither UX nor UI reviewers. Please get Julien's review as well to land. Thanks!

Btw, Bug 932201 is under way landing, which provides another new "NonActiveSimCardError". Do you want to solve that as well or fire another bug?

That string stands for something like:

"Please switch the default SIM to download MMS"

Since you're handling sending failure, not retrieving failure. I'd suggest you can directly put that in the default case.

Comment 29

[Kodanda rami reddy V]

Hi Gene Lian,

Please fire another bug, so that it would be easy to track. If possible please assign the new bug to me.

Regards
Ramu.

Comment 30

[Julien Wajsberg [:julienw]]

Gene, I'd like to know exactly when "InvalidAddressError" is sent. Is it when the network rejects the phone number ? Can we use this to mark a recipient as invalid ?

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 823878 [details] [review]
pull request 13064

Hi and thanks for your patch!

You didn't follow all comments I left in my first review, therefore I've put it again as a comment. Please follow it, change the l10n key, and request review again.

Thanks again!

Comment 32

[Julien Wajsberg [:julienw]]

Hey Kodandi,

Steve needs this for bug 928330 and therefore he'll include your in-progress work in his patch there, so I'm duping this bug there.

Thanks again for your work!