Add API to create PaymentMethod #1141
Conversation
yuki-stripe
force-pushed
the
yuki-create-paymentmethod
branch
3 times, most recently
from
57b532f to
0531de0
Compare
yuki-stripe
force-pushed
the
yuki-create-paymentmethod
branch
from
8a1d6c0 to
07de690
Compare
…expYear are now NSUIntegers, rename STPPaymentMethodParams cardParamsWithCard -> paramsWithCard
yuki-stripe
force-pushed
the
yuki-create-paymentmethod
branch
from
07de690 to
ba3a2f3
Compare
There was a problem hiding this comment.
Looks good! Main change I'd like to see is copy for string properties + strong/assign for the others, and sharing the PaymentMethod type enum between STPPaymentMethod and STPPaymentMethodParams
I've got a couple other comments/suggestions, and a tip about changing the base branch to get CI to run (I'm assuming that it's pointed at the branch from #1140, but didn't actually verify that).
yuki-stripe
changed the base branch from
yuki-add-paymentmethod-models
to
master
|
Should have addressed all the feedback, ptal @danj-stripe |
| - (void)setType:(STPPaymentMethodType)type { | ||
| // If setting unknown and we're already unknown, don't want to override raw value | ||
| if (type != self.type) { | ||
| self.rawTypeString = [STPPaymentMethod stringFromType:type]; |
There was a problem hiding this comment.
It looks like setting type = .unknown will set rawTypeString = nil, but rawTypeString is not nullable
Should it be nullable? Do we want to add an assert, that they never set type = .unknown? Something else?
(It looks like this bug also exists in STPSourceParams, which I know I said would be a good model... sorry! Can you update that class too?)
There was a problem hiding this comment.
Setting type = .unknown seems like user error to me, an assertion against that sounds ok.
Come to think of it, why is type publicly writable anyways?
There was a problem hiding this comment.
Great question! I suspect if it was readonly, that'd work.
It's possible to create a SourcesParam using just alloc/init, and then manually setting any fields needed. The static constructors make it really easy to do the right thing for the common case, for all the various types.
The combination of the rawTypeString and additionalAPIParameters property allow full control over the Source/PaymentMethod being created, since we don't necessarily keep up with every possible change to available sources/paymentmethods.
Plausibly there's a use case for building a type that the SDK knows about, but one where the static constructor interferes, in which case being able to set the type using an enum might be nice. I think that's a sufficiently rare case that readonly is 👍. It'd be a breaking change on SourceParams, but I think that's probably fine.
There was a problem hiding this comment.
Always a fan of more readonlys :)
There was a problem hiding this comment.
Looks like the case described actually happens in the SDK already for STPSourceParams:
STPSourceParams *params = [STPSourceParams new];
params.type = STPSourceTypeCard;
params.token = token.tokenId;
I'm thinking I'll make type readonly for STPPaymentMethodParams but add an assertion for STPSourceParams.
There was a problem hiding this comment.
Looking at the tests, it really seems we intended to accommodate setting this value to Unknown. e.g. one test does XCTAssertNil(sourceParams.rawTypeString);. I'm inclined now to instead mark rawTypeString as nullable on STPSourceParams.
- Make STPPaymentMethodParams' type property readonly
Summary
Add
createPaymentMethod...method toSTPAPIClientMotivation
We're adding support for PaymentMethods!
Testing
Added a unit test for STPPaymentMethodParams
Added a functional test that exercises
createPaymentMethod