Adds verify source endpoint #359

ob-stripe · 2017-01-18T15:44:20Z

r? @will-stripe
cc @stripe/api-libraries @stan-stripe @remi-stripe

This PR adds support for the /v1/sources/src_.../verify endpoint.

AFAIK, only ach_debit sources use this endpoint at the moment, so I hardcoded Values as [2]uint8. I guess that in the future, other source types might expect different types for the values parameter, and we might want to anticipate on that.

Putting the PR up for review, but I'll leave it to you to decide whether we should merge as-is or wait.

ob-stripe · 2017-01-18T16:05:55Z

After discussing it with @remi-stripe, I updated the PR to use the existing Amounts parameter -- the library will send it to the API as values for sources.

This will avoid a breaking change if we eventually add a generic Values parameter in SourceVerifyParams.

will-stripe · 2017-01-18T17:26:16Z

LGTM
Alternatively we can use an array of strings. If we are worried about breaking changes, I would suggest we go in that direction if we can, but I'm not too concerned.

brandur · 2017-01-18T19:10:02Z

source.go

-	AttemptsRemaining uint64             `json:"attempts_remaining"`
-	Status            RedirectFlowStatus `json:"status"`
+	AttemptsRemaining uint64                 `json:"attempts_remaining"`
+	Status            VerificationFlowStatus `json:"status"`


Was this just a mistake from before?

That's my assumption, yes. I should have mentioned it in the PR, sorry about that!

remi-stripe · 2017-03-25T23:22:46Z

@ob-stripe do you know if this still needs to be merged?

ob-stripe · 2017-03-27T16:18:12Z

Yes, this still needs to be merged -- I forgot to remove the "[WIP]" tag in the PR's title.

Assigning to you @brandur-stripe

stan-stripe · 2017-03-27T17:04:11Z

paymentsource/client.go

 		err = s.B.Call("POST", fmt.Sprintf("/customers/%v/sources/%v/verify", params.Customer, id), s.Key, body, &params.Params, source)
 	} else {
-		err = errors.New("Only customer bank accounts can be verified in this manner.")
+		body.Add("values[]", strconv.Itoa(int(params.Amounts[0])))


So in its most general form, code_verification expects strings. We assume here that they are always numbers, which is OK for ACH debits (on bank account or on /v1/sources) but is not general enough for /v1/sources in general?

@stan-stripe You have lots of context on this, so would you mind pitching an alternative? Should we keep Amounts for customer-specific verification and add a Values here? Should we keep the two-element array?

If you look at the signature of SourceVerifyParams[0], it was really built for bank account verification on customer attached bank account objects (despite the thoughtful intent of making it a generic name)

The right type for the payment agnostic /v1/sources/:id/verify endpoint[1] is:

type NewNameOrMaybeSameNameSourceVerifyParams struct { Values []string }

Unclear to me if we should rename the old one, or rename the new one, but these are two different constructs in the API. Hope that helps?

[0]

stripe-go/payment_source.go

Line 37 in 8c52b2c

type SourceVerifyParams struct {

[1] https://github.com/stripe-internal/pay-server/blob/master/api/lib/api_method/source_verify.rb#L13

Unclear to me if we should rename the old one, or rename the new one, but these are two different constructs in the API. Hope that helps?

Yep, that clears things up. Thanks!

@ob-stripe I'd probably suggest introducing a new struct like the one Stan has above and renaming the old one. The breaking change sucks, but looking at Splunk, it doesn't seem to be too widely used from the Go language right now (< 10 calls in the last week).

ob-stripe · 2017-03-28T23:00:08Z

I updated the PR to handle this with a non-breaking change: I added a Values strings array to the SourceVerifyParams struct but left the existing Amounts integer array.

If the Customer param is used, then we assume that the verification is for a bank account and use the Amounts array. Otherwise we assume that it's for a source and use the Values array.

@brandur-stripe @stan-stripe wdyt?

brandur-stripe · 2017-03-28T23:45:11Z

payment_source.go

@@ -38,6 +38,7 @@ type SourceVerifyParams struct {
 	Params
 	Customer string
 	Amounts  [2]uint8
+	Values   []string


Would you mind commenting Amounts here to indicate that it's for bank accounts only and essentially deprecated.

ach_debit sources are not live yet so I think it's a little bit early to flag bank accounts as deprecated.

That said, I can add the comment and we can wait to merge the PR -- it might be a good idea anyway as there might be other changes before release (e.g. the change from verification to code_verification).

Cool. Yeah I was just thinking that if you come into this with very little information, both Amounts and Values are extremely generic names so it's really not intuitive at all that one would be meant for bank accounts and the other sources. It might be helpful to try and annotate this for users in some way.

brandur-stripe

LGTM!

One thought: should we support both Values and Amount for bank account verification?

ob-stripe · 2017-03-29T16:21:15Z

One thought: should we support both Values and Amount for bank account verification?

Personally, I'd say no: /v1/customers/cus_.../sources/ba_.../verify expects an amounts parameter, while /v1/sources/src_.../verify expects a values parameter, and I'd rather not have that sort of magic mapping from one parameter name to another.

brandur-stripe · 2017-03-29T16:52:46Z

Personally, I'd say no: /v1/customers/cus_.../sources/ba_.../verify expects an amounts parameter, while /v1/sources/src_.../verify expects a values parameter, and I'd rather not have that sort of magic mapping from one parameter name to another.

Fair enough. +1.

brandur-stripe · 2017-05-12T22:54:17Z

@ob-stripe Can we proceed on this one?

remi-stripe · 2017-10-07T13:30:29Z

@ob-stripe could you have a look at this one again rebased on the latest master?

ob-stripe · 2017-10-09T08:44:01Z

Done.

@brandur-stripe ptal

remi-stripe · 2017-10-09T13:48:18Z

paymentsource.go

-	Customer string   `form:"-"` // Goes in the URL
+	Amounts  [2]uint8 `form:"amounts"` // Amounts is used when verifying bank accounts
+	Customer string   `form:"-"`       // Goes in the URL
+	Values   []string `form:"values"`  // Values in used when verifying sources


minor typo here

remi-stripe · 2017-10-09T13:48:38Z

paymentsource/client.go

-	} else {
-		err = errors.New("Only customer bank accounts can be verified in this manner.")
+	} else if len(params.Values) > 0 {
+		err = s.B.Call("POST", fmt.Sprintf("/sources/%v/verify", id), s.Key, body, &params.Params, source)
 	}


you're likely missing an else that raises an error no?

👍 Added an else clause. The error message is a bit vague, I'm open to suggestions if you can think of something better!

brandur-stripe · 2017-10-09T16:51:42Z

AFAIK, only ach_debit sources use this endpoint at the moment, so I hardcoded Values as [2]uint8. I guess that in the future, other source types might expect different types for the values parameter, and we might want to anticipate on that.

Thanks for picking this back up again!

@ob-stripe @remi-stripe One another kinda nit: I think one of the things Remi's doing with his big refactoring project right now is to just make all our integers the same type (not sure which one, but probably either int or int64. Do you think we could replace the uint8 with either one of these? It's not like you're going to be saving that many system resources by using a smaller integer size on verification (it's an infrequent operation and our modern 64-bit platforms are probably often storing it in 64 bits anyway) so just trying to fall into a convention here is probably more valuable.

Maybe int64 would be a good choice. We're never going to be storing numbers that are big enough that we'll need uint64 over int64, and it's kind of nice that int64 is unambiguous 64-bit, although honestly int is probably fine too.

ob-stripe · 2017-10-10T12:18:34Z

Rebased on master and updated to use int64 for Amounts.

brandur-stripe · 2017-10-10T17:05:47Z

Nice! int64 is a good choice.

brandur-stripe · 2017-10-10T17:15:25Z

Released as 28.3.0.

Bumps [mocha](https://github.com/freerange/mocha) from 1.13.0 to 1.14.0. - [Release notes](https://github.com/freerange/mocha/releases) - [Changelog](https://github.com/freerange/mocha/blob/main/RELEASE.md) - [Commits](freerange/mocha@v1.13.0...v1.14.0) --- updated-dependencies: - dependency-name: mocha dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

ob-stripe force-pushed the ob-verify-sources branch 2 times, most recently from 5f1dbae to 3d396a7 Compare January 18, 2017 16:03

stripe-ci added the approved label Jan 18, 2017

stripe-ci assigned ob-stripe Jan 18, 2017

brandur reviewed Jan 18, 2017

View reviewed changes

ob-stripe changed the title ~~[WIP] Adds verify source endpoint~~ Adds verify source endpoint Mar 27, 2017

ob-stripe assigned brandur-stripe and unassigned ob-stripe Mar 27, 2017

stan-stripe reviewed Mar 27, 2017

View reviewed changes

ob-stripe force-pushed the ob-verify-sources branch from 3d396a7 to 1d47ff2 Compare March 28, 2017 21:45

stripe-ci removed the approved label Mar 28, 2017

ob-stripe force-pushed the ob-verify-sources branch 3 times, most recently from e681731 to c4d9607 Compare March 28, 2017 22:35

brandur-stripe reviewed Mar 28, 2017

View reviewed changes

ob-stripe force-pushed the ob-verify-sources branch 3 times, most recently from a5ccb81 to f855eb1 Compare October 7, 2017 15:01

remi-stripe reviewed Oct 9, 2017

View reviewed changes

ob-stripe force-pushed the ob-verify-sources branch from f855eb1 to e7d25f1 Compare October 9, 2017 14:54

ob-stripe force-pushed the ob-verify-sources branch 2 times, most recently from d44a7ac to 798b1ea Compare October 10, 2017 12:16

Adds verify source endpoint

26e8c33

ob-stripe force-pushed the ob-verify-sources branch from 798b1ea to 26e8c33 Compare October 10, 2017 12:19

brandur-stripe merged commit 7d462bb into master Oct 10, 2017

brandur-stripe deleted the ob-verify-sources branch October 10, 2017 17:05

mickjermsurawong-stripe mentioned this pull request Mar 15, 2019

Update resources to 8.0-beta stripe/stripe-java#687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds verify source endpoint #359

Adds verify source endpoint #359

ob-stripe commented Jan 18, 2017 •

edited

Loading

ob-stripe commented Jan 18, 2017

will-stripe commented Jan 18, 2017

brandur Jan 18, 2017

ob-stripe Jan 18, 2017

remi-stripe commented Mar 25, 2017

ob-stripe commented Mar 27, 2017

stan-stripe Mar 27, 2017

brandur-stripe Mar 27, 2017

stan-stripe Mar 27, 2017 •

edited

Loading

brandur-stripe Mar 28, 2017

ob-stripe commented Mar 28, 2017 •

edited

Loading

brandur-stripe Mar 28, 2017

ob-stripe Mar 29, 2017

brandur-stripe Mar 29, 2017

brandur-stripe left a comment

ob-stripe commented Mar 29, 2017

brandur-stripe commented Mar 29, 2017

brandur-stripe commented May 12, 2017

remi-stripe commented Oct 7, 2017

ob-stripe commented Oct 9, 2017

remi-stripe Oct 9, 2017

ob-stripe Oct 9, 2017

remi-stripe Oct 9, 2017

ob-stripe Oct 9, 2017

brandur-stripe commented Oct 9, 2017

ob-stripe commented Oct 10, 2017

brandur-stripe commented Oct 10, 2017

brandur-stripe commented Oct 10, 2017

Adds verify source endpoint #359

Adds verify source endpoint #359

Conversation

ob-stripe commented Jan 18, 2017 • edited Loading

ob-stripe commented Jan 18, 2017

will-stripe commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remi-stripe commented Mar 25, 2017

ob-stripe commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stan-stripe Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

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

ob-stripe commented Mar 29, 2017

brandur-stripe commented Mar 29, 2017

brandur-stripe commented May 12, 2017

remi-stripe commented Oct 7, 2017

ob-stripe commented Oct 9, 2017

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 Oct 9, 2017

ob-stripe commented Oct 10, 2017

brandur-stripe commented Oct 10, 2017

brandur-stripe commented Oct 10, 2017

ob-stripe commented Jan 18, 2017 •

edited

Loading

stan-stripe Mar 27, 2017 •

edited

Loading

ob-stripe commented Mar 28, 2017 •

edited

Loading