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

Jetpack: Handle disconnected site case #47121

Merged
merged 12 commits into from Nov 6, 2020

Conversation

rcanepa
Copy link
Contributor

@rcanepa rcanepa commented Nov 4, 2020

When either sites-rewind or sites-scan endpoint returns a response object with no_connected_jetpack code and 412 status, we know there is some issue with the Jetpack Connection. Currently, we are not handling this case, and we show users a blank screen that tells them nothing.

Changes proposed in this Pull Request

  • In both the Jetpack Backup and Jetpack Scan sections, show a special page for when the Jetpack Connection is not working.

Testing instructions

Prerequisites: Jetpack Site, and WPCOM sandbox.

  • Run this PR (both Calypsos).
  • Select a site with a paid plan (we want a site that has both Jetpack Backup and Jetpack Scan).
  • Visit the Jetpack > Backup section.
  • Verify that everything looks normal and you can see your site's latest backup.
  • Visit the Jetpack > Scan section.
  • Verify that everything looks and you can see the current state of Scan.
  • Modify /wp-content/rest-api-plugins/endpoints/sites-rewind.php and /wpcom-sandbox/wp-content/rest-api-plugins/endpoints/sites-scan.php from your sandbox to make these endpoints return a 412. In the first file, on line number 72, you will have to remove the ! from the if condition. In the other file, do the same but on line number 290. Please reach out to me if you need help with this.
  • Sandbox public-api.wordpress.com.
  • Visit the Jetpack > Backup section.
  • Verify that you see a new page that tells you there is a problem with the Jetpack Connection.
  • Verify that the first button takes you to a page within WordPress.com, while the second takes you to our Support pages.
  • Visit the Jetpack > Scan section.
  • Verify that you see a new page that tells you there is a problem with the Jetpack Connection.
  • Verify that the first button takes you to a page within WordPress.com, while the second takes you to our Support pages.
  • Repeat this process in Jetpack Cloud.

Fixes 1164141197617539-as-1198191229840349

Demo

WPCOM - Backup

image

WPCOM - Scan

image

Jetpack Cloud - Backup

image

Jetpack Cloud - Scan

image

@rcanepa rcanepa added Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Jetpack Cloud Anything related to the Jetpack Cloud (cloud.jetpack.com) labels Nov 4, 2020
@rcanepa rcanepa requested a review from a team November 4, 2020 21:45
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Nov 5, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~1946 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
scan          +5828 B  (+1.6%)    +1002 B  (+1.1%)
backup        +5805 B  (+1.2%)     +907 B  (+0.8%)
activity       +153 B  (+0.0%)      +37 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@monsieur-z monsieur-z left a comment

Choose a reason for hiding this comment

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

It tests well and code looks good. Nice job!

It takes a little while to see the connection error message. Maybe a further improvement could be to show the loading state.

There are some unit tests failing, but I'm not sure it's related to your PR. Maybe a rebase will fix it?

@rcanepa rcanepa force-pushed the fix/no-connected-jetpack-rewind-state branch from 8a10d2b to 665c353 Compare November 6, 2020 17:27
@rcanepa
Copy link
Contributor Author

rcanepa commented Nov 6, 2020

It tests well and code looks good. Nice job!

It takes a little while to see the connection error message. Maybe a further improvement could be to show the loading state.

You're right. That's definitely an improvement we should do in the future.

There are some unit tests failing, but I'm not sure it's related to your PR. Maybe a rebase will fix it?

I rebased the PR and those failing tests now are passing. Thanks!

@rcanepa rcanepa merged commit 054d9d1 into master Nov 6, 2020
@rcanepa rcanepa deleted the fix/no-connected-jetpack-rewind-state branch November 6, 2020 18:07
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 6, 2020
@a8ci18n
Copy link

a8ci18n commented Nov 6, 2020

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5127264

Thank you @rcanepa for including a screenshot in the description! This is really helpful for our translators.

@a8ci18n
Copy link

a8ci18n commented Nov 13, 2020

Translation for this Pull Request has now been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Jetpack Cloud Anything related to the Jetpack Cloud (cloud.jetpack.com) Jetpack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants