Handle customer.subscription.deleted webhook - #513
Conversation
sbatson5
left a comment
sbatson5
left a comment
There was a problem hiding this comment.
Just a couple of questions and comments. This is looking good 👍
| {:ok, subscription} | ||
| else | ||
| {:error, :project_not_ready} -> {:error, :project_not_ready} | ||
| {:error, :user_not_ready} -> {:error, :user_not_ready} |
There was a problem hiding this comment.
It seems that all these cases return what we are matching against. What are your thoughts on just matching on nil and then returning everything else.
nil -> {:error, :not_found}
error_tuple -> error_tuple
There was a problem hiding this comment.
I'd like to keep it explicit because I'm expecting we might be doing custom handling/errors at some point. I can move that into a comment above the line, though.
| defp to_create_attributes(%StripeConnectCard{} = card, %StripeConnectCustomer{} = customer, %StripeConnectPlan{} = plan, quantity) do | ||
| defp to_create_attributes( | ||
| %StripeConnectCard{} = card, %StripeConnectCustomer{} = customer, | ||
| %StripeConnectPlan{} = plan, %{"quantity" => quantity} |
There was a problem hiding this comment.
Do we need to pattern match here for StripeConnectCard and StripeConnectPlan etc? This is the only method we have by this name, so the pattern matching seems unnecessary when we could simply do:
defp to_create_attributes(card, customer, plan, %{"quantity" => quantity)
There was a problem hiding this comment.
Good catch. The validate call at the start of the process actually does all the pattern matching required.
| defp retrieve_connect_account(connect_customer_id) do | ||
| customer = StripeConnectCustomer | ||
| |> Repo.get_by(id_from_stripe: connect_customer_id) | ||
| |> Repo.preload(:stripe_connect_account) |
|
|
||
| defp load_subscription(id_from_stripe) do | ||
| subscription = StripeConnectSubscription | ||
| |> Repo.get_by(id_from_stripe: id_from_stripe) |
There was a problem hiding this comment.
Same as above. I believe the style is to indent when we are assigning
| updated_account = | ||
| StripeConnectAccount | ||
| |> Repo.get_by(id_from_stripe: stripe_id) | ||
| updated_account = StripeConnectAccount |> Repo.get_by(id_from_stripe: stripe_id) |
There was a problem hiding this comment.
Thoughts on not piping here? updated_account = Repo.get_by(StripeConnectAccount, id_from_stripe: stripe_id)
|
|
||
| conn = post conn, stripe_connect_events_path(conn, :create), event | ||
|
|
||
| assert response(conn, 200) |
There was a problem hiding this comment.
Thoughts on piping this block?
assert conn |> post(stripe_connect_events_path(conn, :create), event) |> response(200)
joshsmith
added
needs submitter response
ready to merge
and removed
needs code review
labels
|
@begedin feel free to merge when you're comfortable making any changes you'd like in response to code review comments. Don't await another review. |
a88bb45 to
259de3b
Compare
…able, by adding validators
259de3b to
cc9313d
Compare
cc9313d to
c0e2dc7
Compare
begedin
changed the title
3 participants
What's in this PR?
This PR adds handling of the
customer.subscription.deletedevent.Since the actual subscription stripe record is not deleted, instead simply removed from the customer account, and it's status set to cancelled, the solution was to simply handle it identically to
customer.subscription.updated.This raises a question, though.
Should our API, when requesting a user's subscriptions, only return those that are active, or all of them?
If the answer is all of them, then we need to modify our client logic so that the donate UI is hidden/redirected from only if there is an active subscription, not just any subscription as it is right now.
If the answer is only those that are active, then we need to modify our user/project views.
Implementation
The logical place to put the update code was
CodeCorps.StripeService.StripeConnectSubscriptionService.However, I found the code there difficult to understand and manage, so adding additional code there would make it even worse.
In an attempt to remedy this, I refactored the module:
I rewrote a few methods
I added validator modules -
ProjectSubscribableandUserCanSubscribe.These both have a
validate/1method, which receives aProjectorUserrecord respectively. It either returns back an{:ok, record}or an{:error, :project_not_ready}/{:error, :user_not_ready}if the two records do not have the necessary stripe relationships set up.The idea is that these validators are a simple blackbox that determines if a user or a project is able to participate in the donation system. It allows us to have less checks in the subscription creation process reduces the required size of method signatures there, making the code more readable.
Once that was in, I cleaned up further and then moved the logic from
Events.CustomerSubscriptionUpdatedintoStripeConnectSubscriptionService.update_from_stripe/2. This same method is then used byEvents.CustomerSubscriptionDeleted.References
Fixes #509
Progress on: #