891855 - [sms][mms] Gaia should handle the error codes properly to pop up reasonable prompt

Status: RESOLVED FIXED

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

Description

[Gene Lian (I already quit Mozilla)]

Please see bug 891756, comment #6.

The Gaia shouldn't pop up a prompt like "Service currently unavailable" to handle "UnknownError". It doesn't sound precise because the service is actually on. In my opinion, Gaia should only pop up such a prompt when handling "NoSignalError".

Bug 891756 is going to support a new error code "InvalidAddressError" to reflect the error when attempting to send SMS/MMS to an invalid address like "&%&*". Gaia also needs to handle that properly.

Comment 1

[Gene Lian (I already quit Mozilla)]

FYI. The following shows the current error codes we have:

  "NoSignalError"
  "NotFoundError"
  "UnknownError"
  "InternalError"
  "NoSimCardError"
  "RadioDisabledError"
  "InvalidAddressError" (bug 891756 is on going)

IMO, if we really need a prompt to notify users, the prompt messages should be described like the following examples:

  case "UnknownError":
  case "InternalError":
    alert("Send failed");
    break;
  case "InvalidAddressError":
    alert("Send failed (phone number is invalid)");
    break;
  case "NoSimCardError":
    alert("Send failed (no SIM card inserted)");
    break;
  case "RadioDisabledError":
    alert("Send failed (airplane mode is on)");
    break;
  case "NotFoundError":
    alert("Record not found");
    break;
  case "NoSignalError":
    alert("Service is currently unavailable");
    break;

Comment 2

[Julien Wajsberg [:julienw]]

Ayman, could you provide us some good text for these cases ?

We need a title and a body.

Comment 3

[bhavana bajaj [:bajaj]]

triage is waiting on resolution for 891756 to make a call here

Comment 4

[Julien Wajsberg [:julienw]]

since we have no default clause in the current switch case, we'll need to land both bugs together.

Please add a default clause when resolving this bug here.

Comment 5

[Tony Chung [:tchung]]

yes wrong strings, but not a shipstopper

Comment 6

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment #2)
> Ayman, could you provide us some good text for these cases ?
> 
> We need a title and a body.

I think this is more a job for Tyler
So NeedInfo to Tyler to provide the copy.

Comment 7

[Gene Lian (I already quit Mozilla)]

Bug 891756 has just been marked just as leo+ which implies this one should also be marked as leo+, because both Gecko and Gaia parts should be fixed and uplifted at the same time.

Note that if the Gecko returns "InvalidAddressError", but the Gaia doesn't handle that properly, we'd get a weird prompt which has no messages. If you guys think it's OK to display wrong messages on the prompt for shipping (comment #5), then I'm OK with it to fix the Gecko part only.

Renominating for leo+ again.

Comment 8

[Julien Wajsberg [:julienw]]

note: for leo, we might as well just add a "default" clause popping up the generic alert box. This would be safe and permit to remove the "late-l10n" keyword.

Comment 9

[Leo]

Discussed with Mozilla and marked it Leo+ as it has land with 891756. - Veeresh

Comment 10

[Fernando Campo (:fcampo)]

Stealing, cause I'm a bad person

Comment 11

[Borja Salguero [:borjasalguero]]

hehehe, all yours Mr. Campo! ;)

Comment 12

[tyler.altes]

If I understand the cases correctly, here are the message I would use in each scenario. My question are about the final two. What does "Record not found" refer to? And what is the scenario that creates the NoSignalError?


case "UnknownError":
case "InternalError":
    alert("There was a problem sending the message. Please try again.");
    break;
  case "InvalidAddressError":
    alert("Make sure the mobile number you're sending to is valid.");
    break;
  case "NoSimCardError":
    alert("Insert a valid SIM card to continue.");
    break;
  case "RadioDisabledError":
    alert("Turn off airplane mode to send messages.");
    break;


?????????
  case "NotFoundError":
    alert("Record not found");
    break;
  case "NoSignalError":
    alert("Service is currently unavailable");
    break;

Comment 13

[Fernando Campo (:fcampo)]

Hi Tyler, 

