1136147 - [Messages] We should not erase the composer before sending the message

Status: RESOLVED WONTFIX

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

Description

[Denver Gingerich]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/37.0.2062.120 Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

Using Firefox OS 1.3 on a ZTE Open C with Build Identifier 20140523093151:
1. Open the SMS app, click the Compose icon.
2. In the "To:" field, enter any 15-digit phone number.  I tried the iNum test number (see http://www.inum.net/test-sms-and-voice/ ), both as "883510000000094" and "[phone]".  The results were the same in both cases.
3. Type a message in the Message box (I entered my email address).
4. Press Send.


Actual results:

The SMS application returned to an empty Compose screen and no message was sent.

Note that the results are the same if you first add a contact with the 15-digit phone number and attempt to send an SMS to that contact, rather than just typing the number into the "To:" field.


Expected results:

The SMS should have appeared in the message history of the SMS application, and the SMS should have been sent to the iNum test number.  I know my carrier supports these, as I have sent an SMS to the iNum test number before with a different phone and it sent me an email indicating receipt.

It's possible this is related to https://bugzilla.mozilla.org/show_bug.cgi?id=899961 though they seem slightly different.  Also, this issue happens with valid E.164 phone numbers, not just 20+-digit numbers.

Comment 1

[Julien Wajsberg [:julienw]]

Thanks for your report !

I filed bug 1136211 for the Gecko issue, and I keep this one to address the bad behavior in the UI, as we should not clear the composer.

Comment 2

[Julien Wajsberg [:julienw]]

Maybe a contributor will want to have a look to the UI issue. We should not clear the composer when we get an error.

Comment 3

[farhaan]

Hi, I am new to firefox OS how can i start contributing? I want to try my hands on this bug

Comment 4

[Julien Wajsberg [:julienw]]

Hey farhaan, thanks for the proposal !

First of all, I'll advise to follow the guide at [1].

The problem here is that we call cleanFields in [2]. This effectively erase whatever is in the composer. The result is that if there is an error we don't have the message body anymore.

In my opinion, we shouldn't use cleanFields here. Rather we should call Compose.disable().
With this change, we need to check whether double clicking on the Send button is properly prevented.

[1] https://github.com/mozilla-b2g/gaia/tree/master/apps/sms#messages-the-official-messaging-application-for-firefox-os
[2] https://github.com/mozilla-b2g/gaia/blob/4894a491da3e68de17b873ca48db9519fd638628/apps/sms/js/thread_ui.js#L2210


I'm in holiday next week but if you have any question please come on IRC (irc.mozilla.org/#gaia-messaging) and you'll likely find somebody to help you.
Thanks again !

Comment 5

[Julien Wajsberg [:julienw]]

This bug is about the UX behavior in the SMS application, fixing the flags and components.

Comment 6

[farhaan]

I cloned gaia but i am not able to run it on mac can you please help me with that !!

Comment 7

[Julien Wajsberg [:julienw]]

Farhaan, did you follow the steps outlined in https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md ?

Comment 8

[farhaan]

I followed all steps I am stuck on 
FIREFOX -profile profile-sms --no-remote app://sms.gaiamobile.org
this step it says FIREFOX is not a command I dropped on IRC for help but no one is replying . Please Help

Comment 9

[farhaan]

Got it working starting the work on the bug now :)

Comment 10

[farhaan]

I tried to follow steps in comment 1 to replicate the bug but I am getting a proper thread

Comment 11

[Julien Wajsberg [:julienw]]

yeah, now that bug 1136211 is fixed you can't trigger the Gaia issue using the same STR.

There are several ways to simulate this.

I think the easiest is to comment the line in [1] so that we don't get the "sending" event (this event makes the SMS app move from composer to thread), and set the variable MessagesDebugError to a non-empty string to trigger "fail mode" (see [2] and [3]).

[1] https://github.com/mozilla-b2g/gaia/blob/4894a491da3e68de17b873ca48db9519fd638628/apps/sms/js/desktop-only/navigator_moz_mobilemessage.js#L1051
[2] https://github.com/mozilla-b2g/gaia/blob/4894a491da3e68de17b873ca48db9519fd638628/apps/sms/js/desktop-only/navigator_moz_mobilemessage.js#L987-L989
[3] https://github.com/mozilla-b2g/gaia/blob/4894a491da3e68de17b873ca48db9519fd638628/apps/sms/js/desktop-only/navigator_moz_mobilemessage.js#L1059-L1071

