Closed Bug 1136147 Opened 5 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

(b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 affected, b2g-master affected)

RESOLVED WONTFIX
Tracking Status
b2g-v1.4 --- affected
b2g-v2.0 --- affected
b2g-v2.0M --- affected
b2g-v2.1 --- affected
b2g-v2.1S --- affected
b2g-v2.2 --- affected
b2g-master --- affected

People

(Reporter: denver, Assigned: farhaan.bukhsh, Mentored)

References

Details

(Whiteboard: [sms-most-wanted][lang=js][contrib-bzrand])

Attachments

(3 files)

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 "+883510000000094".  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.
No longer blocks: 1136211
Depends on: 1136211
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.
Status: UNCONFIRMED → NEW
Component: Gaia::SMS → RIL
Ever confirmed: true
Whiteboard: [sms-most-wanted]
Maybe a contributor will want to have a look to the UI issue. We should not clear the composer when we get an error.
Mentor: felash
Whiteboard: [sms-most-wanted] → [sms-most-wanted][lang=js]
Hi, I am new to firefox OS how can i start contributing? I want to try my hands on this bug
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 !
Assignee: nobody → farhaan.bukhsh
Summary: SMS cannot be sent to a 15-digit phone number (such as an iNum) → We should not erase the composer before sending the message
Summary: We should not erase the composer before sending the message → [Messages] We should not erase the composer before sending the message
Depends on: 1149802
This bug is about the UX behavior in the SMS application, fixing the flags and components.
Component: RIL → Gaia::SMS
No longer depends on: 1136211, 1149802
See Also: → 1136211
I cloned gaia but i am not able to run it on mac can you please help me with that !!
Flags: needinfo?(felash)
Farhaan, did you follow the steps outlined in https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md ?
Flags: needinfo?(felash)
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
Got it working starting the work on the bug now :)
I tried to follow steps in comment 1 to replicate the bug but I am getting a proper thread
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
Hey Farhaan, how is it going ? :)
Flags: needinfo?(farhaan.bukhsh)
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?
Flags: needinfo?(farhaan.bukhsh)
Heh no problem, I will be there :) I wanted to be sure you were still working on this. No pressure !
Attached image console logs
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.
Flags: needinfo?(felash)
Hey Farhaan, did you follow the information in [1]?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md
Flags: needinfo?(felash)
Yeah I did all that , i got gaia working the only issue i am facing is in duplicating the bug.
Flags: needinfo?(felash)
Yeah I did all that , i got gaia working the only issue i am facing is in duplicating the bug.
The best would be to create a pull request so that I can see your changes more easily :)
Flags: needinfo?(felash)
cool but i didn't make any significant changes, and i didn't forked it either what should i do?
Flags: needinfo?(felash)
Julien , I waiting for your reply can you guide me please??
Flags: needinfo?(felash)
Flags: needinfo?(felash)
Sorry Farhaan I was fairly busy lately. Will look at this again tomorrow, I promise.
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 !
Flags: needinfo?(felash)
Hey Julien , I sent a PR to the repo, if I did something wrong do let me know.
Flags: needinfo?(felash)
Can you please give me the URL to your PR ?
Flags: needinfo?(felash)
https://github.com/mozilla-b2g/gaia/pull/31179

Sorry, i should have sent you the url.
Flags: needinfo?(felash)
Hey julien, Did I do anything wrong ?
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 !
Flags: needinfo?(felash)
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
Flags: needinfo?(felash)
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 !
Flags: needinfo?(felash)
Hey Julien , I did all that and I got positive results now what should I do?
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
Hey Julian , can you be a little elaborate on how to unit test please.
Hey Julian , can you be a little elaborate on how to unit test please.
Where in conversation_test I should call  sinon.assert.notCalled(Compose.clear)  ?
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/
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.
Duplicate of this bug: 1203642
Hey Farhaan, could you find out how to write these tests ?
Flags: needinfo?(farhaan.bukhsh)
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.
Flags: needinfo?(farhaan.bukhsh)
No problem, I was only checking how you were going. No pressure !
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?
:( I made efforts, but again you gave me . Sorry Julien
It's ok because you tried to look by yourself first ! And you still have to find how to use Sinon ;)
But i wanted to do this myself . :( hmm, cool atleast i will try to figure out that now. :)
I sent a pull request I guess its done can you look into it Julien.
https://github.com/mozilla-b2g/gaia/pull/31179/commits
Flags: needinfo?(felash)
Hey Farhaan,

I left some comments on github, please look at them !
Flags: needinfo?(felash)
hey Julien,
Sorry for the goof ups , I tried to clean the code and make unit test right. Can you please check now. :)
Flags: needinfo?(felash)
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...
Flags: needinfo?(felash)
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
Whiteboard: [sms-most-wanted][lang=js] → [sms-most-wanted][lang=js][contrib-bzrand]
Hey Farhaan, how is it working ? Please tell me if you still intend to work on this. No pressure though !
Flags: needinfo?(farhaan.bukhsh)
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 ?
Flags: needinfo?(farhaan.bukhsh)
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 ?
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 3 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.