Closed
Bug 949801
Opened 11 years ago
Closed 10 years ago
[B2G][MMS] Service currently unavailable is prompted when deleting a MMS that is in the process of being sent
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(tracking-b2g:backlog, b2g18 affected, b2g-v1.2 affected)
People
(Reporter: KTucker, Assigned: guhels)
References
Details
(Whiteboard: [fxos-bug-bash-1.2][mentor=:julienw])
Attachments
(2 files, 1 obsolete file)
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
Reporter | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
RIL issue. Gene, I remind that you fixed a similar issue for SMS a long time ago. Could you take a look?
Component: Gaia::SMS → RIL
Flags: needinfo?(gene.lian)
Comment 4•11 years ago
|
||
Here I'm quite sure this is happening the same on Moz RIL and CommRIL.
Comment 5•11 years ago
|
||
Okay - saw this is reproducing on 1.1 as well, so no need to do additional investigation here.
Keywords: qawanted
Comment 6•11 years ago
|
||
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•11 years ago
|
||
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.
Flags: needinfo?(aymanmaat)
Comment 8•11 years ago
|
||
(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.
Flags: needinfo?(aymanmaat)
Comment 9•11 years ago
|
||
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 :)
Flags: needinfo?(gene.lian)
Comment 10•11 years ago
|
||
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?
Flags: needinfo?(gene.lian) → needinfo?(felash)
Comment 11•11 years ago
|
||
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!
Flags: needinfo?(felash)
Whiteboard: [fxos-bug-bash-1.2] → [fxos-bug-bash-1.2][mentor=:julienw]
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
Hello there! Is it still up for grabs? If so I would really like to work on it as my first contribution.
Updated•11 years ago
|
Assignee: nobody → guhels
Comment 14•11 years ago
|
||
Yes! Please go ahead. :)
Comment 15•11 years ago
|
||
Hey Guilherme, yes please! Don't hesitate to ask on IRC or here if you need anything!
Assignee | ||
Comment 16•11 years ago
|
||
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•11 years ago
|
||
(adding a marker so that I don't lose track of this)
Flags: needinfo?(felash)
Comment 18•11 years ago
|
||
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.
Flags: needinfo?(felash)
Comment 19•11 years ago
|
||
Sorry, replace "callsArg" by "callArg" in the previous code.
Comment 20•10 years ago
|
||
Hey Guilherme, were you able to move forward during the christmas holiday? :)
Assignee | ||
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
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•10 years ago
|
||
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 !
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
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 :)
Assignee | ||
Comment 26•10 years ago
|
||
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.
Attachment #8356841 -
Attachment is obsolete: true
Attachment #8357456 -
Flags: review?(felash)
Comment 27•10 years ago
|
||
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 :)
Attachment #8357456 -
Flags: review?(felash) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 28•10 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/3a0747d4774493d8c0864989063928f3fb915e2d
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Comment 30•10 years ago
|
||
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.
blocking-b2g: --- → 1.3?
Comment 31•10 years ago
|
||
(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.
blocking-b2g: 1.3? → backlog
Comment 32•10 years ago
|
||
This pr has been fixed on v1.3 by your patch.
Comment 33•10 years ago
|
||
Seems like partners used it in v1.3, should we do the same to stay sync-ed? (see previous comment)
blocking-b2g: backlog → 1.3?
Comment 34•10 years ago
|
||
(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.
blocking-b2g: 1.3? → backlog
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•