Adds handling of recurring payment #914
Conversation
Follows the recommodation as described at: https://stripe.com/docs/billing/subscriptions/payment
previously failed due to failed payment
kmmatwork
force-pushed
the
feature/payment-recurring
branch
from
b8d70dc to
dceb366
Compare
acceptance-tests/Dockerfile.ngrok
Outdated
| @@ -0,0 +1,18 @@ | |||
| FROM azul/zulu-openjdk:13 | |||
There was a problem hiding this comment.
Today, I have bumped this version to 13.0.1 which is already merged to develop.
| * skipped altogether. | ||
| */ | ||
| @Test | ||
| @EnabledIfEnvironmentVariable(named = "NGROK_AUTH_TOKEN", matches="\\S+") |
There was a problem hiding this comment.
Do you want these tests to be run on CI? Should we add this ENV var to CI setup?
There was a problem hiding this comment.
Yes, but we will first have to create an account for CI with ngrok.io first. And maybe consider a baslc or pro subscription? Being able to use "custom subdomain" would simplify the setup a bit...
| @@ -297,26 +291,26 @@ enum class ProductProperties(val s: String) { | |||
|
|
|||
| enum class ProductClass { | |||
| SIMPLE_DATA, | |||
| SUBSCRIPTION, | |||
There was a problem hiding this comment.
Subscription is not a Product class. Instead, it is a payment type.
(Product Class = Membership + Payment type = Subscription) => Our Membership project which we have removed.
(Product Class = Simple Data + Payment type = Subscription) => Product which will topup x bytes evey y period.
(Product Class = Data with Validity (coming soon) + Payment type = Subscription) => We will have this.
There was a problem hiding this comment.
Maybe it could be an idea to rewrite the "payment paths" in the code to only consider the payment[type] field and not consider the productClass field at all? And maybe make the payment[type] field mandatory?
There was a problem hiding this comment.
Don't know yet how straight forward this will be though. Will have to check first...
| price = Price(amount = 10_00, currency = "USD"), | ||
| properties = mapOf( | ||
| PRODUCT_CLASS.s to SUBSCRIPTION.name, |
There was a problem hiding this comment.
Similar to another comment, product class should be MEMBERSHIP.
Payment type is SUBSCRIPTION.
If no payment type is set, then the default value is ONE TIME PAYMENT.
| PRODUCT_CLASS.s to SUBSCRIPTION.name, | ||
| SEGMENT_IDS.s to "country-us"), | ||
| payment = mapOf( |
There was a problem hiding this comment.
The payment type = subscription has to be set in payment properties.
| .properties( | ||
| mapOf( | ||
| "productClass" to "SUBSCRIPTION", | ||
| "segmentIds" to "country-us" | ||
| ) | ||
| ) | ||
| .payment( | ||
| mapOf( | ||
| "type" to "SUBSCRIPTION", | ||
| "label" to "Daily subscription plan", | ||
| "taxRegionId" to "us" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Here, the payment type is set correctly to "SUBSCRIPTION", but the product class should be MEMBERSHIP.
| @POST | ||
| @Path("{sku}/renew") | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| fun renewSubscription(@Auth token: AccessTokenPrincipal?, |
There was a problem hiding this comment.
Just the word Subscription means mobile Subscription (with MSISDN as ID). Better to call this PaymentSubscription or PlanSubscription to distinguish it from "Mobile" Subscription.
| NotificationType.SUBSCRIPTION_RENEWAL_STARTING -> { | ||
| /* No notification, as a notification will be sent on either successful | ||
| or failed renewal. */ | ||
| } | ||
| } |
There was a problem hiding this comment.
This API is a bit messed up. enum NotificationType can have fields such as title and body which is then directly used instead of using when cases. Also, if FCMStrings is specific to Firebase, they should not be in generic API interfaces - prime-modules.
The simplest way to design it is to think of AppNotifier as an interface whereas FirebaseAppNotifier as one of the many possible implementations of AppNotifier.
We should be able to replace whole FCM with some other implementation with just adding and removing dependency in the gradle build file and no need of any code change.
There was a problem hiding this comment.
| val params = mapOf( | ||
| *(if (sourceId != null) | ||
| arrayOf("source" to sourceId) | ||
| else arrayOf()) | ||
| ) |
There was a problem hiding this comment.
Outer if-else with mapOf inside is more readable.
Also, ?. is more compact way to check nulls.
|
Fyi, more changes to how plans/products gets stored will follow later as part of the |
distinguish them from functions related to mobile subscriptions
…feature/payment-recurring
| ## | ||
|
|
||
| echo "Creating pub-sub subscriptions for recurring payment tests" | ||
| curl -X PUT -H "Content-Type: application/json" -d '{"topic":"projects/'${GCP_PROJECT_ID}'/topics/stripe-event","ackDeadlineSeconds":10}' pubsub-emulator:8085/v1/projects/${GCP_PROJECT_ID}/subscriptions/stripe-event-okhttp-purchase-ok-sub |
There was a problem hiding this comment.
This is so much repeated code that it now arguably would make sense to factor this out into a function. Or not. Your call.
There was a problem hiding this comment.
Will try to move the curl call into a function.
| subscription: String, | ||
| customerId: String, | ||
| timeout: Long = 10000L): Boolean = | ||
| waitForStripeEvent( |
There was a problem hiding this comment.
This code has very convoluted control flow. Consider breaking it up into smaller pieces with descriptive names.
There was a problem hiding this comment.
Will try to move the cmp function out out.
| delay(200L) | ||
| } | ||
| "Done" | ||
| } != null |
There was a problem hiding this comment.
This makes it very obscure what we're checking for. Please consider making a local function with a descriptive name. Code should preferably be trivial to read, and this isn't.
There was a problem hiding this comment.
Basically cut and paste from https://kotlinlang.org/docs/reference/coroutines/cancellation-and-timeouts.html at the bottom of the page.
| ), | ||
| "email" to "[email protected]") | ||
| ) | ||
| val source = Source.create(sourceMap) |
There was a problem hiding this comment.
I would have preferred calling this paymentSource, just to make it blindingly obvious to casual readers (such as myself, in the role of reviewer) what this value is.
| queryParams = mapOf("sourceId" to sourceId) | ||
| } | ||
|
|
||
| Thread.sleep(200) // wait for 200 ms for balance to be updated in db |
There was a problem hiding this comment.
This is brittle. Is there a less brittle way to wait for the database?
There was a problem hiding this comment.
Not sure. The same wait statement is present in all acceptance tests that involves updates to neo4j. Will add a TODO and a Notion tech dept task.
| // expires, which will happen after 4 sec. Waiting a bit longer | ||
| // before checking the outcome. | ||
| StripeEventListener.waitForSubscriptionPaymentToSucceed( | ||
| subscription = "stripe-event-jersey-purchase-ok-sub", |
There was a problem hiding this comment.
What does this magic string mean? Consider putting it into a variable with a descriptive name.
There was a problem hiding this comment.
Name of a subscription to the "stripe-event" pub-sub topic. We need one such subscription for each "recurring payment" acceptance test. The waitForSubscriptionPaymentToSucceed method is there to ensure that we don't wait too long for the expected events.
Not sure if it make sense use a variable for the subscription name. But some more comments about what is going on could be an idea.
| customerId = createCustomer(name = "Test Purchase Plan User", email = email).id | ||
| enableRegion(email = email, region = "us") | ||
|
|
||
| val client = clientForSubject(subject = email) |
There was a problem hiding this comment.
There is a lot of repeated code between this and the previous test. Consider factoring that code into separate methods or perhaps even running the code in a a "setUp" method as part off fixture setup for the test suite.
Repeated code obscures the intent and implementation of the actual test.
There was a problem hiding this comment.
Will try to restructure a bit.
| // Actual charge for renewal will first be done after trial time | ||
| // expires, which will happen after 4 sec. Waiting a bit longer | ||
| // before checking the outcome. | ||
| StripeEventListener.waitForFailedSubscriptionRenewal( |
| // Explicitly renew the subscription with a new card. | ||
| client.renewSubscription(sku, newSourceId, true) | ||
|
|
||
| StripeEventListener.waitForSubscriptionPaymentToSucceed( |
| sourceId: String?, | ||
| @QueryParam("saveCard") | ||
| saveCard: Boolean?): Response = | ||
| if (token == null) { |
There was a problem hiding this comment.
Can authorisation check be delegated to a filter so that it does not show up in the business logic part of the resource implementation? Technically it obviously can, but the question is obviously if it makes sense in this particular situation. If the "token == null" must be there, add a comment describing why this couldn't be delegated to a filter, of if it can, at the very least add a "TODO" indicating that it should.
There was a problem hiding this comment.
Agree with this. I think the same pattern is used in all client facing APIs, and an update should probably fix it for all of them. Will leave a TODO and a Notion tech dept task for fixing this.
| if (sourceId == null) { | ||
| dao.renewPaymentSubscription( | ||
| identity = token.identity, | ||
| sku = sku) |
There was a problem hiding this comment.
In this case the saveCard parameter is discarded? If it's non-null that should indicate an error, or it should be used.
| dao.renewPaymentSubscription( | ||
| identity = token.identity, | ||
| sku = sku, | ||
| sourceId = sourceId, |
There was a problem hiding this comment.
What is the default value of sourceId? If it's null, then perhaps only the "else" branch is necessary and the code can be made more compact without losing generality?
There was a problem hiding this comment.
Yes, this API needs some rethinking.
| val invoiceId: String, /* Not set on plans with trial time. */ | ||
| val chargeId: String? = null, /* Only set on successful payment on creation/renewal. */ | ||
| val created: Long, | ||
| val currentPeriodStart: Long, |
There was a problem hiding this comment.
Add a comment indicating what the times should be interpreted as? Is it seconds since epoch? If so, say so.
There was a problem hiding this comment.
Internally timestamps are always in milliseconds since epoch.
| description: "Successfully renewed the subscription" | ||
| schema: | ||
| $ref: '#/definitions/Product' | ||
| 404: |
There was a problem hiding this comment.
Wasn't there a 402 in there somewhere also?
| either("Failed to complete payment of invoice ${invoiceId}") { | ||
| val receipt = Invoice.retrieve(invoiceId).pay() | ||
| InvoicePaymentInfo(receipt.id, receipt?.charge ?: UUID.randomUUID().toString()) | ||
| val params = if (sourceId != null) |
There was a problem hiding this comment.
Please wrap if in parens. I get confused by this kind of implicitly bounded values. Be explicit.
There was a problem hiding this comment.
Agree with that.
|
|
||
| try { | ||
| runBlocking { | ||
| status = withTimeoutOrNull(timeout) { |
There was a problem hiding this comment.
If this is just a busy-wait loop, then say so in a comment. If not then explain what's going on.
| } | ||
| }) | ||
|
|
||
| fun waitForFailedSubscriptionRenewal(topic: String = StripeEventListener.topic, |
There was a problem hiding this comment.
The control flow of this method is very obscure. Please make it simpler to read by breaking it up into smaller methods with descriptive names rather than one big open-coded method with a very complex internal life.
…elop using github UI
in the recurring payment tests
No description provided.