1045569 - Revoking a url will always revoke the latest-generated url

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

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.

Comment 2

[Andrei Oprea [mention me on [e-mail]]]

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

Comment 3

[Mark Banner (:standard8)]

(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).

Comment 5

[Andrei Oprea [mention me on [e-mail]]]

(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?

Comment 6

[Mark Banner (:standard8)]

(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.

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Revoke the correct call url in loop desktop client

Comment 9

[Romain Gauthier [:tOkeshu]]

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 10

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Revoke the correct call url in loop desktop client

Comment 11

[Romain Gauthier [:tOkeshu]]

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+

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/992700d61f2b

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/992700d61f2b

Comment 14

[u279076]

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