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.

Sign up for GitHub

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

Jump to bottom

Recurring Payments: Add postId to Stripe connection URL state param #13398

Merged

sirreal merged 5 commits into master from update/send-postid-in-stripe-connection-state

Sep 24, 2019

Member

sirreal commented Sep 3, 2019 •

edited

Loading

Companion to D32394-code

See p7rd6c-24Y-p2 for detailed discussion.

Changes proposed in this Pull Request:

Save and navigate in the same window when starting Stripe connection.
If we don't have a postId, open the connection in a new window without the postId added.
Add the postId to the state so we can return after connection.
Add noopener noreferrer if the link will open in a new window (_blank).

Testing instructions:

If you apply D32394-code and sandbox public-api.wordpress.com, you should be able to follow the whole flow.
For a Jetpack site with a paid plan
Add a Recurring Payment block to the editor
Click the Stripe conneciton button
The post should auto-save, then navigate to the strip connection.
With D32394-code, you should return to the same post after connecting (or cancelling)
Without D32394-code, you'll land at /earn on WordPress.com

The post id is base64 encoded in the state query param on the connection URL. You can grab it and inspect to verify that the post ID is correctly added.

Proposed changelog entry for your changes:

None.

sirreal added [Status] In Progress [Block] Payment Button labels

sirreal requested a review from a team

September 3, 2019 12:25

sirreal self-assigned this

matticbot added the Touches WP.com Files label

Contributor

matticbot commented Sep 3, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello sirreal! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32391-code before merging this PR. Thank you!

sirreal changed the title ~~Add postId to Stripe connection URL state param~~ Recurring Payments: Add postId to Stripe connection URL state param

jetpackbot commented Sep 3, 2019 •

edited

Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: October 1, 2019.
Scheduled code freeze: September 24, 2019

Generated by 🚫 dangerJS against bf08ece

sirreal force-pushed the update/send-postid-in-stripe-connection-state branch from e530615 to fe7213f Compare

September 3, 2019 13:32

Contributor

matticbot commented Sep 3, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello sirreal! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32395-code before merging this PR. Thank you!

sirreal force-pushed the update/send-postid-in-stripe-connection-state branch from fe7213f to 4a016a1 Compare

September 3, 2019 13:32

Contributor

matticbot commented Sep 3, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello sirreal! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32396-code before merging this PR. Thank you!

Contributor

matticbot commented Sep 3, 2019

sirreal, Your synced wpcom patch D32391-code has been updated.

simison reviewed

View reviewed changes

extensions/blocks/recurring-payments/edit.jsx Outdated Show resolved Hide resolved

simison reviewed

View reviewed changes

extensions/blocks/recurring-payments/edit.jsx Show resolved Hide resolved

sirreal commented

View reviewed changes

extensions/blocks/recurring-payments/edit.jsx Outdated Show resolved Hide resolved

sirreal added 2 commits

September 5, 2019 15:28


          Add postId to Stripe connection URL state param

34f72bf


          Save on leave to connect

6ccf747

sirreal force-pushed the update/send-postid-in-stripe-connection-state branch from 0f224cf to 6ccf747 Compare

September 5, 2019 14:54

Contributor

matticbot commented Sep 5, 2019

sirreal, Your synced wpcom patch D32391-code has been updated.


          Use _top over _parent

39e087c

Contributor

matticbot commented Sep 5, 2019

sirreal, Your synced wpcom patch D32391-code has been updated.


          Add rel="noopener noreferrer" if _blank

b0d60ab

Contributor

matticbot commented Sep 5, 2019

sirreal, Your synced wpcom patch D32391-code has been updated.

sirreal added [Status] Needs Review [Type] Enhancement and removed [Status] In Progress labels

sirreal requested a review from a team

September 5, 2019 15:05

sirreal commented

View reviewed changes

extensions/blocks/recurring-payments/edit.jsx Outdated Show resolved Hide resolved


          Ternary the target

bf08ece

Contributor

matticbot commented Sep 5, 2019

sirreal, Your synced wpcom patch D32391-code has been updated.

Contributor

ockham commented Sep 5, 2019

