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 alert on WP Admin if Jetpack is not connected #391

Merged
merged 8 commits into from
Feb 18, 2020

Conversation

vbelolapotkov
Copy link
Collaborator

@vbelolapotkov vbelolapotkov commented Feb 3, 2020

Fixes #339 #249 #195
Relates to #196
Resolved conflicts with #394 should not affect #332 🙂

Changes proposed in this Pull Request

  • Display alert on WP Admin if Jetpack is not installed or not activated.
  • Display alert on WP Admin if Jetpack is not connected.
  • Prevent WCPayments plugin initialization when Jetpack is either not activated or not connected.

Testing instructions

For fresh WooCommerce site without WCPayments configured start at step 3.

  1. Go to Admin > Plugins > Installed plugins and deactivate Jetpack.

    • Alert should appear at the top:

Screenshot 2020-02-04 at 13 27 12

- No new fatal error message should appear in the logs.
  1. (Optionally)Admin > Plugins > Installed plugins and delete Jetpack from the site.
    Alert should appear at the top.

Screenshot 2020-02-04 at 13 21 27

  1. Install Jetpack if removed at step 2 or it does not exist after step 2.

    • Alert should appear at the top:

Screenshot 2020-02-04 at 13 27 12

  1. Activate Jetpack but do not set up at the moment. If Jetpack setup page is displayed, go to Dashboard.

    • Another alert should appear at the top

Screenshot 2020-02-04 at 13 29 48

  1. Go to Jetpack setup page, connect it and get back to the admin dashboard.
    At this step tunnelled Http connection is required (details are here in section 2.3.1.1.).

  2. Go to Payments > Settings:

    • All settings should be in place for an existing site.
    • The setup page should be displayed for a fresh site.

  • Tested on mobile (or does not apply)

Stop WCPayments plugin initialization when Jetpack is not connected.

When new instance of WC_Payments_Account class is created as part of
the WCPayments plugin initialization, an HTTP request is sent with Jetpack Client
which leads to the fatal error when Jetpack is not connected.
This is why we have to check if Jetpack is connected before creating an instance of WC_Payments_Account class.
@vbelolapotkov
Copy link
Collaborator Author

This is raw and early implementation. See TODOs in changes for more details.

@allendav
Copy link
Contributor

allendav commented Feb 3, 2020

Will this PR (eventually) also fix #332 ?

@vbelolapotkov
Copy link
Collaborator Author

Yes, It should IMO.

Add Jetpack to plugin dependencies to stop WCPayments plugin
initialization when Jetpack is not installed or deactivated.

Allert message is displayed in wp-admin when Jetpack is not activated.
Move all Jetpack Client related code to the existing class WC_Payments_Http
to keep it in the same place.

Move all Jetpack Client related code to the existing class WC_Payments_Http
Replaced check_for_jetpack_layer introduced in #394.
@LevinMedia
Copy link
Contributor

@vbelolapotkov looking good! Thanks! I noticed when installing jetpack, the notification is still present, I don't think it's necessary to show it at this point

Screen Shot 2020-02-05 at 12 12 24 PM

Lastly, When jetpack is simply not set up, please just remove the "Don't worry, this is a temporary thing." Sentence.

@vbelolapotkov
Copy link
Collaborator Author

vbelolapotkov commented Feb 7, 2020

@LevinMedia thanks for the feedback!

I noticed when installing jetpack, the notification is still present.

Removed all alerts when pluign installation is in progres. 4a4bd98.

Lastly, When jetpack is simply not set up, please just remove the "Don't worry, this is a temporary thing." Sentence.

Fixed with 02c12c8.

@allendav
Copy link
Contributor

I was a little surprised that, with WooCommerce Payments installed and activated, if I then disconnected my Jetpack, the only place I see that WooCommerce Payments is effectively disabled is on the wp-admin Plugins page.

I expected to see this notice basically throughout wp-admin - not just on the Plugins page

Screen Shot 2020-02-11 at 2 17 45 PM

What do you think @LevinMedia ?

@LevinMedia
Copy link
Contributor

@allendav I think it's okay just there for now.

How about we look at adding it elsewhere If we get an influx of people de-activating jetpack and are confused as to why their payments aren't working?

@vbelolapotkov
Copy link
Collaborator Author

vbelolapotkov commented Feb 12, 2020

I was a little surprised that, with WooCommerce Payments installed and activated, if I then disconnected my Jetpack, the only place I see that WooCommerce Payments is effectively disabled is on the wp-admin Plugins page.

@allendav can you reproduce it? @LevinMedia are you experiencing the same? Asking because it works fine on my end. The only pages where it's hidden are Jetpack setup page and plugin installation in progress. Here are the screenshots from wp-admin dashboard and posts. I don't see it on the WooCommerce Admin pages though, so it looks like WooCommerce Admin ignores general wp-admin notices.

Screenshot 2020-02-12 at 15 46 22

Screenshot 2020-02-12 at 15 46 36

@vbelolapotkov vbelolapotkov requested a review from a team February 12, 2020 12:51
Copy link
Contributor

@dwainm dwainm left a comment

Choose a reason for hiding this comment

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

This works well for me. Left one comment.

@@ -139,23 +139,13 @@ public static function check_plugin_dependencies( $silent ) {
return true;
}

// TODO - Remove/update when Jetpack Connection package is all we need.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we moved this todo to an issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't found any specific issue on that topic but I guess it should be part of #111 epic. Would you prefer creating a particular issue for this and adding TODO comment to the wc-payments-http class?

Copy link
Contributor

Choose a reason for hiding this comment

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

That will work well, as long as we don't forget about this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added TODO with f4639fa

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.

Display a WP Admin Notice if WooCommerce Payments is Enabled But Jetpack is Not Connected
4 participants