1002416 - [UX] MozLoopService needs to be able to surface failures to register to the user

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Currently, if the MozLoopService fails to register with the server, then the error is logged, but nothing is displayed to the user. If they are expecting an incoming call, this could be an issue.

This might be more appropriate to fix after FxA have been implemented, although bug 994151 may have to handle part of this anyway (as that will need to delay first registrations and ensure registration has taken place).

Comment 1

[Mark Banner (:standard8)]

We should probably do this quite soon as users need to know about the issues. Though we may want to wait until we've fixed bug 1002414 as that will also be altering/generating additional errors due to registrations etc.

Typical errors I expect are: disconnect from either push server or loop server due to network error, offline issues, etc.

In any case we also need some UX here - to define how we want to notify/warn the user if there is an issue.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

My expectation would be that we'll have a bunch of different kinds of errors that we'll want to do individual things about (eg try to fix as automatically as possible).  Presumably the only thing that generalizes is the base case: "here's an error that we can't help you fix", which before too long sometimes becomes "here's a bug we can't directly help you fix, please submit it to our servers for further analysis".

Comment 3

[Darrin Henein [:darrin]]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #2)
> My expectation would be that we'll have a bunch of different kinds of errors
> that we'll want to do individual things about (eg try to fix as
> automatically as possible). 

Makes sense, though we may want to alert the user in some cases that something is not working as expected (No connection, etc.)

I will add this to my list of UI to add ;)

Comment 4

[Romain Testard [:RT]]

I mark this as blocking 1028894 although Darrin please confirm if it makes sense to expose failures to register in the client toolbar icon.

Comment 5

[Darrin Henein [:darrin]]

There will certainly be failures which we should expose (in our own language) to the user, which will likely be grouped into user facing errors (i.e. "Could not connect", "Call failed", etc.)

Comment 6

[Darrin Henein [:darrin]]

Is something like https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error what was in mind for this bug? This is a generic error & warning bar with a primary text and an optional description (click the chevron) field.

Comment 7

[Romain Testard [:RT]]

I was thinking more like an indication in the Toolbar button so that the user knows that he is not reachable without having to drop the panel down.
Also for in-panel information, the red "Could not connect" indicator visually conflicts with the green "Available " dot. 

So it feels better to:
=> Have an indication on the toolbar button (Green or red dot?)
=> Have an indication on the panel through the current Availability indicator rather than adding a new one conflicting with the existing one? Perhaps we can also disable (grey out) the panel if the user is disconnected from the server?

Comment 8

[Darrin Henein [:darrin]]

(In reply to Romain Testard [:RT] from comment #7)
> I was thinking more like an indication in the Toolbar button so that the
> user knows that he is not reachable without having to drop the panel down.
> Also for in-panel information, the red "Could not connect" indicator
> visually conflicts with the green "Available " dot. 

We should definitely have an indicator in the toolbar icon (which I am hoping to get to next, was waiting on Arcadio and naming before committing to icons) but that won't communicate much more than "something is wrong". This lets us clarify and even give the user some indication on how to resolve it. Agreed, my mockup is wrong in showing the available dot (though the availability indicator is not a connected indicator, just an indicator of who can reach me).

> So it feels better to:
> => Have an indication on the toolbar button (Green or red dot?)
Agreed, this was the plan.

> => Have an indication on the panel through the current Availability
> indicator rather than adding a new one conflicting with the existing one?
Again, the Availability of the user is not directly connected (yet) with their connected state. Also the error may be something other than connection state (ie user is Available but their contact import failed)

> Perhaps we can also disable (grey out) the panel if the user is disconnected
> from the server?
Possibly, yes. We should get a better understanding of the various states of connectedness and how they affect the functionality... i.e. is it all or nothing == connected/not connected? Adam?

Comment 9

[Adam Roach [:abr]]

(In reply to Darrin Henein [:darrin] from comment #8)
> Possibly, yes. We should get a better understanding of the various states of
> connectedness and how they affect the functionality... i.e. is it all or
> nothing == connected/not connected? Adam?

In terms of reachability, it is all or nothing -- basically, if the client can contact the push server, get a push URL, give the push URL to the Loop server, and get a response, he is reachable (at least, within our ability to detect; I'll get to that in a moment). If the client can't do either of these things, then no calls can show up.

Now, in theory, there could be an issue with some portion of the system (e.g., the Loop server or some critical part of the TokBox service goes down) that would prevent calls from succeeding, but we don't have any mechanism for clients to know about that situation at the moment (any solution to detect such a case would introduce some scaling challenges).

Comment 10

[Darrin Henein [:darrin]]

Ok -- great, thanks Adam. This answers my question on whether the availabilty (who can contact me) relates to the service connection state (it does -- you are contactable by no one if you aren't 'connected')

I will make sure the error states align with the availability indicator.

Comment 11

[Romain Testard [:RT]]

Regarding https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error:
What is the difference between the error bar and the warning bar?
For both I think we should prevent the user from generating/sharing/copying URLs given it will give the wrong impression that these will result in call-backs. I suggest we disable (grey-out) the UX when we detect a failure to register on the client side.
Darrin can you please confirm if you agree?

Comment 12

[Darrin Henein [:darrin]]

I agree, any controls rendered unusable by the current error state should be disabled (faded out). The two states (error and warning) are just options for us to expose various types of issues... errors should be typically blocking (no connection, etc) whereas warnings can surface usability related concerns (slow connection, google contacts import failed halfway) where the system is still usable but the user should be aware of some failure.

Comment 13

[Romain Testard [:RT]]

Thanks.

So this bug is about surfacing an error to register (prevents a user a user from receiving a phone call or generating a call-back URL).
The following UX should apply with all controls disabled in the panel:
https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error

Comment 15

[Romain Gauthier [:tOkeshu]]

Attachment: WIP patch to add user facing errors when the registration failed

Comment 16

[Romain Gauthier [:tOkeshu]]

Attachment: WIP patch to add user facing errors when the registration failed

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8472988 [details] [diff] [review]
WIP patch to add user facing errors when the registration failed

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

I've only taken a quick glance at this so far.

If I do:

- Start FF with no server running
- View the panel => Error message (correct)
- Start the server
- View the panel again

=> no error message. In the error console I see:


Full Message: Error: Invariant Violation: replaceState(...): Can only update a mounted or mounting component.


Also, I was expecting the toolbar button to change state. That's probably best covered in a follow-up bug, as it'll need to go through the loop service api if I remember correctly - so please file a bug for that.

Comment 18

[Romain Testard [:RT]]

> Also, I was expecting the toolbar button to change state. That's probably
> best covered in a follow-up bug, as it'll need to go through the loop
> service api if I remember correctly - so please file a bug for that.

I assume this is covered under bug 1047284

Comment 19

[Romain Gauthier [:tOkeshu]]

Attachment: WIP patch to add user facing errors when the registration failed

Comment 20

[Romain Gauthier [:tOkeshu]]

(In reply to Mark Banner (:standard8) from comment #17)
> Full Message: Error: Invariant Violation: replaceState(...): Can only update
> a mounted or mounting component.

I can't reproduce that error with the current trunk.

Comment 21

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

Comment on attachment 8479002 [details] [diff] [review]
WIP patch to add user facing errors when the registration failed

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

Looks good. Minor things to be fixed, naming could be possibly improved, but this patch greatly improves the current situation.

::: browser/components/loop/content/js/panel.jsx
@@ +182,5 @@
>                                         this._onCallUrlReceived);
>      },
>  
>      _onCallUrlReceived: function(err, callUrlData) {
> +      this.props.notifications.clear();

.reset()

::: browser/components/loop/content/shared/css/common.css
@@ +262,5 @@
>  .alert.alert-error {
> +  display: flex;
> +  align-content: center;
> +  padding: 5px;
> +  font-size: 10px;

I didn't check, but usually 10px font size is hardly legible for older people like me =)

Also, I'm wondering why only the error notification seems to be styled that way…

::: browser/components/loop/content/shared/js/models.js
@@ +343,5 @@
> +    /**
> +     * Clears the notification stack.
> +     */
> +    clear: function() {
> +      this.reset();

There's now no point of having this alias, let's just use reset() directly.

@@ +351,5 @@
> +     * Adds a new notification to the stack, triggering rendering of it.
> +     *
> +     * @param  {Object|NotificationModel} notification Notification data.
> +     */
> +    notify: function(notification) {

As this is now just an alias to .add, this could be safely removed, so we could use add() directly.

@@ +362,5 @@
> +     *
> +     * @param  {String} messageId L10n message id
> +     * @param  {String} level     Notification level
> +     */
> +    notifyL10n: function(messageId, level) {

addL10n?

@@ +374,5 @@
> +     * Adds a warning notification to the stack and renders it.
> +     *
> +     * @return {String} message
> +     */
> +    warn: function(message) {

addWarning?

@@ +383,5 @@
> +     * Adds a l10n warning notification to the stack and renders it.
> +     *
> +     * @param  {String} messageId L10n message id
> +     */
> +    warnL10n: function(messageId) {

addWarningL10n?

@@ +392,5 @@
> +     * Adds an error notification to the stack and renders it.
> +     *
> +     * @return {String} message
> +     */
> +    error: function(message) {

addError?

@@ +401,5 @@
> +     * Adds a l10n rror notification to the stack and renders it.
> +     *
> +     * @param  {String} messageId L10n message id
> +     */
> +    errorL10n: function(messageId) {

addErrorL10n?

::: browser/components/loop/content/shared/js/router.js
@@ +23,5 @@
>      _activeView: undefined,
>  
>      /**
>       * Notifications dispatcher.
>       * @type {loop.shared.views.NotificationListView}

This needs updating.

::: browser/components/loop/content/shared/js/views.jsx
@@ +647,5 @@
> +      var notifications = this.props.collection.map(function(notification) {
> +        return <div key={notification.cid}
> +                    className={"alert alert-" + notification.get('level')}>
> +                  <span className="message">{notification.get('message')}</span>
> +                </div>

Nit: The coding style for rendered JSX accross the current codebase is usually:

return (
  <foo>
    <bar/>
  </foo>
);

Let's keep it that way :)

::: browser/components/loop/test/shared/views_test.js
@@ +614,3 @@
>      });
>  
> +    describe.skip("Collection events", function() {

This shouldn't be skipped :)

::: browser/components/loop/ui/ui-showcase.jsx
@@ +90,5 @@
>  
>    var App = React.createClass({
>      render: function() {
> +      var client = new loop.Client({
> +        baseServerUrl: navigator.mozLoop.serverUrl

We probably want a fake string instead here.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8479002 [details] [diff] [review]
WIP patch to add user facing errors when the registration failed

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

I'm still seeing the issue with the panel failing the second time displaying. Furthermore, I see it in the no-errors case as well, e.g. open FF, open panel, close panel, open panel again -> no url, error on console:

JavaScript error: chrome://browser/content/loop/shared/libs/react-0.11.1.js, line 18697: Invariant Violation: replaceState(...): Can only update a mounted or mounting component.

I think that really needs fixing before this can land. If you definitely can't see this, let me know, and I'll see if I can find some time to debug.

Comment 23

[Romain Gauthier [:tOkeshu]]

Attachment: WIP patch to add user facing errors when the registration failed

Comment 24

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

Comment on attachment 8479878 [details] [diff] [review]
WIP patch to add user facing errors when the registration failed

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

LGTM, minor nits you'll probably want to address before landing.

::: browser/components/loop/MozLoopService.jsm
@@ +278,2 @@
>          Cu.reportError("Failed to register with the loop server. error: " + error);
> +        gRegisteredDeferred.reject(error);

Out of curiosity, does that mean that the passed error could be either a string or an object? In that case I'd create an object matching the structure in case we just want to notify for server-unreachable.

::: browser/components/loop/content/shared/js/views.jsx
@@ +625,4 @@
>     * Notification list view.
>     */
> +  var NotificationListView = React.createClass({
> +    displayName: 'NotificationListView',

displayName is not needed using JSX.

@@ +633,4 @@
>      },
>  
> +    getInitialState: function() {
> +      return {collection: []};

Unneeded as this.state.collection is never used.

::: browser/components/loop/test/shared/views_test.js
@@ +623,4 @@
>  
> +      // beforeEach(function() {
> +      //   sandbox.stub(view, "render");
> +      // });

Should be removed.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +92,5 @@
>      render: function() {
> +      var client = new loop.Client({
> +        baseServerUrl: navigator.mozLoop.serverUrl
> +      });
> +      var notifications = new loop.shared.models.NotificationCollection();

I'd want to see these declaration moved outside the render() method, eg. at the top of the file along with other fakes.

Comment 25

[Romain Testard [:RT]]

I noticed the UX changed whilst I was on leave (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error)
Darrin can you please clarify - the amended UX seems to apply only to signed-in users whereas before it was applicable for anything?

Comment 26

[Darrin Henein [:darrin]]

The UX shown is a generic error experience -- we can use this UI for any error we want to surface to teh user. The exact copy and messaging should come from Matej & co.

Each error has a Title, Message and (optional) Action.

Comment 27

[Romain Testard [:RT]]

OK, thanks for clarifying, makes sense now.

Comment 28

[Mark Banner (:standard8)]

Attachment: surface failures to register to the user

Updated to fix the comments from the feedback. I dropped the MozLoopService changes as they seemed to cause a few more issues than expected, bug 1059754 may help fix the actual issue, if it doesn't, then we'll be in the same position (but with user visible errors for most things).

I also added an error example to the ui-showcase.

Comment 29

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

Comment on attachment 8481332 [details] [diff] [review]
surface failures to register to the user

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

Looks great, though the feature is broken until bug 1059754 lands — adding it as a dependency.

Also, a few nits you may want to address before landing.

r+ conditional to bug 1059754 landing first.

::: browser/components/loop/content/js/panel.jsx
@@ +275,5 @@
>        navigator.mozLoop.logInToFxA();
>      },
>  
>      render: function() {
> +      var NotificationListView = loop.shared.views.NotificationListView;

Nit: sharedViews.NotificationListView is shorter.

@@ +281,3 @@
>        return (
>          <div>
> +          <NotificationListView collection={this.props.notifications} />

Nit: As of React 0.11, you can construct the component using a namespace, like <sharedViews.NotificationListView/>

Not sure it would improve legibility here though.

::: browser/components/loop/content/shared/js/router.js
@@ +22,5 @@
>       */
>      _activeView: undefined,
>  
>      /**
>       * Notifications dispatcher.

s/dispatcher/collection

::: browser/components/loop/content/shared/js/views.jsx
@@ +649,5 @@
> +      var notifications = this.props.collection.map(function(notification) {
> +        return <div key={notification.cid}
> +                    className={"alert alert-" + notification.get('level')}>
> +                  <span className="message">{notification.get('message')}</span>
> +                </div>

Please use the style used everywhere else:

render: function() {
  return (
    <tag/>
  );
}

@@ +652,5 @@
> +                  <span className="message">{notification.get('message')}</span>
> +                </div>
> +      });
> +
> +      return <div id="messages">{notifications}</div>;

Call me crazy but I tend to favor this style:

render: function() {
  var cx = React.addons.classSet;
  return (
    <div id="messages">{
      this.props.collection.map(function(notification) {
        return (
          <div key={notification.cid}
               className={"alert alert-" + notification.get('level')}>
            <span className="message">{notification.get('message')}</span>
          </div>
        );
      }, this)
    }</div>;
  );
}

One could also create a Notification component to decrease verbosity of the map closure.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +151,5 @@
>  
>      propTypes: {
>        model: React.PropTypes.instanceOf(sharedModels.ConversationModel)
>                                         .isRequired,
>        // XXX Check more tightly here when we start injecting window.loop.*

(unrelated to current patch, but) wat.

Comment 30

[Romain Gauthier [:tOkeshu]]

Attachment: surface failures to register to the user

Comment 31

[Mark Banner (:standard8)]

Attachment: Rework the Loop notification system to better surface failures to the user and use react based views.

I updated the patch to include some of the comments that got missed from when I did the adjusted version. Discussed some of the changes with :NiKo` over video, and he was happy with them.

Comment 32

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/d85aa5d54a67

Comment 33

[Mark Banner (:standard8)]

I've split out an issue I saw with the panel before landing to bug 1064900 - I decided there were several unanswered questions that we can sort through there.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d85aa5d54a67

Comment 35

[u279076]

Mark, does this need manual testing or are the landed tests sufficient?

Comment 36

[u279076]

Shell, I believe this is on the Google Doc of work tracked for Firefox 34. Should this be tagged for uplift?

Comment 37

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #35)
> Mark, does this need manual testing or are the landed tests sufficient?

Getting general testing on error notifications would be useful, e.g. no server available.

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #36)
> Shell, I believe this is on the Google Doc of work tracked for Firefox 34.
> Should this be tagged for uplift?

This doesn't really introduce anything new, but I think it might be good to uplift to save bitrot nightmares.

Comment 38

[:shell escalante]

added for our loop-uplift based on valid comment to avoid bitrot.

Comment 39

[u279076]

Paul, can you please make sure this gets some testing in the latest Nightly and the Try-server build here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 40

[Paul Silaghi, QA [:pauly]]

(In reply to Romain Testard [:RT] from comment #13)
> The following UX should apply with all controls disabled in the panel:
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error
What should I do in order to see this ?
I only see the error bar with "Sorry, we were unable to retrieve a call url" message which was verified in bug 1059754

Comment 41

[Mark Banner (:standard8)]

(In reply to Paul Silaghi, QA [:pauly] from comment #40)
> (In reply to Romain Testard [:RT] from comment #13)
> > The following UX should apply with all controls disabled in the panel:
> > https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error
> What should I do in order to see this ?
> I only see the error bar with "Sorry, we were unable to retrieve a call url"
> message which was verified in bug 1059754

Yes, that's the only error we have there currently, and the only one handled by this bug iirc. There's more being added soon for FxA by other bugs I believe.

Comment 42

[Paul Silaghi, QA [:pauly]]

Thanks Mark.
Verified fixed in bug 1059754.

Comment 43

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

Comment on attachment 8486279 [details] [diff] [review]
Rework the Loop notification system to better surface failures to the user and use react based views.

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

Comment 44

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/46cfff89e9ec

Comment 45

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

Comment on attachment 8486279 [details] [diff] [review]
Rework the Loop notification system to better surface failures to the user and use react based views.

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.

Comment 46

[u279076]

Paul, this has been uplifted to Aurora. Can you please test this to confirm it's working as expected?

Comment 47

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b1 Win7

Comment 49

[u279076]

Assuming the provided test coverage is sufficient. Please need-info me to request further test coverage.