Closed Bug 1088672 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Points:
8

Tracking

(firefox39 verified)

VERIFIED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox39 --- verified
Blocking Flags:
backlog tech-debt

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [tech-debt][for verification tests see comment 10])

Attachments

(9 files, 1 obsolete file)

17.30 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
34.58 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
25.08 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
7.14 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
10.81 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
53.07 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
19.41 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
109.52 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
14.72 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
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.
backlog: --- → Fx36+
Priority: -- → P2
Whiteboard: [tech-debt]
maire following up with Mark on if enough of this was done that the rest can go to 37
Flags: needinfo?(mreavy)
There's more we need to do, but it can be done in Fx37.  (We'll want to do it early in Fx37.)
backlog: Fx36+ → Fx37+
Flags: needinfo?(mreavy)
Assignee: nobody → standard8
Depends on: 1106474
Depends on: 1117138
backlog: Fx37+ → tech-debt
Depends on: 1140481
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.
Attachment #8575196 - Flags: review?(mdeboer)
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.
Attachment #8575198 - Flags: review?(mdeboer)
This is the relatively simple part to hook up some new actions for the AcceptCallView.
Attachment #8575199 - Flags: review?(mdeboer)
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.
Attachment #8575202 - Flags: review?(mdeboer)
This tidies up the call failed view for incoming calls to only give the "Something went wrong" message and "Cancel" button.
Attachment #8575204 - Flags: review?(mdeboer)
Small patch to fix up the window titles for incoming calls.
Attachment #8575205 - Flags: review?(mdeboer)
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.
Attachment #8575207 - Flags: review?(mdeboer)
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!
Iteration: --- → 39.2 - 23 Mar
Points: --- → 8
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [tech-debt] → [tech-debt][for verification tests see comment 10]
Attachment #8575196 - Flags: review?(mdeboer) → review+
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.
Attachment #8575198 - Flags: review?(mdeboer) → review+
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.
Attachment #8575199 - Flags: review?(mdeboer) → review+
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?
Attachment #8575202 - Flags: review?(mdeboer) → review-
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?
Attachment #8575204 - Flags: review?(mdeboer) → review+
Attachment #8575205 - Flags: review?(mdeboer) → review+
Attachment #8575207 - Flags: review?(mdeboer) → review+
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 ;-)
Status: NEW → ASSIGNED
(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.
Updated patch. The XXX was an error, it was something I'd added in previously and forgotten to remove.
Attachment #8575202 - Attachment is obsolete: true
Attachment #8576004 - Flags: review?(mdeboer)
Attached patch Rollup patchSplinter Review
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?
Attachment #8576012 - Flags: review?(mdeboer)
(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.
I'd forgotten to check that the ui-showcase still worked properly. This is the fix for that.
Attachment #8576219 - Flags: review?(mdeboer)
Rank: 23
Attachment #8576004 - Flags: review?(mdeboer) → review+
Attachment #8576219 - Flags: review?(mdeboer) → review+
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 on attachment 8576012 [details] [diff] [review]
Rollup patch

Awesome! r=me.
Attachment #8576012 - Flags: review?(mdeboer) → review+
Blocks: 1124360
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.