Current implementation requires a title, body, and acceptance-button content for the message displayed (I think that Gene's comment 1 wasn't based on actual code but more like a general proposal).

So based on existing code and your comment, we would end up with something like this:
    case "UnknownError":
    case "InternalError":
      messageTitle = 'Service currently unavailable';
      messageBody = 'There was a problem sending the message. Please try again.';
      buttonLabel = 'OK';
      break;
    case "InvalidAddressError":
      messageTitle = '???';
      messageBody = 'Make sure the mobile number you're sending to is valid.';
      buttonLabel = 'OK';
      break;
    case "NoSimCardError":
      messageTitle = 'Sim card error';
      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 = '???';
      messageBody = 'Record not found';
      buttonLabel = 'OK';
      break;
    case "NoSignalError":
      messageTitle = '???';
      messageBody = 'Service is currently unavailable';
      buttonLabel = 'OK';
      break;
    default:
      messageTitle = '???';
      messageBody = '???';
      buttonLabel = '???';
      break;

I'm adding default clause per comment 4, to show a general error.

And about the cases, my understanding is that NoSignalError is created when Radio is active (not disabled by user) but phone is not connected to the network (tunnel, area without signal, etc). No idea about the NotFoundError

Comment 14

[tyler.altes]

Thanks Fernando, I've added some of the missing texts below, but I am still unclear on the last three. What makes these messages appear and what can the user do?

1.  case "NotFoundError":
      messageTitle = '???';
      messageBody = 'Record not found';
      buttonLabel = 'OK';
      break;
2.  case "NoSignalError":
      messageTitle = '???';
      messageBody = 'Service is currently unavailable';
      buttonLabel = 'OK';
      break;
3.  default:
      messageTitle = '???';
      messageBody = '???';
      buttonLabel = '???';
      break;




Revised texts:

case "UnknownError":
    case "InternalError":
      messageTitle = 'Message not sent';
      messageBody = 'There was a problem sending the message. Please try again.';
      buttonLabel = 'OK';
      break;
    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;

Comment 15

[Gene Lian (I already quit Mozilla)]

So, the Gaia patch can land first before the Gecko patch at bug 891756. I'll wait on this.

Comment 16

[Fernando Campo (:fcampo)]

