stripe-scala
stripe-scala copied to clipboard
mdedetrich
Fix `Plan` and `InputPlan` fields
This resolves #40
WIP: I still need to add unit and integration tests, but this implements the basic approach.
It seems like the integration tests were failing before this PR. Was someone else already investigating that? https://travis-ci.org/mdedetrich/stripe-scala/jobs/355675799
Ugh. I just realized the fixes I'm making here are only necessary because of a very recent Major change in the stripe API: https://stripe.com/docs/upgrades#2018-02-05
I suspect I'm seeing the integration tests on master fail for a similar reason.
@mdedetrich What version is the Stripe test account in CI tied to? Is it configurable?
Ok, so the integration tests don't work on pull requests because the secret API token is required.
If you want to run them, you need a set the env variable STRIPE_TOKEN. If you don't want create a project yourself, you can contact me and I will send you the token.
Regarding the test project version: it is set to a version from 2016. This could be changed but ideally we should be setting the Stripe-Version header with every request. I wanted to implement this (see #21) but never got around to it.
Why do we need this? You can set a project version in Stripe itself but can override that for every request. To me, hardcoding the version stripe-scala expects would be the only sane way forward.
See https://stripe.com/docs/upgrades
If you want to fix this problem too, @stewSquared, it would be most appreciated.
@leonardehrenfried I do have a Stripe API key that I'm using locally. It fails for me on master, so I'm guessing my key is for a newer version of the API (I get Missing required param: type.). Would you mind sending the test key you use to my email? [email protected]
As for the Stripe-Version header, I totally agree. Setting the version explicitly is the sane way to go, especially for a library. How about I submit an implementation that sets it via an implicit StripeVersion the same way we handle ApiKey and EndPoint? We could put a default in Config that corresponds to whatever the library is written against.
On that note, should this library perhaps have branches for breaking changes like this? This is still a fairly recent change from Stripe.
Creating an implicit version is a great idea although I don't think it needs to be too configurable since there isn't an easy way to switch between the version. The expected properties aren't so easily changed in the code.
If we had lots of people working on the different branches I would agree, but as it is it would make an already thinly-"staffed" project harder to maintain.
I believe it's more practical if we just add table to the readme that says that version X of stripe-scala is hardcoded to use version Z of the Stripe API. People can change the version config but it's likely that this will not work.
Or are you interested in maintaining and updating these branches?
Right, a table in the README make much more sense. Someone could always fork off of an older version if they need to patch it.
I've sent you the API key. I've also checked the default version of the test project and it's 2016-10-19.
I suggest you set the hardcoded version to something very recent. In that case a few integration tests will fail (I believe there have been some renamings in the area of transfers, which are now called payouts). But that shouldn't be very arduous to clean up as Stripe doesn't change stuff that often.
If you need a hand, I'm happy to help.
Maybe the solution to the failing integration tests would be to not run them for PRs. They can't work as they don't have an API key.
Another alternative would be to just publish the API key - it's a dummy project with no real data anyway, but I'm still a bit nervous about that option.
WDYT?
Hmm. Publishing even the test key makes me nervous too. How about setting it in travis as an encryted environmental variable in travis? https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings
It is an encrypted variable but that doesn't work for pull requests since that would make it essentially public. Anyone could trivially create a PR that reads and prints the secret value.
Oh, right! Good point. I suppose we don't run integration tests for PRs then. It's not too much hassle to run the tests locally before merging, is it?
I just confirmed this PR passes (none of the tests touch plans/subscriptions)
Are you planning on adding a few tests to this PR, unit or integration ones?
We don't get that many contributions so running them before merging isn't going to be too annoying.
Oh! I still have some tests in my index to clean up and commit. Just caught up in other tasks for the moment!
On Mar 22, 2018 7:00 PM, "Matthew de Detrich" [email protected] wrote:
Is everything good for the PR, or?
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/mdedetrich/stripe-scala/pull/41#issuecomment-375515385, or mute the thread https://github.com/notifications/unsubscribe-auth/AD1iHd_loHtY1hMrg1rdi_W-idg2Vjthks5thFcygaJpZM4SxNoz .
@stewSquared I would rebase against master as another pr was just merged there
@mdedetrich Sure thing! I have a bit more time now, so I'll dig back in to this PR tomorrow.
After rebasing on #43, I get sbt dependency resolution errors:
[error] (*:update) sbt.ResolveException: unresolved dependency: com.thoughtworks.sbt-api-mappings#sbt-api-mappings;2.0.0: not found
Can you push your state so I can take a look? Also Travis will help me debug.
Stewart Stewart [email protected] schrieb am Di., 3. Apr. 2018, 08:19:
After the update, I get sbt dependency resolution errors:
[error] (*:update) sbt.ResolveException: unresolved dependency: com.thoughtworks.sbt-api-mappings#sbt-api-mappings;2.0.0: not found
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mdedetrich/stripe-scala/pull/41#issuecomment-378140388, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJPMltuYuDZw1QtcXGOFeYTZjZmUUW_ks5tkxRsgaJpZM4SxNoz .
@leonardehrenfried I did push my state after rebasing. But, as it happens I must have just been seeing anything temporary. The build loads fine now.
The scala 2.11 build fails because of some missing Either-related compatibility code.
Can you import cats.syntax.either._?
Oh, I didn't notice the cross build. I'll fix it for 2.11 in a moment.
Thanks for being patient with this PR. I've blocked out a calendar slot so I can commit to finishing touches on this PR.
This is currently wip, @stewSquared needs to give us the green light when he's ready.
@mdedetrich Hi! I almost forgot about this. I'm using Stripe again, so I'll do some testing and update this ticket. It might need a rebase or a new PR
@stewSquared I have just updated master with some QoL fixes (i.e. applying new scalafmt, making case class final as well as using the Some constructor) so you will probably have to rebase your branch, I don't think the conflicts should be too bad (just remember to use scalafmt before committing)