Closed Bug 1035348 Opened 6 years ago Closed 5 years ago

Make the "GET /calls" server call happen in the MozLoopService rather than the conversation window

Categories

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

defect

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
mozilla35
Iteration:
35.1
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: standard8, Assigned: pkerr)

References

Details

(Whiteboard: [p=1][loop-uplift])

User Story

Rough plan:

- When handling a push notification, change MozLoopService to do a (hawk) request to the server to GET /calls.
- When it returns, temporarily store any call details (persistent store not reqd).
- Then:
-- Open a chat window
-- Rather than passing the "version" parameter, pass the "callId" from the call details.
-- Expose a new API on mozLoop to get the call details for the "callId" and request that information from the conversation window, instead of the current /calls request.

Attachments

(2 files, 8 obsolete files)

10.53 KB, patch
standard8
: review+
Details | Diff | Splinter Review
14.52 KB, patch
Details | Diff | Splinter Review
For bug 1032700 we need the get of the current calls information to happen in the MozLoopService and then relevant information to be passed to the conversation window.
User Story: (updated)
Priority: -- → P2
Target Milestone: --- → mozilla33
Target Milestone: mozilla33 → mozilla34
This goes with Bug 1032700 which was moved to Fx 35
Target Milestone: mozilla34 → mozilla35
Bug 1032700 just got moved back to 34.
Target Milestone: mozilla35 → mozilla34
Priority: P2 → P1
This should depend on bug 1022594, as I'm doing some work there to make the current call url data request happen before "accept" is called, which will make implementing this much easier.
Depends on: 1022594
Bug 1022594 part 1 has just landed, so this is clear to move forward.
No longer depends on: 1022594
See Also: → 1022594
Whiteboard: p=1
Assignee: nobody → pkerr
Status: NEW → ASSIGNED
Work in progress
Comment on attachment 8481870 [details] [diff] [review]
WIP Move GET/calls to MozLoopService

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +133,5 @@
> +    getCallData: {
> +      enumerable: true,
> +      writable: true,
> +      value: function(loopCallId) {
> +        return JSON.stringify(MozLoopService.getCallData(loopCallId));

I just took a brief look. If you're having trouble getting objects across the mozLoop boundary, you need (and should really need to use anyway) the Cu.cloneInto function that's also used elsewhere in this file. That would save the JSON.stringify/parse translations.
WIP with suggested change to API data handling
Attachment #8481870 - Attachment is obsolete: true
Attached patch Move GET/calls to MozLoopService (obsolete) — Splinter Review
Attachment #8482325 - Attachment is obsolete: true
Follow on patch needed to fix xpcshell test failure.
Attachment #8482385 - Flags: review?(standard8)
Comment on attachment 8482385 [details] [diff] [review]
Move GET/calls to MozLoopService

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

This looks good. I'll give this f+ until we get test fixes though.

Note that you'll also need to fix the unit (aka Marionette) tests - there's a readme in the loop directory about how to run them, but feel free to ping me if you need help.

::: browser/components/loop/MozLoopService.jsm
@@ +300,5 @@
>      // call, they'll have had enough time to see the terms of service. See
>      // bug 1046039 for background.
>      Services.prefs.setCharPref("loop.seenToS", "seen");
>  
> +    /* Request the information on the new call(s) associated with this version. */

nit: prefer // form of comments for in-function comments.
Attachment #8482385 - Flags: review?(standard8) → feedback+
Attached patch Move GET/calls to MozLoopService (obsolete) — Splinter Review
bitrot fixes
Attachment #8482385 - Attachment is obsolete: true
Blocks: 1000237
Attachment #8483783 - Attachment is obsolete: true
Rework of loop tests. For the unit tests, I needed to put back some #ifdef/ifndef DEBUG bits in two places to allow the fake return of a hawkRequest. This is because the request for calls/?=version has moved from the chat window and into MozLoopService.jsm. These internal bits are only exposed in a debug build and the test as skipped for a non-debug build.
Attachment #8485192 - Flags: review?(standard8)
Attachment #8485196 - Flags: review?(standard8)
Comment on attachment 8485196 [details] [diff] [review]
Part 2: fix unit tests. 1) Added hawkRequest stub to allow completion of inbound notification setup where necessary in tests. (These tests will now only run for a debug build) 2) Removed tests of client.requestCallsInfo() which has been removed

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

::: browser/components/loop/MozLoopService.jsm
@@ +623,5 @@
>        deferred.reject("Invalid token data");
>      }
>    },
>  };
> +#ifndef DEBUG

