Fix Plan and InputPlan fields
#41
Conversation
| x.statementDescriptor, | ||
| x.trialPeriodDays)) | ||
|
|
||
| sealed abstract class Product |
There was a problem hiding this comment.
From the documentation:
product: The product whose pricing the created plan will represent. This can either be the ID of an existing product, or a dictionary containing fields used to create a service product.
The names for the two sub classes, ProductId and ServiceProduct from the names of the two italicized phrases, but I'm happy to shorten them if that would be more consistent with the rest of the names.
| * @param intervalCount The number of intervals (specified in the [[interval]] | ||
| * property) between each subscription billing. For example, | ||
| * \[[interval]]=[[Interval.Month]] and [[intervalCount]]=3 | ||
| * bills every 3 months. | ||
| * @param livemode | ||
| * @param name Display name of the plan |
There was a problem hiding this comment.
There was no name parameter for plan, but it's child object had a name parameter for the service product.
stewSquared
force-pushed
the
fix-plan-parameters
branch
from
f2e4b60 to
fea9039
Compare
| "statement_descriptor" -> service.statementDescriptor | ||
| ) | ||
| ) | ||
| mapToPostParams(Option(params ++ mapToPostParams(service.metadata, "metadata")), "product") |
There was a problem hiding this comment.
This was a little bit tricky to work out.
Akka HTP already includes a FormData class that seems relevant here. Might that make handling all these nested maps a bit simpler?
There was a problem hiding this comment.
Yeah, we used to use unfiltered and switch to akka http that's why we are not using it.
Personally, I'm not particularly taken with akka http so I would not want to increase our dependency on it.
|
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 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 As for the 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 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. |
|
Is everything good for the PR, or? |
|
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
<#41 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD1iHd_loHtY1hMrg1rdi_W-idg2Vjthks5thFcygaJpZM4SxNoz>
.
|
|
No worries, just ping us when you are done |
|
@stewSquared I would rebase against master as another pr was just merged there |
|
@mdedetrich Sure thing! |
stewSquared
force-pushed
the
fix-plan-parameters
branch
from
fea9039 to
b6334a3
Compare
|
After rebasing on #43, I get sbt dependency resolution errors: |
|
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
<#41 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJPMltuYuDZw1QtcXGOFeYTZjZmUUW_ks5tkxRsgaJpZM4SxNoz>
.
|
|
@leonardehrenfried I did push my state after rebasing. |
|
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. |
stewSquared
force-pushed
the
fix-plan-parameters
branch
from
b6334a3 to
29dd366
Compare
|
Any chance this will merge soon? |
|
This is currently wip, @stewSquared needs to give us the green light when he's ready. |
|
@stewSquared Any update on this? |
|
@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 No worries, it might need a big rebase though |
|
@stewSquared I have just updated master with some QoL fixes (i.e. applying new scalafmt, making |
This resolves #40
WIP: I still need to add unit and integration tests, but this implements the basic approach.