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

Add verify business details notification #212

Conversation

luizreis
Copy link
Contributor

@luizreis luizreis commented Sep 27, 2019

Fixes #203

Depends on #211 and woocommerce-payments-server #62

Changes proposed in this Pull Request

  • Add new account API endpoint to API client
  • Add a wp-admin notice indicating that the merchant has to connect to a Stripe account, if no account or key is present. Include the OAuth connect link for the merchant to connect
  • Add a wp-admin notice indicating that Stripe has pending requirements for the connected account. Include the Express Dashboard link so the merchant can verify the business details

Testing instructions

  • Navigate to some admin page
    • Before connecting to Stripe, check the notifications - you should see a notice indicating that no account is connected. (If you already have an account connected, disable Test Mode under the plugin settings)
    • If your account has any pending requirement, you should see a notification indicating that you have to verify them. (Trickier to test manually, you could run the server in a WP.com sandbox and overwrite the account endpoint response or go through the OAuth flow to connect an account with a invalid address)

  • Tested on mobile (or does not apply)

Use the account endpoint in Automattic/transact-platform-server#62
to fetch data regarding the Stripe account.
After the Stripe account has already been connected, sometimes Stripe might fail to verify the already provided KYC data or require more data.
In such cases, the account object returned by Stripe will contain a requirements field.
This method uses those fields and the account endpoint to check if there are pending requirements for the connected account.
Display a wp-admin notice for when the store does not have a Stripe account connected, this notice contains the OAuth connect link so that the user can connect to a new/existing account.
Display a wp-admin notice for when Stripe has pending requirements for the connected account and provide a link to the Express Dashboard so the user can go ahead and address them.
@luizreis
Copy link
Contributor Author

@LevinMedia - any suggestions on the notice text for merchants? Currently, I've used (links are to Stripe connect flow and Stripe Dashboard, respectively):

  • "WooCommerce Payments is not connected to Stripe. Please click here to connect."
  • "Your Stripe account has pending requirements. Please go to the Stripe dashboard to address them."

@LevinMedia
Copy link
Contributor

Thanks @luizreis!

RE: messaging:

If for some reason the account_id and key are not set up, display:

Use a green alert:

WooCommerce Payments
Accept credit cards online. Simply verify your business details to activate WooCommerce Payments. [get started]

If the account_id and key are set, but the Stripe account endpoint returns some requirement, we display

Use a red alert:

WooCommerce Payments
We require additional details about your business. Please provide the required information by 4pm Oct 16 to avoid an interruption in your scheduled payouts [Update now]

@luizreis luizreis force-pushed the add/verify-business-details-notification branch 2 times, most recently from 6576125 to 8ba226a Compare September 27, 2019 19:33
@luizreis luizreis force-pushed the update/functional-prototype-settings branch from 5b0ba6e to fdb2601 Compare September 27, 2019 19:40
@luizreis luizreis force-pushed the add/verify-business-details-notification branch from 8ba226a to a1bdf82 Compare September 27, 2019 19:47
Create an admin notice specifying the class. Changed print_admin_error to use this.
The account object contains data regarding the requirements, such as current_deadline, which needs to be used outside of the account_has_pending_requirements method.

To avoid calling the account API multiple times, account became a parameter of that method and the caller is responsible for actually fetching the account data.
Display the default WCPay gateway messages as wp-admin notices when checking the Stripe account connection.
@luizreis luizreis changed the title Add verify business details notification Upon successful activation, display wordpress notification indicating the merchant must verify business details to begin using WooPayments Oct 1, 2019
@luizreis luizreis changed the title Upon successful activation, display wordpress notification indicating the merchant must verify business details to begin using WooPayments Add verify business details notification Oct 1, 2019
@allendav
Copy link
Contributor

allendav commented Oct 2, 2019

Depends on #211 and woocommerce-payments-server #62

Woot woot. https://github.com/Automattic/woocommerce-payments-server/pull/62 merged

Copy link
Contributor

@reykjalin reykjalin left a comment

Choose a reason for hiding this comment

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

All looks good AFAICT! Just address comments and we should be good.

I don't know if this is an actual issue, but when I tried to go to the WooCommerce Payments settings screen before updating the payments server I got this error:

[04-Oct-2019 08:13:12 UTC] PHP Fatal error:  Uncaught Error: Cannot use object of type WP_Error as array in /app/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payment-gateway-wcpay.php:390
Stack trace:
#0 /app/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payments.php(227): WC_Payment_Gateway_WCPay->account_has_pending_requirements(Object(WP_Error))
#1 /app/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payments.php(68): WC_Payments::check_stripe_account_status()
#2 /app/public/wp-content/plugins/woocommerce-payments/woocommerce-payments.php(31): WC_Payments::init()
#3 /app/public/wp-includes/class-wp-hook.php(286): wcpay_init('')
#4 /app/public/wp-includes/class-wp-hook.php(310): WP_Hook->apply_filters(NULL, Array)
#5 /app/public/wp-includes/plugin.php(465): WP_Hook->do_action(Array)
#6 /app/public/wp-settings.php(394): do_action('plugins_loaded')
#7 /app/public/wp-config.php(84): require_once('/app/public/wp-...')
#8 /app/public/wp-load.php(37): require_once('/app/public/wp-...')
#9 /app/public/w in /app/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payment-gateway-wcpay.php on line 390

I think there might be an issue with how the WP_Error is being handled, but I didn't look into it closely.

includes/class-wc-payments.php Show resolved Hide resolved
tests/test-class-wc-payment-gateway-wcpay.php Outdated Show resolved Hide resolved
Check if $api_client is defined before fetching account data in WC_Payments::check_stripe_account_status.
In case the request fails, an WP_Error object is returned from get_account_data.
Check if that happened before checking pending requirements to avoid some issues
with addressing the account properties in a WP_error object.
@luizreis
Copy link
Contributor Author

luizreis commented Oct 4, 2019

I think there might be an issue with how the WP_Error is being handled, but I didn't look into it closely.

@reykjalin - I have added a check to the $account object to see if it is a WP_Error before attempting to check the account status. Could you take a look at it?

Copy link
Contributor

@reykjalin reykjalin left a comment

Choose a reason for hiding this comment

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

Everything looks good to me! :shipit:

I have added a check to the $account object to see if it is a WP_Error before attempting to check the account status. Could you take a look at it?

Seems to work right for now. The error isn't very helpful, but that said I don't really ever expect this particular error to happen in production.

image

Looks good! 😄

@luizreis
Copy link
Contributor Author

luizreis commented Oct 7, 2019

The error isn't very helpful, but that said I don't really ever expect this particular error to happen in production.

I agree. Currently, the only case where the API client request method returns a WP_Error is when status code is 4xx. So it most likely means that this is caused by something wrong in our end. As for the message: I'm just reprinting the one returned by WP.com API, but we could override it and reword to be more meaningful in another issue.

@luizreis luizreis merged commit e91c573 into update/functional-prototype-settings Oct 7, 2019
@luizreis luizreis deleted the add/verify-business-details-notification branch October 7, 2019 12:36
luizreis added a commit that referenced this pull request Oct 7, 2019
@luizreis luizreis restored the add/verify-business-details-notification branch October 7, 2019 12:37
luizreis added a commit that referenced this pull request Oct 7, 2019
* Add account endpoint call to API client

* Add method to check for pending requirements

* Add get login and connect url methods to WCPay gateway

* Add wp-admin notices for issues with Stripe account

* Add connect to Stripe and update business information messages

* Use Gateway messages when checking for Stripe connection issues

* Check if account request succeeds before checking its status
@luizreis luizreis deleted the add/verify-business-details-notification branch October 7, 2019 12:46
@reykjalin
Copy link
Contributor

reykjalin commented Oct 7, 2019

Currently, the only case where the API client request method returns a WP_Error is when status code is 4xx. So it most likely means that this is caused by something wrong in our end.

Oh yeah, I know why it's showing up. I hadn't merged server62 into my local payments server when I saw the error 🙂. I believe that means the endpoint doesn't exist, which actually does make the error message accurate. Just not very helpful because there's no way a store owner would be able to do anything about that.

Which leads me to:

As for the message: I'm just reprinting the one returned by WP.com API, but we could override it and reword to be more meaningful in another issue.

I don't think this is necessary. As an error message for developers it's good 👍.

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.

4 participants