Have you seen bug 1061756 ? I wonder if we can work around it the same way here, I'd rather avoid debug-only if possible.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ -170,5 @@
>  
>            expect(conversation.get("loopVersion")).to.equal("fakeVersion");
>          });
>  
> -        it("should call requestCallsInfo on the client",

These tests should be replaced by the equivalents with navigator.mozLoop.getCallData stubbed.

i.e. "should call navigator.mozLoop.getCallData"
"should display an error if getCallData returns an error"
describe("getCallData successful", ...

etc.

@@ -187,5 @@
> -
> -            sinon.assert.calledOnce(notifier.errorL10n);
> -          });
> -
> -        describe("requestCallsInfo successful", function() {

There's a mismatch of braces here, and that causes a syntax error which, unfortunately, means that the conversation tests are not run.

::: browser/components/loop/test/xpcshell/head.js
@@ +26,5 @@
>  
>  // Ensure loop is always enabled for tests
>  Services.prefs.setBoolPref("loop.enabled", true);
>  
> +function hawkGetCallsRequest(uri, protocol) {

So far, we've been doing this type of thing with the fake loop server, e.g.

http://hg.mozilla.org/mozilla-central/annotate/892768985915/browser/components/loop/test/xpcshell/test_loopservice_registration.js#l56

Is there a reason we can't do that here as well?

::: browser/components/loop/test/xpcshell/xpcshell.ini
@@ +4,5 @@
>  firefox-appdir = browser
>  
>  [test_looppush_initialize.js]
>  [test_loopservice_dnd.js]
> +run-if = debug == true 

nit: whitespace at end of line (if this line stays).
Attachment #8485196 - Flags: review?(standard8) → review-
Attachment #8485192 - Flags: review?(standard8) → review+
(In reply to Mark Banner (:standard8) from comment #14)
> Comment on attachment 8485196 [details] [diff] [review]
> Part 2: fix unit tests. 1) Added hawkRequest stub to allow completion of
> inbound notification setup where necessary in tests. (These tests will now
> only run for a debug build) 2) Removed tests of client.requestCallsInfo()
> which has been removed
> 
> Review of attachment 8485196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +623,5 @@
> >        deferred.reject("Invalid token data");
> >      }
> >    },
> >  };
> > +#ifndef DEBUG
> 
> Have you seen bug 1061756 ? I wonder if we can work around it the same way
> here, I'd rather avoid debug-only if possible.
> 
Thanks for the pointer. Yes, I believe this can relieve the need for the ifdef's.
small bug fix
Attachment #8485192 - Attachment is obsolete: true
1) Reworked units tests to not rely on DEBUG ifdefs. 2) Added new tests for the MozLoopService API getCallData call in functional tests.
Attachment #8485196 - Attachment is obsolete: true
1) Reworked units tests to not rely on DEBUG ifdefs. 2) Added new tests for the MozLoopService API getCallData call in functional tests.
Attachment #8486171 - Attachment is obsolete: true
Attachment #8486174 - Flags: review?(standard8)
Comment on attachment 8486170 [details] [diff] [review]
Part 1: Move GET/calls to MozLoopService

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

::: browser/components/loop/content/js/conversation.jsx
@@ +205,1 @@
>            console.error("Failed to get the sessionData", err);

The tests were failing, and I noticed that its due to this change - err is no longer defined. Lets replace this by

console.error("Failed to get the call data");

Then we'll at least know where the notification came from.
Comment on attachment 8486174 [details] [diff] [review]
Part 2: fix tests. 1) Added hawkRequest stub to allow completion of inbound notification setup where necessary in unit tests. 2) Replaced tests of client.requestCallsInfo() with mozLoop.getCallData

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

Ok, these changes look good with the previous comment about fixing part 1. I don't see the functional tests being added in this patch, so I'm not quite sure what you're referring to there.

If you are adding them, could you land parts 1 & 2, and do the functional tests in a follow-up patch? I've got bugs that I'm working on that I want to depend on these two as it makes them simpler.
Attachment #8486174 - Flags: review?(standard8) → review+
small bug fix
Attachment #8486170 - Attachment is obsolete: true
(In reply to Mark Banner (:standard8) from comment #20)
> Comment on attachment 8486174 [details] [diff] [review]
> Part 2: fix tests. 1) Added hawkRequest stub to allow completion of inbound
> notification setup where necessary in unit tests. 2) Replaced tests of
> client.requestCallsInfo() with mozLoop.getCallData
> 
> Review of attachment 8486174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, these changes look good with the previous comment about fixing part 1. I
> don't see the functional tests being added in this patch, so I'm not quite
> sure what you're referring to there.
> 
> If you are adding them, could you land parts 1 & 2, and do the functional
> tests in a follow-up patch? I've got bugs that I'm working on that I want to
> depend on these two as it makes them simpler.

Sorry Mark, bad nomenclature on my part. I modified marionette tests which I referred to as function tests, in addition to xpcshell tests. I have not more tests pending.
Ok, that's fine.
Carrying forward r+ for part 1 patch.
Keywords: checkin-needed
Iteration: --- → 35.1
Whiteboard: p=1 → p=1[loop-uplift]
Target Milestone: mozilla34 → mozilla35
https://hg.mozilla.org/integration/fx-team/rev/4b9b045ef203
https://hg.mozilla.org/integration/fx-team/rev/a0b9b573cbb6
Keywords: checkin-needed
Whiteboard: p=1[loop-uplift] → [p=1][loop-uplift][for uplift, use hg revs, not patches]
Oh forgot to mention, I fixed a few bits of bitrot in the patches that were pushed.
Follow-up fix as bug 1065052 changed the parameters to hawkRequest, and landed just before this:

https://hg.mozilla.org/integration/fx-team/rev/0f8b752951d9
Blocks: 1065591
https://hg.mozilla.org/mozilla-central/rev/4b9b045ef203
https://hg.mozilla.org/mozilla-central/rev/a0b9b573cbb6
https://hg.mozilla.org/mozilla-central/rev/0f8b752951d9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=1][loop-uplift][for uplift, use hg revs, not patches] → [p=1][loop-uplift]
Paul, does this need manual testing?
Flags: qe-verify?
Flags: needinfo?(pkerr)
Anthony, the unit test should be sufficient. Plus, this change is not meant to be observable in the loop client behavior.
Flags: needinfo?(pkerr)
Thanks Paul.
Flags: qe-verify? → qe-verify-
QA Contact: anthony.s.hughes
Comment on attachment 8486396 [details] [diff] [review]
Part 1: Move GET/calls to MozLoopService

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8486396 - Flags: approval-mozilla-aurora?
Attachment #8486174 - Flags: approval-mozilla-aurora?
Comment on attachment 8486396 [details] [diff] [review]
Part 1: Move GET/calls to MozLoopService

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 #8486396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8486174 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.