949801 - [B2G][MMS] Service currently unavailable is prompted when deleting a MMS that is in the process of being sent

Status: RESOLVED FIXED

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

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: ServiceUnavailableMMS.txt

Description:
If the user deletes a MMS while it is in the process of being sent to another phone, they will be prompted "Service currently unavailable. Message will automatically be sent once service is available."

Repro Steps:
1)  Updated Buri to Build ID: 20131212004004
2)  Tap on the "SMS app" icon.
3)  Tap on the "Compose new SMS" icon.
4)  Enter in a valid phone number into the "To" field.
5)  Tap on the "Paperclip and tap on "Video". 
6)  Tap on a video to attach it to the SMS and then tap "Send".
7)  While the MMS is sending, immediately tap the edit button in the upper right corner of the screen. 
8)  Select the MMS that is being sent and tap on "Delete".
9)  Tap on "OK" and observe the message.

Actual:
The user will be prompted the an incorrect message "Service currently unavailable. Message will automatically be sent once service is available."

Expected:
The MMS is deleted without issue and the incorrect message is not prompted to the user. 

Environmental Variables
Device: Buri v 1.2.0 COM RIL
Build ID: 20131212004004
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8bae10bb0aed
Gaia: 6d02039072a2ae5cf9225a6f4c78ed49decfab5c
Platform Version: 26.0
RIL Version: 01.02.00.019.102 

Notes:
Repro frequency: 100%
See attached: video clip, logcat

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue also occurs on Buri v 1.1.0 COM RIL 

Environmental Variables
Device: Buri v 1.1.0 COM RIL
Build ID: 20131210041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/05117f42088f
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Platform Version: 18.1
RIL Version: 01.01.00.019.281 

The error message "Service currently unavailable" is prompted to the user when they delete a MMS while it is in the process of being sent.

Comment 2

[Julien Wajsberg [:julienw]]

RIL issue.

Gene, I remind that you fixed a similar issue for SMS a long time ago. Could you take a look?

Comment 3

[Jason Smith [:jsmith]]

Does this reproduce on the 1.2 Moz RIL?

Comment 4

[Julien Wajsberg [:julienw]]

Here I'm quite sure this is happening the same on Moz RIL and CommRIL.

Comment 5

[Jason Smith [:jsmith]]

Okay - saw this is reproducing on 1.1 as well, so no need to do additional investigation here.

Comment 6

[Gene Lian (I already quit Mozilla)]

I think Gecko already fixed it at bug 880563. In this case of deleting when sending, we'll return 

  Ci.nsIMobileMessageCallback.NOT_FOUND_ERROR

which is expose to Gaia as "NotFoundError".

I wonder Gaia doesn't handle that properly. Temporally, reassign this to Gaia. Please feel free to assign back if I'm wrong. Thanks!

Comment 7

[Julien Wajsberg [:julienw]]

The error message changed in 1.3 with bug 928330, here are the current error messages for this error:

sendNotFoundErrorTitle      = Message not found
sendNotFoundErrorBody       = The message you're looking for is no longer available.

Gene, do you think this is incorrect?

I think that when deleting when sending, we should not get this error. This error is sent when we try to access ("get") a message that is deleted. In this case, we're deleting a message that is being sent, so getting a "not found" error is strange, because the message was not existing prior the call.

I'd like to ask Ayman what would be the ideal behavior here.

Comment 8

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> The error message changed in 1.3 with bug 928330, here are the current error
> messages for this error:
> 
> sendNotFoundErrorTitle      = Message not found
> sendNotFoundErrorBody       = The message you're looking for is no longer
> available.
> 
> Gene, do you think this is incorrect?
> 
> I think that when deleting when sending, we should not get this error. This
> error is sent when we try to access ("get") a message that is deleted. In
> this case, we're deleting a message that is being sent, so getting a "not
> found" error is strange, because the message was not existing prior the call.
> 
> I'd like to ask Ayman what would be the ideal behavior here.

Hey Julien. If a user deletes a message whilst it is in the process of being sent the user should not receive any messages/notifications associated to the message that is being deleted outside of the expected messages that are associated to the success or failure of the delete action.

Comment 9

[Julien Wajsberg [:julienw]]

Ok, that's my thinking as well.

Therefore Gene, we'd need either another Error value that we could ignore, or no Error at all here :)

Comment 10

[Gene Lian (I already quit Mozilla)]

My originally thought is "NotFoundError" can be interpreted as a kind of send failure because the message we want to send it no longer available due the delete. It means we interpret "NotFoundError" in different ways based on different operations (e.g., send v.s. get). Is that possible for Gaia to do so?

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Therefore Gene, we'd need either another Error value that we could ignore,
> or no Error at all here :)

Sounds strange to me if we don't return any error or success for the send request. I'd prefer returning another kind of error: "MessageDeletedError" if "NotFoundError" on send request doesn't make sense to you.

What do you think?

Comment 11

[Julien Wajsberg [:julienw]]

I've been thinking over the night and it's probably just as easy to special case the error when sending.

So let's do it only in Gaia, no need for another error. Thanks a lot Gene!

Comment 12

[Julien Wajsberg [:julienw]]

For a contributor:

* the code is in line 1939 in js/thread_ui.js, on the "onError" handler for the "sendMMS" call

* if errorName is "NotFoundError", we should not call showMessageError. We should probably call "console.info" with a meaningful log and just return.

* the most important part is in the tests: you'll need to add a test in test/unit/thread_ui_test.js, on the part "onSendClick" behavior. I'd like to convert some of the use of manual "mocks" to sinon, and it will make things easier to test the "onerror" call using spy.yield (see http://sinonjs.org/docs/#spies). I can help on this part if necessary.

Comment 13

[Guilherme Uhelski]

Hello there!

Is it still up for grabs? If so I would really like to work on it as my first contribution.

Comment 14

[Gene Lian (I already quit Mozilla)]

Yes! Please go ahead. :)

Comment 15

[Julien Wajsberg [:julienw]]

Hey Guilherme,

yes please! Don't hesitate to ask on IRC or here if you need anything!

Comment 16

[Guilherme Uhelski]

Hey Julien.

The fix was easy, but I can't get my head around sinonjs (yet!). I know that we need to mock the "NotFoundError" and assert that the console.info was called with the message.

This is what I got so far: https://github.com/guhelski/gaia/commit/ab6c21a239d87d3907c930ad0d595ab5735c2ac0

Also, do you think the added message is meaningful enough?

Comment 17

[Julien Wajsberg [:julienw]]

(adding a marker so that I don't lose track of this)

Comment 18

[Julien Wajsberg [:julienw]]

So, if you go in the "onSendClick" suite in the thread_ui_test file, you should be able to do something like this:

* start a new suite "sendMMS errors" _inside_ the onSendClick suite
* add a setup with :

  this.sinon.spy(MockMessageManager, 'sendMMS');
  this.sinon.spy(ErrorDialog.prototype, 'show');
  .. set up a MMS in the composer
  .. call onSendClick

* add 2 tests, one called "NotFoundError" with:

  MockMessageManager.sendMMS.callsArg(2, { name: "NotFounderror" });
  sinon.assert.notCalled(ErrorDialog.prototype.show);

one called "generic error" with:

  MockMessageManager.sendMMS.callsArg(2, { name: "GenericError" });
  sinon.assert.called(ErrorDialog.prototype.show);

This code might need changes to work correctly ;)

To see whether this is really right, the new "NotFoundError" should fail if you comment your new code in thread_ui.js, and pass if you have the new code :)

Hope this helps!

Note that we'll be away for 2 weeks and we come back January 6th, so don't worry if you don't see any sign of us.

Comment 19

[Julien Wajsberg [:julienw]]

Sorry, replace "callsArg" by "callArg" in the previous code.

Comment 20

[Julien Wajsberg [:julienw]]

Hey Guilherme, were you able to move forward during the christmas holiday? :)

Comment 21

[Guilherme Uhelski]

Hey Julien!

Sorry for the late reply, I'm still in transit due to the holidays but as soon as I get the chance to touch my machine I'll let you know how the progress went.

Comment 22

[Guilherme Uhelski]

Julien.

Looks like we have some green :)

I've add the tests: https://github.com/guhelski/gaia/commit/ddeeea53279ab330299b3df8bb6eb6042394f22e and they are all passing.

Please let me know if you think they are good to go, so I'll make it into a single commit and create a new pull request.

Comment 23

[Julien Wajsberg [:julienw]]

Hey Guilherme,

Looking quickly at the test code, I think it's better to call "click" on the send button element rather than call "onSendClick" on ThreadUI, because this is closer to what happens in real life (from the outside), and also tests that the event handler is correctly attached. But otherwise it looks great!

Can you create a pull request already? You can attach it on the bug using the link "paste text as attachment" and put directly the pull request URL. I don't mind if it's one or 2 commits at this stage, but you can make it one single commit to make things easier.

Thanks a lot !

Comment 24

[Guilherme Uhelski]

Attachment: Pull request

Comment 25

[Guilherme Uhelski]

Hey Julien.

I've made the change you mentioned and created the pull request.

Thank you for the awesome help, if everything is good I think it's time to move to the next one :)

Comment 26

[Guilherme Uhelski]

Attachment: Pull request

gjslint complained about double quoted strings on the previous pull request. I didn't know that I could squash all the commits into the same PR... so I ended up creating a new one.

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8357456 [details] [review]
Pull request

looks fine :) r=me with the last nit I ask

This means you just need to fix the comment I added on github, do one single commit, add "r=julienw" at the end of the first line in the commit log, and push on your pull request.

Some git commands that can be useful for you:
* use "git commit --amend" to modify your HEAD. This is useful for very simple changes or changing the commit log. However don't use it too much, it's still better to do individual commits and use git rebase -i to squash (see later).

* use "git rebase -i <base hash>" to rewrite your history, squash commits, remove commits

* use "git push -f HEAD" to rewrite the history of your current branch, but be careful, this should _not_ become your default command because you will break stuff

Once you're ready, just add 'checkin-needed' in the Keywords part of the bug. You can ask me review or feedback again if you don't feel comfortable with your changes.

Bravo for your first bug :)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/3a0747d4774493d8c0864989063928f3fb915e2d

Comment 30

[Julien Wajsberg [:julienw]]

Duplicate Bug 976495 was minused 1.3-, but I'm renominating here on the basis that we have a working patch, in case it would change the blocking decision.

Comment 31

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #30)
> Duplicate Bug 976495 was minused 1.3-, but I'm renominating here on the
> basis that we have a working patch, in case it would change the blocking
> decision.

It won't change the blocking decision. Having a patch, even if low risk, can't affect a blocking decision. We actually need a rationale why this exact bug would block the release.

However - this bug in particular isn't a regression from a partner release, so this isn't a blocker.

Comment 32

[田旻]

This pr has been fixed on v1.3 by your patch.

Comment 33

[Julien Wajsberg [:julienw]]

Seems like partners used it in v1.3, should we do the same to stay sync-ed? (see previous comment)

Comment 34

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> Seems like partners used it in v1.3, should we do the same to stay sync-ed?
> (see previous comment)

Partners can chose to take the patch in their own branch, but I don't think that changes the blocking decision to take a patch to 1.3 generally. This isn't a regression, so we will not block on this.