Fix StripeConnectAccount #596
Conversation
|
I did some long overdue fixes here, which we needed due to some of the changes the PR contains
Note on replacing strategyThe strategy had several problems. The first one is that it uses the associated record's default view in order to serialize. The problem there is that it wrongly assumes that the attributes we send in a This is wrong for several records now. Additionally, it adds all attributes defined by the view to the payload. A |
|
Started cleanup of model tests Added two new helpers, |
| @@ -93,17 +125,31 @@ defmodule CodeCorps.StripeService.Adapters.StripeConnectAccountAdapter do | |||
| |> Map.take(@non_stripe_attributes) | |||
| end | |||
|
|
|||
| defp remove_read_only_values(attributes, [head | tail]) do | |||
| remove_read_only_values(attributes, head) | |||
| remove_read_only_values(attributes, tail) | |||
There was a problem hiding this comment.
How does this work? Seems to me like it only returns the result from removing read only values from the final item in the list (after it recursively gets through the tail). Are we missing some chaining here?
There was a problem hiding this comment.
I think we should remove this altogether, actually.
| def transform_attributes(attributes, mapping) do | ||
| attributes_map = map_keys_to_atoms(attributes) | ||
| mapping |> Enum.reduce(%{}, &map_attribute(&1, &2, attributes_map)) | ||
| end |
There was a problem hiding this comment.
Might not be a bad Idea to have a unit test for this specifically. Separate issue might be good enough for now, though.
There was a problem hiding this comment.
👍 for separate issue.
| %{ | ||
| amount: 1, # in cents | ||
| currency: "usd", | ||
| id: "month", | ||
| id: "month_project_" <> to_string(project_id), |
There was a problem hiding this comment.
Did you mean month_plan_ here or is _project_ intentional?
There was a problem hiding this comment.
@begedin project is intentional since plans are per-project.
| @@ -62,6 +62,36 @@ defmodule CodeCorps.StripeService.Adapters.StripeConnectAccountAdapter do | |||
| {:verification_fields_needed, [:verification, :fields_needed]} | |||
| ] | |||
|
|
|||
| @stripe_update_read_only [ | |||
There was a problem hiding this comment.
Is this still used for something? I tried searching for it through the file but got nothing.
| else | ||
| {:error, %Ecto.Changeset{} = changeset} -> {:error, changeset} | ||
| {:error, %Stripe.APIErrorResponse{} = error} -> {:error, error} | ||
| _ -> {:error, :unhandled} |
There was a problem hiding this comment.
We will likely want to sweep through the code at some point and remove these _ -> unhandled cases. They basically swallow any unexpected errors in the process. We should explicitly list any failures we support and handle. If anything unexpected fails, we should treat it as a bug.
This is basically equivalent with us wrapping a huge chunk of code into a try-catch that catches any sort of exception.
Update view Modify MapUtils.keys_to_string to do a deep transform Add update policy for connect account Replace json_payload strategy with more accurate build_json_payload helper Fix model tests Update Stripe Add necessary sweet_xml for arc
What's in this PR?
Attempt to fix StripeConnectAccount's update fn to allow for updating.