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

Sandbox live switch, Change onboarding flow #40

Merged
merged 42 commits into from
Oct 7, 2020

Conversation

websupporter
Copy link
Member

@websupporter websupporter commented Sep 25, 2020

Builds upon #35 as mentioned in my comment there. Fixes #22, Fixes #21

With this PR you can now alter all the time between sandbox and production mode.

With #21, we do not know anymore if we are currently in sandbox or production mode. We seperate the two hosts, so we can later instantiate different Api endpoint objects with different hosts. This enables us to call two different hosts in the same PHP process. In the current scope, just seperating the hosts (and not secrets/keys) should be sufficient.
Change toggle functionality. Load connect-to-sandbox-button and connect-to-production-button in case credentials are missing
@websupporter websupporter changed the title Sandbox live switch Sandbox live switch, Change onboarding flow Sep 25, 2020
@chickenn00dle chickenn00dle self-assigned this Sep 28, 2020
@chickenn00dle
Copy link
Contributor

Hey again @websupporter

I did some more testing this morning and came across one other issue:

When a connection fails, it appears some API credentials are still saved: https://d.pr/i/tO5QlD

As a result, the gateway can be enabled despite not being properly connected: https://d.pr/i/Nqwe9M

And as expected, when attempting to place an order when not connected results in an error each time: https://d.pr/i/ILxI8s

I've also left some comments above specific to the code changes.

Let me know if you have any questions!

@websupporter
Copy link
Member Author

websupporter commented Oct 1, 2020

When a connection fails, it appears some API credentials are still saved

This can happen. We receive the client_id & secret via a JavaScript callback, which gives us temporary keys with which we go to the PayPal API to receive the fixed values. Usually, if this fails, you get an alert() informing you about this failure:
https://github.com/woocommerce/paypal-for-woocommerce/blob/issue-22-sandbox-live-switch/modules/ppcp-onboarding/src/Endpoint/class-loginsellerendpoint.php#L113-L132

Email and merchant id are received, once the merchant clicks "back to store" in the modal:
https://github.com/woocommerce/paypal-for-woocommerce/blob/issue-22-sandbox-live-switch/modules/ppcp-wc-gateway/src/Settings/class-settingslistener.php#L89-L123

In your case, you went back but the credentials couldn't be fetched. It seems sometimes PayPal sandbox returns there a status 500 and fails for some reason.

As a result, the gateway can be enabled despite not being properly connected

You are now in the so-called "progressive onboarding". In the old way - once you enter an email address- you are in progressive mode, which already enables you to receive payments. This is where connect.woo is utilized. I expect this will play a role again, once we come to the Woo onboarding of new merchants. So, this would be expected.

Is merchant email enough to determine connectivity

This is why the merchant email is sufficient to determine connectivity. Already you should be able to enable your gateway and do some settings. The gateway is limited though. You cant do DCC as an example.

when attempting to place an order when not connected results in an error each time

This is then a bug, which - at least in my instance - was due to the dirty state of my application. fa1c2e4 takes care of this. My problem is that dcc_enabled was set to true. This resulted in an attempt to reach already the PayPal-API (to get a customer-client-id necessary for DCC), but we can't reach this API yet directly.

UX solution when the call for client_id/secret fails late (user clicks "go back" before we have the result of the API and the API returns 500):
In the SettingsListener we can check if we have a client id and if not return the merchant with a notice like "did not work, please try again" (321a324)

@chickenn00dle
Copy link
Contributor

Thanks for the detailed explanation and for clearing that up @websupporter!

I tested these latest changes and they are working as expected.

About the notice that appears when client_id/secret fail, could we more clearly explain to merchants the state of their connection? Right now it says the following:

We could not properly fetch the necessary credentials. Please try again.

This seems as though the connection has failed altogether, but in reality, as you've explained above, the plugin is "connected" but with limitations. With that in mind, perhaps we could change the above to the following:

We could not complete the onboarding process. Some features, such as card processing, will not be available. To fix this, please reload the page and try again.

One other small issue. The admin notice above remains even after I retry or switch modes/save settings because the URL param persists.

Finally, I am wondering if we could do away with the alert altogether and fallback to admin notices here? Right now, when onboarding fails and we are in the progressive state, we see an alert letting us know of the failure, followed by an admin notice. I would say one of these two is enough.

The alert may be needed to catch other error cases, so this may not be viable, but it would be nice if we could rely on admin notices for any onboarding issues instead.

@websupporter
Copy link
Member Author

Hi @chickenn00dle, incorporated your suggestions :)

@chickenn00dle
Copy link
Contributor

Thanks @websupporter!

I tried to test this but am seeing the connect server is returning a 404 and so the connect buttons don't render:

Is this a known issue?

@websupporter
Copy link
Member Author

Hi @chickenn00dle, the endpoints of the connect platform changed. I updated the branch accordingly. should work again.

@chickenn00dle
Copy link
Contributor

Hey @websupporter

Thanks! The connect buttons are rendering again 🙌

Unfortunately, I am now running into some inconsistent behavior. I'm not sure if this has anything to do with changes to the connect platform, but here are some of the problems I am running into:

  • When the plugin is first installed, when attempting to enable sandox and connect, the plugin does not properly store credentials as sandbox: https://d.pr/i/ENTW5E
  • Sometimes only the client and secret IDs are saved after completing the connection flow: https://d.pr/i/QVi6Ro
  • Other times only the Email and Merchant ID are saved (this is expected): https://d.pr/i/WxM0Ag
  • Finally, sometimes I am able to connect with no issues (all credentials are saved)

I tried to find some pattern to the above issues, but was unable to do so. Perhaps you can shed some light into this given you have the entire picture with regard to the connect platform and the onboarding process.

@websupporter
Copy link
Member Author

I am having trouble to reproduce this behavior:

When the plugin is first installed, when attempting to enable sandox and connect, the plugin does not properly store credentials as sandbox

I think the problem here is that an error gets thrown before sandbox_on=true is stored. I fixed that in 722b67a

Sometimes only the client and secret IDs are saved after completing the connection flow

I can't really reproduce that. The only way to me this could happen is when the connection flow is not followed until the end 🤔

@chickenn00dle
Copy link
Contributor

Thanks @websupporter!

I think the problem here is that an error gets thrown before sandbox_on=true is stored. I fixed that in 722b67a

This seems to have solved the problem.

I can't really reproduce that. The only way to me this could happen is when the connection flow is not followed until the end 🤔

I did some testing this morning/afternoon and am strangely unable to reproduce this as well now. I am still seeing instances of client secret/id failing the first connection attempt, but since this scenario is accounted for this isn't really a blocker.

Thanks a ton for all of your hard work on this and I appreciate your patience with the back and forth 🙇‍♂️

Merging.

@chickenn00dle chickenn00dle merged commit fca2c71 into master Oct 7, 2020
@chickenn00dle chickenn00dle deleted the issue-22-sandbox-live-switch branch October 7, 2020 04:19
@websupporter
Copy link
Member Author

Likewise: Thanks for your patience and great feedback ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sandbox/Live switch Requiring an email before being able to connect adds some confusion to the process.
2 participants