In addressing some comments on bug 1033988, I just noticed that bug 1000134 implemented revoking a url in such a way that it will always revoke the latest generated call url, rather than the url that the call was being received for. I couldn't find a bug for follow-up, so filing this one.

As currently implemented, we are getting the callToken from /call_url when we are requesting the call url in the panel, and then that's stored in a pref. What should be happening is in the conversation window: - in ConversationModel#setReady, store sessionData.callToken in the ConversationModel (that comes via Client#requestCallsInfo) -- See https://docs.services.mozilla.com/loop/apis.html#get-calls-version-version - Then in ConversationRouter#declineAndBlock we should use the saved callToken from the ConversationModel & pass that through.

I reported this bug here https://bugzilla.mozilla.org/show_bug.cgi?id=1044367

(In reply to Andrei Oprea [:andreio] from comment #2) > I reported this bug here https://bugzilla.mozilla.org/show_bug.cgi?id=1044367 Ok, thanks I didn't see that. It is useful if you're able to, to link it to the bug that introduced it, or reference it. Then it makes it easier for others to find (e.g. I looked at the dependencies on bug 1000134 and didn't see it).

(In reply to Mark Banner (:standard8) from comment #1) > As currently implemented, we are getting the callToken from /call_url when > we are requesting the call url in the panel, and then that's stored in a > pref. > > What should be happening is in the conversation window: > > - in ConversationModel#setReady, store sessionData.callToken in the > ConversationModel (that comes via Client#requestCallsInfo) > -- See > https://docs.services.mozilla.com/loop/apis.html#get-calls-version-version Using loopVersion we would get back (possibly) an array of call urls. Do we want to call .deleteCallUrl on all of them ? If not how to we pick ? also setReady is called (as of now) when the call gets accepted, otherwise conversation.initiate is never called. That means that there must be an explicit request to "requestCallInfo" and the callback would save the sessionData using setReady. Is that correct?

(In reply to Andrei Oprea [:andreio] from comment #5) > (In reply to Mark Banner (:standard8) from comment #1) > > As currently implemented, we are getting the callToken from /call_url when > > we are requesting the call url in the panel, and then that's stored in a > > pref. > > > > What should be happening is in the conversation window: > > > > - in ConversationModel#setReady, store sessionData.callToken in the > > ConversationModel (that comes via Client#requestCallsInfo) > > -- See > > https://docs.services.mozilla.com/loop/apis.html#get-calls-version-version > Using loopVersion we would get back (possibly) an array of call urls. Do we > want to call .deleteCallUrl on all of them ? If not how to we pick ? The existing setup already selects the first item in the array: http://hg.mozilla.org/mozilla-central/annotate/005424a764da/browser/components/loop/content/shared/js/models.js#l129 Despite what the comment says, bug 1035348 is going to do the work to move the call out of the conversation window and into the background service. Hence the conversation window will only be able to get one bit of info. > also setReady is called (as of now) when the call gets accepted, otherwise > conversation.initiate is never called. That means that there must be an > explicit request to "requestCallInfo" and the callback would save the > sessionData using setReady. Is that correct? In bug 1022594, I'm doing the work so that requestCallInfo happens before the call is accepted, this would then mean getting the information for delete is just a matter of taking it out of the model. I'll add a dependency here so that we can track the order correctly.

Part 1 of bug 1022594 has now landed, so I think we're clear to move forward.

Comment on attachment 8474568 [details] [diff] [review] Revoke the correct call url in loop desktop client Review of attachment 8474568 [details] [diff] [review]: ----------------------------------------------------------------- The tests should be updated to reflect the comment. The rest looks fine. Waiting for the update to review that ASAP :) ::: browser/components/loop/test/desktop-local/conversation_test.js @@ +441,5 @@ > > sinon.assert.calledOnce(deleteCallUrl); > }); > > + it("should get callToken from conversation model", function() { This part of the code is actually testing an implementation detail. I would prefer to see deleteCallUrl tested with the correct token argument. The motivation behind that is, if conversation.get is called but the value is never used after that, the tests will be green and the code incorrect.

Comment on attachment 8475210 [details] [diff] [review] Revoke the correct call url in loop desktop client Review of attachment 8475210 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. r+

Does this need manual testing or is the automation coverage sufficient?