Tell me if you have any other issue.

Comment 12

[Julien Wajsberg [:julienw]]

Hey Farhaan, how is it going ? :)

Comment 13

[farhaan]

hey Julien , sorry was bit busy with my semester exams . They are getting over on 2nd and then i will probably bug you a lot. can i?

Comment 14

[Julien Wajsberg [:julienw]]

Heh no problem, I will be there :) I wanted to be sure you were still working on this. No pressure !

Comment 15

[farhaan]

Attachment: changes made to the file

Comment 16

[farhaan]

Attachment: console logs

Comment 17

[farhaan]

Hey Julien I am sending you screenshots of what i did to duplicate the bug but what i am getting is a screen saying Messages and nothing else . Please guide me.

Comment 18

[Julien Wajsberg [:julienw]]

Hey Farhaan, did you follow the information in [1]?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md

Comment 19

[farhaan]

Yeah I did all that , i got gaia working the only issue i am facing is in duplicating the bug.

Comment 20

[farhaan]

Yeah I did all that , i got gaia working the only issue i am facing is in duplicating the bug.

Comment 21

[Julien Wajsberg [:julienw]]

The best would be to create a pull request so that I can see your changes more easily :)

Comment 22

[farhaan]

cool but i didn't make any significant changes, and i didn't forked it either what should i do?

Comment 23

[farhaan]

Julien , I waiting for your reply can you guide me please??

Comment 24

[Julien Wajsberg [:julienw]]

Sorry Farhaan I was fairly busy lately. Will look at this again tomorrow, I promise.

Comment 25

[Julien Wajsberg [:julienw]]

Farhaan,

you need to follow the instructions in [1]. Especially you need to fork gaia, do a local clone, create a branch for your changes, push the branch to your fork, and start a pull request.

I also heavily suggest that you read the chapters 1 2 and 3 of [2]. You can also look at [3] once you read the other chapters.
This is important to understand how to work and contribute to Firefox OS. This will also be useful if you ever work on a large project in your future, so this is not something that is only for Firefox OS.

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch
[2] http://git-scm.com/book/en/v2
[3] http://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project


If you want me to help you, it's a lot easier to have a pull request. So that I can see your code more easily, and comment directly on the pull request to help you.

Thank you !

Comment 26

[farhaan]

Hey Julien , I sent a PR to the repo, if I did something wrong do let me know.

Comment 27

[Julien Wajsberg [:julienw]]

Can you please give me the URL to your PR ?

Comment 28

[farhaan]

https://github.com/mozilla-b2g/gaia/pull/31179

Sorry, i should have sent you the url.

Comment 29

[farhaan]

Hey julien, Did I do anything wrong ?

Comment 30

[Julien Wajsberg [:julienw]]

OK Farhaan, sorry for the delay. Last week I was in a workweek and I had to focus to some other work.

I managed to reproduce the bug with these changes: https://github.com/mozilla-b2g/gaia/commit/dca6a34fba8f68adc11ff2004bd38bbea80f046c

Then you can open the app in Firefox, enter the "new message" panel, and try to send a SMS.
You'll see we get an error dialog, and when the dialog is dismissed, you'll see that we cleaned the composer data.

This is what I'd like to fix here.

Likely the easiest is to just remove the call to `cleanFields` in `sendMessages` because we call it in lots of other places already (beforeLeave, afterLeave, initializeRendering, beforeEnterComposer). I'm sure we could rationalize this later, but this is not for this bug.

I hope it's now clearer for you !

Comment 31

[farhaan]

Hey Julien ,
 I guess it's done , can you please check and let me know.

PR: https://github.com/mozilla-b2g/gaia/pull/31179/commits

Comment 32

[Julien Wajsberg [:julienw]]

Thanks Farhaan,

now you need to:
1. rebase your work on top of latest master. This can be easily done with the following commands, assuming you have a remote named "moz" for Mozilla's gaia repository:

  git fetch moz
  git rebase moz/master

If you don't have a remote for Mozilla's gaia repository, you can simply add it this way:

  git remote add moz https://github.com/mozilla-b2g/gaia

You should not have any conflicts for your changes but in case you have some you can follow this guide: https://help.github.com/articles/resolving-merge-conflicts-after-a-git-rebase/.

After rebasing, you'll need to use (once !) the option '-f' when pushing to your own branch:

  git push -f origin HEAD

2. Fix the unit tests if necessary -- if not necessary, try to look why.
In the unit tests I'd especially like to ensure we don't call "cleanFields" at this moment.

You have instructions to run unit tests at this URL: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md#running-unit-tests

Please needinfo me in case you need more information !

Comment 33

[GitHub Autolander Bot]

Attachment: [gaia] farhaanbukhsh:master > mozilla-b2g:master

Comment 34

[farhaan]

Hey Julien , I did all that and I got positive results now what should I do?

Comment 35

[Julien Wajsberg [:julienw]]

Hey farhaan, I don't see changes to unit tests; can you try to add an assertion that cleanFields is not called ?

Look in conversation_test.js, you'll need to call sinon.assert.notCalled(Compose.clear) (with Compose.clear being stubbed.

I'll leave you try to find what yo do exactly because it's late here. If you have issues I can help later on.

Comment 36

[farhaan]

Hey Julian , can you be a little elaborate on how to unit test please.

Comment 37

[farhaan]

Hey Julian , can you be a little elaborate on how to unit test please.

Comment 38

[farhaan]

Where in conversation_test I should call  sinon.assert.notCalled(Compose.clear)  ?

Comment 39

[Julien Wajsberg [:julienw]]

Farhaan, so far, I did all the work myself. I'd like that you step up and try to understand what you're doing.

So I'll let you search by yourself for several days before helping.

You can read documentation about "sinon" at [1], this is our mocking library. Otherwise if you have questions about how unit tests work I can help you, but I don't want to do your work. I mean, I could do it, but I don't see how this is interesting for you.

So I'll leave you working on this by yourself a little more :)

[1] http://sinonjs.org/docs/

Comment 40

[farhaan]

Sorry Julien , I will try researching about it and do, what I wanted to ask is do I have to write the unit test or its already there. I really want to do it myself.

Comment 42

[Julien Wajsberg [:julienw]]

Hey Farhaan, could you find out how to write these tests ?

Comment 43

[farhaan]

Hey Julien , I looked into it then I had sessionals going in college so that took a lot of time , I was near to writing it, I will do it Julien I am getting so much to learn just give me sometime.

Comment 44

[Julien Wajsberg [:julienw]]

No problem, I was only checking how you were going. No pressure !

Comment 45

[farhaan]

So these all things I tried:s
 -I tried to find test case already written and function i can put there.
 - I learnt that stub has to called before the function you want to check
 - finally i figured out i need to write my own test in message failing suite
am I on the right track?

Comment 46

[Julien Wajsberg [:julienw]]

I think it's enough to check that it's not called at [1].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/conversation/test/unit/conversation_test.js#L5083

Comment 47

[farhaan]

:( I made efforts, but again you gave me . Sorry Julien

Comment 48

[Julien Wajsberg [:julienw]]

It's ok because you tried to look by yourself first ! And you still have to find how to use Sinon ;)

Comment 49

[farhaan]

But i wanted to do this myself . :( hmm, cool atleast i will try to figure out that now. :)

Comment 50

[farhaan]

I sent a pull request I guess its done can you look into it Julien.
https://github.com/mozilla-b2g/gaia/pull/31179/commits

Comment 51

[Julien Wajsberg [:julienw]]

Hey Farhaan,

I left some comments on github, please look at them !

Comment 52

[farhaan]

hey Julien,
Sorry for the goof ups , I tried to clean the code and make unit test right. Can you please check now. :)

Comment 53

[Julien Wajsberg [:julienw]]

I missed some important cases in the past, sorry about that.
Please tell me if the work is too complicated for you, I can understand there is quite more work than what I thought initially...

Comment 54

[farhaan]

I am enjoying working on it, over that you are mentoring me in an awesome way so I get to learn a lot.I don't mind complications at all. But why are you saying this? :P

Comment 55

[Julien Wajsberg [:julienw]]

Hey Farhaan, how is it working ? Please tell me if you still intend to work on this. No pressure though !

Comment 56

[farhaan]

I am sorry I am taking much time but my college schedule is a bit messed , i would like to continue working on this , can I ?

Comment 57

[Julien Wajsberg [:julienw]]

Yes you can, as long as you're still wishing to do it.

Do you have an idea of when you'll be able to continue ?

Comment 58

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 59

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(