1088672 - Rewrite the Incoming call flow (direct calls) in the Flux style

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

The incoming call code is a mess with business logic intertwined with the views. It is really hard to read, and its been feeling very fragile.

We should migrate it to share, and extend the conversationStore that the outgoing call code is written on, and hence to the flux style.

Comment 1

[:shell escalante]

maire following up with Mark on if enough of this was done that the rest can go to 37

Comment 2

[Maire Reavy [:mreavy]]

There's more we need to do, but it can be done in Fx37.  (We'll want to do it early in Fx37.)

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 1. Rewrite Loop's incoming call handling in the flux style. Rename some views for better naming.

This is a multi-part patch as its quite big overall and I've tried to split it up logically. This patch set depends on the patch in bug 1140481. There's a github branch with all the patches available here:

https://github.com/Standard8/gecko-dev/compare/fx-team...Standard8:bug-1140481-storemixin


This first patch simply renames a couple of views - the outgoing conversation view is now going to be used for incoming as well, and the "IncomingCallView" was slightly ambiguous so I changed it to "AcceptCallView".

In both cases I'm moving these towards using "Call" rather than "Conversation" as we agreed a little while ago when talking about tech-debt.

Comment 4

[Mark Banner (:standard8)]

Attachment: Part 2. Rewrite Loop's incoming call handling in the flux style. Switch incoming calls to use flux based conversation conversation store and get them working as far as the accept view.

This second part switches the incoming calls to use the new flux based views and stores. It gets it working up to the point where the AcceptCallView is displayed - the buttons don't work yet; that's in the next patch.

Note: removing the old views is done at the end.

Comment 5

[Mark Banner (:standard8)]

Attachment: Part 3. Rewrite Loop's incoming call handling in the flux style. Get the accept and cancel buttons working again on the accept call view.

This is the relatively simple part to hook up some new actions for the AcceptCallView.

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 4. Rewrite Loop's incoming call handling in the flux style. Put back alerts and make window unload be handled correctly.

Here we move tidy up the unload handling and make the start/stopAlerting work again.

The clearCallInProgress call is handled in conversationStore's _endSession function already, hence removing it from the global unload handler.

Comment 7

[Mark Banner (:standard8)]

Attachment: Part 5. Rewrite Loop's incoming call handling in the flux style. Correct the Call failed view to show only what's needed for incoming calls.

This tidies up the call failed view for incoming calls to only give the "Something went wrong" message and "Cancel" button.

Comment 8

[Mark Banner (:standard8)]

Attachment: Part 6. Rewrite Loop's incoming call handling in the flux style. Fix window titles for the incoming call view.

Small patch to fix up the window titles for incoming calls.

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 7. Rewrite Loop's incoming call handling in the flux style. Remove the now redundant non-flux based code for incoming calls.

Removes the old non-flux based incoming call views and the tests. I'd like to remove more, but I think that's about it.

I might move the ConversationModel out of the shared models.js, but if I do, I'll do that in a separate bug.

Comment 10

[Mark Banner (:standard8)]

For verification:

- Check that joining a conversation still works fine (just in case something broke).
- Set up direct calls between two computers/browsers with having signed in via FxA.
- Then test:
-- Accept call works
-- Cancel call button works (for both caller & callee before the call has been accepted)
-- Cancel & Block button works (check in the contacts list after blocking)
-- Closing the window whilst on the accept screen give the response "<name> is unavailable" to the caller
-- Closing the caller's window just after having the callee accepted the call gives "Something went wrong"
-- Ending the call should work fine (going to the feedback view)
-- Title bar for the callee shows as the caller's email address

Anything else that might need testing!

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8575198 [details] [diff] [review]
Part 2. Rewrite Loop's incoming call handling in the flux style. Switch incoming calls to use flux based conversation conversation store and get them working as far as the accept view.

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

