Debug Tests: Improve connection test, and allow overrides by roccotripaldi · Pull Request #14740 · Automattic/jetpack

roccotripaldi · 2020-02-19T21:43:39Z

Changes proposed in this Pull Request:

Allowed failing and passing tests to override the proposed defaults for the Site Health, while preserving the functionality of test messaging in the CLI and API test environments.
improved the messaging on the Connection test for the Site Health page.

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

This is step one in improving Jetpack's site health section
See: p6TEKc-3st-p2

Testing instructions:

Test that CLI tests still function
Test that all Jetpack Site Health tests still look okay ( Tools -> Site Health )
Take a look a the new Site Health card for a connected site:

Take a look at the new Site Health card for an unconnected site:

Proposed changelog entry for your changes:

n/a

- Allow successful tests to provide their own message and label for the Site Health page. - Adjust the message and label for the `test__check_if_connected` test

- Adds the ability for tests to supply an action label and override other labels. - Improves the failing state for `test__check_if_connected`

jetpackbot · 2020-02-19T21:49:12Z

	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 0f090d7

_inc/lib/debugger/class-jetpack-cxn-test-base.php

_inc/lib/debugger/class-jetpack-cxn-tests.php

Allow passing tests to override defaults with an HTML description.

- perserve plain text messages to use on API tests - allow tests to override plain text with HTML for the Site Health page.

roccotripaldi · 2020-02-20T20:34:29Z

Thanks for the review @kraftbj - i had no idea that these tests were also used in CLI. I've made changes to preserve CLI functionality while allowing overrides for HTML in Site Health

jeherve · 2020-02-21T09:37:39Z

_inc/lib/debugger/class-jetpack-cxn-tests.php

