1033579 - Loop Client needs to send channel in call setup messages

Status: VERIFIED FIXED

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

Description

[Adam Roach [:abr]]

Client needs to send new "channel" field in call setup messages. See Bug 1033032 for details.

Comment 1

[Maire Reavy [:mreavy]]

Hi Adam - coming out of our conversation with Rob and Michael yesterday, what release should this target?

Comment 2

[Adam Roach [:abr]]

Attachment: Add channel to POST /calls

Comment 3

[Adam Roach [:abr]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #1)
> Hi Adam - coming out of our conversation with Rob and Michael yesterday,
> what release should this target?

Apparently, this slipped between the cracks. If you'd asked me back when we had the first conversations around the topic, I'd have said "33".

It's literally a three line change (see the attached patch) -- this simply needs to be tested, and I think it's a candidate to uplift to 34 and 35.

Comment 4

[Mark Banner (:standard8)]

Attachment: Add channel to POST calls for Loop to allow different servers based on the channel.

Updated patch to fix and add unit tests.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

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

r=dmose

Comment 6

[Mark Banner (:standard8)]

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

Comment 7

[Mark Banner (:standard8)]

To verify on Desktop:

- Set up two browsers with different FxA accounts
- Select one to make an outgoing call
- Open the browser console
- Right-click in the browser console and select "Log Request and Response Bodies"
- Initiate a direct outgoing call from that browser (doesn't matter if it doesn't connect)
- Examine the console, look for:

POST https://loop.services.mozilla.com/calls

(the domain may be different if you're not on production)

- Click on that message, the Inspect Network Request window opens

Expected Results

=> The request body should have a "channel" option which is set to the update channel of the Firefox you're on, e.g. "nightly", "aurora" etc. If you're on a dev build, this should be "default", unless you've explicitly modified it.


To verify on standalone:

- Get a call url & load it in the browser
- Open the web console
- Right-click in the web console and select "Log Request and Response Bodies"
- Initiate a call from that page (doesn't matter if it doesn't connect)
- Examine the console, look for:

POST https://loop.services.mozilla.com/calls/<token>

(the domain may be different if you're not on production)

- Click on that message, the Inspect Network Request window opens


Expected Results

=> The request body should have a "channel" option with value "standalone"

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Maire Reavy [:mreavy]]

Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

Approval Request Comment
[Feature/regressing bug #]: send channel message to external servers
[User impact if declined]: Our partner's servers could not distinguish Nightly/Aurora users from Beta/Release users which would make partner testing virtually impossible
[Describe test coverage new/current, TBPL]: unit tests, manual testing/verification
[Risks and why]: No risk outside of Hello; minimal risk to Hello -- our partner needs this message in order to do proper testing prior to SDK deployment
[String/UUID change made/needed]: No strings

Comment 10

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
Our partner's servers need to know if a call is coming from a Nightly/Aurora user or a Beta/Release user in order to do testing

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Tried on Nightly 10/25 (Linux) and works (Once I got it to enable the request bodies)

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

I have a couple of questions about this change before approving for uplift.

Is this change really necessary? Shouldn't the partner be able to identify the channel based on the version #?

Is this expected to be a short lived addition to the packet for initial testing or is this needed long term?

Comment 13

[Maire Reavy [:mreavy]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #12)
> I have a couple of questions about this change before approving for uplift.
> 
> Is this change really necessary? Shouldn't the partner be able to identify
> the channel based on the version #?

My apologies that it isn't captured here, but see bug 1033032 comments 1 and 3 by abr.  The UA version doesn't tell us which API key it should be using (see https://bugzilla.mozilla.org/show_bug.cgi?id=1033032#c3 for details).  Basically, it's set up to use one API key for Nightly/Aurora, and another for Beta and Release builds, so the UA version doesn't tell you whether it's 35 Aurora or 35 Beta, which would use different keys.

> Is this expected to be a short lived addition to the packet for initial
> testing or is this needed long term?

This is a long-term addition to help our partner manage handling of backward-compatibility and to route requests to servers compatible with the protocols used in different Firefox versions. This provides a solution so they can move forward with features in the servers.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #13)
> My apologies that it isn't captured here, but see bug 1033032 comments 1 and
> 3 by abr.  The UA version doesn't tell us which API key it should be using
> (see https://bugzilla.mozilla.org/show_bug.cgi?id=1033032#c3 for details). 
> Basically, it's set up to use one API key for Nightly/Aurora, and another
> for Beta and Release builds, so the UA version doesn't tell you whether it's
> 35 Aurora or 35 Beta, which would use different keys.

Why can't the API key be set with the version specific preferences/config that we use for other features? If the API key determines the server (or server version) with which the client should communicate, why does the channel name need to be passed to the server? It seems to me that the client should inform the server which version of the API it wants to use. The server should not care at all about client specifics like version/channel.

If we really do need to proceed in this way for some reason, it is also possible to determine the channel using the version plus calendar date. We keep the version info up-to-date on the wiki and the channel can be scraped from there instead of having the client pass this information.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

I spoke with Maire. She'll add some further details about why this change is required as is as least for the short term. Long term I would like to see a solution that doesn't use the channel names as I don't like the idea of including them in an API between Firefox and TokBox. Aurora+

Plan is to test on Aurora on Sunday and get this change into Beta4.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cf8a756d9a6

Comment 17

[Maire Reavy [:mreavy]]

In the short term, we need to unblock our partner so they can roll out server features without breaking older clients, and also test the segregation procedures before actually doing the rollouts.  This change matches the agreed-on design our partner has been coding to. 

For background:  (Note - I'm dredging my memory, since the reasoning wasn't captured in this bug or the one it refers to - which is a large part of why the questions came up now.)  IIRC, one of the reasons the solution uses channels instead of SDK versions is so that there's a consistent "If I'm on Nightly/NN, I can call people using X" where I believe X is up through current release at the time Nightly/NN is built (where NN is 35, 36, etc), but I may not be able to call people on Release+1. I know we talked about several possible compatibility matrices, but those never ended up in any of the bugs and aren't on any of the wiki documentation pages that we've built thus far.  If that's incorrect and there is a wiki page about this, please reference it here.

Lawrence has expressed real concern about using "nightly/aurora/beta/release" (especially aurora) because longer term, channel name meanings may change (many proposals to change what aurora is are on the table, for example) -- even the textual names themselves may change.  They also (at best) only indirectly identify compatibility issues. 

I'm opening a new bug (Bug 1089354) to discuss changing "nightly", "aurora", "beta", etc to something we can lock down (and translate "nightly" etc to those new values).  We also should document the design, and consider if there's a better way to achieve the goals, perhaps by simply using the version number (in theory changes should be landing in a version, and once out of Nightly the API to the server should remain fairly stable (for that version) after the current spurt of uplifts).

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Verified on Aurora Nightly 10/26 Linux

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8510492 [details] [diff] [review]
Add channel to POST calls for Loop to allow different servers based on the channel.

># HG changeset patch
># User Adam Roach [:abr] <[e-mail]>
>
>Bug 1033579 - Add channel to POST calls for Loop to allow different servers based on the channel.
>
>diff --git a/browser/components/loop/content/js/client.js b/browser/components/loop/content/js/client.js
>index c70c038..a518216 100644
>--- a/browser/components/loop/content/js/client.js
>+++ b/browser/components/loop/content/js/client.js
>@@ -227,17 +227,19 @@ loop.Client = (function($) {
>      * @param {Function} cb Callback(err, result)
>      */
>     setupOutgoingCall: function(calleeIds, callType, cb) {
>       // For direct calls, we only ever use the logged-in session. Direct
>       // calls by guests aren't valid.
>       this.mozLoop.hawkRequest(this.mozLoop.LOOP_SESSION_TYPE.FXA,
>         "/calls", "POST", {
>           calleeId: calleeIds,
>-          callType: callType
>+          callType: callType,
>+          channel: this.mozLoop.appVersionInfo ?
>+                   this.mozLoop.appVersionInfo.channel : "unknown"
>         },
>         function (err, responseText) {
>           if (err) {
>             this._failureHandler(cb, err);
>             return;
>           }
> 
>           try {
>diff --git a/browser/components/loop/standalone/content/js/standaloneClient.js b/browser/components/loop/standalone/content/js/standaloneClient.js
>index cd4df3e..7b5569a 100644
>--- a/browser/components/loop/standalone/content/js/standaloneClient.js
>+++ b/browser/components/loop/standalone/content/js/standaloneClient.js
>@@ -110,17 +110,17 @@ loop.StandaloneClient = (function($) {
>         throw new Error("missing required parameter loopToken");
>       }
> 
>       var req = $.ajax({
>         url:         this.settings.baseServerUrl + "/calls/" + loopToken,
>         method:      "POST",
>         contentType: "application/json",
>         dataType:    "json",
>-        data: JSON.stringify({callType: callType})
>+        data: JSON.stringify({callType: callType, channel: "standalone"})
>       });
> 
>       req.done(function(sessionData) {
>         try {
>           cb(null, this._validate(sessionData, expectedCallsProperties));
>         } catch (err) {
>           console.error("Error requesting call info", err.message);
>           cb(err);
>diff --git a/browser/components/loop/test/desktop-local/client_test.js b/browser/components/loop/test/desktop-local/client_test.js
>index cd91064..e09eeb2 100644
>--- a/browser/components/loop/test/desktop-local/client_test.js
>+++ b/browser/components/loop/test/desktop-local/client_test.js
>@@ -267,17 +267,33 @@ describe("loop.Client", function() {
>       it("should make a POST call to /calls", function() {
>         client.setupOutgoingCall(calleeIds, callType);
> 
>         sinon.assert.calledOnce(hawkRequestStub);
>         sinon.assert.calledWith(hawkRequestStub,
>           mozLoop.LOOP_SESSION_TYPE.FXA,
>           "/calls",
>           "POST",
>-          { calleeId: calleeIds, callType: callType }
>+          { calleeId: calleeIds, callType: callType, channel: "unknown" }
>+        );
>+      });
>+
>+      it("should include the channel when defined", function() {
>+        mozLoop.appVersionInfo = {
>+          channel: "beta"
>+        };
>+
>+        client.setupOutgoingCall(calleeIds, callType);
>+
>+        sinon.assert.calledOnce(hawkRequestStub);
>+        sinon.assert.calledWith(hawkRequestStub,
>+          mozLoop.LOOP_SESSION_TYPE.FXA,
>+          "/calls",
>+          "POST",
>+          { calleeId: calleeIds, callType: callType, channel: "beta" }
>         );
>       });
> 
>       it("should call the callback if the request is successful", function() {
>         var requestData = {
>           apiKey: "fake",
>           callId: "fakeCall",
>           progressURL: "fakeurl",
>diff --git a/browser/components/loop/test/standalone/standalone_client_test.js b/browser/components/loop/test/standalone/standalone_client_test.js
>index 843c46c..013d694 100644
>--- a/browser/components/loop/test/standalone/standalone_client_test.js
>+++ b/browser/components/loop/test/standalone/standalone_client_test.js
>@@ -123,17 +123,17 @@ describe("loop.StandaloneClient", function() {
>         });
> 
>       it("should post data for the given call", function() {
>         client.requestCallInfo("fake", "audio", callback);
> 
>         expect(requests).to.have.length.of(1);
>         expect(requests[0].url).to.be.equal("http://fake.api/calls/fake");
>         expect(requests[0].method).to.be.equal("POST");
>-        expect(requests[0].requestBody).to.be.equal('{"callType":"audio"}');
>+        expect(requests[0].requestBody).to.be.equal('{"callType":"audio","channel":"standalone"}');
>       });
> 
>       it("should receive call data for the given call", function() {
>         client.requestCallInfo("fake", "audio-video", callback);
> 
>         var sessionData = {
>           sessionId: "one",
>           sessionToken: "two",

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d43a7b8995a6

Comment 21

[u279076]

Paul, please verify this is fixed in the next Beta build.

Comment 22

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b5 on Win 7, OS X 10.9.5 using the STR in comment 7