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

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla35
Points:
8
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

(Whiteboard: [loop-outcall1][loop-uplift])

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 1066502
Assignee

Updated

5 years ago
Whiteboard: [loop-outcall1]
Assignee

Comment 1

5 years ago
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.
Assignee

Comment 2

5 years ago
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.
Attachment #8490128 - Attachment is obsolete: true
Attachment #8490682 - Flags: review?(nperriault)
Assignee

Comment 3

5 years ago
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.
Assignee

Updated

5 years ago
Attachment #8490687 - Attachment description: 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. → 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
Attachment #8490687 - Flags: review?(nperriault)
Assignee

Updated

5 years ago
Attachment #8490682 - Attachment is obsolete: true
Attachment #8490682 - Flags: review?(nperriault)
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.
Attachment #8490687 - Flags: review?(nperriault) → review+
Assignee

Updated

5 years ago
Iteration: --- → 35.2
Target Milestone: --- → mozilla35
Assignee

Comment 6

5 years ago
This does the simple renames that I'd left to make part 1 clearer.
Attachment #8490828 - Flags: review?(nperriault)
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.
Attachment #8490828 - Flags: review?(nperriault) → review+

Updated

5 years ago
Flags: qe-verify?
Whiteboard: [loop-outcall1] → [loop-outcall1][loop-uplift]
I don't think this will need explicit QE verification. Please needinfo me to request manual testing.
Flags: qe-verify? → qe-verify-
(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?
Assignee

Comment 12

5 years ago
(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.
Assignee

Updated

5 years ago
No longer blocks: 1066502
Depends on: 1066502
Assignee

Comment 13

5 years ago
Ok, although I expected more work here, the work changed slightly, and ongoing work is in bug 972017. Hence marking this as fixed now.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8490815 - Flags: approval-mozilla-aurora?
Attachment #8490828 - Flags: approval-mozilla-aurora?
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.
Attachment #8490815 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8490828 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.