User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11 Steps to reproduce: 1. Launch SMS application. 2. Edit a new message. 3. Input a number in address bar with a valid character, for example ,'10086;' 4. Send the message. Actual results: The message is still sent successfully. Expected results: The application should check address before sending a message.

Build Information: gecko: revision="3cbade1974968bb1e0fbb0c3386239715244a7a7" gaia: revision="aab72f365d73f624ede32b522f27d072c409e42e" gonk-misc: revision="654358494ba601a46ef9838debc95417ae464cc6" dalvik: revision="ca1f327d5acc198bb4be62fa51db2c039032c9ce" librecovery: revision="e1bd90051c9e937221eb1f91c94e3cde747311a7" moztt: revision="6ee1f8987ef36d688f97064c003ad57849dfadf2" external/jsmin: revision="cec896f0affaa0226c02605ad28d42df1bc0e393" external/opensans: revision="b5b4c226ca1d71e936153cf679dda6d3d60e2354" device/qcom/b2g_common/mozilla-b2g: revision="41c17a6abfd5f488ec99d9aa246f5b07583403c7"

It's not an issue. This sounds to be a carrier specified topic.

This issue is when users input ";" in the number...

Triage: BB-, after you send a message to a invalid number. you will see an error message.

screenshot of sending a message with invalid character

I think "13616$$&#" should be treated as invalid message receriver. Build based below commit: gaia: commit 73270c32928f070e49b265ea75ff4062ddb82d67 Merge: 9ce1358 a698a8b Author: David Scravaglieri <dscravaglieri@mozilla.com> Date: Thu Dec 27 01:26:22 2012 -0800 Merge pull request #7205 from dscravag/nightly2 Nightly 2012-12-27 gecko commit af3e3684a7db91cd3cf546465df0c9c8fd80acb0 Author: Chris Jones <jones.chris.g@gmail.com> Date: Thu Dec 27 02:43:55 2012 -0800 Bug 822398: Use PL_DHASH_REMOVE to remove an entry we're enumerating. r=kanru a=blocking-basecamp

from my phone 12/26 nightly (comment 8) i can see the error in sending ! add tracking-b2g18+

(In reply to Joe Cheng [:jcheng] from comment #8) > Created attachment 696220 [details] > (In reply to Firefox_Mozilla from comment #7) > I think "13616$$&#" should > be treated as invalid message receriver. I just tested, I'm reproducing. Is there a list somewhere of chars accepted ?

FYI, on Android, when you enter an invalid recipient like in this bug, you get an alert popup.

I have a first patch that does the job :)

