1066567 - Refactor the outgoing conversation logic out of the router for the standalone UI

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

As part of bug 972017 we need to be making outgoing calls from the desktop.

To do that, it would be best if we re-used most of the logic from the standalone side which is currently embedded in the standalone router.

As a side effect, we may also need to alter some of the incoming code for the conversation window.

I'm not yet sure what this will look like at the end, hence giving it a p=8 as this is going to require some exploration to get right. Using some of the flux ideas may help us along.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP Part 1. Stop use of the backbone router in the standalone UI to simplify code. The views now exclusively manage what is displayed according to state.

This is WIP, putting here for safe keeping - although this is mainly done now apart from fixing up unit tests.

The main idea here is to drop the backbone router, and to replace it by views which render acorrding to the state. This is the start of moving towards the flux ideas.

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 1. Stop use of the backbone router in the standalone UI to simplify code. The views now exclusively manage what is displayed according to state.

Ready for review, with tests updated. Functionality wise, this is the same as the code previously.

The first part of this refactor had the aim of dropping the backbone router (which had complicated and unnecessary navigates controlling the views), and replace it by a entirely react based view.

The new view is WebappRootView, and this controls switching of the views based on its state. It will show either of the unsupported views, or the new OutgoingConversationView, or if it can't do anything, the home view.

The OutgoingConversationView now manages the sub-views that are required for the actual conversation, it keeps track of the state of the conversation and renders appropraitely. The OutgoingConversationView still does a lot of "control" of the conversation wrt managing the other classes, events to the network etc. We will look at moving these more towards a flux idea in the next patches.

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 1. Stop use of the backbone router in the standalone UI to simplify code. The views now exclusively manage what is displayed according to state. v2

Just realised that the home route wasn't working - if there was no hash, then it would fail to load any view rather than loading the home view.

Comment 4

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

Comment on attachment 8490687 [details] [diff] [review]
Part 1. Stop use of the backbone router in the standalone UI to simplify code. The views now exclusively manage what is displayed according to state. v2

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

This so much better I could cry; r=me with comments addressed (expect nits, your call).

I'm pondering if the whitespace changes shouldn't be part of another bug, so we fix this asap and don't have to waste time resolving conflicts when rebasing our work here.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +681,5 @@
> +        return (
> +          <OutgoingConversationView
> +             client={this.props.client} conversation={this.props.conversation}
> +             helper={this.props.helper} notifications={this.props.notifications}
> +             sdk={this.props.sdk}

Nit: Having one prop per line would be more readable.

@@ +727,5 @@
>  
> +    // Obtain the loopToken and pass it to the conversation
> +    var locationHash = helper.locationHash();
> +    if (locationHash) {
> +      var loopToken = helper.locationHash().match(/\#call\/(.*)/)[1];

var loopToken = locationHash.match…

Also, declaring loopToken before entering the if statement is good practice.

@@ +734,5 @@
>  
> +    React.renderComponent(<WebappRootView
> +      client={client} conversation={conversation}
> +      helper={helper} notifications={notifications}
> +      sdk={OT}

Nit: Having one prop per line would be more readable.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +87,4 @@
>        });
>      });
>  
> +    describe("#_setupWebSocket", function() {

If this is tested, this should probably be declared as part of the public API… though as this is about to be refactored soon, just leave a comment about it for now.

@@ +230,4 @@
>                  state: "connecting"
>                });
>  
> +              expect(ocView.state.callStatus).eql("connected");

I think we generally want to test for DOM changes rather than state values (can't find the link to react docs claiming that this is good practice).

I agree this is a bit more verbose and less unitish, but I think it's safe to test for the actual output of the render function rather than internal state.

@@ +281,5 @@
> +        it("should setup the websocket if session token is available",
> +          function() {
> +            ocView.startCall();
> +
> +            sinon.assert.calledOnce(loop.CallConnectionWebSocket.prototype.promiseConnect);

Nit: you can assig a stubbed method to a var:

var promiseConnect = sandbox.stub(loop.CallConnectionWebSocket.prototype, "promiseConnect");
// …
sinon.assert.calledOnce(promiseConnect);

@@ +428,5 @@
>  
> +            it("should set display the StartConversationView on any other error",
> +               function() {
> +                client.requestCallInfo.callsArgWith(2, {errno: 104});
> +                conversation.setupOutgoingCall();

Nit: Blank line before the action.

@@ +433,3 @@
>  
> +                TestUtils.findRenderedComponentWithType(ocView,
> +                loop.webapp.StartConversationView);

Nit: indentation.

@@ +483,5 @@
> +        client: client,
> +        helper: webappHelper,
> +        sdk: sdk,
> +        conversation: conversationModel
> +      };

Nit: It feels like this could be moved directly in mountTestComponent.

Comment 5

[Mark Banner (:standard8)]

Attachment: Part 1. Stop use of the backbone router in the standalone UI to simplify code. The views now exclusively manage what is displayed according to state.

Updated patch, addressed review comments

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 2. Rename some old variables in the Loop OutgoingConversationView.

This does the simple renames that I'd left to make part 1 clearer.

Comment 7

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

Comment on attachment 8490828 [details] [diff] [review]
Part 2. Rename some old variables in the Loop OutgoingConversationView.

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

Much better indeed.

Comment 8

[Mark Banner (:standard8)]

Parts 1 & 2 landed:

https://hg.mozilla.org/integration/fx-team/rev/9a66e4f7e5c7
https://hg.mozilla.org/integration/fx-team/rev/04d71fb63b17

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a66e4f7e5c7
https://hg.mozilla.org/mozilla-central/rev/04d71fb63b17

Comment 10

[u279076]

I don't think this will need explicit QE verification. Please needinfo me to request manual testing.

Comment 11

[u279076]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/9a66e4f7e5c7
> https://hg.mozilla.org/mozilla-central/rev/04d71fb63b17

By the way, are we done here? Can this bug be marked resolved fixed?

Comment 12

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #11)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> > https://hg.mozilla.org/mozilla-central/rev/9a66e4f7e5c7
> > https://hg.mozilla.org/mozilla-central/rev/04d71fb63b17
> 
> By the way, are we done here? Can this bug be marked resolved fixed?

No, we've at least one more part, if not a few more. The first steps were the make things a little simpler, now for the complex part.

Comment 13

[Mark Banner (:standard8)]

Ok, although I expected more work here, the work changed slightly, and ongoing work is in bug 972017. Hence marking this as fixed now.

Comment 14

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

Comment on attachment 8490815 [details] [diff] [review]
Part 1. Stop use of the backbone router in the standalone UI to simplify code. The views now exclusively manage what is displayed according to state.

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/7638339c5def
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8808a96401f

Comment 16

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

Comment on attachment 8490815 [details] [diff] [review]
Part 1. Stop use of the backbone router in the standalone UI to simplify code. The views now exclusively manage what is displayed according to state.

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.