1032741 - /call_url no longer works with latest loop-server - changed parameter name

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

/call_url's parameter for "call_url" got changed to "callUrl" in:

https://github.com/mozilla-services/loop-server/pull/129

(no bug # afaict).

This will break devs trying out the server, but additionally when the server is next deployed it will break the nightly builds.

Comment 1

[Rémy Hubscher (:natim)]

Yes MLP and MVP server won't be compatible.

Comment 2

[Adam Roach [:abr]]

So, if we're going to change this name, changing it now is probably the easiest time.

Mark: during the transitional period, can you do something like:

  var callUrl = callUrlData.call_url || callUrlData.callUrl;

So that we work with both old and new servers? Otherwise we need a flag day, and that annoys people.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

What's the rationale for the change?  It's not clear from reading this bug why the right fix isn't to revert the server change and leave things compatible.

Comment 4

[Rémy Hubscher (:natim)]

The goal is to be consistent with the all API which uses camelCase for object keys.
We have spotted only two variables not in camelCase (call_url and simple_push_url)

Comment 5

[Mark Banner (:standard8)]

We should do this after bug 1020876 as that's rearranging the current client.js a bit.

Comment 6

[Mark Banner (:standard8)]

Attachment: Adapt to latest Loop server changes - update parameter name for /calls_url and add a callType parameter when starting a call

This is what we need to work correctly with the latest server. There's changes to both the desktop and the client.

This is built and tested against the fx-team branch, as the latest changes I needed haven't landed in central yet.

I've already got bug 1033988 filed to handle the removal of the two-line part that's only needed whilst we transition.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8450099 [details] [diff] [review]
Adapt to latest Loop server changes - update parameter name for /calls_url and add a callType parameter when starting a call

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

Looks good overall, but you might want to address a few comments before landing:

::: browser/components/loop/standalone/content/js/standaloneClient.js
@@ +88,5 @@
>       * Posts a call request to the server for a call represented by the
>       * loopToken. Will return the session data for the call.
>       *
>       * @param  {String} loopToken The loopToken representing the call
>       * @param  {Function} cb Callback(err, sessionData)

Missing docs for callType

::: browser/components/loop/standalone/content/js/webapp.js
@@ +97,5 @@
>          }),
> +        outgoing: true,
> +        // For now, we assume both audio and video as there is no
> +        // other option to select.
> +        callType: "audio-video"

I could totally see that default value being defined in the model.

::: browser/components/loop/test/shared/models_test.js
@@ +96,5 @@
>                outgoing: false
>              });
>  
> +            sinon.assert.calledOnce(requestCallsInfoStub);
> +            sinon.assert.calledWith(requestCallsInfoStub);

I think you want to check for a passed parameter here :)

Comment 8

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) from comment #7)
> ::: browser/components/loop/standalone/content/js/webapp.js
> @@ +97,5 @@
> >          }),
> > +        outgoing: true,
> > +        // For now, we assume both audio and video as there is no
> > +        // other option to select.
> > +        callType: "audio-video"
> 
> I could totally see that default value being defined in the model.

Yeah, I'm torn, as its just for initiate at the moment, I think it'll be best to keep it as it is to be explicit about what we're doing. As me progress towards adding more features, we can work out what we want to do with it.

Comment 9

[Mark Banner (:standard8)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4a345fd13ab

Comment 11

[u279076]

Does this need manual testing or is testsuite coverage sufficient?

Comment 12

[u279076]

Mark, can you please answer comment 11?

Comment 13

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #11)
> Does this need manual testing or is testsuite coverage sufficient?

This is integral to general call functionality, and is such tested every time a call is made.