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.

Sign up for GitHub

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

Jump to bottom

Debug tests: adds sync health card, and allows test to be hidden on site health page #14897

Merged
merged 2 commits into from
Mar 17, 2020
Merged

Debug tests: adds sync health card, and allows test to be hidden on site health page #14897

merged 2 commits into from
Mar 17, 2020

Conversation

roccotripaldi
Copy link
Member

@roccotripaldi roccotripaldi commented Mar 5, 2020 •
edited
Loading

Fixes n/a

Changes proposed in this Pull Request:

  • On the site health page, if sync is disabled, let's show a recommended improvement:
    Screen Shot 2020-03-06 at 2 54 08 PM

  • This PR also refactors debug tests to allow a test to be hidden on the site health page. For example, current a skipped test will still appear on the Site Health page as being successfully passed. For sync health, there are some cases where we don't want to show it at all.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Adds a new debug test

Testing instructions:

  • Apply this PR to a testing site.
  • Ensure the site is connected.
  • On the debug page for your site (https://jetpack.com/support/debug/?url=$url) disable sync
  • Visit the site health page in wp-admin ( tools->site health )
  • Ensure you see the card for Jetpack sync
  • There should be no sync card when Jetpack is disconnected, nor when Jetpack is connected and sync is enabled.

Proposed changelog entry for your changes:

  • None

@roccotripaldi roccotripaldi added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Design Review Design has been added. Needs a review! [Package] Sync labels Mar 5, 2020
@roccotripaldi roccotripaldi added this to the 8.4 milestone Mar 5, 2020
@roccotripaldi roccotripaldi requested review from kraftbj and a team March 5, 2020 15:03
@jetpackbot
Copy link

jetpackbot commented Mar 5, 2020 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against fff50c6

keoshi
keoshi previously approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

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

Looks great to me, Rocco! 🚀

mdbitz
mdbitz requested changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@mdbitz mdbitz left a comment

Choose a reason for hiding this comment

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

Pending removal of the local debug statement. and confirmation of link endpoint.

_inc/lib/debugger/class-jetpack-cxn-test-base.php Outdated Show resolved Hide resolved
_inc/lib/debugger/class-jetpack-cxn-tests.php Outdated
$name,
__( 'Jetpack Sync has been disabled on your site.', 'jetpack' ),
'enable_sync',
admin_url( '#' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want to land on the Admin page in this case. It might be worth checking if we have a end user visible guide on Sync, how to enable, or etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's an outstanding question. i have no idea where to send people for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@roccotripaldi You mentioned there being a filter to turn off sync. Is this documented anywhere in our support pages? If so, we should link it there. If not, maybe JPOP Happy can create a document for it?

@keoshi keoshi added [Status] Design Review Complete and removed [Status] Needs Design Review Design has been added. Needs a review! labels Mar 5, 2020
@roccotripaldi roccotripaldi force-pushed the add/sync-health-card branch from dd2f1f6 to 9ada076 Compare March 5, 2020 16:02
@roccotripaldi
Copy link
Member Author

Since there is currently no public facing place that talks about Sync settings, i've updated the card to read:

Screen Shot 2020-03-06 at 2 54 08 PM

The action links to our github repo:
https://github.com/Automattic/jetpack/blob/master/packages/sync/src/class-settings.php

I think we should merge it! There's plenty of time this month before release if we can get up a public facing sync page.

@roccotripaldi
[not verified] # This is a combination of 4 commits.
8a09d2d 
Debug tests

- allow skipped tests to be ommited from the Site Health page
- add a test for Sync health

Debug tests: adding a short description for enabling sync

Debug: add a short description for the connection failure test

Debug: sync health test

Since there's not currently any public facing docs on Sync settings
let's link the action to Github.
@roccotripaldi
Copy link
Member Author

This is rebased, and ready to go in my opion.

mdbitz
mdbitz approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@mdbitz mdbitz left a comment

Choose a reason for hiding this comment

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

Approved, Verified shows message when sync is disabled.

@mdbitz mdbitz added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 16, 2020
@roccotripaldi roccotripaldi merged commit 91acb92 into master Mar 17, 2020
@roccotripaldi roccotripaldi deleted the add/sync-health-card branch March 17, 2020 02:39
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 17, 2020
jeherve added a commit that referenced this pull request Mar 23, 2020
@jeherve
Changelog: add #14897
1bc0209
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keoshi keoshi keoshi left review comments

@mdbitz mdbitz mdbitz approved these changes

@kraftbj kraftbj Awaiting requested review from kraftbj

Assignees
No one assigned
Labels
[Package] Sync [Status] Design Review Complete
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.
6 participants
@roccotripaldi @jetpackbot @keoshi @mdbitz @jeherve @matticbot