Add support for the PaymentIntent resource #606

remi-stripe · 2018-06-27T23:19:06Z

This resource is gated but stripe-mock has been updated to support the APIs so tests run as expected.

r? @brandur-stripe
cc @stripe/api-libraries
cc @michelle-stripe @jenan-stripe

remi-stripe · 2018-06-28T19:49:02Z

paymentintent.go

+	ListParams `form:"*"`
+}
+
+type PaymentIntentNextSourceActionValue struct {


We will need to do something smarter here as the shape of value changes based on type. This will be closer to what we do with BalanceTransaction and source

remi-stripe · 2018-06-28T21:07:57Z

paymentintent.go

+	switch s.Type {
+	case PaymentIntentNextActionAuthorizeWithURL:
+		fmt.Printf("%v", data)
+		err = json.Unmarshal(data, &s.Value.AuthorizeWithUrl)


I know this part is incorrect because at that point data still contains the top-level info but I haven't yet figured out out I can fix it.

brandur-stripe

Thanks for the continued epic work Remi! Left a few comments below, but looking great.

Especially whether there's a way to stub the tests easily beyond just skipping?

Basically the recommended way to do this in Go is to set up an HTTP test server that the client will interact with. If we can expect this in stripe-mock ~soon, I don't think it's worth the effort, and using t.Skip is the right option for now (we get a few guarantees by virtue that we know the tests compile, which is something at least).

ptal @remi-stripe

brandur-stripe · 2018-06-28T21:33:55Z

paymentintent.go

+	Created              int64                           `json:"created"`
+	Currency             string                          `json:"currency"`
+	Customer             *Customer                       `json:"customer"`
+	Livemode             bool                            `json:"livemode"`


Missing Description.

brandur-stripe · 2018-06-28T21:34:14Z

paymentintent.go

+	OnBehalfOf           *Account                        `json:"on_behalf_of"`
+	ReceiptEmail         string                          `json:"receipt_email"`
+	ReturnURL            string                          `json:"return_url"`
+	SaveSourceToCustomer bool                            `json:"save_source_to_customer"`


I'm not seeing SaveSourceToCustomer in the latest documentation. Can you double-check that it's necessary?

it's a param only

brandur-stripe · 2018-06-28T21:36:11Z

client/api.go

@@ -148,6 +149,8 @@ type API struct {
 	PaymentSource *paymentsource.Client
 	// ExchangeRates is the client used to invoke /exchange_rates APIs.
 	ExchangeRates *exchangerate.Client
+	// PaymentIntents is the client used to invoke /exchange_rates APIs.


Slight fix needed in the /exchange_rates part of the comment.

BTW, we should try to remember to alphabetize both these lists later :)

yep I can take after this PR, I did not want to mix both

brandur-stripe · 2018-06-28T21:38:29Z

paymentintent.go

+	ListParams `form:"*"`
+}
+
+type PaymentIntentSourceActionAuthorizeWithUrl struct {


Should probably make this URL instead of Url.

brandur-stripe · 2018-06-28T21:38:43Z

paymentintent.go

+// PaymentIntentSourceActionValue describes the `value` property in `next_source_action`
+// The `type` in the parent should indicate which object is fleshed out.
+type PaymentIntentSourceActionValue struct {
+	AuthorizeWithUrl *PaymentIntentSourceActionAuthorizeWithUrl `json:"-"`


And similarly, with the name of this field: AuthorizeWithURL.

brandur-stripe · 2018-06-28T21:41:58Z

paymentintent_test.go

+
+	assert.Equal(t, PaymentIntentNextActionAuthorizeWithURL, intent.NextSourceAction.Type)
+	assert.Equal(t, "https://stripe.com", intent.NextSourceAction.Value.AuthorizeWithUrl.URL)
+}


Would you mind adding a test specifically for PaymentIntentSourceAction's UnmarshalJSON? IMO, we should try to have a specific test for every type that has a custom UnmarshalJSON implementation.

It's fairly similar to ExternalAccount's unmarshaling, so I'd use the test TestExternalAccount_UnmarshalJSON in account_test.go for inspiration.

We could also then remove the next_source_action from TestPaymentIntent_UnmarshalJSON now that there's a more local test to check the same thing, but it's up to you.

brandur-stripe · 2018-06-28T21:45:36Z

paymentintent/client.go

+
+func getC() Client {
+	return Client{stripe.GetBackend(stripe.APIBackend), stripe.Key}
+}


Would you mind taking a stab at getting the new paymentintent/ package to pass Golint? Basically you'd be doing what I did in the PR that activated it for account and charge:

Get Golint running locally:

go get -u golang.org/x/lint/golint golint ./paymentintent

Fix the linting problems (using the same style of comment from ./charge basically:

paymentintent/client.go:1:1: package comment should be of the form "Package paymentintent ..." paymentintent/client.go:23:1: exported method Client.New should have comment or be unexported paymentintent/client.go:24:2: don't use underscores in Go names; var payment_intent should be paymentIntent paymentintent/client.go:35:1: exported method Client.Get should have comment or be unexported paymentintent/client.go:37:2: don't use underscores in Go names; var payment_intent should be paymentIntent paymentintent/client.go:48:1: exported method Client.Update should have comment or be unexported paymentintent/client.go:50:2: don't use underscores in Go names; var payment_intent should be paymentIntent paymentintent/client.go:61:1: exported method Client.Cancel should have comment or be unexported paymentintent/client.go:63:2: don't use underscores in Go names; var payment_intent should be paymentIntent paymentintent/client.go:74:1: exported method Client.Capture should have comment or be unexported paymentintent/client.go:76:2: don't use underscores in Go names; var payment_intent should be paymentIntent paymentintent/client.go:87:1: exported method Client.Confirm should have comment or be unexported paymentintent/client.go:89:2: don't use underscores in Go names; var payment_intent should be paymentIntent paymentintent/client.go:100:1: exported method Client.List should have comment or be unexported

Add the package to the whitelist in Makefile:

lint: golint -set_exit_status ./account golint -set_exit_status ./charge golint -set_exit_status ./paymentintent

brandur-stripe · 2018-06-28T21:46:13Z

paymentintent.go

+		err = json.Unmarshal(data, &s.Value.AuthorizeWithUrl)
+	}
+
+	return err


This UnmarshalJSON implementation is fine by me!

brandur-stripe · 2018-06-28T21:50:09Z

paymentintent/client.go

+}
+
+func (c Client) New(params *stripe.PaymentIntentParams) (*stripe.PaymentIntent, error) {
+	payment_intent := &stripe.PaymentIntent{}


And LOL, I didn't even notice these right away, but saw them in the Linter's error messages, but these of course should be paymentIntent instead of payment_intent.

brandur-stripe · 2018-06-28T22:10:41Z

@remi-stripe Okay, here's a working UnmarshalJSON implementation that I came up with:

func (s *PaymentIntentSourceAction) UnmarshalJSON(data []byte) error {
	type paymentIntentSourceAction PaymentIntentSourceAction
	var v paymentIntentSourceAction
	if err := json.Unmarshal(data, &v); err != nil {
		return err
	}

	var err error
	*s = PaymentIntentSourceAction(v)
	s.Value = &PaymentIntentSourceActionValue{}

	// Unmarshal data a second time so that we can get the raw bytes for the
	// `value` field
	var rawObject map[string]*json.RawMessage
	if err := json.Unmarshal(data, &rawObject); err != nil {
		return err
	}

	switch s.Type {
	case PaymentIntentNextActionAuthorizeWithURL:
		err = json.Unmarshal(*rawObject["value"], &s.Value.AuthorizeWithUrl)
	}

	return err
}

And also, change the struct definition so that we have no JSON annotation for Value:

type PaymentIntentSourceAction struct {
	Type  PaymentIntentNextActionType     `json:"type"`
	Value *PaymentIntentSourceActionValue `json:"-"` // See custom UnmarshalJSON implementation
}

I think overall it's pretty clean, with just a couple caveats:

It does have to call Unmarshal a second time. The first pass isn't doing much work though, so it's probably not too bad performance-wise.
It's not ideal that we have to come up with yet another pattern for unmarshaling this late in the game :( I'm not sure there's much we can do about it here, but we it'd be kind of nice to get faster feedback from API library implementations for API review ... when there's a smell, it tends to be more noticeable here. The good news is that if we add strongly-typed structs for sources, Source's UnmarshalJSON will probably look a lot like this one's (although not exactly).

remi-stripe · 2018-06-28T22:38:28Z

Okay addressed all the comments and fixed the unmarshal logic thanks to Brandur. Only thing left is to decide changes to the /confirm parameters shape.

brandur-stripe · 2018-06-28T23:18:25Z

paymentintent/client.go

+	Key string
+}
+
+// New creates a payment_intent.


Nit, but would you mind if we used the more human-readable "payment intent" for these comments?

damn search and replace. Nice catch and fixed!

brandur-stripe · 2018-06-28T23:19:00Z

Thanks for getting all that review feedback in so quickly!

remi-stripe · 2018-07-05T16:16:34Z

paymentintent.go

+	ReceiptEmail        string                          `json:"receipt_email"`
+	ReturnURL           string                          `json:"return_url"`
+	Shipping            ShippingDetails                 `json:"shipping"`
+	Source              *PaymentSource                  `json:"source"`


Since we only support real Sources, should this be Source directly? I think so

Arg, I just took a look at the source code and it's allows to be either a source or a card.

I really like the idea of it just being a Source as well, but we might have to check with the team for advice on how to handle this.

yep, talked to them about it and we want to support Cards moving forward so it will stay a PaymentSource unfortunately

remi-stripe · 2018-07-05T16:22:10Z

paymentintent.go

+	ReceiptEmail        string                          `json:"receipt_email"`
+	ReturnURL           string                          `json:"return_url"`
+	Shipping            ShippingDetails                 `json:"shipping"`
+	Source              *Source                         `json:"source"`


Since PaymentIntent will only support Sources and nothing else, I think it's the right call to use Source here.

remi-stripe · 2018-07-07T16:08:17Z

Okay now that stripe-mock is updated the tests run and I think it is now ready to merge but needs a full review to make sure we did not miss anything

brandur-stripe · 2018-07-09T14:41:32Z

paymentintent/client.go

+// Iter is an iterator for lists of PaymentIntents.
+// The embedded Iter carries methods with it;
+// see its documentation for details.
+type Iter struct {


I think we ended up updating all these Iter comments in your other Lint PRs. Can we change this one too? (And maybe the comment on PaymentIntent() directly below too.

Nice catch, fixed other comments too

brandur-stripe · 2018-07-09T14:42:02Z

Left a minor comment on comments, but otherwise looks great.

remi-stripe · 2018-07-09T17:09:23Z

PTAL @brandur-stripe

brandur-stripe · 2018-07-09T18:30:08Z

Thanks Remi!

brandur-stripe · 2018-07-09T18:33:53Z

Released as 36.0.0.

brandur-stripe · 2018-07-09T20:39:46Z

And I just realized that I jumped the gun on this a little bit because we're probably going to tweak a few parts of this API. I think it's okay though since this is still behind a gate and it won't really be possible to start relying on it yet, so we can make those changes without breaking anyone.

remi-stripe · 2018-07-09T20:57:28Z

@brandur-stripe My understanding was that the API was stable and changes would be versioned/punted to later on so I think it was ready to be merged. At least that's the feeling I got internally!

brandur-stripe · 2018-07-09T21:12:43Z

@remi-stripe Oh great! I'd thought for some reason that some of the changes we'd talked about a few weeks ago were still WIP. Very happy to hear that they're already good to go. Thanks.

* Improve error message for missing revised line item * Stop connectionSuccessful, not used in latest JS code * Documentation from video walkthrough * Adding brian to pr template

remi-stripe commented Jun 28, 2018

View reviewed changes

remi-stripe force-pushed the remi-add-payment-intent branch from cba89ac to 92546f6 Compare June 28, 2018 21:07

remi-stripe commented Jun 28, 2018

View reviewed changes

remi-stripe force-pushed the remi-add-payment-intent branch from 92546f6 to 5b03e0d Compare June 28, 2018 21:08

brandur-stripe reviewed Jun 28, 2018

View reviewed changes

stripe-ci assigned remi-stripe Jun 28, 2018

brandur-stripe reviewed Jun 28, 2018

View reviewed changes

remi-stripe force-pushed the remi-add-payment-intent branch from fe2545d to 8bba7db Compare June 28, 2018 22:18

brandur-stripe reviewed Jun 28, 2018

View reviewed changes

remi-stripe force-pushed the remi-add-payment-intent branch from 8bba7db to f37e1b4 Compare June 28, 2018 23:22

remi-stripe commented Jul 5, 2018

View reviewed changes

remi-stripe force-pushed the remi-add-payment-intent branch from f37e1b4 to 1171f3f Compare July 5, 2018 16:21

remi-stripe commented Jul 5, 2018

View reviewed changes

remi-stripe force-pushed the remi-add-payment-intent branch from d7746cd to d7cc37d Compare July 7, 2018 16:06

remi-stripe changed the title ~~[WIP] Add support for the PaymentIntent resource~~ Add support for the PaymentIntent resource Jul 7, 2018

remi-stripe removed their assignment Jul 7, 2018

stripe-ci assigned brandur-stripe Jul 7, 2018

brandur-stripe reviewed Jul 9, 2018

View reviewed changes

Add support for the PaymentIntent resource

1f4a6cf

remi-stripe force-pushed the remi-add-payment-intent branch from d7cc37d to 1f4a6cf Compare July 9, 2018 17:08

brandur-stripe merged commit c547b00 into master Jul 9, 2018

brandur-stripe deleted the remi-add-payment-intent branch July 9, 2018 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the PaymentIntent resource #606

Add support for the PaymentIntent resource #606

remi-stripe commented Jun 27, 2018 •

edited

Loading

remi-stripe Jun 28, 2018

remi-stripe Jun 28, 2018

brandur-stripe left a comment

brandur-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

remi-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

remi-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

brandur-stripe Jun 28, 2018

brandur-stripe commented Jun 28, 2018 •

edited

Loading

remi-stripe commented Jun 28, 2018

brandur-stripe Jun 28, 2018

remi-stripe Jun 28, 2018

brandur-stripe commented Jun 28, 2018

remi-stripe Jul 5, 2018

brandur-stripe Jul 5, 2018 •

edited

Loading

remi-stripe Jul 5, 2018

remi-stripe Jul 5, 2018

remi-stripe commented Jul 7, 2018

brandur-stripe Jul 9, 2018

remi-stripe Jul 9, 2018

brandur-stripe commented Jul 9, 2018

remi-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

remi-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

Add support for the PaymentIntent resource #606

Add support for the PaymentIntent resource #606

Conversation

remi-stripe commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Jun 28, 2018 • edited Loading

remi-stripe commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Jun 28, 2018

Choose a reason for hiding this comment

brandur-stripe Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remi-stripe commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Jul 9, 2018

remi-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

remi-stripe commented Jul 9, 2018

brandur-stripe commented Jul 9, 2018

remi-stripe commented Jun 27, 2018 •

edited

Loading

brandur-stripe commented Jun 28, 2018 •

edited

Loading

brandur-stripe Jul 5, 2018 •

edited

Loading