Closed Bug 891855 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed)

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: airpingu, Assigned: fcampo)

References

Details

(Keywords: late-l10n, Whiteboard: [u=commsapps-user c=messaging p=0] )

Attachments

(1 file, 3 obsolete files)

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.
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;
Ayman, could you provide us some good text for these cases ?

We need a title and a body.
Flags: needinfo?(aymanmaat)
triage is waiting on resolution for 891756 to make a call here
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.
yes wrong strings, but not a shipstopper
blocking-b2g: leo? → -
(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.
Flags: needinfo?(aymanmaat) → needinfo?(tyler.altes)
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.
blocking-b2g: - → leo?
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.
Discussed with Mozilla and marked it Leo+ as it has land with 891756. - Veeresh
blocking-b2g: leo? → leo+
Priority: -- → P1
Target Milestone: --- → 1.1 QE4 (15jul)
Assignee: nobody → fbsc
Stealing, cause I'm a bad person
Assignee: fbsc → fernando.campo
Whiteboard: [u=commsapps-user c=messaging p=0]
hehehe, all yours Mr. Campo! ;)
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;
Flags: needinfo?(tyler.altes)
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
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;
So, the Gaia patch can land first before the Gecko patch at bug 891756. I'll wait on this.
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';
Flags: needinfo?(tyler.altes)
Flags: needinfo?(gene.lian)
(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';
Flags: needinfo?(gene.lian)
(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';
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';
Flags: needinfo?(tyler.altes)
Attached patch Patch v1 (obsolete) — Splinter Review
Maybe I messed something with the flags and people, so please correct any if I'm wrong
Attachment #777173 - Flags: review?(l10n)
Attachment #777173 - Flags: review?(gene.lian)
Attachment #777173 - Flags: feedback?(tyler.altes)
Looks good.
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.
Attachment #777173 - Flags: review?(l10n)
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?
Attachment #777173 - Flags: review?(gene.lian) → review+
Comment on attachment 777173 [details] [diff] [review]
Patch v1

Adding borja as reviewer as requested
Attachment #777173 - Flags: review+ → review?(fbsc)
(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.
Updating patch with tests and including link to PullRequest
Attachment #777173 - Attachment is obsolete: true
Attachment #777173 - Flags: review?(fbsc)
Attachment #777173 - Flags: feedback?(tyler.altes)
Attachment #777674 - Flags: review?(fbsc)
Attachment #777674 - Flags: feedback?(tyler.altes)
Attachment #777674 - Flags: review?(fbsc) → review+
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
(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.
(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.
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.
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.
Attachment #778425 - Flags: review?(gene.lian)
Attachment #778425 - Flags: feedback?(l10n)
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.
Attachment #778425 - Flags: feedback?(l10n) → feedback+
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.
Attachment #778425 - Flags: review?(gene.lian) → review+
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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!
Attachment #777674 - Attachment is obsolete: true
Attachment #777674 - Flags: feedback?(tyler.altes)
John  can you take a look at the uplift when you have a moment?

Thanks
Flags: needinfo?(jhford)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
needinfoing just as a notification
Flags: needinfo?(fernando.campo)
Thanks Dale, it was in my radar but I get caught up on other stuff. Will fix it in minutes
Flags: needinfo?(fernando.campo)
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
Attachment #778425 - Attachment is obsolete: true
Attachment #779823 - Flags: feedback?(dale)
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
Attachment #779823 - Flags: feedback?(dale) → feedback+
(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
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Uplifted 5aac3408cdefed4afb1b14d57a69d713c69b46b9 to:
v1-train: d801bd57aabb931634e1561af4064fdf374695be
Flags: needinfo?(jhford)
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.