-
Notifications
You must be signed in to change notification settings - Fork 994
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.
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
Add metadata
accessors
#808
Conversation
Both of these fields are accessible from the customer when using ephemeral keys
r? @bg-stripe |
@@ -16,6 +16,8 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|||
- (NSDictionary *)stp_dictionaryByRemovingNulls; | |||
|
|||
- (NSDictionary<NSString *, NSString *> *)stp_dictionaryByRemovingNonStrings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do this. We don't currently enforce this type for other classes like STPSource and I don't think we should change that or add the restriction here. It is not clear to me from the API docs that they must in fact be string pairs (as opposed to having number, dictionary, or array values). I think we should just leave as NSDictionary *
or at most NSDictionary<NSString *, id> *
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm actually for this. It is an API constraint, and though the docs aren't super clear we do say:
Note: You can specify up to 20 keys, with key names up to 40 characters long and values up to 500 characters long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting. I didn't realize we specified that. In that case I'm ok with changing our API to match this constraint 👍
Just did a run through of our docs – looks like bank account, PII, and file creations don't support metadata, but you can attach metadata to a card. Should we also add a |
Hmm, it looks like bank accounts do support metadata to me[0]. We should probably add to card params and bank account params, and forward through the metadata on [0] https://stripe.com/docs/api#customer_create_bank_account |
oh hmm. I was looking at this page [0], which I think is different from the "attach a bank account source to a customer" API. Definitely possible that metadata on bank account tokenization is supported and just not documented – we should test it out. |
Yeah I'm not sure. It's actually super unclear to me how this works, as the token API does not mention metadata at all (including for cards) but the Customer docs say that both cards and bank accounts fetched from them can have metadata (but how would you get those details on without having added them to the token?). Probably need to sync with someone to sort out how this works and get the docs updated. |
I guess actually the docs imply that metadata only is addable if you add the bank account/card directly to the customer and not the "make a token then add that token" method, which seems odd and confusing. Let's sync offline on this. |
After further discussion, since metadata really only makes sense for / is settable by Customer-related endpoints which are generally not handled by the SDK (because you need a private key) we decided it's fine to just add read-only support for them to the models and not to add them into the params objects. |
Summary
Add
metadata
accessors toSTPBankAccount
andSTPCard
The data is populated on these two objects when working with customers fetched using ephemeral keys.
Motivation
Following up from #807
Testing
Updated unit tests. Also added extra guards to ensure the metadata keys/values are strings.