::: browser/components/loop/content/js/conversationViews.jsx
@@ +151,5 @@
>  
>      getDefaultProps: function() {
>        return {
>          showMenu: false,
> +        callType: true

If this is a placeholder default value, can you add a comment saying this? I mean, this being a Boolean instead of a string as defined and used here is not entirely intuitive.
Perhaps setting this in the defaultProps is unnecessary, as it's a required prop anyway. This would also be a good time to add `showMenu` to the `propTypes` definition.

@@ +975,5 @@
> +          enableCancelButton={this._isCancellable()}
> +        />);
> +      }
> +
> +      // For incoming calls that are in accepting state display the

nit: '... state, display...'

@@ +982,5 @@
> +        return (<AcceptCallView
> +          callType={this.state.callType}
> +          callerId={this.state.callerId}
> +          dispatcher={this.props.dispatcher}
> +        />);

nit: putting the `/>` on its own line makes my editor (Sublime) go bonkers. Can we start putting this after the last attribute?

@@ +987,5 @@
> +      }
> +
> +      // Otherwise we're still gathering or connecting, so
> +      // don't display anything.
> +      return null;

I don't think you need to explicitly return anything here.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +243,1 @@
>          outgoing: windowType === "outgoing",

I think you're trying to sort alphabetically here, can you persist that?

