Add support for /v1/topups endpoints #1110
Conversation
jkakar-stripe
force-pushed
the
jkakar/topup-client
branch
from
50960b8 to
613a80a
Compare
|
@jkakar-stripe Hey Jamu! We just noticed that this PR never quite made it even though its sisters in six other languages did. Do you remember if there was any particular reason why this one didn't finish up? Thanks! |
|
(And also, we're definitely interested in trying to get this done while we still have some context in our heads, so we'd like to help out in getting it through however possible.) |
|
You may have to mark the option class with the IHaveMetadata interface. Thanks for taking the time to learn everything, this looks really great, fwiw. 👍 |
|
@brandur-stripe The main issue is that the account used to test the changes isn't opted into the feature. I think an obvious next step is to do that and then iterate on whatever problems come up. |
|
|
||
| // Sleep for 5 seconds to ensure the Source is chargeable | ||
| // 1 or 2 seconds are unfortunately not enough. | ||
| System.Threading.Thread.Sleep(5000); |
There was a problem hiding this comment.
This is awful, but I can't see another way for now 😢
There was a problem hiding this comment.
Fuuu :/ Yeah, I don't know of one either.
|
|
||
| [JsonProperty("expected_availability_date")] | ||
| [JsonConverter(typeof(StripeDateTimeConverter))] | ||
| public DateTime? ExpectedAvailabilityDate { get; set; } |
There was a problem hiding this comment.
Weirdly this value is null in Test so I had to make it optional with the ?. I feel like we should consider making all properties always optional to be resilient to changes on Stripe's end.
There was a problem hiding this comment.
I think what you have here is correct given that it's also marked optional in the backend too.
I'm not too sure that we should go around applying this too liberally because the ? does require a little extra code to use for the user (making it a little less sound ergonomically).
On that note though. I was cross-referencing this with the API resource and noticed these fields are also optional:
BalanceTransactionIdDescriptionFailureCodeFailureMessageStatementDescriptor
Should we add ? for them as well? These are all strings so I think the answer is "no" when comparing to other optional properties throughout the library, but it seems a little strange that nullable is only used for certain types.
There was a problem hiding this comment.
Yeah I think for strings it's fine. But it'd be great to figure out a better way than throwing the ? when a test breaks because it means we never fully test the edge-cases
| public Dictionary<string, string> Metadata { get; set; } | ||
|
|
||
| [JsonProperty("source")] | ||
| public string SourceId { get; set; } |
There was a problem hiding this comment.
I named this SourceId because it has to be a real Source id src_XXX and can't be just a token. I'm ambivalent on whether Source is a better name but worry that one day we'll turn that into a "string or hash" version again. What do you think?
If so, we likely want to match that on the StripeTopup resource.
There was a problem hiding this comment.
SourceId works for me — there's probably some inconsistency throughout, but it matches well with the expandable conventions that we have in here (e.g., BalanceTransationId).
|
Left one comment above, but looks good. Thanks for taking this over Remi. |
|
Answered the comment so I think for now we're good. The test failure is also unrelated (and that test works locally for me, I ran it a few times). Should we remove the tests for that resource while it's gated or are we okay with other devs not being able to run the full test suite? |
Don't worry about it if you don't know the answer right away, but do you know if those test failures are just intermittent? The only thing is that we're going to need a successful build to get a released shipped.
It's far from perfect, but IMO it's probably okay — when I run the suite I often just copy the test key from the project anyway. What do you think? |
Not sure, I had never seen those before and the Plan resource has not changed in a while so I don't get what's happening. I could not find a "rebuild" button unfortunately to force it to trigger again.
Works for me. |
Kk thanks. Your other branch worked, so I think it's probably intermittent. (And FWIW, also can't find a rebuild button, but some documentation suggests that there should be one. I suspect there might be a permissions issue in here somewhere.) |
brandur-stripe
changed the title
|
Released as 16.2.0. |
|
Gah. Remi, this is somewhat annoying, but I'm noticing quite a few build failures after pulling this one in. I'd say roughly one in two or so are failing. The reason for the failures is the non-determinism around whether a source becomes chargeable in time — apparently waiting five seconds isn't always enough. Any ideas around the best way to fix this? Should we just do a longer sleep, or is there some other improvement that we should be pushing for somewhere? Thanks. |
|
@brandur-stripe IMO we should simply disable the tests for that resource until we move to stripe-mock. I will open a separate PR for this. |
|
Pushed #1192 Let me know what you think! |
|
Yep, thanks Remi. That makes sense to me. |
This add standard retrieve, create and update client support for the new
/v1/topupsendpoint.