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

Display an admin notice if neither Jetpack transport is available for WCPay #394

Merged
merged 4 commits into from
Feb 4, 2020

Conversation

allendav
Copy link
Contributor

@allendav allendav commented Feb 3, 2020

Fixes #332
Related to #339 cc @vbelolapotkov

Changes proposed in this Pull Request

  • Test for either Jetpack transport before allowing the plugin to activate (and fatal)

Testing instructions

  • Deactivate Jetpack while WCPay is active.
  • Ensure no Fatal Error results.
  • Ensure you are prompted to install, activate and connect Jetpack "at this time."


$message = sprintf(
/* translators: %1: WooCommerce Payments version */
__( 'WooCommerce Payments %1$s requires Jetpack at this time. Please install, activate and connect Jetpack.', 'woocommerce-payments' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think "at this time" is necessary, since it's referring to a specific version. (Though if we want to avoid implying that this is a permanent dependency, we could clarify that in a separate clause.)

Also, "some people" might be thrown off by the lack of comma after "activate" 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we did oxford commas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved language in f6c5255 but didn't add a comma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so TIL Oxford Commas means everyone gets a comma, and that it is a company style standard, so I relented and added a comma in a86c100

includes/class-wc-payments.php Outdated Show resolved Hide resolved
Copy link
Contributor

@dechov dechov left a comment

Choose a reason for hiding this comment

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

Looks good! Left one more comment but its not a blocker. (And neither is the comma…)

includes/class-wc-payments.php Show resolved Hide resolved
@allendav allendav merged commit 87742a3 into master Feb 4, 2020
@allendav allendav deleted the fix/332-jetpack-fatal branch February 4, 2020 17:02
vbelolapotkov added a commit that referenced this pull request Feb 5, 2020
Replaced check_for_jetpack_layer introduced in #394.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal Error when Jetpack is disabled
2 participants