Closed Bug 935060 Opened 11 years ago Closed 7 years ago

[Messages] Unable to send empty message

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: Elio, Assigned: rishav_, Mentored)

References

Details

Attachments

(4 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131105040203

Steps to reproduce:

Send an empty SMS to my operator in order to receive balance information


Actual results:

"Send" button disabled


Expected results:

"Send" button should be enabled also when no text is written.
The balance info for my operator for example works only if an empty SMS is sent to them, so Im unable to get any info from that
Flagging our UX designer Ayman on this.

I think the use case is sensible. Ayman, what do you think of this? It's your call!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(aymanmaat)
Years ago we had the same debate in Maemo in https://bugs.maemo.org/show_bug.cgi?id=5409 and there are several reasons listed why it should be allowed to send empty SMS.
Thanks Andre, this is a very interesting discussion indeed. I agree we should present a warning message in this case.

Now it's Ayman's decision. :)
(In reply to Julien Wajsberg [:julienw] from comment #3)
> Thanks Andre, this is a very interesting discussion indeed. I agree we
> should present a warning message in this case.
> 
> Now it's Ayman's decision. :)

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Thanks Andre, this is a very interesting discussion indeed. I agree we
> should present a warning message in this case.
> 
> Now it's Ayman's decision. :)

Hey guys, this is an interesting one, I had no idea about this. All the benchmarks i have access to reference do not allow the sending of a message when the message field is empty. However if this prevents a user from communicating with their operator (for example to obtain their balance information as outlined in comment 0 ) we should certainly allow the sending of empty messages.

If we are going to allow the send CTA to be active when there is no content in the message field we should certainly implement a warning message to handle cases where users select it by accident. Here is what i think:

**New Message**

1) open message app
2) open new message composer. 
3) send CTA is disabled until content is entered into the ’to field’
4) user selects send with content in ’to field’ but message field empty
	- warning dialogue is presented stating:
		- <Header>Empty Message</Header>
		- <Body>Do you want to send an empty message</Body>
		- <CTA1>Cancel</CTA1><CTA2>OK</CTA2>

upon tap of CTA1 (Cancel)
	- Warning dialogue is closed 
	- user is returned to view from which warning dialogue was launched.

upon tap of CTA2
	- Warning dialogue is closed 
	- message is sent, flow follows currently implemented path

**Reply to Thread**

1) open message app
2) open existing thread.  
3) send CTA is enabled 
4) user selects send with message field empty
	- warning dialogue is presented stating:
		- <Header>Empty Message</Header>
		- <Body>Do you want to send an empty message</Body>
		- <CTA1>Cancel</CTA1><CTA2>OK</CTA2>

upon tap of CTA1 (Cancel)
	- Warning dialogue is closed 
	- user is returned to view from which warning dialogue was launched.

upon tap of CTA2
	- Warning dialogue is closed 
	- message is sent, flow follows currently implemented path

..how does that sound?
Flags: needinfo?(aymanmaat)
Sounds good to me.

1 question:
* Can we leave out the header? That would let us use the standard "window.confirm" function for an easy patch.

We should ask Tyler for good messages.
Flags: needinfo?(aymanmaat)
Any news on this?
Thanks for the heads up.

Just asked Ayman, he's fine with leaving out the header and we can use window.confirm then.

We just need a good message now. Tyler, could you please help? See comment 4, we need a good text for the "body" part.

Thanks!

Elio, do you think you could provide a patch for this or do you prefer that we do it?
Flags: needinfo?(aymanmaat) → needinfo?(tyler.altes)
Julien, feel free to do it, Im not that experienced with the tech part yet :)
Ok, no problem, we'll wait for Tyler's answer first :)
Whiteboard: [mentor=:julienw]
Hi, I think Ayman's message is fine, with the exception of punctuation. Then I would change the confirm button to correspond with the question.

- <Body>Do you want to send an empty message?</Body>
- <CTA1>Cancel</CTA1><CTA2>Send</CTA2>
Flags: needinfo?(tyler.altes)
For a contributor, the relevant code is in thread_ui.js (enableSend function and onSendClick function) and maybe in compose.js (in onContentChanged) too.
I am taking up these issue.

thanks Julien for the info!

I ll soon upload the required patch.
Kindly assign these issue to me as I am not able to assign it to myself.
Hi all.

I made a few changes to the code of thread_ui.js and compose.js to enable the send button for sending blank messages.
But the problem is that the mozMobileMessage.send function, when the message is empty, sends an "undefined" message.
Thus, I tried to used this function with an empty sms parameter (i.e content = ''), and the sms never send, I mean, there's a loading loop animation over the empty message.

Am I supposed to modify the mozMobileMessage.send function?

About the confirm button, can I get any help to do that?

Can you assign this to me? It would be great!

Thank you all for the information.

Cheers and happy new year!
Hi Nico and Ankit,

Thanks for your messages. Sorry for not answering earlier as I was in holiday.

Ankit, since I see no patch from you, is it ok if I assign the bug to Nico ?

Nico, the "confirm" panel should be done using the usual DOM0 "confirm()" function :) Yes, as simple as this!

About the issue with the "send" function, I'll need info gecko developers, as there may be a bug on their end. I think we're supposed to use send with "content = ''" but then either it should work or we should get an error.

Gene, could you provide some help on this topic?
Assignee: nobody → ndel314
Flags: needinfo?(gene.lian)
Attached file First attempt (obsolete) —
Hello!

First, thank you Julien for the assignment of this bug to me.

Some relevant issues are:

- Change "ok" to "send" of the confirmation panel 
- Solve the problem of the empty message is never sent

I only changed the code in "onSendClick" and "enableSend" functions.
Maybe the part of "content[0] = '';" can be written in a smarter way; the Compose.clear() function doesn't do what I wanted.

Hope you can check this.

Regards!
Thank you for catching this!

> I made a few changes to the code of thread_ui.js and compose.js to enable
> the send button for sending blank messages.
> But the problem is that the mozMobileMessage.send function, when the message
> is empty, sends an "undefined" message.

One question. I don't understand this comment. Do you mean you set the |message| parameter to call the API:

  jsval send(in jsval number, in DOMString message,
             [optional] in jsval sendParameters);

to be an empty string (i.e., "") and the receiver end will receive a message "undefined"?

> Thus, I tried to used this function with an empty sms parameter (i.e content
> = ''), and the sms never send, I mean, there's a loading loop animation over
> the empty message.

I don't understand either. What's this different from the above-mentioned?

> Am I supposed to modify the mozMobileMessage.send function?

IMO, I think Gecko should be able to send empty string. If the sending circle keeps spinning, it means the Gecko doesn't correctly set the delivery state to "sent" [1]. Need to look into the starting point of sending SMS [2]. We may open a separate bug to address the Gecko part or just directly solved it in the same bug.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3487
[2] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3400
Flags: needinfo?(gene.lian)
Hey Julien

I ll upload the patch today..
Hello Gene!

> One question. I don't understand this comment. Do you mean you set the
> |message| parameter to call the API:
> 
>   jsval send(in jsval number, in DOMString message,
>              [optional] in jsval sendParameters);
> 
> to be an empty string (i.e., "") and the receiver end will receive a message
> "undefined"?

The first thing I did was to enable the "send" button for empty messages case, and what I got after sending the first blank message was that message isn't empty but "undefined".

> I don't understand either. What's this different from the above-mentioned?

Yep, I didn't explained that well.
The difference is that the above-mentioned is not "empty" in a way, because the send function acts different between both. I mean, when I used the function send with a blank message (i.e, ''), sends an empty message but the circle keeps spinning, and the another case sends an message with "undefined" word in it.
I think the Compose.getContent() function lefts something more than "''" when the message is empty.
I'll check it out. 

> > Am I supposed to modify the mozMobileMessage.send function?
> 
> IMO, I think Gecko should be able to send empty string. If the sending
> circle keeps spinning, it means the Gecko doesn't correctly set the delivery
> state to "sent" [1]. Need to look into the starting point of sending SMS
> [2]. We may open a separate bug to address the Gecko part or just directly
> solved it in the same bug.

I agree with that.

> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js#3487
> [2]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js#3400

You could see how the code that I'll uploaded works?
I'll make a patch then.

Cheers!
The "undefined" case comes probably from WebIDL: if the message body is undefined (as in "the variable is undefined"), then WebIDL will convert it to a String "undefined". And that's why we get "undefined" as text.

I've seen that when working on the Contacts API, but that's always been weird to me. I think the only sensible ways to work with this in WebIDL is either to throw if we don't have a String, accept undefined and null as real values, or treat undefined as the empty string. The default behavior is very wrong to me.
This is the patch of the first attempt to resolve the #935060 bug.
Comment on attachment 8356166 [details] [diff] [review]
bug-935060-fix1 First attempt patch

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

A few general notes: 

- Please make sure all of the current tests pass
- Please add new tests for the new functionality
- Please be sure to run both `make hint APP=sms` and `make lint APP=sms` (there are currently 6 known errors, your code should introduce no new errors)

::: thread_ui.js
@@ +1931,2 @@
>        return;
> +    }*/

If this is no longer needed, please remove it entirely

@@ +1957,5 @@
>  
> +    // If it's an empty message, the content is '' (bug 935060)
> +    if (Compose.isEmpty()) {
> +      content[0] = '';
> +      if(!window.confirm('Do you want to send an empty message?'))

This won't work with l10n

@@ +1958,5 @@
> +    // If it's an empty message, the content is '' (bug 935060)
> +    if (Compose.isEmpty()) {
> +      content[0] = '';
> +      if(!window.confirm('Do you want to send an empty message?'))
> +        return;

nit: there is a space missing between if and (. 


Please always use curly braces for conditional bodies (or while, for etc)
Attachment #8356166 - Flags: review-
Also, please r? me when ready, thanks!
Note that we'll need a Gecko patch before merging this. Gene, can you file a new bug and produce a patch to fix the "empty string as body" case?
Flags: needinfo?(gene.lian)
Comment on attachment 8356166 [details] [diff] [review]
bug-935060-fix1 First attempt patch

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

Please request review from Rick when you've finished!

Also, please open a github pull request so that a Travis jobs are run.

::: thread_ui.js
@@ +1957,5 @@
>  
> +    // If it's an empty message, the content is '' (bug 935060)
> +    if (Compose.isEmpty()) {
> +      content[0] = '';
> +      if(!window.confirm('Do you want to send an empty message?'))

l10n means localization. Have a look to the uses of "navigator.mozL10n.get" in the code.

@@ +1958,5 @@
> +    // If it's an empty message, the content is '' (bug 935060)
> +    if (Compose.isEmpty()) {
> +      content[0] = '';
> +      if(!window.confirm('Do you want to send an empty message?'))
> +        return;

also please add blocks even for single-line "if" :)
Whiteboard: [mentor=:julienw] → [mentor=rwaldron]
Fire bug 957084 for the Gecko part.
Flags: needinfo?(gene.lian)
(In reply to Julien Wajsberg [:julienw] from comment #25)
> Comment on attachment 8356166 [details] [diff] [review]
> bug-935060-fix1 First attempt patch
> 
> Review of attachment 8356166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please request review from Rick when you've finished!
> 
> Also, please open a github pull request so that a Travis jobs are run.
> 
> ::: thread_ui.js
> @@ +1957,5 @@
> >  
> > +    // If it's an empty message, the content is '' (bug 935060)
> > +    if (Compose.isEmpty()) {
> > +      content[0] = '';
> > +      if(!window.confirm('Do you want to send an empty message?'))
> 
> l10n means localization. Have a look to the uses of "navigator.mozL10n.get"
> in the code.

Unfortunately, `navigator.mozL10n.get` will fail if the user changes the language while the app is open. 

Tell me what you think: 

1. Somewhere in index.html, we'll have something like this: 
   <span id="confirm-empty-message" data-l10n-id="confirm-empty-message" class="hidden">Do you want to send an empty message?</span>
2. Add an entry in locales: confirm-empty-message = Do you want to send an empty message?
3. During ThreadUI.init or Compose.init, call `navigator.mozL10n.localize(element, element.dataset.l10nId);`
4. Update the confirm call to: `!window.confirm(document.getElementById('confirm-empty-message').textContent)`

NOTE: This wasn't tested and is only meant to kickstart discussion for an alternative to using `navigator.mozL10n.get`. We could also abstract the operation to provide l10n handling to all of the remaining uses of `navigator.mozL10n.get` as well.
Summary: Unable to send empty SMS' → [Messages] Unable to send empty message
(In reply to Rick Waldron [:rwaldron] from comment #27)
> (In reply to Julien Wajsberg [:julienw] from comment #25)
> > Comment on attachment 8356166 [details] [diff] [review]
> > bug-935060-fix1 First attempt patch
> > 
> > Review of attachment 8356166 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please request review from Rick when you've finished!
> > 
> > Also, please open a github pull request so that a Travis jobs are run.
> > 
> > ::: thread_ui.js
> > @@ +1957,5 @@
> > >  
> > > +    // If it's an empty message, the content is '' (bug 935060)
> > > +    if (Compose.isEmpty()) {
> > > +      content[0] = '';
> > > +      if(!window.confirm('Do you want to send an empty message?'))
> > 
> > l10n means localization. Have a look to the uses of "navigator.mozL10n.get"
> > in the code.
> 
> Unfortunately, `navigator.mozL10n.get` will fail if the user changes the
> language while the app is open. 
> 
> Tell me what you think: 
> 
> 1. Somewhere in index.html, we'll have something like this: 
>    <span id="confirm-empty-message" data-l10n-id="confirm-empty-message"
> class="hidden">Do you want to send an empty message?</span>
> 2. Add an entry in locales: confirm-empty-message = Do you want to send an
> empty message?
> 3. During ThreadUI.init or Compose.init, call
> `navigator.mozL10n.localize(element, element.dataset.l10nId);`
> 4. Update the confirm call to:
> `!window.confirm(document.getElementById('confirm-empty-message').
> textContent)`
> 
> NOTE: This wasn't tested and is only meant to kickstart discussion for an
> alternative to using `navigator.mozL10n.get`. We could also abstract the
> operation to provide l10n handling to all of the remaining uses of
> `navigator.mozL10n.get` as well.

I fixed that issue using `navigator.mozL10n.get` adding to each language `emptyMessage-confirmation   = Do you want to send an empty message?` (with translation for each language, in sms.lang.properties).
This allows me to use mozL10n.get like this way:

var question = navigator.mozL10n.get('emptyMessage-confirmation');
      if (!window.confirm(question)) {
        return;
      }

And this worked fine.
The problem with this solution is that the button is "OK" instead of "Send".

Any way to change that?

I'll test your idea. And tell me your opinion of this solution.

Regards.
(In reply to Julien Wajsberg [:julienw] from comment #25)
> Comment on attachment 8356166 [details] [diff] [review]
> bug-935060-fix1 First attempt patch
> 
> Review of attachment 8356166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please request review from Rick when you've finished!
> 
> Also, please open a github pull request so that a Travis jobs are run.
> 
> ::: thread_ui.js
> @@ +1957,5 @@
> >  
> > +    // If it's an empty message, the content is '' (bug 935060)
> > +    if (Compose.isEmpty()) {
> > +      content[0] = '';
> > +      if(!window.confirm('Do you want to send an empty message?'))
> 
> l10n means localization. Have a look to the uses of "navigator.mozL10n.get"
> in the code.
> 
> @@ +1958,5 @@
> > +    // If it's an empty message, the content is '' (bug 935060)
> > +    if (Compose.isEmpty()) {
> > +      content[0] = '';
> > +      if(!window.confirm('Do you want to send an empty message?'))
> > +        return;
> 
> also please add blocks even for single-line "if" :)

Hey Julien,

I fixed all these errors and I'm familiarizing with the rules :D.

But before reply to Rick and upload the patch again, I prefer to finish the Gecko part.
(In reply to ndel314 from comment #28)
> (In reply to Rick Waldron [:rwaldron] from comment #27)
> > (In reply to Julien Wajsberg [:julienw] from comment #25)
> > > Comment on attachment 8356166 [details] [diff] [review]
> > > bug-935060-fix1 First attempt patch
> > > 
> > > Review of attachment 8356166 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Please request review from Rick when you've finished!
> > > 
> > > Also, please open a github pull request so that a Travis jobs are run.
> > > 
> > > ::: thread_ui.js
> > > @@ +1957,5 @@
> > > >  
> > > > +    // If it's an empty message, the content is '' (bug 935060)
> > > > +    if (Compose.isEmpty()) {
> > > > +      content[0] = '';
> > > > +      if(!window.confirm('Do you want to send an empty message?'))
> > > 
> > > l10n means localization. Have a look to the uses of "navigator.mozL10n.get"
> > > in the code.
> > 
> > Unfortunately, `navigator.mozL10n.get` will fail if the user changes the
> > language while the app is open. 
> > 
> > Tell me what you think: 
> > 
> > 1. Somewhere in index.html, we'll have something like this: 
> >    <span id="confirm-empty-message" data-l10n-id="confirm-empty-message"
> > class="hidden">Do you want to send an empty message?</span>
> > 2. Add an entry in locales: confirm-empty-message = Do you want to send an
> > empty message?
> > 3. During ThreadUI.init or Compose.init, call
> > `navigator.mozL10n.localize(element, element.dataset.l10nId);`
> > 4. Update the confirm call to:
> > `!window.confirm(document.getElementById('confirm-empty-message').
> > textContent)`
> > 
> > NOTE: This wasn't tested and is only meant to kickstart discussion for an
> > alternative to using `navigator.mozL10n.get`. We could also abstract the
> > operation to provide l10n handling to all of the remaining uses of
> > `navigator.mozL10n.get` as well.
> 
> I fixed that issue using `navigator.mozL10n.get` adding to each language
> `emptyMessage-confirmation   = Do you want to send an empty message?` (with
> translation for each language, in sms.lang.properties).

In the future, you don't need to do this—it's handled by the l10n experts ;)

> This allows me to use mozL10n.get like this way:
> 
> var question = navigator.mozL10n.get('emptyMessage-confirmation');
>       if (!window.confirm(question)) {
>         return;
>       }
> 
> And this worked fine.

If you change the language on the phone (while the application is running) is this string updated? (using your manually entered translations)

> The problem with this solution is that the button is "OK" instead of "Send".

Is there a spec for this? "OK" might be "OK" ;)

> 
> Any way to change that?

Not for the standard `confirm` prompt.
>
Rick, yep, I agree, but I figured that on the fly translation for such transient dialogs is not so important, and does not warrant adding complexity to the app instead of using DOM0's confirm. I agree this is subjective though :)

Would you agree or would you rather do the in-app confirm dialog?
(In reply to Julien Wajsberg [:julienw] from comment #31)
> Rick, yep, I agree, but I figured that on the fly translation for such
> transient dialogs is not so important, and does not warrant adding
> complexity to the app instead of using DOM0's confirm. I agree this is
> subjective though :)
> 
> Would you agree or would you rather do the in-app confirm dialog?

I agree with your rationale here, let's keep with the simple `confirm`
So, having fixed the Gecko part we get back here :).

> 1. Somewhere in index.html, we'll have something like this: 
>    <span id="confirm-empty-message" data-l10n-id="confirm-empty-message"
> class="hidden">Do you want to send an empty message?</span>
> 2. Add an entry in locales: confirm-empty-message = Do you want to send an
> empty message?
> 3. During ThreadUI.init or Compose.init, call
> `navigator.mozL10n.localize(element, element.dataset.l10nId);`
> 4. Update the confirm call to:
> `!window.confirm(document.getElementById('confirm-empty-message').
> textContent)`

Rick and Julien, this is the simple 'confirm' implementation that you discussed above?

Can I add the entry in locales directly? What would happen with the other languages?

If it's correct I'll start to implement it as soon as posible.

Regards!
Flags: needinfo?(waldron.rick)
Flags: needinfo?(felash)
(In reply to Nicolás Del Piano from comment #33)
> So, having fixed the Gecko part we get back here :).
> 
> > 1. Somewhere in index.html, we'll have something like this: 
> >    <span id="confirm-empty-message" data-l10n-id="confirm-empty-message"
> > class="hidden">Do you want to send an empty message?</span>
> > 2. Add an entry in locales: confirm-empty-message = Do you want to send an
> > empty message?
> > 3. During ThreadUI.init or Compose.init, call
> > `navigator.mozL10n.localize(element, element.dataset.l10nId);`
> > 4. Update the confirm call to:
> > `!window.confirm(document.getElementById('confirm-empty-message').
> > textContent)`
> 
> Rick and Julien, this is the simple 'confirm' implementation that you
> discussed above?

nope, we don't need the complex localize+textContent because, well, this doesn't actually bring any improvement :)

You just need to use navigator.mozL10n.get('confirm-empty-message') as confirm argument.

> 
> Can I add the entry in locales directly? What would happen with the other
> languages?

Yep, the localizers will see the new key and will translate accordingly.
Flags: needinfo?(waldron.rick)
Flags: needinfo?(felash)
Attached patch Bug Bug 935060 first patch (obsolete) — Splinter Review
Hello,

I upload the patch.

Some issues:

I leave the part of 'confirm-empty-message' empty. So there is no message for now. It's ok this? Or have I to put something and leave this to the translators?
Attachment #8355733 - Attachment is obsolete: true
Attachment #8356166 - Attachment is obsolete: true
Flags: needinfo?(waldron.rick)
Yes, please add the new l10n-id with the text:

  Do you want to send an empty message?

(see comment 10 :) )

Thanks!
Attached patch Bug 935060 - locales text (obsolete) — Splinter Review
Thanks Julien.

Sorry for the two patches. After the review I will mix them in one.
Flags: needinfo?(waldron.rick)
Comment on attachment 8369289 [details] [diff] [review]
Bug 935060 - locales text

look fine,

can you merge this into the other patch and do a github pull request with all these changes ?

You can attach a pull request on a bug by pasting directly the pull request URL (click "paste attachment as text" when attaching a new file).
 
And then ask review again :)

Thanks!
Attachment #8369289 - Flags: feedback+
Thanks Julien!

Sorry for the delay...

Here is the full patch to be reviewed.

I don't know if I did the pull request, I attached the patch normally. What's the difference?
Attachment #8368939 - Attachment is obsolete: true
Attachment #8369289 - Attachment is obsolete: true
Flags: needinfo?(waldron.rick)
Flags: needinfo?(felash)
(In reply to Nicolás Del Piano from comment #39)
> Created attachment 8380430 [details] [diff] [review]
> This patch enables the sending of empty messages.
> 
> Thanks Julien!
> 
> Sorry for the delay...
> 
> Here is the full patch to be reviewed.
> 
> I don't know if I did the pull request, I attached the patch normally.
> What's the difference?

The difference is that you'd push your working branch to github, make a pull request to mozilla-b2g/gaia and then paste that link as Julien describes.
Flags: needinfo?(waldron.rick)
Comment on attachment 8380431 [details] [diff] [review]
Bug 935060 - [Messages] Unable to send empty message patch.patch

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

::: apps/sms/js/thread_ui.js
@@ +912,5 @@
>  
>    enableSend: function thui_enableSend() {
>      this.initSentAudio();
>  
>      // should disable if we have no message input

This comment needs to be updated

@@ +913,5 @@
>    enableSend: function thui_enableSend() {
>      this.initSentAudio();
>  
>      // should disable if we have no message input
> +    var disableSendMessage = Compose.isResizing;

This change will have broken many tests—which means those tests need to be updated and run to ensure they aren't failing.

@@ +2072,5 @@
>      }
>  
> +    // If it's an empty message, the content is ''
> +    if (Compose.isEmpty()) {
> +      content[0] = '';

Is this necessary?

@@ +2075,5 @@
> +    if (Compose.isEmpty()) {
> +      content[0] = '';
> +      var question = navigator.mozL10n.get('confirm-empty-message');
> +      if (!window.confirm(question)) {
> +        return;

Please add two tests: 

1. mock confirm to return true
2. mock confirm to return false
> >  
> > +    // If it's an empty message, the content is ''
> > +    if (Compose.isEmpty()) {
> > +      content[0] = '';
> 
> Is this necessary?

Without that, if we attempt to send an empty message, it sends the word "undefined"; maybe some character remains on the message that makes this behavior.

What do you think about this?

> Please add two tests: 
> 1. mock confirm to return true
> 2. mock confirm to return false

I'll check this.
(In reply to Nicolás Del Piano from comment #43)
> > >  
> > > +    // If it's an empty message, the content is ''
> > > +    if (Compose.isEmpty()) {
> > > +      content[0] = '';
> > 
> > Is this necessary?
> 
> Without that, if we attempt to send an empty message, it sends the word
> "undefined"; maybe some character remains on the message that makes this
> behavior.
> 
> What do you think about this?

"'content.push('')" is probably more readable.

Also, please check how this behaves when you add a subject (both empty and non-empty) and  this consequently converts to MMS (when non-empty). We might want to do "if (!content.length) { content.push(''); }" instead. And maybe move this to Compose.getContent (but in that case we need to check that other consulers like drafts still work fine).
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #44)
> (In reply to Nicolás Del Piano from comment #43)
> > > >  
> > > > +    // If it's an empty message, the content is ''
> > > > +    if (Compose.isEmpty()) {
> > > > +      content[0] = '';
> > > 
> > > Is this necessary?
> > 
> > Without that, if we attempt to send an empty message, it sends the word
> > "undefined"; maybe some character remains on the message that makes this
> > behavior.
> > 
> > What do you think about this?
> 
> "'content.push('')" is probably more readable.
> 
> Also, please check how this behaves when you add a subject (both empty and
> non-empty) and  this consequently converts to MMS (when non-empty). We might
> want to do "if (!content.length) { content.push(''); }" instead. And maybe
> move this to Compose.getContent (but in that case we need to check that
> other consulers like drafts still work fine).

Ok, there will be more clever if we change that on compose.js.

This is the isEmpty method of Compose class:

get isEmpty() {
      return !dom.subject.value.length;
    }

!content.length and content.isEmpty have the same result.

When trying to send an empty MMS, an error occurs:
"There is a problem with the attached file and it cannot be downloaded"

Should we may be able to send an empty MMS?
I think this problem is generated here:
var smilSlides = content.reduce(thui_generateSmilSlides, []);
(In reply to Nicolás Del Piano from comment #45)
> (In reply to Julien Wajsberg [:julienw] from comment #44)
> > (In reply to Nicolás Del Piano from comment #43)
> > > > >  
> > > > > +    // If it's an empty message, the content is ''
> > > > > +    if (Compose.isEmpty()) {
> > > > > +      content[0] = '';
> > > > 
> > > > Is this necessary?
> > > 
> > > Without that, if we attempt to send an empty message, it sends the word
> > > "undefined"; maybe some character remains on the message that makes this
> > > behavior.
> > > 
> > > What do you think about this?
> > 
> > "'content.push('')" is probably more readable.
> > 
> > Also, please check how this behaves when you add a subject (both empty and
> > non-empty) and  this consequently converts to MMS (when non-empty). We might
> > want to do "if (!content.length) { content.push(''); }" instead. And maybe
> > move this to Compose.getContent (but in that case we need to check that
> > other consulers like drafts still work fine).
> 
> Ok, there will be more clever if we change that on compose.js.
> 
> This is the isEmpty method of Compose class:
> 
> get isEmpty() {
>       return !dom.subject.value.length;
>     }

Nope, this is the "isEmpty" for the "subject" subobject :)

There is another "isEmpty" method for the whole "Compose" object ;)

> 
> !content.length and content.isEmpty have the same result.

Not really, search for "state.empty" in the file.

> 
> When trying to send an empty MMS, an error occurs:
> "There is a problem with the attached file and it cannot be downloaded"

Actually, this is misleading, because this is what's happening when we _display_ the MMS with an empty content.

That said, I really thought this didn't happen nowadays... Have you tried to send a MMS with a subject but no content (that is bug 948958, so you may want to rebase with a newer master if you're seeing this) or a MMS with empty subject and no content (which is not possible before your patch is done, and should not be a MMS) ?

> 
> Should we may be able to send an empty MMS?
> I think this problem is generated here:
> var smilSlides = content.reduce(thui_generateSmilSlides, []);

I think this is not the problem here.
(In reply to Julien Wajsberg [:julienw] from comment #47)
> (In reply to Nicolás Del Piano from comment #45)
> > (In reply to Julien Wajsberg [:julienw] from comment #44)
> > > (In reply to Nicolás Del Piano from comment #43)
> > > > > >  
> > > > > > +    // If it's an empty message, the content is ''
> > > > > > +    if (Compose.isEmpty()) {
> > > > > > +      content[0] = '';
> > > > > 
> > > > > Is this necessary?
> > > > 
> > > > Without that, if we attempt to send an empty message, it sends the word
> > > > "undefined"; maybe some character remains on the message that makes this
> > > > behavior.
> > > > 
> > > > What do you think about this?
> > > 
> > > "'content.push('')" is probably more readable.
> > > 
> > > Also, please check how this behaves when you add a subject (both empty and
> > > non-empty) and  this consequently converts to MMS (when non-empty). We might
> > > want to do "if (!content.length) { content.push(''); }" instead. And maybe
> > > move this to Compose.getContent (but in that case we need to check that
> > > other consulers like drafts still work fine).
> > 
> > Ok, there will be more clever if we change that on compose.js.
> > 
> > This is the isEmpty method of Compose class:
> > 
> > get isEmpty() {
> >       return !dom.subject.value.length;
> >     }
> 
> Nope, this is the "isEmpty" for the "subject" subobject :)
> 
> There is another "isEmpty" method for the whole "Compose" object ;)
> 
> > 
> > !content.length and content.isEmpty have the same result.
> 
> Not really, search for "state.empty" in the file.
> 
> > 
> > When trying to send an empty MMS, an error occurs:
> > "There is a problem with the attached file and it cannot be downloaded"
> 
> Actually, this is misleading, because this is what's happening when we
> _display_ the MMS with an empty content.
> 
> That said, I really thought this didn't happen nowadays... Have you tried to
> send a MMS with a subject but no content (that is bug 948958, so you may
> want to rebase with a newer master if you're seeing this) or a MMS with
> empty subject and no content (which is not possible before your patch is
> done, and should not be a MMS) ?
> 
> > 
> > Should we may be able to send an empty MMS?
> > I think this problem is generated here:
> > var smilSlides = content.reduce(thui_generateSmilSlides, []);
> 
> I think this is not the problem here.

Please, forget what I wrote above...

When I try to send an MMS with no content and some subject, it's sends normally.

> !content.length and content.isEmpty have the same result.

What I meant with this is that I had the same behavior when sending an empty message (It sends correctly).

You're right (I don't know why I got confused with that :S), the method "isEmpty" that returns "state.empty" is the one that are we looking for.

So, what is your opinion of "!content.lenght" instead of "content.isEmpty"?
Do you want to change the if condition directly, or do another thing?

I will see the adding of the mock tests :).
Discussed on IRC, "if (!content.length)" is the way to go!
Hey Nicolas, I wanted to make sure you're still working on this :)

Thanks !
Flags: needinfo?(ndel314)
Mentor: waldron.rick
Whiteboard: [mentor=rwaldron]
Hello, sorry for my late response.

The patch that I have submitted solves the issue, and I guess it has the minimal changes to fix the bug, however I need to write some mock test to it.

Anyone could give me a hint on this (i.e., what kind of tests have I to write)?
Also, how could I test it to see which tests are breaking?

Thanks!
Flags: needinfo?(waldron.rick)
Flags: needinfo?(ndel314)
Flags: needinfo?(felash)
The easiest is to follow what's in [1].

So basically:
* install firefox nightly
* set the FIREFOX environment variable to this install
* run bin/gaia-test (possibly with "-f" if you want to force creating a new profile)
* run (in a separate window): APP=sms make test-agent-test

Then you should see the unit tests that are failing (or maybe not).

The unit tests for the SMS app are located in the directory "apps/sms/test/unit", you'll see there is one test file per js file. You need to find the correct place to change here.

One of my rule is that every app change needs to have a unit test change :)

Don't hesitate to request some help if you need it, sharing your pull request :)

[1] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests#Launching_the_test_runner_in_Firefox
Mentor: waldron.rick → felash
Flags: needinfo?(waldron.rick)
Flags: needinfo?(felash)
Hi Nicolas
Are you still working on this?
Thanks
Flags: needinfo?(ndel314)
Flags: needinfo?(rishav006)
Assignee: ndel314 → nobody
Flags: needinfo?(rishav006)
Flags: needinfo?(ndel314)
Hi Jenny 
As ayaman opinion was very long ago, julien suggested me to discuss this with you.
What's your opinion on this? Do we need this feature?
Thanks
Flags: needinfo?(jelee)
Hi Kumar,
Given the recent change of 2.2 planning, the UX team will evaluate all feature and UI requests after v3 planning, so I'll revisit the topic until then, thanks for understanding!
Flags: needinfo?(jelee)
Hey Jenny,

I don't think we need to wait for v3 planning to answer the question for this simple feature request from a user. To make the story short: the user needs this feature in order to get balance from his operator. I definitely support doing this, but I'd like to have your stamp before doing so, in case you have serious concerns.

Thanks a lot :)
Flags: needinfo?(jelee)
Hi Julien
Is there any operator that gives balance info after sending empty message.(not talking about balance showed after reducing of message charge)
Thanks
This is what the reporter is saying in comment 0 :)
ni? Elio to give more detail on user story and on comment 57.

Thanks
Flags: needinfo?(ping)
Hi there!
Unfortunately I cant remember the details, but I remember I couldnt get my balance via SMS, due to this.
I have changed my plan and mobile phone now, so cant really replicate it as it has been a long time.

It was Eagle Mobile in Albania btw.
Flags: needinfo?(ping)
(In reply to Julien Wajsberg [:julienw] from comment #56)
> Hey Jenny,
> 
> I don't think we need to wait for v3 planning to answer the question for
> this simple feature request from a user. To make the story short: the user
> needs this feature in order to get balance from his operator. I definitely
> support doing this, but I'd like to have your stamp before doing so, in case
> you have serious concerns.
> 
> Thanks a lot :)

My only concern would be sending out accidental empty message that cost money, but since sending empty message has actual function, I don't think we should block it. Please go ahead and implement this feature ;) !
Flags: needinfo?(jelee)
Just to confirm relevance of this feature one more time :) I've seen some custom partner code recently. And they provide this feature with confirmation dialog to eliminate accidentally sent empty messages.
Hello,
I would like to work on this bug,
Please assign this bug to me,
Thank you.
Assignee: nobody → anusha91rao
See comment 52 for information about how to run unit tests, and note that there is a 'work in progress' patch attached to this bug too. So you might want to start from this patch, unless it's too old and does not work anymore.

Thanks!
Anusha, are you still working on this? Feel free to comment or come over irc, if you need any help.
Flags: needinfo?(anusha91rao)
Yes was caught up with something,
Will keep you updated .
Flags: needinfo?(anusha91rao)
Assignee: anusha91rao → nobody
Hi Anusha,
It's been long time, any update on the patch? ping on irc if you need any help?
Assigning to nobody so that other can pick it up :)
Thanks
Flags: needinfo?(rishav006)
Hey Julien,

This is long pending bug, want to work on this.
But before starting i need STR and few clarification (i read previous comments, but still want to confirm again, also discussion is too old).
 
1. what does it mean by empty message. Empty message == empty string i.e ''  ?
2. What will receiver receive on the other end. Undefined or  empty message or something else ?

It will be great if we get output from other device which support this functionality (i.e country/device/operator which support empty message to get balance info). (it will be great if we can have screenshot or video otherwise okay).

I am still afraid of gecko end. How it handle this. Will it allow this. Do we need gecko changes ? 

[Discussion was very old, so i want to get update on it before starting :)]

Thanks
Flags: needinfo?(rishav006) → needinfo?(felash)
Assignee: nobody → rishav006
(In reply to kumar rishav (:rishav_) from comment #69)
> It will be great if we get output from other device which support this
> functionality (i.e country/device/operator which support empty message to
> get balance info).

See comment 2.
Yup, I saw that comment and related discussion too.
But what i was asking here is, what user will receive at other end. Let's say, i sent a empty message to my friend, what he will receive.

Thanks
(In reply to kumar rishav (:rishav_) from comment #69)
> Hey Julien,
> 
> This is long pending bug, want to work on this.
> But before starting i need STR and few clarification (i read previous
> comments, but still want to confirm again, also discussion is too old).
>  
> 1. what does it mean by empty message. Empty message == empty string i.e ''
> ?

Yes :)

> 2. What will receiver receive on the other end. Undefined or  empty message
> or something else ?

Empty message !

> 
> It will be great if we get output from other device which support this
> functionality (i.e country/device/operator which support empty message to
> get balance info). (it will be great if we can have screenshot or video
> otherwise okay).

I hope you can do without ;)

> I am still afraid of gecko end. How it handle this. Will it allow this. Do
> we need gecko changes ? 

It's easy to check ;)

> 
> [Discussion was very old, so i want to get update on it before starting :)]

Thanks !

Please ping me if you need anything more :)
Flags: needinfo?(felash)
I guess, we will need UI/UX for prompt box .

Thanks
Flags: needinfo?(felash)
(In reply to kumar rishav (:rishav_) from comment #73)
> I guess, we will need UI/UX for prompt box .
> 
> Thanks

Redirect to UX Bryant.

Hi Bryant, could you please review the old decision again and see if there's any missing? Thanks!
Flags: needinfo?(felash) → needinfo?(bmao)
The prompt box is the same as any other we have in the system already...
(In reply to Steve Chung [:steveck] from comment #74)
> (In reply to kumar rishav (:rishav_) from comment #73)
> > I guess, we will need UI/UX for prompt box .
> > 
> > Thanks
> 
> Redirect to UX Bryant.
> 
> Hi Bryant, could you please review the old decision again and see if there's
> any missing? Thanks!

Wow, Its a long discussion @@. I agree on the decision of allowing user to send empty message since it has its necessary function. Ayman's design suggestion is quite clear to me, and the prompt message is fine too.

Kumar, do you need anything from me for this implementation?
Flags: needinfo?(bmao) → needinfo?(rishav006)
Thanks Bryant :)
Flags: needinfo?(rishav006)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

Hi Julien,
Here is the patch. Have a look. Hope it's ok. Yet to add comments though.

Thanks
Attachment #8680467 - Flags: feedback?(felash)
Attachment #8680467 - Flags: feedback?(felash) → review?(felash)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

Hey Oleg, do you have the bandwidth to review this ?

Thanks !
Attachment #8680467 - Flags: review?(felash) → review?(azasypkin)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

Left several comments at github, but overall looks good + please rebase your PR on the latest master, it seems there are some integration tests that are failing right now with the PR.

Thanks!
Attachment #8680467 - Flags: review?(azasypkin)
Status: NEW → ASSIGNED
Attachment #8680467 - Flags: review?(azasypkin)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

One recommendation at GitHub and please add one more unit test for the case when user doesn't confirm sending of an empty message. Also take a look at Treeherder - looks like unit tests are failing with your PR.

Please add your changes as a separate commit :)

Thanks!
Attachment #8680467 - Flags: review?(azasypkin)
Attached image empty-message.png
Hey Bryant,

Could you please confirm that an empty message looks acceptable from UX POV :)

Thanks!
Attachment #8692637 - Flags: ui-review?(bmao)
Attachment #8680467 - Flags: review?(azasypkin)
Sorry for the delay Rishav, just stumbled upon one more case where I'd like to have UX opinion.

Hey Bryant, here is question for you:

* Should we hide "Select text" context menu item for the empty messages? It's a bit confusing when you choose this menu item and nothing happens :)

P.S. "Forward" menu item for such messages basically opens empty New Message view, but maybe it's still better to have this option just for the consistency :)

Thanks!
Flags: needinfo?(bmao)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

Removing r? for now, let's wait for Bryant's replies first to have less review cycles :)

And the code looks good to me btw.
Attachment #8680467 - Flags: review?(azasypkin)

(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #84)
> Sorry for the delay Rishav, just stumbled upon one more case where I'd like
> to have UX opinion.
> 
> Hey Bryant, here is question for you:
> 
> * Should we hide "Select text" context menu item for the empty messages?
> It's a bit confusing when you choose this menu item and nothing happens :)
> 
> P.S. "Forward" menu item for such messages basically opens empty New Message
> view, but maybe it's still better to have this option just for the
> consistency :)
> 
> Thanks!

Hi Oleg,

Sorry for the late reply. Here are some of my suggestions:

1) Empty message looks perfect.
2) Do we have disable state for the menu item? if yes, i think it would be better to keep the select text in disable state.
3) Agree!

let me know if there are any problems ;)
Flags: needinfo?(bmao)
(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #84)
> 
> * Should we hide "Select text" context menu item for the empty messages?
> It's a bit confusing when you choose this menu item and nothing happens :)
> 
 IMO, it's fine to keep like this to maintain consistency. Also, if message is empty, then select text will also be empty, and it's obvious. Also, hardly anyone think about select text in case of message empty.

Thanks!

Sorry for delay .
Flags: needinfo?(azasypkin)
(In reply to kumar rishav (:rishav_) from comment #87)
> (In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #84)
> > 
> > * Should we hide "Select text" context menu item for the empty messages?
> > It's a bit confusing when you choose this menu item and nothing happens :)
> > 
>  IMO, it's fine to keep like this to maintain consistency. Also, if message
> is empty, then select text will also be empty, and it's obvious. Also,
> hardly anyone think about select text in case of message empty.

My main point was that when user chooses "Select text" for the empty message nothing happens that may look like a bug, but we don't have support for disabled menu items right now, so let's keep it like this and improve later if needed.
Flags: needinfo?(azasypkin)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

Review? as It's OK from Bryant.
Attachment #8680467 - Flags: review?(azasypkin)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

Sorry for the delay!

I've added some nits, but delaying r+ because of perma-red SMS notification and new_activity integration tests - please rebase your PR on the latest master (you can now squash all your commits into one if you prefer) and run integration tests.

Also keep in mind that new_activity and share_activity tests are disabled on Treeherder, but you can run them locally to make sure that they are green (local env should not be affected by intermittent issues). Sorry for such complexity :/

As soon as you're ready I'm at your disposal, without delay this time :)

Thanks a lot!
Attachment #8680467 - Flags: review?(azasypkin)
Forgot to mention that for new_activity test you may want to wait for Steve's PR in bug 1235842 (it's about to land) - it fixes the reason why new activity tests are failing permanently.
Comment on attachment 8692637 [details]
empty-message.png

Setting ui-review+ (see Bryant's comment 86).
Attachment #8692637 - Flags: ui-review?(bmao) → ui-review+
Attachment #8680467 - Flags: review?(azasypkin)
Comment on attachment 8680467 [details] [review]
[gaia] kumarrishav:Bug-935060 > mozilla-b2g:master

Sorry, but it looks like Gij 17 [1] is failing with your PR (notification tests in particular [2]) :/

Have you tried to run integration tests locally (to include temporarily disabled tests as well)?

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=e0de0f0eb3cfaee646d2b0219f6883c51751561f

[2] https://public-artifacts.taskcluster.net/K275-mnZTiCYPZqPi4Os1g/3/public/logs/live_backing.log
Attachment #8680467 - Flags: review?(azasypkin)
Hi Oleg,
But it passed on my local branch. See the attached image.
Why this weird behavior ? 

Thanks
(And now, you have my screenshot. :P. You can see my Username is Ultron and i am trying my hands on Visual studios ;) ha ha ha )
Flags: needinfo?(azasypkin)
(In reply to kumar rishav (:rishav_) from comment #95)
> Created attachment 8711370 [details]
> Screenshot from 2016-01-24 03-34-19.png
> 
> (And now, you have my screenshot. :P. You can see my Username is Ultron and
> i am trying my hands on Visual studios ;) ha ha ha )

Nice! Could you please run new_activity and share_activity tests locally as well? And if they are green + green Treeherder - set r?! :)
Flags: needinfo?(azasypkin)
Mass closing of Gaia::SMS bugs. End of an era :(
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: