Desktop client needs to reject incoming calls with a reason of "busy" if already on a call.

VERIFIED FIXED in Firefox 34

Status

enhancement
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: RT, Assigned: pkerr)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap +
qe-verify +

Firefox Tracking Flags

(firefox34 verified, firefox35 verified)

Details

(Whiteboard: p=2[loop-uplift][loop-inccall1][fig:verified])

User Story

The desktop client should return a reason of "busy" if:
* Another incoming call request is being processed but not answered yet
* The desktop client is on a call

Rough Impl (after bug 1035348 is implemented):

For the purposes of this bug, it is suggested that an "active" call is one that is in the state from where the conversation window is opened, until the call is ended or it is closed.

- MozLoopService will need to know when calls have finished, this could be via an event that MozLoopService listens to.
- For each call notification, check if a conversation window is "active"
-- if not, open a new conversation window.
-- if there is, open a websocket to the server, and reject the call with busy.

Attachments

(1 attachment, 11 obsolete attachments)

20.26 KB, patch
MattN
: review+
Details | Diff | Splinter Review
The desktop client should return a reason of "busy" if:
* Another incoming call request is being processed but not answered yet
* The desktop client is on a call

The callee won't see any notification - the ability to be notified of an incoming call whilst on a call will be addressed post MVP..
The caller will see a notification as per https://bugzilla.mozilla.org/show_bug.cgi?id=1000186
Currently these types of incoming calls are silently lost.
Adam: the user story I'm proposing here is going to require two gets to the server for the same info. I'm not convinced that is the best thing.

Either we need to split up the GET /calls into two (the call with progressUrl & then the full call info), or we see if we can find some other way of passing the full call information from the loop service to the conversation window (query params?).
User Story: (updated)
Flags: needinfo?(adam)
I think this would be a good bug to move up in priority if its solution is not too involved.
Target Milestone: mozilla34 → mozilla33
(In reply to Mark Banner (:standard8) from comment #1)
> Adam: the user story I'm proposing here is going to require two gets to the
> server for the same info. I'm not convinced that is the best thing.
> 
> Either we need to split up the GET /calls into two (the call with
> progressUrl & then the full call info), or we see if we can find some other
> way of passing the full call information from the loop service to the
> conversation window (query params?).

It would seem a bit odd to change the network API significantly just to accommodate the model we've chosen for our privilege isolation. I think we should solve this by having the service perform the GET /calls, and then store the corresponding information under a key (callId?) that is then passed in as the parameter to openChatWindow (rather than the version). The conversation window would then do something like "navagator.mozLoopAPI.getCallInfo(callId)" to get the entire structure.

Does that make sense?
Flags: needinfo?(adam)
Oh, I forgot to mention -- in addition to the service needing to know about call end events to know whether to reply to a call as busy, we also need to use these events to erase the stored call information I talk about above.
create separate (blocking) bug for get on calls from conversation window into Mozloop service.  this bug is busy notification.
Whiteboard: p=2
(In reply to sescalante from comment #5)
> create separate (blocking) bug for get on calls from conversation window
> into Mozloop service.  this bug is busy notification.

Split out to bug 1035348
User Story: (updated)
Duplicate of this bug: 1038721
Priority: P2 → P3
Target Milestone: mozilla33 → mozilla34
Target Milestone: mozilla34 → mozilla35
I discussed this with Maire yesterday, and we decided this should probably be addressed in Fx 34, as currently there is the possibility for the user to have multiple concurrent calls (or at least in a call & receive another incoming call), which we don't want at this time.
Target Milestone: mozilla35 → mozilla34
Priority: P3 → P1
Assignee: nobody → pkerr
Status: NEW → ASSIGNED
Target Milestone: mozilla34 → 34 Sprint 3- 9/1
Work in progress - must be applied after bug 1035348
Work in progress - must be applied after bug 1035348
Attachment #8482026 - Attachment is obsolete: true
WIP update
Attachment #8482030 - Attachment is obsolete: true
Attachment #8482371 - Attachment is obsolete: true
Attachment #8482386 - Flags: review?(standard8)
Comment on attachment 8482386 [details] [diff] [review]
LoopService reject with reason = busy an incoming call when busy

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

I've not tested this patch, as there's a few bits of bitrot unfortunately, however, I like the idea of where it is going, and I think this does what we want.

::: browser/components/loop/content/js/conversation.jsx
@@ +171,5 @@
>      /**
>       * @override {loop.shared.router.BaseConversationRouter.endCall}
>       */
>      endCall: function() {
> +      navigator.mozLoop.releaseCallData(this._conversation.get("callId"));

Might need to ensure we do this on window close as well, i.e. if the user clicks the 'x'.
Attachment #8482386 - Flags: review?(standard8) → feedback+
Attachment #8482386 - Attachment is obsolete: true
Attachment #8483784 - Attachment is obsolete: true
Attachment #8485198 - Flags: review?(standard8)
I'll try and look at this patch tomorrow.
Added releaseCallData checks to functions tests.
Attachment #8485198 - Attachment is obsolete: true
Attachment #8485198 - Flags: review?(standard8)
Attachment #8486172 - Flags: review?(standard8)
Whiteboard: p=2 → p=2[loop-uplift]
Iteration: --- → 35.1
Comment on attachment 8486172 [details] [diff] [review]
LoopService reject with reason = busy an incoming call when busy

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

There's a few comments below that need addressing.

What I'd really also like is to get an xpcshell test to test the busy notifications. I think you should be able to do this by using a combination of what's in test_looppush_initialize and some of the other tests (e.g. test_loopservice_dnd). If you need help or more ideas, let me know.

::: browser/components/loop/MozLoopService.jsm
@@ +110,5 @@
> +      (reason => {console.warn("MozLoopService::callProgessSocket - ", reason);});
> +
> +    // Allow _websocket to be set for testing.
> +    if (!this._websocket && !Services.io.offline) {
> +      this._websocket = Cc["@mozilla.org/network/protocol;1?name=wss"]

This needs the protocol type to be based on the scheme of the url passed in. Currently, I can't test this as-is on localhost as it tries to use the secure socket.

I would suggest moving the newURI call to before this section, and then using uri.scheme to get the right protocol type.

@@ +206,5 @@
> +    try {
> +      this._websocket.sendMsg(JSON.stringify(aMsg));
> +    }
> +    catch (error) {
> +      console.error("MozLoopService::_send - ", error);

Should this be calling this._onError?

@@ +532,5 @@
> +      // This instance of CallProgressSocket should stay alive until the underlying
> +      // websocket is closed since it is passed to the websocket as the nsIWebSocketListener.
> +      callProgress.connect(() => {callProgress.sendBusy();});
> +    }
> +    catch (error) {}

Is there a reason not to log the error here? What's the expected failure case?

::: browser/components/loop/content/js/conversation.jsx
@@ +175,5 @@
>      /**
>       * @override {loop.shared.router.BaseConversationRouter.endCall}
>       */
>      endCall: function() {
> +      navigator.mozLoop.releaseCallData(this._conversation.get("callId"));

At the moment, this isn't happening if I simply close the window using the 'x' - the service thinks we're still in the busy state. I suspect we need some form of onclose handler.
Attachment #8486172 - Flags: review?(standard8)
Attachment #8486172 - Flags: review-
Attachment #8486172 - Flags: feedback+
Blocks: 1066014
No longer blocks: 1000186
Severity: normal → enhancement
Whiteboard: p=2[loop-uplift] → p=2[loop-uplift][loop-inccall1]
> 
> @@ +532,5 @@
> > +      // This instance of CallProgressSocket should stay alive until the underlying
> > +      // websocket is closed since it is passed to the websocket as the nsIWebSocketListener.
> > +      callProgress.connect(() => {callProgress.sendBusy();});
> > +    }
> > +    catch (error) {}
> 
> Is there a reason not to log the error here? What's the expected failure
> case?
> 
Any expected problem should be handled by the CallProgressSocket instance. So, this catch should not be necessary.
> 
> ::: browser/components/loop/content/js/conversation.jsx
> @@ +175,5 @@
> >      /**
> >       * @override {loop.shared.router.BaseConversationRouter.endCall}
> >       */
> >      endCall: function() {
> > +      navigator.mozLoop.releaseCallData(this._conversation.get("callId"));
> 
> At the moment, this isn't happening if I simply close the window using the
> 'x' - the service thinks we're still in the busy state. I suspect we need
> some form of onclose handler.

I added an unload event handler to make sure closing the chat window still clears the call.
Fixed: chat window direct close issue, websocket uri method hard coded to wss, and nits. Additional tests to follow.
Attachment #8486172 - Attachment is obsolete: true
Attachment #8488908 - Attachment is obsolete: true
Attachment #8490153 - Attachment is obsolete: true
Blocks: 1070065
Moved tests to a follow on bug (bug 1070065) to allow QA testing of the full busy/reject notification chain.
Added error message for required argument for CallProgressSocket.connect()
Attachment #8492219 - Attachment is obsolete: true
Comment on attachment 8492301 [details] [diff] [review]
LoopService reject with reason = busy an incoming call when busy

I am continuing to work on the busy/reject unit test in a separate bug to allow check-in of the functional work. Taking more time than expected to merge with the new version of the tests.
Attachment #8492301 - Flags: review?(standard8)
Attachment #8492301 - Flags: review?(standard8) → review?(MattN+bmo)
Attachment #8492301 - Attachment is obsolete: true
Attachment #8492301 - Flags: review?(MattN+bmo)
Attachment #8492342 - Flags: review?(MattN+bmo)
Target Milestone: 34 Sprint 3- 9/1 → ---
Comment on attachment 8492342 [details] [diff] [review]
LoopService reject with reason = busy an incoming call when busy

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

Thanks. I skimmed the patch and looked over the changes to address the last review comments.

::: browser/components/loop/MozLoopService.jsm
@@ +113,5 @@
> +    this._onError = onError ||
> +      (reason => {console.warn("MozLoopService::callProgessSocket - ", reason);});
> +
> +    if (!onSuccess) {
> +      this._onError("missing onSucces argument");

Nit: missing an "s" in onSuccess here
Attachment #8492342 - Flags: review?(MattN+bmo) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/60995a021d74
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: p=2[loop-uplift][loop-inccall1] → p=2[loop-uplift][loop-inccall1][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/60995a021d74
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: p=2[loop-uplift][loop-inccall1][fixed-in-fx-team] → p=2[loop-uplift][loop-inccall1]
Target Milestone: --- → mozilla35
Flags: qe-verify+
Flags: in-moztrap?
QA Contact: anthony.s.hughes
Paul, can you please test this in the latest Nightly and the Try-server build here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rjesup@wgate.com-f9eb2cbac352

We should also make sure this gets a Moztrap test for Loop 34/35.
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
Whiteboard: p=2[loop-uplift][loop-inccall1] → p=2[loop-uplift][loop-inccall1][fig:verifyme]
(In reply to Romain Testard [:RT] from comment #0)
> The caller will see a notification as per
> https://bugzilla.mozilla.org/show_bug.cgi?id=1000186
> Currently these types of incoming calls are silently lost.
So, I don't see any 'busy' notification right now, the call simply ends, but I guess that's expected due to the above, right ?
Flags: needinfo?(paul.silaghi) → needinfo?(rtestard)
This bug adds functionality in the MozLoopService client that will send back a reject/busy call progress message on the call progress websocket. That notification would then be passed on to the calling client.
So, is there a way to verify this ?
Flags: needinfo?(pkerr)
Since the call progress websocket is over TLS, I captured the http to websocket upgrade and that a packet was sent to the Loop server.
Flags: needinfo?(pkerr)
The only way you've really got at the moment from the UI, is to see that when you try a call, it returns to the main screen straight away.

There's a non-UI possibility - on the link clicker end, open a web console, and enter:

localStorage.setItem("debug.websocket", true);

When you then try connecting a call, you should find that there's a progress message with terminated in it and a reason of "busy".

To reset that "pref", do:

localStorage.removeItem("debug.websocket");

(In reply to Paul Silaghi, QA [:pauly] from comment #32)
> (In reply to Romain Testard [:RT] from comment #0)
> > The caller will see a notification as per
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1000186
> > Currently these types of incoming calls are silently lost.
> So, I don't see any 'busy' notification right now, the call simply ends, but
> I guess that's expected due to the above, right ?

We don't have exact UX yet. When bug 1000240 gets deployed, then I think you'll see the fact that "something went wrong".
Thanks Mark.
Verified fixed FF 35.0a1 (2014-10-03) and the try build
Status: RESOLVED → VERIFIED
Flags: needinfo?(rtestard)
Whiteboard: p=2[loop-uplift][loop-inccall1][fig:verifyme] → p=2[loop-uplift][loop-inccall1][fig:verified]
Comment on attachment 8492342 [details] [diff] [review]
LoopService reject with reason = busy an incoming call when busy

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8492342 - Flags: approval-mozilla-aurora?
Comment on attachment 8492342 [details] [diff] [review]
LoopService reject with reason = busy an incoming call when busy

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.
Attachment #8492342 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1079811
Mark, I've added a draft smoketest to Moztrap. Can you please review it for accuracy and completeness?

https://moztrap.mozilla.org/manage/case/14888/
Flags: needinfo?(standard8)
Flags: in-moztrap?
Flags: in-moztrap+
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #41)
> Mark, I've added a draft smoketest to Moztrap. Can you please review it for
> accuracy and completeness?
> 
> https://moztrap.mozilla.org/manage/case/14888/

Yes, that looks good, thanks.
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.