(In reply to khu from comment #3) > This issue is when users input ";" in the number... now, I think that using ";" in the number effectively "closes" the current number as a recipient. Anyway, we're doing a sanitization in Gecko before sending the number, therefore this bug is a "nice to have". I'd suggest doing the sanity check only when we get a "failed" event, to tell the user why this fails. We do this already for airplane mode, and this would prevent annoying the user before sending the message, if the message can still be sent. Ayman, what does your wisdom say here ?

(In reply to Julien Wajsberg [:julienw] from comment #13) > (In reply to khu from comment #3) > > This issue is when users input ";" in the number... > > now, I think that using ";" in the number effectively "closes" the current > number as a recipient. Anyway, we're doing a sanitization in Gecko before > sending the number, therefore this bug is a "nice to have". > Well, referencing wireframe pack: HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0 page: 06 - by typing directly into the ‘to’ field. - user indicates that string is complete by pressing either ‘return’ or semi colon ‘;’ So the initial test detailed in comment 1 should not have progressed beyond point 3. It should at that point actually have triggered a completely different bug. I am surprised that has not happened. > I'd suggest doing the sanity check only when we get a "failed" event, to > tell the user why this fails. We do this already for airplane mode, and this > would prevent annoying the user before sending the message, if the message > can still be sent. > > Ayman, what does your wisdom say here ? Julien, I agree with your proposal for V1.1 and indeed this is how it is supposed to work. For later versions it would be nice to be able to 'proactively' check for invalid strings up front before 'send'. But for now this would be a new feature as it was not specified and therefore should not be developed for v1.1.

(In reply to ayman maat :maat from comment #14) > (In reply to Julien Wajsberg [:julienw] from comment #13) > > (In reply to khu from comment #3) > > > This issue is when users input ";" in the number... > > > > now, I think that using ";" in the number effectively "closes" the current > > number as a recipient. Anyway, we're doing a sanitization in Gecko before > > sending the number, therefore this bug is a "nice to have". > > > > Well, referencing > wireframe pack: HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0 > page: 06 > > - by typing directly into the ‘to’ field. > - user indicates that string is complete by pressing either ‘return’ or semi > colon ‘;’ > > So the initial test detailed in comment 1 should not have progressed beyond > point 3. It should at that point actually have triggered a completely > different bug. I am surprised that has not happened. That's because this STR was done before the multi-recipient work was uplifted (ie: with the 1.0.1 behavior). > > > I'd suggest doing the sanity check only when we get a "failed" event, to > > tell the user why this fails. We do this already for airplane mode, and this > > would prevent annoying the user before sending the message, if the message > > can still be sent. > > > > Ayman, what does your wisdom say here ? > > Julien, I agree with your proposal for V1.1 and indeed this is how it is > supposed to work. Great, thanks ! What should we display in that case ? An alert box like we do for the airplane mode error ? Also, I think we should go back to the "new thread" view in that case, so that the user can correct the number. Which is something we never do right now. Otherwise, the user would be stuck in a thread with a wrong "number", with no possibility correct the number to send the same message correctly. I suspect it's not so difficult, because we already do something like that when we receive an activity (a number + a body). We should also delete the message sent to a wrong number. Ayman ? > > For later versions it would be nice to be able to 'proactively' check for > invalid strings up front before 'send'. But for now this would be a new > feature as it was not specified and therefore should not be developed for > v1.1. This would prevent going to the "existing thread" view, but this is probably harder than we think; expecially, we should also check invalid numbers coming from a contact.

referencing comment 15

Ayman, in the v1.1 case, where we send first, get a failure, and only then display the box, I think the second action should not be "send" but rather "ignore" (because the message was already sent). What's your advice ?

Moreover, if all recipients are wrong, then we should probably only have the "edit" choice.

(In reply to Julien Wajsberg [:julienw] from comment #17) > Ayman, in the v1.1 case, where we send first, get a failure, and only then > display the box, I think the second action should not be "send" but rather > "ignore" (because the message was already sent). What's your advice ? Is it possible for us to check the validity of the recipients before we action the send? or is that not practical for V1.1? Got to ask to be sure as thats what I would like to do, but if it is not practical then i will adjust the wireframes accordingly.

I'm concerned that we try to be too strict/restrictive before sending. If we check after a failure, then we're sure that the network forbid the sending and that it's wrong, and now we can find why it's wrong. But if we check before the failure, I'm worried that we might end up checking too much. If we were earlier in the 1.1 process, we could do that, because we'd have more time to tune the check, but doing this now worries me. However that would probably make the code simpler, ie we would not need to switch between the thread view and the new thread view, because we would not switch when sending in the first place. Borja, do you have an advice here ?

(In reply to Julien Wajsberg [:julienw] from comment #20) > I'm concerned that we try to be too strict/restrictive before sending. If we > check after a failure, then we're sure that the network forbid the sending > and that it's wrong, and now we can find why it's wrong. But if we check > before the failure, I'm worried that we might end up checking too much. > > If we were earlier in the 1.1 process, we could do that, because we'd have > more time to tune the check, but doing this now worries me. > > However that would probably make the code simpler, ie we would not need to > switch between the thread view and the new thread view, because we would not > switch when sending in the first place. > > Borja, do you have an advice here ? Ok thanks Julien, that sounds like a reasoned approach. I will wait for input from Borja before updating the wireframes. Hope to get them done Monday then we can move forwards and close this bug.

Hi all. After reading all the comments I'm really concerned about the version where we have to fix the bug. @zhang: I've seen the snapshot and the UI is quite old (actually it seems to be v1.0.1, because Gaia commit is 6 months old..), could you check this? Because we have to consider if we should fix this *only* in master or in both (v1.0.1 and master) @julien: After checking this issue with Ayman, I think that we should (at least) inform the user when is *creating* the recipient, letting the opportunity of updating on the fly. For doing this, we could check the recipient content before creating it, and if there is any error we should notify through some alert about the wrong status, and focusing in the element which is wrong. I think that it should not be a big change and we could offer a good interaction, and opening a followup for developing the complete solution designed by Ayman. Wdyt? If you agree I could work on this. Thanks!

ok for me. However take really care to _not_ be too strict...

Hei folks, is that what you had in mind? (please don't take a look to the ugly and unstranslated alert message) Reading your last comment just tried to get ideas from both, doing a prechecking, but not being really hard, we just check 2 regex, once for phone numbers (same one than used in dialer web activities), and another one for emails. Is this what you had in mind? Cheers! F.

No validation need to be in the Recipient.prototype.add method—it just creates an unnecessary dependency on Utils. Any validation should be done before the call to ThreadUI.recipients.add(...)

(In reply to Julien Wajsberg [:julienw] from comment #20) > I'm concerned that we try to be too strict/restrictive before sending. If we > check after a failure, then we're sure that the network forbid the sending > and that it's wrong, and now we can find why it's wrong. But if we check > before the failure, I'm worried that we might end up checking too much. > Or worse, we reject based on an a false assumption. I strongly advise against trying to write a "valid email" regex to do any kind of checking: revalidEmail.test("waldron.rick@gmail.com") // false? It's not a matter of "hacking away" until you get it right—just send the email or send the message and relay the failure response accordingly. The carrier will always notify of failed messages. When I was developing SMS/shortcode query and response services this was actually a hard requirement of the carriers: do not attempt to "know better" then their system.

(In reply to Rick Waldron from comment #25) > No validation need to be in the Recipient.prototype.add method—it just > creates an unnecessary dependency on Utils. Any validation should be done > before the call to ThreadUI.recipients.add(...) About adding the dependence to the Utils class, is as easy as create that method on the Recipients class it self. Not adding this sanity check to the method it self will force us to: - Wrote more lines of check, since we call the ThreadUI.recipients.add several times - We can introduce more errors, adding an extra call to ThreadUI.recipients.add and forgetting to add the checking will become an error. IMHO, is better to add an implicit sanity check to a method that it needs (later in my next comment will reply if it needs it or not) than have to do this explicitly, it will save time, and will keep us developers to shoot to our feet. Thanks

(In reply to Rick Waldron from comment #26) > (In reply to Julien Wajsberg [:julienw] from comment #20) > > I'm concerned that we try to be too strict/restrictive before sending. If we > > check after a failure, then we're sure that the network forbid the sending > > and that it's wrong, and now we can find why it's wrong. But if we check > > before the failure, I'm worried that we might end up checking too much. > > > > Or worse, we reject based on an a false assumption. I strongly advise > against trying to write a "valid email" regex to do any kind of checking: > revalidEmail.test("waldron.rick@gmail.com") // false? It's not a matter of > "hacking away" until you get it right—just send the email or send the > message and relay the failure response accordingly. Right, that's why this is not a PR, is just a test based on comments #22 and #23 about trying a solution. Is just a PoC to validate if this is what we want to do. It contains bugs, and also helps us to take decisions like you comment, is this a valid approach? are we being too restrictive? > > The carrier will always notify of failed messages. When I was developing > SMS/shortcode query and response services this was actually a hard > requirement of the carriers: do not attempt to "know better" then their > system. Here again, I'm more than happy to bring this back to the conversation, I don't have the experience that you comment about the carriers requirements, but that doesn't mean is not there :), could you provide some examples of this cases? As well, Rick, what happen in a multirecipient sms/mms, it's send to the valid ones? Thanks a lot Rick! F.

Comment on attachment 766962 [details] [diff] [review] initial proposal In this case I would modify the following things: - I would move the UI (in this case the 'alert') to 'Thread_ui.js' - method 'assimilateRecipients' - for having Recipients as an object not tied to the UI. As we are throwing an exception when the number is undefined, we could use the same for 'invalid' format and adding a try/catch. - Currently adding an email is not a feature, so I would remove this from the checking. The patter used I dont know if it's the right one, because I think we should accept [a-z] & [A-Z], something like /[^+\-()#*0-9A-Za-z]/ig , Wdyt? Thanks! - IMHO +1 for adding this validation into the 'add' function. We are checking if is undefined or not, and all this checks belongs to 'Recipient' object. Thanks Francisco!

(In reply to Francisco Jordano [:arcturus] from comment #28) > (In reply to Rick Waldron from comment #26) > > (In reply to Julien Wajsberg [:julienw] from comment #20) > > > I'm concerned that we try to be too strict/restrictive before sending. If we > > > check after a failure, then we're sure that the network forbid the sending > > > and that it's wrong, and now we can find why it's wrong. But if we check > > > before the failure, I'm worried that we might end up checking too much. > > > > > > > Or worse, we reject based on an a false assumption. I strongly advise > > against trying to write a "valid email" regex to do any kind of checking: > > revalidEmail.test("waldron.rick@gmail.com") // false? It's not a matter of > > "hacking away" until you get it right—just send the email or send the > > message and relay the failure response accordingly. > > Right, that's why this is not a PR, is just a test based on comments #22 and > #23 about trying a solution. Is just a PoC to validate if this is what we > want to do. It contains bugs, and also helps us to take decisions like you > comment, is this a valid approach? are we being too restrictive? > > > > > The carrier will always notify of failed messages. When I was developing > > SMS/shortcode query and response services this was actually a hard > > requirement of the carriers: do not attempt to "know better" then their > > system. > > Here again, I'm more than happy to bring this back to the conversation, I > don't have the experience that you comment about the carriers requirements, > but that doesn't mean is not there :), could you provide some examples of > this cases? I'm not sure I understand what cases you're asking for, please clarify. The services I wrote were text-to-shortcode query (http://lifehacker.com/383679/nutritional-values-are-an-sms-away-at-dietcom) and diary logging (another feature of the same shortcode) > > As well, Rick, what happen in a multirecipient sms/mms, it's send to the > valid ones? Yes, the invalid recipients are allowed to fail when the carrier determines they are a failure.

(In reply to Borja Salguero [:borjasalguero] from comment #29) > Comment on attachment 766962 [details] [diff] [review] > initial proposal > > In this case I would modify the following things: > - I would move the UI (in this case the 'alert') to 'Thread_ui.js' - method > 'assimilateRecipients' - for having Recipients as an object not tied to the > UI. As we are throwing an exception when the number is undefined, we could > use the same for 'invalid' format and adding a try/catch. > > - Currently adding an email is not a feature, so I would remove this from > the checking. The patter used I dont know if it's the right one, because I > think we should accept [a-z] & [A-Z], something like /[^+\-()#*0-9A-Za-z]/ig > , Wdyt? Thanks! > > - IMHO +1 for adding this validation into the 'add' function. We are > checking if is undefined or not, and all this checks belongs to 'Recipient' > object. Validation of a value is orthogonal to the storage of a value. Doing any kind of validation is going to cause problems and this should absolutely be left to the carrier, after the "send" has occurred.

Problem with this approach (that I recommended at first) is that we can't give an early feedback to the user. I think we can try to validate the really wrong cases.

No new information to reconsider this as a blocker

Francisco, for the prompt 'Recipients' has it own UI stuff, and you can find it here https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/recipients.js#L903, so the alert could be there.

(In reply to Borja Salguero [:borjasalguero] from comment #34) > Francisco, for the prompt 'Recipients' has it own UI stuff, and you can find > it here > https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/recipients. > js#L903, so the alert could be there. This should be handled by the carrier, not by the application.

-1 to validating, this should be done because of rejection by the carrier -- though a ; should or , should probably cause a recipient to be "finalized", anything else is not worth "validating" IMO

When the carrier rejects a number, can we know which number was rejected (in case of several recipients) ?

The more I think about this bug the more I would like a validation by carrier. In fact we will need to protect ourself for valid number that some carriers would not accept, so better to do this in a single place.

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #36) > -1 to validating, this should be done because of rejection by the carrier -- > though a ; should or , should probably cause a recipient to be "finalized", > anything else is not worth "validating" IMO ...interesting as I have a couple of android devices here that validate the phone number upon send, but before the phone number is sent so they are doing it within the application. However an iphone (with the same SIM) validating the phone number allowing the message to be sent and to fail so validating with the carrier.. Does not seem to be a 'standard' practice i can see. a ; or , should defiantly cause a recipient to be "finalized"

in fact i know the androids are doing some level of phone number sanity check 'in application' because upon 'send' being selected I receive 'invalid recipient' error message when an invalid phone number is input into the 'to field'. At the moment i am suspecting that the in app validation is throwing an error when at least two conditions are not met: 1) the contact information in the 'to field' contains no numbers, AND 2) the contact information in the 'to field' is not in email format I would suggest that we could handel the same, the rest we could pass to carrier. WDYT?

scrub comment 40 - made a couple of mistakes. here is what I wanted to say: in fact i know the androids are doing some level of phone number sanity check 'in application' because upon 'send' being selected I receive 'invalid recipient' error message when an invalid phone number is input into the 'to field' and there is no SIM inserted. At the moment i am suspecting that the in app validation is throwing an error when at least two conditions are met: 1) the contact information in the 'to field' contains no numbers, AND 2) the contact information in the 'to field' is not in email format I would suggest that we could handel the same, the rest we could pass to carrier. WDYT?

needinfo reference comment 41

(In reply to ayman maat :maat from comment #40) > in fact i know the androids are doing some level of phone number sanity > check 'in application' because upon 'send' being selected I receive 'invalid > recipient' error message when an invalid phone number is input into the 'to > field'. > > At the moment i am suspecting that the in app validation is throwing an > error when at least two conditions are not met: > > 1) the contact information in the 'to field' contains no numbers, AND The presence of a single non-dialable character would work, anything more rigid will cause trouble. This essentially what android is doing: http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/2.2_r1.1/android/telephony/PhoneNumberUtils.java#914 > 2) the contact information in the 'to field' is not in email format http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/1.5_r4/android/text/util/Regex.java#Regex.0EMAIL_ADDRESS_PATTERN Translated to JS... var remail = new RegExp( "[a-zA-Z0-9\\+\\.\\_\\%\\-]{1,256}" + "\\@" + "[a-zA-Z0-9][a-zA-Z0-9\\-]{0,64}" + "(" + "\\." + "[a-zA-Z0-9][a-zA-Z0-9\\-]{0,25}" + ")+" ); Basically, if the value is "dialable" or possibly an email address, it's ok, otherwise it's invalid > > I would suggest that we could handel the same, the rest we could pass to > carrier. WDYT? The very loose validation above should be ok.

Except we're supposed to work with numbers using letters (translated then to numbers). I suppose we could at least check if the first character is either + or a number (because afaik even numbers-with-letters start with a number). I don't get why we should accept email adresses in the sms/mms app.

(In reply to Julien Wajsberg [:julienw] from comment #37) > When the carrier rejects a number, can we know which number was rejected (in > case of several recipients) ? I suppose you're talking about MMS because only the MMS can allow multiple receivers. The MMSC cannot return feedback to the sender about which receivers are invalid or not. It can receive the MMS all the time even if it contains invalid numbers. If the MMS contains invalid numbers, the MMS will not be delivered to those invalid numbers. However, it can still be delivered to those valid numbers.

(In reply to Julien Wajsberg [:julienw] from comment #44) > I don't get why we should accept email adresses in the sms/mms app. The Gecko part doesn't support sending MMS to e-mail addresses yet (Bug 866938). It's supposed to be the new feature in the next version.

I am with Gene. I'll suggest to close the bug and create a bug for new feature if needed. Kevin or Joe, can you comment on this?

(In reply to Gene Lian [:gene] from comment #45) > (In reply to Julien Wajsberg [:julienw] from comment #37) > > When the carrier rejects a number, can we know which number was rejected (in > > case of several recipients) ? > > I suppose you're talking about MMS because only the MMS can allow multiple > receivers. The API accepts multiple receivers for SMS too, even if behind the scene it sends multiple SMS. So what happens if one of the numbers is wrong ? Do we get multiple "sending" event ? do we get some "error" events and some "sending" events ? > > The MMSC cannot return feedback to the sender about which receivers are > invalid or not. It can receive the MMS all the time even if it contains > invalid numbers. so we can never have an error from the carrier ?

(In reply to Al Tsai [:atsai] from comment #47) > I am with Gene. I'll suggest to close the bug and create a bug for new > feature if needed. > > Kevin or Joe, can you comment on this? Sorry, we still need to somehow warn the user if a message was not sent.

Can we have a sync on Monday about this, maybe the IRC channel? Sure we can meet and try to unblock this a bit. IMHO I'm getting closer to not filter at the beginning but as some of you comment I'm afraid of possible consequences and new uses cases when we have a multirecipient message. Thanks!

I'm still -1 to any validation - let the carrier reject it. Perhaps give us a better status message. If we really want validation though, it shouldn't be very hard to add to assimilateRecipients

Note that the current message when we try to send to a wrong number is quite bad, it says we have a network problem, which is wrong. Can we do what I propose in comment 13 ? In case of a generic (ie: non-network) failure, we should really display another message to the user. I'm quite sad because I proposed this 1 month ago, and this would add a new string, which is quite late imho.

I'll file a new bug for the message problem to make it clearer.

Filed bug 891756 about this.

Comment on attachment 766962 [details] [diff] [review] initial proposal Review of attachment 766962 [details] [diff] [review]: ----------------------------------------------------------------- so I think it's pretty clear we don't want to do that now, right ? plus I don't see why we validate emails too ?

(In reply to Julien Wajsberg [:julienw] from comment #55) > Comment on attachment 766962 [details] [diff] [review] > initial proposal > > Review of attachment 766962 [details] [diff] [review]: > ----------------------------------------------------------------- > > so I think it's pretty clear we don't want to do that now, right ? > plus I don't see why we validate emails too ? Right, my opinion has evolved also since I added that patch, I would prefer not to do any prechecking and let the RIL tell us what happened.

this bug is closely related to: https://bugzilla.mozilla.org/show_bug.cgi?id=888245

i would like to discuss this bug at the oslo work week along with 888245 so marking whiteboard [osloww]

I'm very close to duping this to bug 907088, too much conflicting dicussions happened here.

Moving on