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

Happychat: Add better messaging when there are connectivity issues #12754

Merged
merged 3 commits into from
Apr 5, 2017

Conversation

mattwondra
Copy link
Member

@mattwondra mattwondra commented Apr 3, 2017

This combined with PR #12706 will fix issue #12183.

This PR adds a few Socket.IO lifecycle event dispatches that we can use to get more information on the status of a HC connection. This information is used to disable the text input and show a notice when certain connection issues are occurring, specifically:

  • User tries to connect to HC but the server is not available
  • User has an open connection and the HC server disconnects
  • User has an open connection and their network goes down, causing a disconnect

For the last case, we may want to tweak the pingTimeout and pingInterval settings on the server. By default they are set to 60000ms and 25000ms respectively — which basically means it will take anywhere from 60–85s from a network connection failure until the user is notified of the failure. During this time the user may be firing off messages that will never reach the HE and won't show in their timeline, which is probably pretty frustrating.

To test

HC is down on connecting

Point Calypso at your local HC but don't bring the service up. Go to http://calypso.localhost:3000/me/chat. You should see a notice Connecting you with a Happiness Engineer… followed quickly by We're having trouble connecting to chat. Please bear with us while we try to reconnect…. During this whole time the chat message box should be disabled.

Starting the local HC service should make the notice go away and HC should behave as expected.

HC service goes down after connecting

Point Calypso at your local HC with the service up. Go to http://calypso.localhost:3000/me/chat. You should see a notice Connecting you with a Happiness Engineer… followed by the notice disappearing. Start chatting with an operator.

Kill the local HC service. In Calypso you should immediately see a notice We're having trouble connecting to chat. Please bear with us while we try to reconnect…. The chat message box should be disabled.

Starting the HC service again should bring the chat back to normal status.

User's internet connection goes away

Point Calypso to staging HC. Go to http://calypso.localhost:3000/me/chat. You should see a notice Connecting you with a Happiness Engineer… followed by the notice disappearing. Start chatting with an operator.

Turn off your wifi. Wait for up to 90s and you should see a notice We're having trouble connecting to chat. Please check your internet connection while we try to reconnect…. The chat message box should be disabled.

Turning wifi on again should bring the chat back to normal status.

@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] L Large sized issue label Apr 3, 2017
@mattwondra mattwondra force-pushed the update/happychat/refactor-helpers branch from e92a9cf to 3cca632 Compare April 3, 2017 21:00
@mattwondra mattwondra force-pushed the fix/happychat/better-connection-issue-messaging branch from 2be30fa to 275098d Compare April 3, 2017 21:13
@matticbot matticbot added [Size] M Medium sized issue and removed [Size] L Large sized issue labels Apr 3, 2017
@mattwondra mattwondra changed the title Add better messaging when Happychat has connectivity issues Happychat: Add better messaging when there are connectivity issues Apr 3, 2017
@mattwondra mattwondra requested review from jordwest and oandregal April 3, 2017 21:58
@mattwondra mattwondra added Happychat [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 3, 2017

if ( ! isServerReachable ) {
return translate( "We're having trouble connecting to chat. Please check your internet connection while we try to reconnect…" );
}

switch ( connectionStatus ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a shared constant to communicate connectionStatus values between this and the reducer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is on my to-do list before I move off the middleware cleanup tasks, but I didn't want to pollute existing PRs with that stuff.

export const HAPPYCHAT_CONNECTION_ERROR_PING_TIMEOUT = 'ping timeout';
export const HAPPYCHAT_CONNECTION_ERROR_TRANSPORT_CLOSE = 'transport close';
export const HAPPYCHAT_CONNECTION_ERROR_TRANSPORT_ERROR = 'transport error';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these constants for? Apart from HAPPYCHAT_CONNECTION_ERROR_PING_TIMEOUT I haven't seen them in use anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These come from the Socket.IO client library — search for calls to onClose( and the passed-in values are the possible reasons for the connection error.

I'll add a comment here to document this better.

Copy link
Contributor

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a much better experience!

The first two test cases worked as expected. The last one didn't work for me with a full local HC, but it did work when using staging HC.

I'm not familiar with the inner details of SocketIO, but my belief is that the reason why full local HC kept working is unrelated to this PR: SocketIO may be maintaining the connection between local calypso and local HC because both run under localhost or it is using a local protocol such as bonjour as a fallback mechanism.

@oandregal oandregal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 4, 2017
@mattwondra
Copy link
Member Author

SocketIO may be maintaining the connection between local calypso and local HC because both run under localhost

Yep, I believe this is the case. :)

@mattwondra mattwondra force-pushed the update/happychat/refactor-helpers branch from 3cca632 to 8de92f3 Compare April 5, 2017 15:54
@mattwondra mattwondra force-pushed the fix/happychat/better-connection-issue-messaging branch from ac568f1 to 38702c3 Compare April 5, 2017 16:00
@mattwondra mattwondra force-pushed the fix/happychat/better-connection-issue-messaging branch from 38702c3 to 739ccc8 Compare April 5, 2017 16:46
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] M Medium sized issue labels Apr 5, 2017
@mattwondra mattwondra changed the base branch from update/happychat/refactor-helpers to master April 5, 2017 16:46
@mattwondra mattwondra merged commit dc3670c into master Apr 5, 2017
@mattwondra mattwondra deleted the fix/happychat/better-connection-issue-messaging branch April 5, 2017 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Happychat [Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants