Simplify error notices for broken connections #16655

leogermani · 2020-07-30T19:53:53Z

#16194 introduced the ability for Jetpack to identify, confirm and handle XML-RPC errors. It also introduced admin notices to display such errors to users.

Up until now we were only displaying errors for Blog token issues. This PR adds error messages for user tokens errors too.

Refactor

This PR also simplifies the code that was displaying the errors. At first, there was this idea of having an abstraction that would allow us to treat each error code differently, having a class to handle each one of them.

However, this abstraction was in place to handle theoretical situations while, in practice, we were simply displaying the same generic error message in all case. Therefore I decided to get rid of the abstraction for now and keep it simple. When needed, it's easy to add a per error code handling there.

Note: I think it's completely safe to just remove the "blog token error handling" class and all the hooks introduced in 8.7 because they have not been used anywhere or by any other plugin using the connection package.

Changes proposed in this Pull Request:

Adds user feedback for XML-RPC user token related errors

Jetpack product discussion

p1HpG7-9q9-p2

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

No

Testing instructions:

Start with a connected Jetpack site and the Jetpack Debug helper plugin activated
Go to Broken Token and click "Set invalid user token (current user)"
Go to XML-RPC Errors.
Click the link to visit the Jetpack debugger
Back to XML-RPC error, confirm you have an error under "Verified error"
Visit any Dashboard page and confirm you see the error in admin notices
Visit the Jetpack Dashboard and confirm you see the error at the top of the page along with a CTA to restore the connection

Proposed changelog entry for your changes:

Adds user feedback for XML-RPC user token related errors

jetpackbot · 2020-07-30T20:01:28Z

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-16655

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

Generated by 🚫 dangerJS against 64acb5b

fgiannar

Hey @leogermani ,

Nice work!
Tested this and can verify that I am able to see a no_valid_token error in XML-RPC errors, but it appears under Current Unverified Errors section instead.
The error notice and corresponding CTA appeared as expected.

Finally, a small code related suggestion: Do you believe it would make sense to add an error-handling interface, so that all error handler classes implement a contract for the admin_notice method and possibly a message property related getter/setter?

leogermani · 2020-08-03T22:46:33Z

Thanks @fgiannar

Tested this and can verify that I am able to see a no_valid_token error in XML-RPC errors, but it appears under Current Unverified Errors section instead.

Actually it appears on both Unverified and Verified sections. If you saw the error notices, I trust it was there.

The error notice and corresponding CTA appeared as expected.

Good

Finally, a small code related suggestion: Do you believe it would make sense to add an error-handling interface, so that all error handler classes implement a contract for the admin_notice method and possibly a message property related getter/setter?

Yes, it definitely makes sense to have an interface. It would be a very generic interface though, because, in theory, each error handler can perform very different actions. Not all of them would necessarily display error on admin notice.

I didn't create it yet because I'm avoiding premature abstraction... While in theory we can have many different types of error handlers, in practice, right now, we only have 2 that are actually identical... they could be the same. They only trigger different actions and filters to allow third party users (also theoretical) to act on these errors. So I'm actually tending to ask if any abstraction there is worth at this point, and if we should just have one simple method or class that adds the error to the admin notice hook. What do you think?

(I'm actually waiting the partial reconnect PR to be merge to make Jetpack plugin use these hooks and customize the error messages, with a button to take the user straight to the reconnect flow.)

fgiannar · 2020-08-04T13:09:31Z

Actually it appears on both Unverified and Verified sections. If you saw the error notices, I trust it was there.

The only thing that troubles me here is that while testing, I only managed to see it under the Unverified section, which could be inconsistent with the fact that it is displayed as an error to the users (I was under the impression that we only show Verified)

So I'm actually tending to ask if any abstraction there is worth at this point, and if we should just have one simple method or class that adds the error to the admin notice hook. What do you think?

Having a simple method or class that adds the error to the admin notice hook sounds reasonable.

leogermani · 2020-08-07T14:39:52Z

packages/connection/src/class-error-handler.php

+
+		$errors[] = array(
+			'code'    => 'invalid_blog_token',
+			'message' => __( 'Your connection with WordPress.com seems to be broken. If you\'re experiencing issues, please try reconnecting.', 'jetpack' ),


The duplicated error message here is intentional.

Leaving it separated from the filter in the method above allows Jetpack plugin to have full control over it's Dashboard. If a plugin filters the message and modifies or disables it, it won't affect the Jetpack plugin.

leogermani · 2020-08-07T14:40:57Z

@fgiannar I've refactored the code and kept it simpler. We don't need all that abstraction right now. I updated the PR description with some additional info.

I also added a page in the documentation explaining how to customize or disable the error messages :)

fgiannar

@leogermani
Tested this using the instructions and noticed the following weird behaviour:
The first time I invalidate the user token, I can see the XML-RPC error and also the error notice in the Dashboard.
After restoring my connection, if I try to repeat the above process, I don't see the error at all.
The same happens when I remove all user related tokens and set up the connection from the admin UI.
I reproduced this both in my local env and a JN site.
Could you check and let me know if you can reproduce this as well?

fgiannar · 2020-08-10T10:59:25Z

packages/connection/src/class-error-handler.php

+	public function jetpack_react_dashboard_error( $errors ) {
+
+		$errors[] = array(
+			'code'    => 'invalid_blog_token',


Hi @leogermani ,

Returning a blog token specific error code (invalid_blog_token) in the (now) generic error handler confuses me a bit. Could you please elaborate on this?

Yeah, this is wrong! Nice catch

leogermani · 2020-08-10T12:54:33Z

@leogermani
Tested this using the instructions and noticed the following weird behaviour:
The first time I invalidate the user token, I can see the XML-RPC error and also the error notice in the Dashboard.
After restoring my connection, if I try to repeat the above process, I don't see the error at all.
The same happens when I remove all user related tokens and set up the connection from the admin UI.
I reproduced this both in my local env and a JN site.
Could you check and let me know if you can reproduce this as well?

This is expected because we have the gates in place making sure we just run all the verification process once in a while to avoid overflows.

On the plugin side you can easily disable this by setting define( 'JETPACK_DEV_DEBUG', true );.

On wpcom side it's more tricky, but here I explain how to do it: D47732-code (you'd have to sandbox public-api.wordpress.com.

You can also only disable the gate on the plugin. The error will appear as Unverified. You can then click the "Verify error (via api)" button and manually verify it.

fgiannar

Awesome, thanks for adding a generic error code and pointing out to me how to disable the gate!
LGTM

leogermani · 2020-08-10T14:00:20Z

@kraftbj @jeherve This PR is ready to merge but wanted to get your ok on something.

On this PR I'm removing some classes and filters added in 8.7 that I'm sure no one is using (considering woocommerce payments as our only client right now).

Since we had backward compatibility issues before, and this should most likely trigger our checks for removing methods and filters, just wanted to give you a heads up that this should look clean and safe.

leogermani · 2020-08-18T17:45:14Z

@kraftbj @jeherve new ping. I think this might have gone out of the radar

jeherve · 2020-08-19T16:30:38Z

I'm removing some classes and filters added in 8.7 that I'm sure no one is using (considering woocommerce payments as our only client right now).

Yeah, this should be fine. 👍

* 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) ...

* add handler class for user tokens errors * refactor and simplify xmlrpc error notices * add docs to error messages * change error code to a generic code

leogermani added the [Status] Needs Review This PR is ready for review. label Jul 30, 2020

leogermani added this to the 8.9 milestone Jul 30, 2020

leogermani self-assigned this Jul 30, 2020

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Jul 30, 2020

fgiannar reviewed Aug 3, 2020

View reviewed changes

fgiannar added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Aug 3, 2020

leogermani added 2 commits August 6, 2020 19:06

add handler class for user tokens errors

13d7c69

refactor and simplify xmlrpc error notices

6d1f30d

leogermani force-pushed the add/xmlrpc-error-messages-for-user-tokens branch from 9655dba to 6d1f30d Compare August 6, 2020 22:07

add docs to error messages

b6b0f7b

leogermani changed the title ~~add handler class for user tokens errors~~ Simplify error notices for broken connections Aug 7, 2020

leogermani commented Aug 7, 2020

View reviewed changes

leogermani added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Aug 7, 2020

fgiannar reviewed Aug 10, 2020

View reviewed changes

fgiannar added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Aug 10, 2020

change error code to a generic code

64acb5b

leogermani added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Aug 10, 2020

fgiannar approved these changes Aug 10, 2020

View reviewed changes

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 10, 2020

leogermani added [Status] Needs Review This PR is ready for review. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 10, 2020

jeherve added [Package] Connection [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Aug 19, 2020

jeherve 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

jeherve approved these changes Aug 19, 2020

View reviewed changes

leogermani merged commit de97747 into master Aug 19, 2020

leogermani deleted the add/xmlrpc-error-messages-for-user-tokens branch August 19, 2020 17:00

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 19, 2020

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Aug 25, 2020

jeherve added a commit that referenced this pull request Aug 25, 2020

Changelog: add #16655

4ec6b56

Simplify error notices for broken connections #16655

Simplify error notices for broken connections #16655

Uh oh!

Conversation

leogermani commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactor

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

jetpackbot commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgiannar left a comment

Choose a reason for hiding this comment

Uh oh!

leogermani commented Aug 3, 2020

Uh oh!

fgiannar commented Aug 4, 2020

Uh oh!

leogermani Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani commented Aug 7, 2020

Uh oh!

fgiannar left a comment

Choose a reason for hiding this comment

Uh oh!

fgiannar Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani commented Aug 10, 2020

Uh oh!

fgiannar left a comment

Choose a reason for hiding this comment

Uh oh!

leogermani commented Aug 10, 2020

Uh oh!

leogermani commented Aug 18, 2020

Uh oh!

jeherve commented Aug 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

leogermani commented Jul 30, 2020 •

edited

Loading

jetpackbot commented Jul 30, 2020 •

edited

Loading