Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toast Notification should handle connection failed #12609

Merged
merged 2 commits into from Jun 30, 2017

Conversation

Projects
None yet
3 participants
@pickypg
Copy link
Member

pickypg commented Jun 30, 2017

I noticed this happen while using the Notifier to send a failure to it from Angular that was unable to reach the server. This allows the Notifier to display a helpful error rather than triggering one itself.

Note: err.data is explicitly null in this case, so it has it, but it is not useful, which is why this triggers a browser exception.

@pickypg pickypg requested a review from spalger Jun 30, 2017

rtn += 'Error ' + err.status + ' ' + err.statusText + ': ' + err.data.message;
if (err.status === -1) {
// status = -1 indicates that the request was failed to reach the server
rtn += 'Connection failed! Is Kibana running?';

This comment has been minimized.

Copy link
@tsullivan

tsullivan Jun 30, 2017

Contributor

This message feels a little accusatory towards the user. How about something. Like:

A request to the Kibana server has failed to connect. Please check if the Kibana server is running and that your browser has a working connection to the Internet.

This comment has been minimized.

Copy link
@pickypg

pickypg Jun 30, 2017

Author Member

I like it, but I'm going to drop the to the Internet part at the end to avoid confusing users running it locally / internally on an intranet.

This comment has been minimized.

Copy link
@pickypg

pickypg Jun 30, 2017

Author Member

Ended up with:

An HTTP request has failed to connect. Please check if the Kibana server is running and that your browser has a working connection, or contact your system administrator.

@pickypg

This comment has been minimized.

Copy link
Member Author

pickypg commented Jun 30, 2017

please test this

@tsullivan
Copy link
Contributor

tsullivan left a comment

LGTM

@pickypg pickypg merged commit 2d0e2e4 into elastic:master Jun 30, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

@pickypg pickypg deleted the pickypg:fix/allow-connection-failed branch Jun 30, 2017

pickypg added a commit that referenced this pull request Jun 30, 2017

@pickypg

This comment has been minimized.

Copy link
Member Author

pickypg commented Jun 30, 2017

5.x/5.6: 5e5ea2d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.