WebPush refactoring: separate build and deliver steps

lib/pleroma/web/push/impl.ex

@@ -19,69 +19,72 @@ defmodule Pleroma.Web.Push.Impl do
   @body_chars 140
   @types ["Create", "Follow", "Announce", "Like", "Move", "EmojiReact", "Update"]
 
-  @doc "Performs sending notifications for user subscriptions"
+  @doc "Builds webpush notification payloads for the subscriptions enabled by the receiving user"
-  @spec perform(Notification.t()) :: list(any) | :error | {:error, :unknown_type}
+  @spec build(Notification.t()) ::
-  def perform(
+          list(%{content: map(), subscription: Subscription.t()})
+          | :error
+          | {:error, :unknown_type}
+  def build(
         %{
           activity: %{data: %{"type" => activity_type}} = activity,
-          user: %User{id: user_id}
+          user: user
         } = notification
       )
       when activity_type in @types do
-    user = User.get_cached_by_ap_id(notification.activity.data["actor"])
+    notification_actor = User.get_cached_by_ap_id(notification.activity.data["actor"])
+    avatar_url = User.avatar_url(notification_actor)
 
-    gcm_api_key = Application.get_env(:web_push_encryption, :gcm_api_key)
-    avatar_url = User.avatar_url(user)
     object = Object.normalize(activity, fetch: false)
-    user = User.get_cached_by_id(user_id)
     direct_conversation_id = Activity.direct_conversation_id(activity, user)
 
-    for subscription <- fetch_subscriptions(user_id),
+    subscriptions = fetch_subscriptions(user.id)
-        Subscription.enabled?(subscription, notification.type) do
+
-      %{
+    subscriptions
-        access_token: subscription.token.token,
+    |> Enum.filter(&Subscription.enabled?(&1, notification.type))
-        notification_id: notification.id,
+    |> Enum.map(fn subscription ->
-        notification_type: notification.type,
+      payload =
-        icon: avatar_url,
+        %{
-        preferred_locale: "en",
+          access_token: subscription.token.token,
-        pleroma: %{
+          notification_id: notification.id,
-          activity_id: notification.activity.id,
+          notification_type: notification.type,
-          direct_conversation_id: direct_conversation_id
+          icon: avatar_url,
+          preferred_locale: "en",
+          pleroma: %{
+            activity_id: notification.activity.id,
+            direct_conversation_id: direct_conversation_id
+          }
         }
-      }
+        |> Map.merge(build_content(notification, notification_actor, object))
-      |> Map.merge(build_content(notification, user, object))
+        |> Jason.encode!()
-      |> Jason.encode!()
+
-      |> push_message(build_sub(subscription), gcm_api_key, subscription)
+      %{payload: payload, subscription: subscription}
-    end
+    end)
-    |> (&{:ok, &1}).()
   end
 
-  def perform(_) do
+  def build(_) do
     Logger.warning("Unknown notification type")
     {:error, :unknown_type}
   end
 
-  @doc "Push message to web"
+  @doc "Deliver push notification to the provided webpush subscription"
-  def push_message(body, sub, api_key, subscription) do
+  @spec deliver(%{payload: String.t(), subscription: Subscription.t()}) :: :ok | :error
-    try do
+  def deliver(%{payload: payload, subscription: subscription}) do
-      case WebPushEncryption.send_web_push(body, sub, api_key) do
+    gcm_api_key = Application.get_env(:web_push_encryption, :gcm_api_key)
-        {:ok, %{status: code}} when code in 400..499 ->
+    formatted_subscription = build_sub(subscription)
-          Logger.debug("Removing subscription record")
-          Repo.delete!(subscription)
-          :ok
 
-        {:ok, %{status: code}} when code in 200..299 ->
+    case WebPushEncryption.send_web_push(payload, formatted_subscription, gcm_api_key) do
-          :ok
+      {:ok, %{status: code}} when code in 200..299 ->
+        :ok
 
-        {:ok, %{status: code}} ->
+      {:ok, %{status: code}} when code in 400..499 ->
-          Logger.error("Web Push Notification failed with code: #{code}")
+        Logger.debug("Removing subscription record")
-          :error
+        Repo.delete!(subscription)
+        :ok
+
+      {:ok, %{status: code}} ->
+        Logger.error("Web Push Notification failed with code: #{code}")
+        :error
 
-        error ->
-          Logger.error("Web Push Notification failed with #{inspect(error)}")
-          :error
-      end
-    rescue
       error ->
         Logger.error("Web Push Notification failed with #{inspect(error)}")
         :error
@@ -140,9 +143,7 @@ defmodule Pleroma.Web.Push.Impl do
 
     content_text = content <> "\n"
 
-    options_text =
+    options_text = Enum.map_join(options, "\n", fn x -> "○ #{x["name"]}" end)
-      Enum.map(options, fn x -> "○ #{x["name"]}" end)
-      |> Enum.join("\n")
 
     [content_text, options_text]
     |> Enum.join("\n")

lib/pleroma/workers/web_pusher_worker.ex

@@ -5,6 +5,7 @@
 defmodule Pleroma.Workers.WebPusherWorker do
   alias Pleroma.Notification
   alias Pleroma.Repo
+  alias Pleroma.Web.Push.Impl
 
   use Pleroma.Workers.WorkerHelper, queue: "web_push"
 
@@ -15,7 +16,8 @@ defmodule Pleroma.Workers.WebPusherWorker do
       |> Repo.get(notification_id)
       |> Repo.preload([:activity, :user])
 
-    Pleroma.Web.Push.Impl.perform(notification)
+    Impl.build(notification)
+    |> Enum.each(&Impl.deliver(&1))
   end
 
   @impl Oban.Worker

test/pleroma/web/push/impl_test.exs

@@ -32,17 +32,6 @@ defmodule Pleroma.Web.Push.ImplTest do
     :ok
   end
 
-  @sub %{
-    endpoint: "https://example.com/example/1234",
-    keys: %{
-      auth: "8eDyX_uCN0XRhSbY5hs7Hg==",
-      p256dh:
-        "BCIWgsnyXDv1VkhqL2P7YRBvdeuDnlwAPT2guNhdIoW3IP7GmHh1SMKPLxRf7x8vJy6ZFK3ol2ohgn_-0yP7QQA="
-    }
-  }
-  @api_key "BASgACIHpN1GYgzSRp"
-  @message "@Bob: Lorem ipsum dolor sit amet, consectetur  adipiscing elit. Fusce sagittis finibus turpis."
-
   test "performs sending notifications" do
     user = insert(:user)
     user2 = insert(:user)
@@ -68,39 +57,65 @@ defmodule Pleroma.Web.Push.ImplTest do
         type: "mention"
       )
 
-    assert Impl.perform(notif) == {:ok, [:ok, :ok]}
+    Impl.build(notif)
+    |> Enum.each(fn push -> assert match?(:ok, Impl.deliver(push)) end)
   end
 
   @tag capture_log: true
   test "returns error if notif does not match " do
-    assert Impl.perform(%{}) == {:error, :unknown_type}
+    assert Impl.build(%{}) == {:error, :unknown_type}
-  end
-
-  test "successful message sending" do
-    assert Impl.push_message(@message, @sub, @api_key, %Subscription{}) == :ok
   end
 
   @tag capture_log: true
   test "fail message sending" do
-    assert Impl.push_message(
+    user = insert(:user)
-             @message,
+
-             Map.merge(@sub, %{endpoint: "https://example.com/example/bad"}),
+    insert(:push_subscription,
-             @api_key,
+      user: user,
-             %Subscription{}
+      endpoint: "https://example.com/example/bad",
-           ) == :error
+      data: %{alerts: %{"follow" => true}}
+    )
+
+    other_user = insert(:user)
+    {:ok, _, _, activity} = CommonAPI.follow(user, other_user)
+
+    notif =
+      insert(:notification,
+        user: user,
+        activity: activity,
+        type: "follow"
+      )
+
+    [push] = Impl.build(notif)
+
+    assert Impl.deliver(push) == :error
   end
 
   test "delete subscription if result send message between 400..500" do
-    subscription = insert(:push_subscription)
+    user = insert(:user)
 
-    assert Impl.push_message(
+    bad_subscription =
-             @message,
+      insert(:push_subscription,
-             Map.merge(@sub, %{endpoint: "https://example.com/example/not_found"}),
+        user: user,
-             @api_key,
+        endpoint: "https://example.com/example/not_found",
-             subscription
+        data: %{alerts: %{"follow" => true}}
-           ) == :ok
+      )
 
-    refute Pleroma.Repo.get(Subscription, subscription.id)
+    other_user = insert(:user)
+    {:ok, _, _, activity} = CommonAPI.follow(user, other_user)
+
+    notif =
+      insert(:notification,
+        user: user,
+        activity: activity,
+        type: "follow"
+      )
+
+    [push] = Impl.build(notif)
+
+    assert Impl.deliver(push) == :ok
+
+    refute Pleroma.Repo.get(Subscription, bad_subscription.id)
   end
 
   test "deletes subscription when token has been deleted" do