From f2d6e4b99a51e59bf130c2d7d8d00bab89659413 Mon Sep 17 00:00:00 2001 From: zafer Date: Tue, 25 Feb 2025 00:41:50 +0300 Subject: [PATCH] make invoice payments idempotent --- lib/algora/bounties/bounties.ex | 33 ++++--- lib/algora/contracts/contracts.ex | 27 +++--- lib/algora/payments/schemas/transaction.ex | 5 +- lib/algora/psp/psp.ex | 89 +++++++++++++++---- .../controllers/webhooks/github_controller.ex | 17 ++-- ...917_add_idempotency_key_to_transaction.exs | 11 +++ test/algora/bounties_test.exs | 14 +-- .../webhooks/github_controller_test.exs | 53 +++++++++++ test/support/stripe_mock.ex | 17 ++-- 9 files changed, 208 insertions(+), 58 deletions(-) create mode 100644 priv/repo/migrations/20250224211917_add_idempotency_key_to_transaction.exs diff --git a/lib/algora/bounties/bounties.ex b/lib/algora/bounties/bounties.ex index a141d343d..0bd87892a 100644 --- a/lib/algora/bounties/bounties.ex +++ b/lib/algora/bounties/bounties.ex @@ -616,7 +616,8 @@ defmodule Algora.Bounties do net_amount: amount, total_fee: Money.sub!(gross_amount, amount), line_items: line_items, - group_id: tx_group_id + group_id: tx_group_id, + idempotency_key: "session-#{Nanoid.generate()}" }), {:ok, _transactions} <- create_transaction_pairs(%{ @@ -641,7 +642,7 @@ defmodule Algora.Bounties do end @spec create_invoice( - %{owner: User.t(), amount: Money.t()}, + %{owner: User.t(), amount: Money.t(), idempotency_key: String.t()}, opts :: [ ticket_ref: %{owner: String.t(), repo: String.t(), number: integer()}, tip_id: String.t(), @@ -651,7 +652,7 @@ defmodule Algora.Bounties do ] ) :: {:ok, PSP.invoice()} | {:error, atom()} - def create_invoice(%{owner: owner, amount: amount}, opts \\ []) do + def create_invoice(%{owner: owner, amount: amount, idempotency_key: idempotency_key}, opts \\ []) do tx_group_id = Nanoid.generate() line_items = @@ -672,7 +673,8 @@ defmodule Algora.Bounties do net_amount: amount, total_fee: Money.sub!(gross_amount, amount), line_items: line_items, - group_id: tx_group_id + group_id: tx_group_id, + idempotency_key: idempotency_key }), {:ok, _transactions} <- create_transaction_pairs(%{ @@ -686,15 +688,19 @@ defmodule Algora.Bounties do }), {:ok, customer} <- Payments.fetch_or_create_customer(owner), {:ok, invoice} <- - PSP.Invoice.create(%{ - auto_advance: false, - customer: customer.provider_id - }), + PSP.Invoice.create( + %{ + auto_advance: false, + customer: customer.provider_id + }, + %{idempotency_key: idempotency_key} + ), {:ok, _line_items} <- line_items |> Enum.map(&LineItem.to_invoice_item(&1, invoice, customer)) - |> Enum.reduce_while({:ok, []}, fn params, {:ok, acc} -> - case PSP.Invoiceitem.create(params) do + |> Enum.with_index() + |> Enum.reduce_while({:ok, []}, fn {params, index}, {:ok, acc} -> + case PSP.Invoiceitem.create(params, %{idempotency_key: "#{idempotency_key}-#{index}"}) do {:ok, item} -> {:cont, {:ok, [item | acc]}} {:error, error} -> {:halt, {:error, error}} end @@ -711,7 +717,8 @@ defmodule Algora.Bounties do net_amount: net_amount, total_fee: total_fee, line_items: line_items, - group_id: group_id + group_id: group_id, + idempotency_key: idempotency_key }) do %Transaction{} |> change(%{ @@ -724,12 +731,14 @@ defmodule Algora.Bounties do net_amount: net_amount, total_fee: total_fee, line_items: Util.normalize_struct(line_items), - group_id: group_id + group_id: group_id, + idempotency_key: idempotency_key }) |> Algora.Validations.validate_positive(:gross_amount) |> Algora.Validations.validate_positive(:net_amount) |> Algora.Validations.validate_positive(:total_fee) |> foreign_key_constraint(:user_id) + |> unique_constraint([:idempotency_key]) |> Repo.insert() end diff --git a/lib/algora/contracts/contracts.ex b/lib/algora/contracts/contracts.ex index d41af42b7..c1232ca62 100644 --- a/lib/algora/contracts/contracts.ex +++ b/lib/algora/contracts/contracts.ex @@ -395,7 +395,7 @@ defmodule Algora.Contracts do defp maybe_generate_invoice(contract, charge) do invoice_params = %{auto_advance: false, customer: contract.client.customer.provider_id} - with {:ok, invoice} <- Invoice.create(invoice_params), + with {:ok, invoice} <- Invoice.create(invoice_params, %{idempotency_key: "contract-#{contract.id}"}), {:ok, _line_items} <- create_line_items(contract, invoice, charge.line_items) do {:ok, invoice} end @@ -441,14 +441,19 @@ defmodule Algora.Contracts do end defp create_line_items(contract, invoice, line_items) do - Enum.reduce_while(line_items, {:ok, []}, fn line_item, {:ok, acc} -> - case Algora.PSP.Invoiceitem.create(%{ - invoice: invoice.id, - customer: contract.client.customer.provider_id, - amount: MoneyUtils.to_minor_units(line_item.amount), - currency: to_string(line_item.amount.currency), - description: line_item.description - }) do + line_items + |> Enum.with_index() + |> Enum.reduce_while({:ok, []}, fn {line_item, index}, {:ok, acc} -> + case Algora.PSP.Invoiceitem.create( + %{ + invoice: invoice.id, + customer: contract.client.customer.provider_id, + amount: MoneyUtils.to_minor_units(line_item.amount), + currency: to_string(line_item.amount.currency), + description: line_item.description + }, + %{idempotency_key: "contract-#{contract.id}-#{index}"} + ) do {:ok, item} -> {:cont, {:ok, [item | acc]}} {:error, error} -> {:halt, {:error, error}} end @@ -460,7 +465,9 @@ defmodule Algora.Contracts do defp maybe_pay_invoice(contract, invoice, txs) do pm_id = contract.client.customer.default_payment_method.provider_id - case Invoice.pay(invoice.id, %{off_session: true, payment_method: pm_id}) do + case Invoice.pay(invoice.id, %{off_session: true, payment_method: pm_id}, %{ + idempotency_key: "contract-#{contract.id}" + }) do {:ok, stripe_invoice} -> if stripe_invoice.paid, do: release_funds(contract, stripe_invoice, txs) {:ok, stripe_invoice} diff --git a/lib/algora/payments/schemas/transaction.ex b/lib/algora/payments/schemas/transaction.ex index 78effa04b..450b1f9ec 100644 --- a/lib/algora/payments/schemas/transaction.ex +++ b/lib/algora/payments/schemas/transaction.ex @@ -31,6 +31,7 @@ defmodule Algora.Payments.Transaction do field :succeeded_at, :utc_datetime_usec field :reversed_at, :utc_datetime_usec field :group_id, :string + field :idempotency_key, :string belongs_to :timesheet, Algora.Contracts.Timesheet belongs_to :contract, Contract @@ -63,7 +64,8 @@ defmodule Algora.Payments.Transaction do :contract_id, :original_contract_id, :user_id, - :succeeded_at + :succeeded_at, + :idempotency_key ]) |> validate_required([ :id, @@ -79,6 +81,7 @@ defmodule Algora.Payments.Transaction do :original_contract_id, :user_id ]) + |> unique_constraint([:idempotency_key]) |> foreign_key_constraint(:user_id) |> foreign_key_constraint(:contract_id) |> foreign_key_constraint(:original_contract_id) diff --git a/lib/algora/psp/psp.ex b/lib/algora/psp/psp.ex index bdace42c7..0e67138c0 100644 --- a/lib/algora/psp/psp.ex +++ b/lib/algora/psp/psp.ex @@ -26,33 +26,75 @@ defmodule Algora.PSP do @type error :: Stripe.Error.t() - @type invoice :: Stripe.Invoice.t() + @type metadata :: %{ + optional(String.t()) => String.t() + } + + @type invoice :: Algora.PSP.Invoice.t() defmodule Invoice do @moduledoc false + @type t :: Stripe.Invoice.t() + + @spec create(params, options) :: {:ok, t} | {:error, Algora.PSP.error()} + when params: + %{ + optional(:auto_advance) => boolean, + :customer => Stripe.id() | Stripe.Customer.t() + } + | %{}, + options: %{ + :idempotency_key => String.t() + } + def create(params, opts), do: Algora.PSP.client(__MODULE__).create(params, Keyword.new(opts)) + + @spec pay(Stripe.id() | t, params, options) :: {:ok, t} | {:error, Algora.PSP.error()} + when params: + %{ + optional(:off_session) => boolean, + optional(:payment_method) => String.t() + } + | %{}, + options: %{ + :idempotency_key => String.t() + } + def pay(invoice_id, params, opts), do: Algora.PSP.client(__MODULE__).pay(invoice_id, params, Keyword.new(opts)) - def create(params), do: Algora.PSP.client(__MODULE__).create(params) - def pay(invoice_id, params), do: Algora.PSP.client(__MODULE__).pay(invoice_id, params) def retrieve(id), do: Algora.PSP.client(__MODULE__).retrieve(id) - def retrieve(id, opts), do: Algora.PSP.client(__MODULE__).retrieve(id, opts) + def retrieve(id, opts), do: Algora.PSP.client(__MODULE__).retrieve(id, Keyword.new(opts)) end - @type invoiceitem :: Stripe.Invoiceitem.t() + @type invoiceitem :: Algora.PSP.Invoiceitem.t() defmodule Invoiceitem do @moduledoc false - - def create(params), do: Algora.PSP.client(__MODULE__).create(params) + @type t :: Stripe.Invoiceitem.t() + + @spec create(params, options) :: {:ok, t} | {:error, Algora.PSP.error()} + when params: + %{ + optional(:amount) => integer, + :currency => String.t(), + :customer => Stripe.id() | Stripe.Customer.t(), + optional(:description) => String.t(), + optional(:invoice) => Stripe.id() | Stripe.Invoice.t() + } + | %{}, + options: %{ + :idempotency_key => String.t() + } + def create(params, opts), do: Algora.PSP.client(__MODULE__).create(params, Keyword.new(opts)) end - @type transfer :: Stripe.Transfer.t() + @type transfer :: Algora.PSP.Transfer.t() defmodule Transfer do @moduledoc false + @type t :: Stripe.Transfer.t() - @spec create(params, options) :: {:ok, Algora.PSP.transfer()} | {:error, Algora.PSP.error()} + @spec create(params, options) :: {:ok, t} | {:error, Algora.PSP.error()} when params: %{ :amount => pos_integer, :currency => String.t(), :destination => String.t(), - optional(:metadata) => Stripe.Types.metadata(), + optional(:metadata) => Algora.PSP.metadata(), optional(:source_transaction) => String.t(), optional(:transfer_group) => String.t(), optional(:description) => String.t(), @@ -64,73 +106,82 @@ defmodule Algora.PSP do def create(params, opts), do: Algora.PSP.client(__MODULE__).create(params, Keyword.new(opts)) end - @type session :: Stripe.Session.t() + @type session :: Algora.PSP.Session.t() defmodule Session do @moduledoc false + @type t :: Stripe.Session.t() @type line_item_data :: Stripe.Session.line_item_data() @type payment_intent_data :: Stripe.Session.payment_intent_data() def create(params), do: Algora.PSP.client(__MODULE__).create(params) end - @type payment_method :: Stripe.PaymentMethod.t() + @type payment_method :: Algora.PSP.PaymentMethod.t() defmodule PaymentMethod do @moduledoc false + @type t :: Stripe.PaymentMethod.t() def attach(params), do: Algora.PSP.client(__MODULE__).attach(params) def retrieve(id), do: Algora.PSP.client(__MODULE__).retrieve(id) end - @type payment_intent :: Stripe.PaymentIntent.t() + @type payment_intent :: Algora.PSP.PaymentIntent.t() defmodule PaymentIntent do @moduledoc false + @type t :: Stripe.PaymentIntent.t() def create(params), do: Algora.PSP.client(__MODULE__).create(params) end - @type setup_intent :: Stripe.SetupIntent.t() + @type setup_intent :: Algora.PSP.SetupIntent.t() defmodule SetupIntent do @moduledoc false + @type t :: Stripe.SetupIntent.t() def retrieve(id, params), do: Algora.PSP.client(__MODULE__).retrieve(id, params) end - @type customer :: Stripe.Customer.t() + @type customer :: Algora.PSP.Customer.t() defmodule Customer do @moduledoc false + @type t :: Stripe.Customer.t() def retrieve(id), do: Algora.PSP.client(__MODULE__).retrieve(id) def create(params), do: Algora.PSP.client(__MODULE__).create(params) end - @type account :: Stripe.Account.t() + @type account :: Algora.PSP.Account.t() defmodule Account do @moduledoc false + @type t :: Stripe.Account.t() def retrieve(id), do: Algora.PSP.client(__MODULE__).retrieve(id) def create(params), do: Algora.PSP.client(__MODULE__).create(params) def delete(id), do: Algora.PSP.client(__MODULE__).delete(id) end - @type account_link :: Stripe.AccountLink.t() + @type account_link :: Algora.PSP.AccountLink.t() defmodule AccountLink do @moduledoc false + @type t :: Stripe.AccountLink.t() def create(params), do: Algora.PSP.client(__MODULE__).create(params) end - @type login_link :: Stripe.LoginLink.t() + @type login_link :: Algora.PSP.LoginLink.t() defmodule LoginLink do @moduledoc false + @type t :: Stripe.LoginLink.t() def create(params), do: Algora.PSP.client(__MODULE__).create(params) end - @type balance_transaction :: Stripe.BalanceTransaction.t() + @type balance_transaction :: Algora.PSP.BalanceTransaction.t() defmodule BalanceTransaction do @moduledoc false + @type t :: Stripe.BalanceTransaction.t() def retrieve(id), do: Algora.PSP.client(__MODULE__).retrieve(id) end end diff --git a/lib/algora_web/controllers/webhooks/github_controller.ex b/lib/algora_web/controllers/webhooks/github_controller.ex index 49bbf671f..9fbfc1602 100644 --- a/lib/algora_web/controllers/webhooks/github_controller.ex +++ b/lib/algora_web/controllers/webhooks/github_controller.ex @@ -137,11 +137,14 @@ defmodule AlgoraWeb.Webhooks.GithubController do autopay_result = if autopayable_bounty do + idempotency_key = "bounty-#{autopayable_bounty.id}" + with {:ok, invoice} <- Bounties.create_invoice( %{ owner: autopayable_bounty.owner, - amount: autopayable_bounty.amount + amount: autopayable_bounty.amount, + idempotency_key: idempotency_key }, ticket_ref: %{ owner: payload["repository"]["owner"]["login"], @@ -152,10 +155,14 @@ defmodule AlgoraWeb.Webhooks.GithubController do claims: claims ), {:ok, _invoice} <- - Algora.PSP.Invoice.pay(invoice, %{ - payment_method: autopayable_bounty.owner.customer.default_payment_method.provider_id, - off_session: true - }) do + Algora.PSP.Invoice.pay( + invoice, + %{ + payment_method: autopayable_bounty.owner.customer.default_payment_method.provider_id, + off_session: true + }, + %{idempotency_key: idempotency_key} + ) do Logger.info("Autopay successful (#{autopayable_bounty.owner.name} - #{autopayable_bounty.amount}).") :ok else diff --git a/priv/repo/migrations/20250224211917_add_idempotency_key_to_transaction.exs b/priv/repo/migrations/20250224211917_add_idempotency_key_to_transaction.exs new file mode 100644 index 000000000..15103f45f --- /dev/null +++ b/priv/repo/migrations/20250224211917_add_idempotency_key_to_transaction.exs @@ -0,0 +1,11 @@ +defmodule Algora.Repo.Migrations.AddIdempotencyKeyToTransaction do + use Ecto.Migration + + def change do + alter table(:transactions) do + add :idempotency_key, :string + end + + create unique_index(:transactions, [:idempotency_key]) + end +end diff --git a/test/algora/bounties_test.exs b/test/algora/bounties_test.exs index 16a1a69ec..b81d5c21b 100644 --- a/test/algora/bounties_test.exs +++ b/test/algora/bounties_test.exs @@ -179,17 +179,21 @@ defmodule Algora.BountiesTest do assert {:ok, invoice} = Bounties.create_invoice( - %{owner: owner, amount: amount}, + %{owner: owner, amount: amount, idempotency_key: "bounty-#{bounty.id}"}, ticket_ref: ticket_ref, bounty_id: bounty.id, claims: [claim] ) assert {:ok, _invoice} = - PSP.Invoice.pay(invoice, %{ - payment_method: payment_method.provider_id, - off_session: true - }) + PSP.Invoice.pay( + invoice, + %{ + payment_method: payment_method.provider_id, + off_session: true + }, + %{idempotency_key: "bounty-#{bounty.id}"} + ) charge = Repo.one!(from t in Transaction, where: t.type == :charge) assert Money.equal?(charge.net_amount, amount) diff --git a/test/algora_web/controllers/webhooks/github_controller_test.exs b/test/algora_web/controllers/webhooks/github_controller_test.exs index e36c871ab..4b1a84019 100644 --- a/test/algora_web/controllers/webhooks/github_controller_test.exs +++ b/test/algora_web/controllers/webhooks/github_controller_test.exs @@ -510,6 +510,59 @@ defmodule AlgoraWeb.Webhooks.GithubControllerTest do assert is_nil(transfer) end + test "prevents duplicate transaction creation when receiving multiple PR closed events", ctx do + issue_number = :rand.uniform(1000) + pr_number = :rand.uniform(1000) + + customer = insert!(:customer, user: ctx[:org]) + _payment_method = insert!(:payment_method, is_default: true, customer: customer) + + process_scenario!(ctx, [ + %{ + event_action: "issue_comment.created", + user_type: :admin, + body: "/bounty $100", + params: %{"issue" => %{"number" => issue_number}} + }, + %{ + event_action: "pull_request.opened", + user_type: :unauthorized, + body: "/claim #{issue_number}", + params: %{"pull_request" => %{"number" => pr_number}} + }, + %{ + event_action: "pull_request.closed", + user_type: :unauthorized, + body: "/claim #{issue_number}", + params: %{"pull_request" => %{"number" => pr_number, "merged_at" => DateTime.to_iso8601(DateTime.utc_now())}} + } + ]) + + assert Repo.aggregate(from(t in Transaction, where: t.type == :charge), :count) == 1 + assert Repo.aggregate(from(t in Transaction, where: t.type == :debit), :count) == 1 + assert Repo.aggregate(from(t in Transaction, where: t.type == :credit), :count) == 1 + + {:ok, log} = + with_log(fn -> + process_scenario(ctx, [ + %{ + event_action: "pull_request.closed", + user_type: :unauthorized, + body: "/claim #{issue_number}", + params: %{ + "pull_request" => %{"number" => pr_number, "merged_at" => DateTime.to_iso8601(DateTime.utc_now())} + } + } + ]) + end) + + assert log =~ "Autopay failed" + + assert Repo.aggregate(from(t in Transaction, where: t.type == :charge), :count) == 1 + assert Repo.aggregate(from(t in Transaction, where: t.type == :debit), :count) == 1 + assert Repo.aggregate(from(t in Transaction, where: t.type == :credit), :count) == 1 + end + test "handles split bounty payments between two users when PR is merged", ctx do issue_number = :rand.uniform(1000) pr_number = :rand.uniform(1000) diff --git a/test/support/stripe_mock.ex b/test/support/stripe_mock.ex index 475211023..4911259ea 100644 --- a/test/support/stripe_mock.ex +++ b/test/support/stripe_mock.ex @@ -3,11 +3,16 @@ defmodule Algora.Support.StripeMock do defmodule Invoice do @moduledoc false - def create(_params) do - {:ok, %Stripe.Invoice{id: "inv_#{Algora.Util.random_int()}", paid: false, status: "open"}} + def create(_params, opts) do + {:ok, + %Stripe.Invoice{ + id: "inv_#{:erlang.phash2(opts[:idempotency_key])}", + paid: false, + status: "open" + }} end - def pay(_invoice_id, %{payment_method: "pm_card_declined"}) do + def pay(_invoice_id, %{payment_method: "pm_card_declined"}, _opts) do {:error, %Stripe.Error{ source: :stripe, @@ -16,15 +21,15 @@ defmodule Algora.Support.StripeMock do }} end - def pay(invoice_id, _params) do + def pay(invoice_id, _params, _opts) do {:ok, %Stripe.Invoice{id: invoice_id, paid: true, status: "paid"}} end end defmodule Invoiceitem do @moduledoc false - def create(_params) do - {:ok, %Stripe.Invoiceitem{id: "ii_#{Algora.Util.random_int()}"}} + def create(_params, opts) do + {:ok, %Stripe.Invoiceitem{id: "ii_#{:erlang.phash2(opts[:idempotency_key])}"}} end end