@@ +310,5 @@
> +        var callState = this.getStoreState("callState");
> +        if (this._websocket &&
> +            (callState === CALL_STATES.CONNECTING ||
> +             callState === CALL_STATES.ALERTING)) {
> +           // Let the server know the user has hung up.

nit: indentation.

@@ +496,5 @@
> +      // the call failed view.
> +      //
> +      // https://wiki.mozilla.org/Loop/Architecture/MVP#Termination_Reasons
> +      //
> +      // For outgoing calls, we treat terminations as failures.

Can you move this comment to its own docblock comment for this member function?

@@ +504,5 @@
> +        // For outgoing calls we can treat everything as connection failure.
> +        this.dispatcher.dispatch(new sharedActions.ConnectionFailure({
> +          reason: progressData.reason
> +        }));
> +

nit: superfluous newline.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8575199 [details] [diff] [review]
Part 3. Rewrite Loop's incoming call handling in the flux style. Get the accept and cancel buttons working again on the accept call view.

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

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +1221,5 @@
>      });
>    });
>  
>    describe("AcceptCallView", function() {
> +    var view;

I think you have two options here:

1. Remove `var view;` here and prepend each `view = mountTe...` with a `var `
2. Make `mountTestComponent()` set the view variable and remove `view = ` in the tests.

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8575202 [details] [diff] [review]
Part 4. Rewrite Loop's incoming call handling in the flux style. Put back alerts and make window unload be handled correctly.

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

Please take another look at this patch.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +419,5 @@
>        }.bind(this));
>      },
>  
>      /**
> +     * Called with the window is unloaded, either by code, or by the user

nit: s/with/when/ - I think you changed this accidentally.

@@ +424,2 @@
>       * explicitly closing it.  Expected to do any necessary housekeeping, such
> +     * as shutting down the call cleanly and adding any relevant telemtry data.

nit: another whoopsie?

@@ +426,3 @@
>       */
>      windowUnload: function() {
> +      // XXX Send decline on websocket?

Can you elaborate here?

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8575204 [details] [diff] [review]
Part 5. Rewrite Loop's incoming call handling in the flux style. Correct the Call failed view to show only what's needed for incoming calls.

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

::: browser/components/loop/content/js/conversationViews.jsx
@@ +827,5 @@
>        }));
>      },
>  
>      render: function() {
> +      var retryClasses = React.addons.classSet({

nit: please alias to cx()

@@ +849,5 @@
>        return (
>          <div className="call-window">
>            <h2>{ this._getTitleMessage() }</h2>
>  
> +          <p className="btn-label">{subMessage}</p>

wouldn't it be cleaner to make rendering this label conditional instead of putting an empty string in it?

Comment 15

[Mike de Boer [:mikedeboer]]

For the next round, could you attach a folded patch to test things out with? I'd also like to see how this looks like when looked at in its entirety.

We're nearly there, it's looking awesome!

Can we land this after the main tab-sharing work has landed on m-c? I'm not really looking forward to unbitrotting any remaining patches when uplifting to Aurora ;-)

Comment 16

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> >      getDefaultProps: function() {
> >        return {
> >          showMenu: false,
> > +        callType: true
> 
> Perhaps setting this in the defaultProps is unnecessary, as it's a required
> prop anyway.

Yeah, mistake on my part - I was originally passing in boolean values and changed it to the callType to be clearer.

> @@ +982,5 @@
> > +        return (<AcceptCallView
> > +          callType={this.state.callType}
> > +          callerId={this.state.callerId}
> > +          dispatcher={this.props.dispatcher}
> > +        />);
> 
> nit: putting the `/>` on its own line makes my editor (Sublime) go bonkers.
> Can we start putting this after the last attribute?

I think I prefer it on a new line, but I guess I'm open. I think the only thing I'd want to do is to check the others agree first so that we can move forward with a consistent style from now on.

> > +      // Otherwise we're still gathering or connecting, so
> > +      // don't display anything.
> > +      return null;
> 
> I don't think you need to explicitly return anything here.

React complains if you don't. I think it'd also violate some linters as you'd be returning a value from one place but nothing from somewhere else.

Comment 17

[Mark Banner (:standard8)]

Attachment: Part 4. Rewrite Loop's incoming call handling in the flux style. Put back alerts and make window unload be handled correctly.

Updated patch. The XXX was an error, it was something I'd added in previously and forgotten to remove.

Comment 18

[Mark Banner (:standard8)]

Attachment: Rollup patch

This is the roll-up patch for all the previous parts (after I've addressed the comments).

I'm not sure that we need to hold this of for tab sharing landings. The tab sharing is for rooms only, all the files affected are conversation specific - apart from conversation.html & conversation.js(x) and maybe actions.js.

Those diffs though I don't think would conflict with any of the tab sharing code - or am I missing something?

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #18)
> I'm not sure that we need to hold this of for tab sharing landings. The tab
> sharing is for rooms only, all the files affected are conversation specific
> - apart from conversation.html & conversation.js(x) and maybe actions.js.
> 
> Those diffs though I don't think would conflict with any of the tab sharing
> code - or am I missing something?

Indeed, please forget that I mentioned this. There are no UI patches for tab sharing to land after this anyway, at least not targeted for Fx38.

Comment 20

[Mark Banner (:standard8)]

Attachment: Part 8. Rewrite Loop's incoming call handling in the flux style. Update the ui-showcase.

I'd forgotten to check that the ui-showcase still worked properly. This is the fix for that.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8576219 [details] [diff] [review]
Part 8. Rewrite Loop's incoming call handling in the flux style. Update the ui-showcase.

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

::: browser/components/loop/ui/ui-showcase.jsx
@@ +238,5 @@
>    var App = React.createClass({
>      render: function() {
>        return (
>          <ShowCase>
> +

nit: superfluous newline.

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8576012 [details] [diff] [review]
Rollup patch

Awesome! r=me.

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/624232e720c8
https://hg.mozilla.org/integration/fx-team/rev/8119c5b06253
https://hg.mozilla.org/integration/fx-team/rev/dc2dded44d22
https://hg.mozilla.org/integration/fx-team/rev/132acc4464e6
https://hg.mozilla.org/integration/fx-team/rev/a009386fbb96
https://hg.mozilla.org/integration/fx-team/rev/ea81bbb5d288
https://hg.mozilla.org/integration/fx-team/rev/9303663e42d3
https://hg.mozilla.org/integration/fx-team/rev/c154c877d4e4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/624232e720c8
https://hg.mozilla.org/mozilla-central/rev/8119c5b06253
https://hg.mozilla.org/mozilla-central/rev/dc2dded44d22
https://hg.mozilla.org/mozilla-central/rev/132acc4464e6
https://hg.mozilla.org/mozilla-central/rev/a009386fbb96
https://hg.mozilla.org/mozilla-central/rev/ea81bbb5d288
https://hg.mozilla.org/mozilla-central/rev/9303663e42d3
https://hg.mozilla.org/mozilla-central/rev/c154c877d4e4

Comment 25

[Bogdan Maris, Desktop QA]

We did some exploratory testing on hello using guidance from comment 10 on Firefox 39 beta 1 across platforms. No new issue was found regarding this fix, so marking this as verified fixed.