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.

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

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

Merged
merged 5 commits into from
Sep 24, 2019
Merged

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

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34f72bf
Add postId to Stripe connection URL state param
sirreal Sep 3, 2019
6ccf747
Save on leave to connect
sirreal Sep 5, 2019
39e087c
Use _top over _parent
sirreal Sep 5, 2019
b0d60ab
Add rel="noopener noreferrer" if _blank
sirreal Sep 5, 2019
bf08ece
Ternary the target
sirreal Sep 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 66 additions & 5 deletions extensions/blocks/recurring-payments/edit.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
/**
* External dependencies
*/

import classnames from 'classnames';
import SubmitButton from '../../shared/submit-button';
import apiFetch from '@wordpress/api-fetch';
import { __, sprintf } from '@wordpress/i18n';
import { trimEnd } from 'lodash';
import formatCurrency, { getCurrencyDefaults } from '@automattic/format-currency';
import { addQueryArgs, getQueryArg, isURL } from '@wordpress/url';
import { compose } from '@wordpress/compose';
import { withDispatch, withSelect } from '@wordpress/data';

import {
Button,
Expand Down Expand Up @@ -313,10 +315,45 @@ class MembershipsButtonEdit extends Component {
);
};

getConnectUrl() {
const { postId } = this.props;
const { connectURL } = this.state;

if ( ! isURL( connectURL ) ) {
return null;
}

if ( ! postId ) {
return connectURL;
}

let decodedState;
try {
const state = getQueryArg( connectURL, 'state' );
decodedState = JSON.parse( atob( state ) );
} catch ( err ) {
if ( process.env.NODE_ENV !== 'production' ) {
console.error( err ); // eslint-disable-line no-console
}
return connectURL;
}

decodedState.from_editor_post_id = postId;

return addQueryArgs( connectURL, { state: btoa( JSON.stringify( decodedState ) ) } );
}

render = () => {
const { className, notices, attributes } = this.props;
const { connected, connectURL, products } = this.state;
const { attributes, className, notices, postId } = this.props;
const { connected, products } = this.state;
const { align } = attributes;

const stripeConnectUrl = this.getConnectUrl();

// If we know the postId, assume we'll return to the editor. Navigate the top window.
// Otherwise, open a new window.
const stripeConnectTarget = postId ? '_top' : '_blank';

const inspectorControls = (
<InspectorControls>
<PanelBody title={ __( 'Product', 'jetpack' ) }>
Expand Down Expand Up @@ -401,7 +438,15 @@ class MembershipsButtonEdit extends Component {
'jetpack'
) }
</p>
<Button isDefault isLarge href={ connectURL } target="_blank">
<Button
isDefault
isLarge
disabled={ ! stripeConnectUrl }
sirreal marked this conversation as resolved.
Show resolved Hide resolved
href={ stripeConnectUrl }
target={ stripeConnectTarget }
rel={ stripeConnectTarget === '_blank' ? 'noopener noreferrer' : undefined }
onClick={ this.props.autosaveAndNavigateToConnection }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

#13394 does that 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙁

>
{ __( 'Connect to Stripe or set up an account', 'jetpack' ) }
</Button>
<br />
Expand Down Expand Up @@ -459,4 +504,20 @@ class MembershipsButtonEdit extends Component {
};
}

export default withNotices( MembershipsButtonEdit );
export default compose( [
withSelect( select => ( { postId: select( 'core/editor' ).getCurrentPostId() } ) ),
withDispatch( dispatch => ( {
autosaveAndNavigateToConnection: async event => {
const { href, target } = event.target;

// Special handling if we're opening in _this_ window. Otherwise, just let the navigation happen.
if ( href && target !== '_blank' ) {
event.preventDefault();
await dispatch( 'core/editor' ).autosave();
// Using window.top to escape from the editor iframe on WordPress.com
window.top.location.href = href;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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. :-)

Copy link
Member Author

@sirreal sirreal Sep 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

}
},
} ) ),
withNotices,
] )( MembershipsButtonEdit );