The flow took me back to https://example.com/wp-login.php?redirect_to=https%3A%2F%2Fexample.com%2Fwp-admin%2Fpost.php%3Fstripe_connect_success%3Dgutenberg%26post%3D123%26action%3Dedit&reauth=1, meaning I had to sign in again (and was dropped into general wp-admin rather than the relevant post) 🤔

simison reviewed

View reviewed changes

extensions/blocks/recurring-payments/edit.jsx

+              										href={ stripeConnectUrl }
+              										target={ stripeConnectTarget }
+              										rel={ stripeConnectTarget === '_blank' ? 'noopener noreferrer' : undefined }
+              										onClick={ this.props.autosaveAndNavigateToConnection }

Member

simison Sep 6, 2019

I think we'll need to add tracking here but that's good for a follow up PR.

Contributor

ockham Sep 6, 2019

#13394 does that 👍

Contributor

ockham Sep 24, 2019

Actually, it only does in case we're using the Stripe Connect Nudge, but not if we're still using the in-block connect button 🙁

simison mentioned this pull request

Gutenberg extensions: Stripe connection notices #13441

Merged

ockham approved these changes

View reviewed changes

Contributor

ockham left a comment

Gave this some more testing (some of which during a call wiht @sirreal). The issue I was seeing earlier seems to be rather spurious, so it's hopefully due to something with my test site (I was thinking maybe because I'm not using a 'static' ngrok domain?)

Recent tries have all worked, both when canceling the Stripe connection flow, and when connecting (using the dev mode dummy connection).

Let's 🚢 it 👍

jeherve added [Status] Ready to Merge and removed [Status] Needs Review labels

jeherve added this to the 7.8 milestone

jeherve approved these changes

View reviewed changes

Member

jeherve left a comment

Shouldn't add_filter( 'jetpack_block_editor_enable_upgrade_nudge', '__return_true' ); present me with the nudge for this block?

Other than that, it seems to work well.

chriskmnds reviewed

View reviewed changes

extensions/blocks/recurring-payments/edit.jsx

+              				event.preventDefault();
+              				await dispatch( 'core/editor' ).autosave();
+              				// Using window.top to escape from the editor iframe on WordPress.com
+              				window.top.location.href = href;

Contributor

chriskmnds Sep 19, 2019

Do we need to guard here for potentially undefined window?
cc @ockham

Contributor

ockham Sep 19, 2019

We have a redirect like this in the block nudge already. I'm not sure it's dangerous to have window unguarded in cases like this; while we do use the block nudge for server side rendering, it's only the plain 'dumb' component rather than the withDispatch() case, so we're not actually ever using it in a context where window doesn't exist -- and I think we'd notice pretty much immediately if we did, since it'll totally break the build. (The same argument applies to this occurrence.)

My take would thus be YAGNI -- let's only add it once we need it, e.g. if the build fails.

Contributor

chriskmnds Sep 20, 2019

Ok, makes sense I guess. I am not familiar with the server side rendering stuff - just curious as you mentioned it previously. So YAGNI - if the build fails, then fix? Not sure that was the case previously tbh (I don't remember builds failing, but it was a fairly different case + may be wrong/missing something). I'm clueless on the ssr stuff. :-)

Member Author

sirreal Sep 20, 2019 •

edited

Loading

It's a good question, guarding browser globals is cheap and saves pain down the road. The problem here is that none of this code block makes sense outside the browser. We can't event.preventDefault if we can't handle the redirect. That leads to being unable to save the content before navigation, and at that point we might as well just error.

Contributor

ockham Sep 23, 2019

What Jon said -- it's just a noop outside the browser (we do however have an href in the 'dumb' component as the fallback for the SSR case, so a noop should be okay -- but we essentially do that, at least in the Upgrade Nudge case, by using the dumb rather than the smart component in SSR).

jeherve modified the milestones: 7.8, 7.9

sirreal merged commit f55f55e into master

sirreal deleted the update/send-postid-in-stripe-connection-state branch

September 24, 2019 08:17

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge labels

sirreal removed the [Status] Needs Changelog label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Payment Button Touches WP.com Files [Type] Enhancement