As said in comment 13(
> [...] my understanding is that NoSignalError is created when
> Radio is active (not disabled by user) but phone is not connected to the
> network (tunnel, area without signal, etc). [...]

For NotFoundError, maybe Gene can throw some light.
And about the default case, I guess is just for make it more errorproof, when API return a case not in the list (highly improbable). Maybe we could use the same one as UnknownError, as I see there's no specific message? WDYT?

case UnknownError:
case InternalError:
default:
  messageTitle = 'Message not sent';
  messageBody = 'There was a problem sending the message. Please try again.';
  buttonLabel = 'OK';

Comment 17

[Gene Lian (I already quit Mozilla)]

(In reply to Fernando Campo (:fcampo) from comment #16)
> As said in comment 13(
> > [...] my understanding is that NoSignalError is created when
> > Radio is active (not disabled by user) but phone is not connected to the
> > network (tunnel, area without signal, etc). [...]
> 
> For NotFoundError, maybe Gene can throw some light.

The only case of returning NotFoundError when failing to send an message is: deleting the message when it's still under the sending/retrieving processes (i.e., when the sending/retrieving circle is still spinning). It means we cannot update the delivery status for the message since it has been removed from the DB.

I understand it might not be reasonable to tell users "the record is not found" when failing to send in any way, so maybe we can just let it fall through to the default case like "fail to send".

> And about the default case, I guess is just for make it more errorproof,
> when API return a case not in the list (highly improbable). Maybe we could
> use the same one as UnknownError, as I see there's no specific message? WDYT?

Sounds good! We should make all the unrecognizable cases fall through to the UnknownError/InternalError case.

> 
> case UnknownError:
> case InternalError:
> default:
>   messageTitle = 'Message not sent';
>   messageBody = 'There was a problem sending the message. Please try again.';
>   buttonLabel = 'OK';

Comment 18

[Fernando Campo (:fcampo)]

(In reply to Gene Lian [:gene] from comment #17)
> The only case of returning NotFoundError when failing to send an message is:
> deleting the message when it's still under the sending/retrieving processes
> (i.e., when the sending/retrieving circle is still spinning). It means we
> cannot update the delivery status for the message since it has been removed
> from the DB.
> I understand it might not be reasonable to tell users "the record is not found" when failing to send in > any way, so maybe we can just let it fall through to the default case like "fail to send".
 I still think that some specific info is useful, even more when there's only one clear possibility for this to happen. Maybe something like:
case 'NotFoundError':
  messageTitle = 'Message not found';
  messageBody = 'The message is no longer in the database. It might have been deleted';
  buttonLabel = 'OK';

Comment 19

[tyler.altes]

Ok, thanks for all the info guys. Here are the three missing messages:

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 "default":
  messageTitle = 'Message not sent';
  messageBody = 'There was a problem sending the message. Please try again.';
  buttonLabel = 'OK';

Comment 20

[Fernando Campo (:fcampo)]

Attachment: Patch v1

Maybe I messed something with the flags and people, so please correct any if I'm wrong

Comment 21

[tyler.altes]

Looks good.

Comment 22

[Axel Hecht [:Pike]]

Comment on attachment 777173 [details] [diff] [review]
Patch v1

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

I personally disagree with the triage decision in bug 891756, which is easy, because it's not documented at all. I've commented in the bug.

If we end up having to take it, we should try to take as few strings as possible.

Comment 23

[Gene Lian (I already quit Mozilla)]

Comment on attachment 777173 [details] [diff] [review]
Patch v1

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

Looks good to me! However, I'm a Gecko engineer so I might not qualified to review the Gaia patch. Could you please find Borja or someone else to review? Thanks!

::: apps/sms/locales/sms.en-US.properties
@@ +73,4 @@
>  deleteThreads-confirmation2 = Delete selected message threads?
>  noMessages-title            = No messages
>  noMessages-text             = Start communicating now
> +sendGeneralErrorTitle2      = Message not sent

Why do you append "2" to the original names? Can we just keep the original name instead?

Comment 24

[Fernando Campo (:fcampo)]

Comment on attachment 777173 [details] [diff] [review]
Patch v1

Adding borja as reviewer as requested

Comment 25

[Fernando Campo (:fcampo)]

(In reply to Gene Lian [:gene] from comment #23)
> Why do you append "2" to the original names? Can we just keep the original
> name instead?
This is needed when making changes on strings for l10n for the translation team to be properly notified.

(In reply to Axel Hecht [:Pike] from comment #22)
> Comment on attachment 777173 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 777173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I personally disagree with the triage decision in bug 891756, which is easy,
> because it's not documented at all. I've commented in the bug.
> 
> If we end up having to take it, we should try to take as few strings as
> possible.
So I'm gonna wait to see the evolution of bug 891756 and put you as a reviewer then.

Comment 26

[Fernando Campo (:fcampo)]

Attachment: Patch v2 - https://github.com/mozilla-b2g/gaia/pull/11041

Updating patch with tests and including link to PullRequest

Comment 27

[Fernando Campo (:fcampo)]

So just as a reminder, we are waiting for the l10n decisions on bug 891756 (and the consequent review if needed) before merging this one

Comment 28

[Gene Lian (I already quit Mozilla)]

(In reply to Fernando Campo (:fcampo) from comment #27)
> So just as a reminder, we are waiting for the l10n decisions on bug 891756
> (and the consequent review if needed) before merging this one

Hi Fernando, although it's not yet decided to be blocking for v1-train or not, could you please merge this to master first? I'll be after you to ensure compatibility.

Comment 29

[Fernando Campo (:fcampo)]

(In reply to Gene Lian [:gene] from comment #28)
> Hi Fernando, although it's not yet decided to be blocking for v1-train or
> not, could you please merge this to master first? I'll be after you to
> ensure compatibility.

We shouldn't merge anything with changes on l10n till a driver gives a r+ due to the string freeze, and after comment 22 from Axel I doubt we have it. I'm gonna ping him to see what can we do here.

Comment 30

[Axel Hecht [:Pike]]

From a few weeks ago, https://groups.google.com/d/msg/mozilla.dev.gaia/SrFrWrBNl20/scS-Qa1LXl8J, Dietrich posted:

* If there's any possible way you can *not* make any planned string changes, please do everything you can to make that so. 

This patch can get away with just using one generic error for branch, for example.

Technical ways to implement that would be to actually write a special patch for branch, or to land this in two patches on master, one for generic, and a follow-up for detailed messages.

That said, I don't think that blocking on a blocker that blocks without explanation is the right way to change any strings, in particular this late in the schedule.

Comment 31

[Fernando Campo (:fcampo)]

Attachment: Patch v3 - https://github.com/mozilla-b2g/gaia/pull/11075

So following comment 30, new patch version just adding the new error cases, without modifying any string due to the string-freeze.

We should open a followup for the new strings to be added for the specific cases.

Comment 32

[Axel Hecht [:Pike]]

Comment on attachment 778425 [details] [diff] [review]
Patch v3 - https://github.com/mozilla-b2g/gaia/pull/11075

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

Thanks, this looks like a good path forward.

Comment 33

[Gene Lian (I already quit Mozilla)]

Comment on attachment 778425 [details] [diff] [review]
Patch v3 - https://github.com/mozilla-b2g/gaia/pull/11075

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

Looks good to me. Thanks!

So, this patch is now safe to be landed to both master and b2g18? Or just master? If b2g18 is OK to land, then we can also land bug 891756 to b2g18. Right? Sorry I still don't get the policy. Please let me know you guys' final decision then I'll follow that.

Comment 34

[Fernando Campo (:fcampo)]

Merged on master https://github.com/mozilla-b2g/gaia/commit/7f3d9b6583aa42eb31c6d93d75715e8516475e04

As we need this on v1, I'll ask for the uplift (guess that's what you meant with b2g18, gene?)

Thanks for your help guys. We still need to open a followup for the proper strings, but that should be for next string freeze period.

Comment 35

[Gene Lian (I already quit Mozilla)]

(In reply to Fernando Campo (:fcampo) from comment #34)
> Merged on master
> https://github.com/mozilla-b2g/gaia/commit/
> 7f3d9b6583aa42eb31c6d93d75715e8516475e04
> 
> As we need this on v1, I'll ask for the uplift (guess that's what you meant
> with b2g18, gene?)

Yes, b2g18 (Gecko) = v1-train (Gaia). I'll land bug 891756 to master/b2g18 as well. Thanks!

Comment 36

[Fernando Campo (:fcampo)]

John  can you take a look at the uplift when you have a moment?

Thanks

Comment 37

[Dale Harvey (:daleharvey)]

This has a syntax error which caused travis failures (https://travis-ci.org/mozilla-b2g/gaia/builds/9344996), reverted in

https://github.com/mozilla-b2g/gaia/commit/603a92c3358cd09129a11a7d9e3b41f68269704c

Comment 38

[Dale Harvey (:daleharvey)]

needinfoing just as a notification

Comment 39

[Fernando Campo (:fcampo)]

Thanks Dale, it was in my radar but I get caught up on other stuff. Will fix it in minutes

Comment 40

[Fernando Campo (:fcampo)]

Attachment: Link to PR - https://github.com/mozilla-b2g/gaia/pull/11124

Asking for feedback just to check for more obvious errors. Only change respect previous patch is the syntax error solved, so I guess the r+ from the v3 still applies

Comment 41

[Dale Harvey (:daleharvey)]

Comment on attachment 779823 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/11124

I am good with this cheers, investigating the travis failure thats stopping this go green

Comment 42

[Fernando Campo (:fcampo)]

(In reply to Dale Harvey (:daleharvey) from comment #41)
> Comment on attachment 779823 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/11124
> 
> I am good with this cheers, investigating the travis failure thats stopping
> this go green
As it seems unrelated (dialer), I merged the PR to get this solved
master https://github.com/mozilla-b2g/gaia/commit/5aac3408cdefed4afb1b14d57a69d713c69b46b9

Comment 43

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

Uplifted 5aac3408cdefed4afb1b14d57a69d713c69b46b9 to:
v1-train: d801bd57aabb931634e1561af4064fdf374695be