+				sprintf(
+					'<p>%s</p><p>%s</p>',
+					__( 'A healthy connection ensures Jetpack essential services are provided to your WordPress site, such as Stats and Site Security.', 'jetpack' ),
+					__( '✅ Your site is connected to Jetpack.', 'jetpack' )


Maybe we should pull the emoji out of the translation and use a placeholder instead? I am not sure translators / translator tools can all handle adding emoji.

Agreed — ideally this should reuse a Dashicon instead of being an emoji:

Green/Healthy/Good:

Passed

Read/Error/Bad:

Error

jeherve · 2020-02-21T09:38:42Z

_inc/lib/debugger/class-jetpack-cxn-tests.php

+				__( 'Your site is not connected to Jetpack', 'jetpack' ),
+				__( 'Learn more about this process', 'jetpack' ),
+				sprintf(
+					'<p>%s</p><p>%s<strong>%s</strong></p>',


Just a nit-pick, but I personally find it easier to read when we use numbered placeholders when we have more than a couple of them. Up to you!

jeherve · 2020-02-21T09:41:28Z

_inc/lib/debugger/class-jetpack-cxn-test-base.php

 	 * @return array Test results.
 	 */
-	public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical' ) {
+	public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = 'Resolve', $description = false ) {


Maybe we can pull Resolve out of there so it can still be translated?

Suggested change

public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = 'Resolve', $description = false ) {

public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = '', $description = false ) {

if ( empty( $action_label ) ) {

$action_label = __( 'Resolve', 'jetpack' );

}

keoshi

Looks good to me! I've changed the failed state a tiny bit in iteration no. 2 though, which will be posted shortly.

keoshi · 2020-02-21T11:48:11Z

Designs i2 posted p6TEKc-3t2-p2

- adds dashicons instead of emoji - improves translation logic

jeherve

It tests well for me. 👍

kraftbj · 2020-02-24T14:16:18Z

A couple of comments:

Seeing the function args grow and grow, it may make sense to migrate to passing an args array.
Breaking out the HTML description from the plain text one is giving me pause. To me, it feels like a case where we would possibly be duplicating a lot of text. Using wpautop, could we do this without an extra field? Links are the one thing I'm less sure about though we can make them clickable through the filter that does that in Core already.

Having multiple strings show up in different places could complicate seeking support or answers if it isn't clear that multiple messages are referring to the same thing.

…e-added as part of next sprint when multiple action links are supported

mdbitz · 2020-02-24T15:34:58Z

Per the i2 designs the primary action text should be "Reconnect your site now" for failed connections.

mdbitz · 2020-02-24T15:51:37Z

Note about changelog we could use something around the lines of:

Site Health Tests
Updated Connect test with detailed descriptions and actions to resolve failing tests.

* 8.3 release: changelog * Changelog: add #14516 * Changelog: add #14574 * Bring in changes from 8.2.1 and 8.2.2 * Update stable version * Bring in 8.2.3 changes * Changelog: add #14714 * Changelog: add #14639 * Changelog: add #14678 * Changelog: add #14673 * Changelog: add #14687 * Changelog: add #14704 * Changelog: add #14702 * Changelog: add #14541 * Changelog: add #14657 * Changelog: add #14622 * Changelog: add #14582 * Changelog: add #14638 * Changelog: add #14633 * Changelog: add #14571 * Changelog: add #14592 * Changelog: add #14539 * Changelog: add #14514 * Changelog: add #14643 * Changelog: add #14494 * Changelog: add #13739 * Changelog: add #14707 * Changelog: add #14736 * Changelog: add #14706 * Changelog: add #14730 * Changelog: add #14685 * Changelog: add #14727 * Changelog: add #14711 * Changelog: add #14742 * Changelog: add #14746 * Changelog: add #14725 * Changelog: add #13999 * Changelog: add #14740 * Changelog: add #14759 * Changelog: add #14703 * Changelog: add #14753 * Changelog: add #14754 * Changelog: add #14645 * Cahngelog: add #14599

roccotripaldi added 2 commits February 19, 2020 15:15

Jetpack CXN tests:

36fba01

- Allow successful tests to provide their own message and label for the Site Health page. - Adjust the message and label for the `test__check_if_connected` test

Debug tests

a2bbd34

- Adds the ability for tests to supply an action label and override other labels. - Improves the failing state for `test__check_if_connected`

roccotripaldi added the [Status] In Progress label Feb 19, 2020

roccotripaldi requested a review from a team February 19, 2020 21:43

kraftbj reviewed Feb 19, 2020

View reviewed changes

_inc/lib/debugger/class-jetpack-cxn-test-base.php Outdated Show resolved Hide resolved

kraftbj reviewed Feb 19, 2020

View reviewed changes

_inc/lib/debugger/class-jetpack-cxn-tests.php Outdated Show resolved Hide resolved

jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it Connect Flow Connection banners, buttons, ... labels Feb 20, 2020

roccotripaldi added 2 commits February 20, 2020 14:39

Debug tests

d73d5f6

Allow passing tests to override defaults with an HTML description.

Debug tests

63b0e73

- perserve plain text messages to use on API tests - allow tests to override plain text with HTML for the Site Health page.

roccotripaldi changed the title ~~In progress: update jetpack debug connection tests~~ Debug Tests: Improver connection test, and allow overrides Feb 20, 2020

roccotripaldi requested a review from keoshi February 20, 2020 20:23

roccotripaldi added [Status] Needs Review This PR is ready for review. [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Needs Design Review Design has been added. Needs a review! and removed [Status] In Progress labels Feb 20, 2020

roccotripaldi added this to the 8.3 milestone Feb 20, 2020

jeherve requested changes Feb 21, 2020

View reviewed changes

jeherve 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 Feb 21, 2020

keoshi reviewed Feb 21, 2020

View reviewed changes

keoshi added [Status] Design Review Complete and removed [Status] Needs Design Review Design has been added. Needs a review! labels Feb 21, 2020

Debug tests

1fddafa

- adds dashicons instead of emoji - improves translation logic

roccotripaldi 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 Feb 21, 2020

jeherve previously approved these changes Feb 21, 2020

View reviewed changes

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 Feb 21, 2020

jeherve assigned roccotripaldi Feb 21, 2020

roccotripaldi changed the title ~~Debug Tests: Improver connection test, and allow overrides~~ Debug Tests: Improve connection test, and allow overrides Feb 22, 2020

[not verified] Updated Action to per i2 designs, learn more will be r…

0f090d7

…e-added as part of next sprint when multiple action links are supported

mdbitz dismissed jeherve’s stale review via 0f090d7 February 24, 2020 15:33

mdbitz added [Status] Needs Review This PR is ready for review. and removed [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Testing We need to add this change to the testing call for this month's release labels Feb 24, 2020

jeherve approved these changes Feb 24, 2020

View reviewed changes

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 Feb 24, 2020

mdbitz merged commit 0eb0505 into master Feb 24, 2020

mdbitz deleted the update/site-health-connection branch February 24, 2020 15:43

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

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

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

Changelog: add #14740

feb2d5b

-	public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = 'Resolve', $description = false ) {
+	public static function failing_test( $name, $message, $resolution = false, $action = false, $severity = 'critical', $label = false, $action_label = '', $description = false ) {
+		if ( empty( $action_label ) ) {
+			$action_label = __( 'Resolve', 'jetpack' );
+		}

Conversation

roccotripaldi commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

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

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roccotripaldi commented Feb 20, 2020

Uh oh!

jeherve Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

keoshi Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

jeherve Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

jeherve Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

keoshi left a comment

Choose a reason for hiding this comment

Uh oh!

keoshi commented Feb 21, 2020

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

kraftbj commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdbitz commented Feb 24, 2020

Uh oh!

mdbitz commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

roccotripaldi commented Feb 19, 2020 •

edited

Loading

jetpackbot commented Feb 19, 2020 •

edited

Loading

kraftbj commented Feb 24, 2020 •

edited

Loading