Skip to content

Conversation

@sergeymitr
Copy link
Contributor

@sergeymitr sergeymitr commented Aug 18, 2020

If Jetpack encounters a communication error, it suggest reconnecting.
That doesn't always help. In fact, there is a myriad of other reasons why the error might be thrown, and blindly suggesting to restore connection might only mislead users.

Changes proposed in this Pull Request:

This PR introduces different types of error:

  • ApiError is clearly a connection error, and might be solved by reconnecting.

Screen Shot 2020-08-19 at 9 53 47 AM

  • JsonParseError is a JSON parsing issue. This usually means there's an error output included into the JSON response, so we suggest the user to check the error log, and don't show the "Restore Connection" button.

Screen Shot 2020-08-18 at 4 14 38 PM

  • Anything else: we do nothing, as we have no idea what's going on. Most likely it's a network issue, and we don't have an easy solution for that.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

  1. If you don't run yarn watch, please rebuild the JS after checking out the branch.
  2. Go to the Jetpack Dashboard, make sure no error notices appear.
Connection Error
  1. Break your blog_token, refresh the dashboard and wait until it loads.
  2. Confirm that you see the error notice (see example above), and the "Restore Connection" button.
  3. Click on the "Restore Connection", the Dashboard should reload
  4. The error notice should not appear, the blog_token should get restored.
JSON parsing error
  1. Find the Jetpack_Core_Json_Api_Endpoints::get_site_data() API endpoint method:
    public static function get_site_data() {
  2. Add some output into the first line of the method (e.g. echo 'something';) to break the output JSON.
  3. Confirm that you see the error notice (see example above). The "Restore Connection" button must not appear.
Why not both?

This is a special use case that needed to be fixed separately, so we need to test it too.
10. Keep your echo 'something'; in place, but in addition to that break the blog_token. So the connection will be broken, but the error response will also contain invalid JSON.
11. Reload the Dashboard, and confirm that you see the "update to communicate" error notice (see example above). The "Restore Connection" button must not appear, as it's pointless trying to restore the connection as we won't be able to parse the response anyway.
12. Fix the JSON by removing your echo 'something'; line from the Jetpack_Core_Json_Api_Endpoints::get_site_data() method. Keep the token broken for now.
13. Reload the dashboard. You should see the "Restore Connection" error notice.
14. Click on the "Restore Connection", the page should reload, the error notice should no longer appear.

Thanks for testing! 🙂

Proposed changelog entry for your changes:

  • Improved error notices in the Jetpack Dashboard.

@sergeymitr sergeymitr added [Status] Needs Review This PR is ready for review. Admin Page React-powered dashboard under the Jetpack menu [Pri] Normal Connect Flow Connection banners, buttons, ... labels Aug 18, 2020
@sergeymitr sergeymitr added this to the 8.9 milestone Aug 18, 2020
@sergeymitr sergeymitr requested a review from a team August 18, 2020 21:23
@sergeymitr sergeymitr self-assigned this Aug 18, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented Aug 18, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16883

Scheduled Jetpack release: September 1, 2020.
Scheduled code freeze: August 25, 2020

Generated by 🚫 dangerJS against e5e08bc

If Jetpack encounters a communication error, it suggest reconnecting.
That doesn't always help. In fact, there is a myriad of other reasons why the error might be thrown, and blindly suggesting to restore connection might only mislead users.

This commit introduces different types of error:
- `ApiError` is clearly a connection error, and might be solved by disconnecting.
- `JsonParseError` is a JSON parsing issue. This usually means there's an error output included into the JSON response, so we suggest the user to check the error log, and don't show the "Restore Connection" button.
- Anything else: we do nothing, as we have no idea what's going on. Most likely it's a network issue, and we don't have an easy solution for that.
@sergeymitr sergeymitr force-pushed the update/meaningful-error-notices branch from c8d178b to 56b3fd2 Compare August 18, 2020 22:31
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

That's going to be a nice improvement 🤩

I wonder if we should consider adding whatever raw message we may be getting in some cases from the API response to the end of the error messages. In some cases, it may give more information as to what's happening?

@sergeymitr sergeymitr added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Aug 19, 2020
The error notices are improved even further:
- The `error_code` is being displayed in the notice, which will make debugging much simpler if the user reaches out to tech support.
- The "Missing Site ID" error is also displayed in the notice, and clicking "Restore Connection" actually solves the problem.
@sergeymitr
Copy link
Contributor Author

@jeherve that's a great idea, I added that 👍
The new commit introduces two things:

  • error_code in the notice
  • error notice displayed for the site id missing error, and suggests to reconnect to restore it

Please re-review the PR.

@sergeymitr sergeymitr added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Aug 19, 2020
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

This is great @sergeymitr, thanks for that!

I also tested forcing some error to the site request by adding this to my .htaccess

RewriteRule ^wp-json\/jetpack\/v4\/site$  - [R=503,L]

And it worked as expected!

@leogermani leogermani added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Aug 19, 2020
@sergeymitr sergeymitr merged commit 7f4b040 into master Aug 19, 2020
@sergeymitr sergeymitr deleted the update/meaningful-error-notices branch August 19, 2020 19:20
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 19, 2020
davidlonjon added a commit that referenced this pull request Aug 20, 2020
* master: (23 commits)
  Premium Blocks: set blocks availability (#16898)
  Compat Package: Fix method declaration compatibility (#16900)
  Jetpack Dashboard: More meaningful error notices. (#16883)
  Connection REST API: Unit test for the `remote_authorize` request. (#16879)
  use blog token to request jetpack.updateBlog (#16698)
  Improve Story block media loading (#16663)
  Simplify error notices for broken connections (#16655)
  Use new heartbeat package (#16285)
  wrap-paid-block: remove component. deprecated. (#16895)
  Social Previews: improve preview description handling (#16889)
  Stats module use blog token (#16727)
  Form Block: add a new Consent Field, a new Newsletter setting, and a new newsletter variation (#16808)
  AAG: Backup card, fall back to VP content in case of /rewind API error. (#16867)
  Donations: Fix dependencies (#16892)
  Creative Mail: update option to lowercase (#16861)
  Premium Blocks: Implement the new design (#16611)
  Requests to Stats CSV use the blog token (#16716)
  Update spacing around sharing buttons to avoid no bottom margin below the customize link. (#16811)
  Jetpack SSO: Cleaning up the `requestNonce` API request. (#16830)
  Donations: Update plans when currency changes (#16844)
  ...
pereirinha pushed a commit that referenced this pull request Sep 10, 2020
)

If Jetpack encounters a communication error, it suggest reconnecting.
That doesn't always help. In fact, there is a myriad of other reasons why the error might be thrown, and blindly suggesting to restore connection might only mislead users.

This commit introduces different types of error:
- `ApiError` is clearly a connection error, and might be solved by disconnecting.
- `JsonParseError` is a JSON parsing issue. This usually means there's an error output included into the JSON response, so we suggest the user to check the error log, and don't show the "Restore Connection" button.
- Anything else: we do nothing, as we have no idea what's going on. Most likely it's a network issue, and we don't have an easy solution for that.

Other improvements:
- The `error_code` is being displayed in the notice, which will make debugging much simpler if the user reaches out to tech support.
- The "Missing Site ID" error is also displayed in the notice, and clicking "Restore Connection" actually solves the problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Admin Page React-powered dashboard under the Jetpack menu Connect Flow Connection banners, buttons, ... [Pri] Normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants