Conversation
|
|
||
| defp do_process_async(json, handler) do | ||
| Task.Supervisor.start_child(:webhook_processor, fn -> do_process(json, handler) end) | ||
| def process_async(%{"id" => id, "livemode" => livemode} = json, handler) do |
There was a problem hiding this comment.
It looks like you can consolidate this def with the one on line 32 unless I'm missing something:
def process_async(%{"id" => id, "livemode" => livemode} = json, handler)
case event_matches_environment?(livemode)
true -> do_process_async(id, json["user_id"], handler, json)
false -> {:error, :ignored_by_environment}
end
endThere was a problem hiding this comment.
@rossta the one on line 32 is doing pattern matching against the user_id key, so on platform events the signature will be off.
There was a problem hiding this comment.
Yah, but isn't it just passing the user_id value into the same place where it would be nil in the other example? So I thought json["user_id"] as the second param to the true case would handle both implementations but I could be wrong.
There was a problem hiding this comment.
@rossta yeah, I'm not sure how I feel about it. On the one hand I like the explicitness of saying that there are two different ways we handle this, but I suppose it's the same outcome.
There was a problem hiding this comment.
I did miss your code example somehow earlier so just read "consolidate" somehow.
There was a problem hiding this comment.
I get what you mean about being explicit. In that case, I'm usually inclined to extract an intention-revealing variable or method to help highlight the difference. That may not be the best way to handle it as I'm still learning my way around the conventions in the codebase.
There was a problem hiding this comment.
It's possible that this interface should be redesigned to have two public fn's, one for processing platform and one for connect events, and then the controller does some pattern matching against user_id. I believe that every connect event should have a user_id, but I could be wrong.
|
|
||
| defp find_or_create_event(id_from_stripe, type) do | ||
| defp event_matches_environment?(livemode) do | ||
| case Application.get_env(:code_corps, :stripe_env) do |
There was a problem hiding this comment.
The livemode value is just true/false, no? How about just the following as the method body?
livemode && Application.get_env(:code_corps, :stripe_env) == :prod
There was a problem hiding this comment.
@rossta we still want to be able to process events in staging, so we would never handle events with this situation.
There was a problem hiding this comment.
I tried thinking about how to improve this because it reads strange, but think it's isolated enough to not make a difference, and is kind of addressed in the moduledoc.
|
|
||
| defp set_processed(%StripeEvent{} = event) do | ||
| event |> StripeEvent.update_changeset(%{status: "processed"}) |> Repo.update | ||
| end |
There was a problem hiding this comment.
I often struggle with the question of whether or not to introduce another helper method for helper methods like set_(errored|processed|unhandled) when all three have similar implementation. However, another layer of indirection isn't always helpful and I think it's fine the way you have it here with already-small methods.
There was a problem hiding this comment.
Yeah, I wasn't sure about this, hemmed and hawed, but ultimately this is probably okay. Was thinking we could just have done set_status/2 with a string as a second argument.
|
@joshsmith Nice work! I made a few comments on little stuff. Probably warrants a more-thorough review from someone who knows the codebase better than me. |
| defp retrieve_event_from_api(id, user_id), do: @api.Event.retrieve(id, connect_account: user_id) | ||
|
|
||
| defp create_event(id_from_stripe, endpoint, type, user_id) do | ||
| %StripeEvent{} |> StripeEvent.create_changeset(%{endpoint: endpoint, id_from_stripe: id_from_stripe, type: type, user_id: user_id}) |> Repo.insert |
There was a problem hiding this comment.
Do we follow any conventions on when to split out a pipe to a new line?
There was a problem hiding this comment.
We nominally use credo but don't have it enabled on the repo and honestly haven't run it in CLI in awhile.
|
I'm going to refactor this to make a single public API that pattern matches against the handlers. I think given that there are two endpoints, it makes sense to split the API here to respond to the two handlers for those endpoints. At the controller level, we can pattern match against Think this will be a good refactoring, but will do it as a separate commit in case y'all disagree. |
|
Decided not to refactor. If needed later, I'll circle back. For now, that's an optimization. |
3 participants
What's in this PR?
Adds everything in #550 that we need, namely retrieving both connect and platform events from Stripe before processing.
I believe there are some opportunities for refactoring, so would really love advice on that.
References
Fixes #550