From 28fc9559b31bfe7ca6b16099e2d740355e03dd74 Mon Sep 17 00:00:00 2001 From: SimpleTest Date: Fri, 24 Jul 2026 12:40:00 +0300 Subject: [PATCH] fix(push): preserve devices on provider configuration errors --- .../push/device_delivery_worker.ex | 21 ++++- lib/who_need_help/push/fcm_adapter.ex | 34 ++++++-- test/who_need_help/push_product_test.exs | 85 ++++++++++++++++++- 3 files changed, 130 insertions(+), 10 deletions(-) diff --git a/lib/who_need_help/push/device_delivery_worker.ex b/lib/who_need_help/push/device_delivery_worker.ex index 4ea8d9c..c3157b7 100644 --- a/lib/who_need_help/push/device_delivery_worker.ex +++ b/lib/who_need_help/push/device_delivery_worker.ex @@ -50,9 +50,13 @@ defmodule WhoNeedHelp.Push.DeviceDeliveryWorker do _ = Notifications.disable_invalid_device(device) :ok - {:error, {:rejected, status, _body}} when status in [400, 401, 403, 404, 410] -> - _ = Notifications.disable_invalid_device(device) - {:cancel, :device_rejected} + {:error, {:rejected, status, body}} -> + if invalid_device_rejection?(device.provider, status, body) do + _ = Notifications.disable_invalid_device(device) + {:cancel, :device_rejected} + else + {:cancel, {:provider_rejected, status}} + end {:error, {:invalid_configuration, _field} = reason} -> {:cancel, reason} @@ -61,4 +65,15 @@ defmodule WhoNeedHelp.Push.DeviceDeliveryWorker do {:error, reason} end end + + defp invalid_device_rejection?(:web_push, status, _body) when status in [404, 410], + do: true + + defp invalid_device_rejection?(:fcm, _status, %{ + "fcm_error_code" => error_code + }) + when error_code in ["INVALID_ARGUMENT", "UNREGISTERED"], + do: true + + defp invalid_device_rejection?(_provider, _status, _body), do: false end diff --git a/lib/who_need_help/push/fcm_adapter.ex b/lib/who_need_help/push/fcm_adapter.ex index ff50af5..243c506 100644 --- a/lib/who_need_help/push/fcm_adapter.ex +++ b/lib/who_need_help/push/fcm_adapter.ex @@ -25,10 +25,10 @@ defmodule WhoNeedHelp.Push.FCMAdapter do {:ok, %{id: name, duplicate: false}} {:ok, %Req.Response{status: status, body: body}} when status in @retryable_statuses -> - {:error, {:retryable, status, sanitize(body)}} + {:error, {:retryable, status, sanitize_error(body)}} {:ok, %Req.Response{status: status, body: body}} -> - {:error, {:rejected, status, sanitize(body)}} + {:error, {:rejected, status, sanitize_error(body)}} {:error, error} -> {:error, {:transport, transport_reason(error)}} @@ -101,10 +101,34 @@ defmodule WhoNeedHelp.Push.FCMAdapter do defp priority(_kind), do: "normal" - defp sanitize(%{"error" => error}) when is_map(error), - do: Map.take(error, ["code", "status"]) + @doc false + def sanitize_error(%{"error" => error}) when is_map(error) do + fcm_error_code = + error + |> Map.get("details", []) + |> Enum.find_value(fn + %{ + "@type" => "type.googleapis.com/google.firebase.fcm.v1.FcmError", + "errorCode" => error_code + } + when is_binary(error_code) -> + error_code - defp sanitize(_body), do: nil + _detail -> + nil + end) + + error + |> Map.take(["code", "status"]) + |> maybe_put_fcm_error_code(fcm_error_code) + end + + def sanitize_error(_body), do: nil + + defp maybe_put_fcm_error_code(error, nil), do: error + + defp maybe_put_fcm_error_code(error, fcm_error_code), + do: Map.put(error, "fcm_error_code", fcm_error_code) defp transport_reason(%Req.TransportError{reason: reason}), do: reason defp transport_reason(%{__struct__: module}), do: module diff --git a/test/who_need_help/push_product_test.exs b/test/who_need_help/push_product_test.exs index c761fb7..f7d1f7f 100644 --- a/test/who_need_help/push_product_test.exs +++ b/test/who_need_help/push_product_test.exs @@ -244,7 +244,50 @@ defmodule WhoNeedHelp.PushProductTest do refute inspect(payload) =~ "exact" end - test "device delivery retries transient failures and disables rejected registrations", + test "FCM retains only the provider error code needed to classify a rejected FID" do + invalid_fid_response = %{ + "error" => %{ + "code" => 400, + "message" => "The Firebase Installation ID is invalid", + "status" => "INVALID_ARGUMENT", + "details" => [ + %{ + "@type" => "type.googleapis.com/google.firebase.fcm.v1.FcmError", + "errorCode" => "INVALID_ARGUMENT" + } + ] + } + } + + assert FCMAdapter.sanitize_error(invalid_fid_response) == %{ + "code" => 400, + "status" => "INVALID_ARGUMENT", + "fcm_error_code" => "INVALID_ARGUMENT" + } + + invalid_payload_response = %{ + "error" => %{ + "code" => 400, + "message" => "The data payload is invalid", + "status" => "INVALID_ARGUMENT", + "details" => [ + %{ + "@type" => "type.googleapis.com/google.rpc.BadRequest", + "fieldViolations" => [%{"field" => "message.data"}] + } + ] + } + } + + assert FCMAdapter.sanitize_error(invalid_payload_response) == %{ + "code" => 400, + "status" => "INVALID_ARGUMENT" + } + + refute inspect(FCMAdapter.sanitize_error(invalid_fid_response)) =~ "Firebase Installation" + end + + test "device delivery retries transient failures and only disables a rejected FID", context do Application.put_env(:who_need_help, :fcm_adapter, RecordingDeviceAdapter) @@ -282,7 +325,45 @@ defmodule WhoNeedHelp.PushProductTest do assert is_nil(Repo.reload!(device).disabled_at) Application.put_env(:who_need_help, :device_delivery_options, %{ - fcm: [test_pid: self(), result: {:error, {:rejected, 404, nil}}] + fcm: [ + test_pid: self(), + result: + {:error, + {:rejected, 403, + %{"status" => "PERMISSION_DENIED", "fcm_error_code" => "SENDER_ID_MISMATCH"}}} + ] + }) + + assert {:cancel, {:provider_rejected, 403}} = + perform_job(DeviceDeliveryWorker, %{ + "notification_id" => notification.id, + "device_id" => device.id + }) + + assert is_nil(Repo.reload!(device).disabled_at) + + Application.put_env(:who_need_help, :device_delivery_options, %{ + fcm: [ + test_pid: self(), + result: {:error, {:rejected, 400, %{"status" => "INVALID_ARGUMENT"}}} + ] + }) + + assert {:cancel, {:provider_rejected, 400}} = + perform_job(DeviceDeliveryWorker, %{ + "notification_id" => notification.id, + "device_id" => device.id + }) + + assert is_nil(Repo.reload!(device).disabled_at) + + Application.put_env(:who_need_help, :device_delivery_options, %{ + fcm: [ + test_pid: self(), + result: + {:error, + {:rejected, 404, %{"status" => "NOT_FOUND", "fcm_error_code" => "UNREGISTERED"}}} + ] }) assert {:cancel, :device_rejected} =