Make notification handling marginally less painful

lib/clacks/activity.ex

@@ -12,8 +12,8 @@ defmodule Clacks.Activity do
   schema "activities" do
     field :data, :map
     field :local, :boolean
-    field :actor, :string
 
+    belongs_to :actor, Clacks.Actor, foreign_key: :actor_ap_id, references: :ap_id, type: :string
     has_one :object, Clacks.Object, on_delete: :nothing, foreign_key: :id
 
     timestamps()
@@ -21,8 +21,8 @@ defmodule Clacks.Activity do
 
   def changeset(%__MODULE__{} = schema, attrs) do
     schema
-    |> cast(attrs, [:data, :local, :actor])
+    |> cast(attrs, [:data, :local, :actor_ap_id])
-    |> validate_required([:data, :local, :actor])
+    |> validate_required([:data, :local, :actor_ap_id])
   end
 
   @spec changeset_for_creating(activity :: map(), local :: boolean()) :: Ecto.Changeset.t()
@@ -30,7 +30,7 @@ defmodule Clacks.Activity do
     changeset(%__MODULE__{}, %{
       data: activity,
       local: local,
-      actor: activity["actor"]
+      actor_ap_id: activity["actor"]
     })
   end
 
@@ -69,6 +69,8 @@ defmodule Clacks.Activity do
         fragment("COALESCE(?->'object'->>'id', ?->>'object')", a.data, a.data) == ^object_id
     )
     |> preload_object()
+    # todo: may not need to preload actor by default
+    |> preload_actor()
     |> Repo.one()
   end
 
@@ -91,6 +93,11 @@ defmodule Clacks.Activity do
     |> preload([_a, object: object], object: object)
   end
 
+  @spec preload_actor(Ecto.Queryable.t()) :: Ecto.Query.t()
+  def preload_actor(query) do
+    preload(query, :actor)
+  end
+
   @spec fetch(ap_id :: String.t()) :: t() | nil
   def fetch(ap_id) do
     case Clacks.ActivityPub.Fetcher.fetch_activity(ap_id) do

lib/clacks/inbox.ex

@@ -4,9 +4,11 @@ defmodule Clacks.Inbox do
 
   defp store_activity(%{"actor" => actor, "id" => ap_id} = activity, local \\ false)
        when is_binary(actor) do
+    # remove the embedded object (if there is one) from the activity
     activity_without_embedded_object =
       case Map.get(activity, "object") do
         %{"id" => object_id} ->
+          # todo: this assumes we already have stored the object
           Map.put(activity, "object", object_id)
 
         _ ->
@@ -17,7 +19,7 @@ defmodule Clacks.Inbox do
       Activity.changeset(Activity.get_cached_by_ap_id(ap_id) || %Activity{}, %{
         data: activity_without_embedded_object,
         local: local,
-        actor: actor
+        actor_ap_id: actor
       })
 
     case Repo.insert_or_update(changeset) do

lib/clacks/notification.ex

@@ -12,30 +12,37 @@ defmodule Clacks.Notification do
 
     belongs_to :user, Clacks.User
     belongs_to :activity, Clacks.Activity, type: FlakeId.Ecto.Type
+    belongs_to :referenced_activity, Clacks.Activity, type: FlakeId.Ecto.Type
 
     timestamps()
   end
 
   def changeset(%__MODULE__{} = schema, attrs) do
     schema
-    |> cast(attrs, [:type, :user_id, :activity_id])
+    |> cast(attrs, [:type, :user_id, :activity_id, :referenced_activity_id])
-    |> validate_required([:type, :user_id])
+    |> validate_required([:type, :user_id, :activity_id])
     |> validate_inclusion(:type, @valid_types)
   end
 
-  @spec create(type :: String.t(), activity :: Activity.t(), actor :: Actor.t()) ::
+  @spec create(
+          type :: String.t(),
+          activity :: Activity.t(),
+          actor :: Actor.t(),
+          referenced_activity :: Activity.t() | nil
+        ) ::
           {:ok, Notification.t()} | {:error, any()}
 
-  def create(type, _, _) when not (type in @valid_types) do
+  def create(type, _, _, _) when not (type in @valid_types) do
     {:error, "invalid notification type '#{type}'"}
   end
 
-  def create(type, activity, actor) do
+  def create(type, activity, actor, referenced_activity) do
     changeset =
       changeset(%__MODULE__{}, %{
         type: type,
         user_id: actor.user_id,
-        activity_id: activity.id
+        activity_id: activity.id,
+        referenced_activity_id: if(referenced_activity, do: referenced_activity.id, else: nil)
       })
 
     Repo.insert(changeset)
@@ -48,7 +55,7 @@ defmodule Clacks.Notification do
       ) do
     case Actor.get_cached_by_ap_id(followee_ap_id) do
       %Actor{local: true} = followee ->
-        create("follow", activity, followee)
+        create("follow", activity, followee, nil)
 
       _ ->
         :ok
@@ -62,7 +69,7 @@ defmodule Clacks.Notification do
     Enum.each(tags, fn %{"href" => mentioned_actor_id} ->
       case Actor.get_cached_by_ap_id(mentioned_actor_id) do
         %Actor{local: true} = mentioned_actor ->
-          create("mention", activity, mentioned_actor)
+          create("mention", activity, mentioned_actor, nil)
 
         _ ->
           :ok
@@ -74,11 +81,15 @@ defmodule Clacks.Notification do
         %Activity{data: %{"type" => type, "object" => original_object_ap_id}} = activity
       )
       when type in ["Announce", "Like"] do
-    with %Activity{local: true, actor: local_actor_id} <-
+    with %Activity{local: true, actor: %Actor{local: true} = original_activity_actor} =
-           Activity.get_by_object_ap_id(original_object_ap_id, "Create"),
+           original_activity <-
-         %Actor{local: true} = original_activity_actor <-
+           Activity.get_by_object_ap_id(original_object_ap_id, "Create") do
-           Actor.get_cached_by_ap_id(local_actor_id) do
+      create(
-      create(String.downcase(type), activity, original_activity_actor)
+        String.downcase(type),
+        activity,
+        original_activity_actor,
+        original_activity
+      )
     else
       _ ->
         :ok

lib/clacks/object.ex

@@ -55,7 +55,7 @@ defmodule Clacks.Object do
 
   @spec fetch(url :: String.t(), synthesize_create :: boolean(), return :: :object | :activity) ::
           t() | Activity.t() | nil
-  def fetch(url, synthesize_create \\ true, return \\ :object) do
+  def fetch(url, synthesize_create \\ true, return \\ :object) when is_binary(url) do
     case Clacks.ActivityPub.Fetcher.fetch_object(url) do
       nil ->
         nil

lib/clacks/timeline.ex

@@ -1,5 +1,5 @@
 defmodule Clacks.Timeline do
-  alias Clacks.{Repo, Actor, Activity, User, Notification}
+  alias Clacks.{Repo, Actor, Activity, Object, User, Notification}
   import Clacks.Paginator
   import Ecto.Query
 
@@ -82,34 +82,54 @@ defmodule Clacks.Timeline do
     |> Repo.all()
   end
 
-  @spec notifications(actor :: Actor.t(), params :: map()) :: [
+  @spec notifications(Actor.t(), map()) :: [Notification.t()]
-          {:follow, activity :: Activity.t(), actor :: Actor.t()}
-          | {:mention, activity :: Activity.t(), actor :: Actor.t()}
-          | {:announce, announce :: Activity.t(), announce_actor :: Actor.t(),
-             activity :: Activity.t(), actor :: Actor.t()}
-          | {:like, like :: Activity.t(), like_actor :: Actor.t(), activity :: Activity.t(),
-             actor :: Actor.t()}
-        ]
   def notifications(actor, params) do
     Notification
     |> where([n], n.user_id == ^actor.user_id)
-    |> join(:inner, [n], activity in Activity, on: activity.id == n.activity_id)
-    |> join(:inner, [n, activity], actor in Actor, on: activity.actor == actor.ap_id)
-    |> join(:left, [n, activity, actor], other in Activity,
-      on:
-        n.type in ["announce", "like"] and fragment("?->>'type'", other.data) == "Create" and
-          fragment("?->>'object'", activity.data) == fragment("?->'object'->>'id'", other.data)
-    )
     |> paginate(params)
     |> limit(^Map.get(params, "limit", 20))
+    |> join(:left, [n], a in Activity,
+      as: :activity,
+      on: a.id == n.activity_id
+    )
+    |> join(:left, [n], a in Activity,
+      as: :referenced_activity,
+      on: a.id == n.referenced_activity_id
+    )
+    |> join(:left, [n, activity: a], o in Object,
+      as: :object,
+      on:
+        fragment("?->>'id'", o.data) ==
+          fragment("COALESCE(?->'object'->>'id', ?->>'object')", a.data, a.data)
+    )
+    |> join(:left, [n, referenced_activity: a], o in Object,
+      as: :referenced_object,
+      on:
+        fragment("?->>'id'", o.data) ==
+          fragment("COALESCE(?->'object'->>'id', ?->>'object')", a.data, a.data)
+    )
+    |> join(:left, [n, activity: activity], actor in Actor,
+      as: :activity_actor,
+      on: activity.actor_ap_id == actor.ap_id
+    )
+    # note: we shouldn't need to load the actor for the referenced_activity here,
+    # because notifications for a given actor always reference activities from that same actor
     |> select(
-      [notification, activity, actor, original_activity],
+      [
-      {notification, activity, actor, original_activity}
+        n,
+        activity: activity,
+        referenced_activity: referenced_activity,
+        object: object,
+        referenced_object: referenced_object,
+        activity_actor: activity_actor
+      ],
+      %Notification{
+        n
+        | activity: %Activity{activity | object: object, actor: activity_actor},
+          referenced_activity: %Activity{referenced_activity | object: referenced_object}
+      }
     )
     |> Repo.all()
-    |> Enum.map(fn {notification, activity, actor, original_activity} ->
-      {String.to_existing_atom(notification.type), activity, actor, original_activity}
-    end)
   end
 
   defp restrict_to_actor(query, actor_id) do

lib/clacks_web/templates/frontend/notifications.html.eex

@@ -11,17 +11,17 @@
   <%= for notification <- @notifications do %>
     <li>
       <%= case notification do %>
-        <% {:like, like_activity, actor, original_activity} -> %>
+        <% %{type: "like", activity: %{actor: like_actor} = like_activity, referenced_activity: original_activity} -> %>
-          <%= render "_action_status.html", class: "notification", conn: @conn, action: :like, action_activity: like_activity, action_actor: actor, original_activity: original_activity, original_note: original_activity.data["object"], original_actor: @current_user.actor %>
+          <%= render "_action_status.html", class: "notification", conn: @conn, action: :like, action_activity: like_activity, action_actor: like_actor, original_activity: original_activity, original_note: original_activity.object.data, original_actor: @current_user.actor %>
 
-        <% {:announce, announce_activity, actor, original_activity} -> %>
+          <% %{type: "announce", activity: %{actor: announce_actor} = announce_activity, referenced_activity: original_activity} -> %>
-          <%= render "_action_status.html", class: "notification", conn: @conn, action: :announce, action_activity: announce_activity, action_actor: actor, original_activity: original_activity, original_note: original_activity.data["object"], original_actor: @current_user.actor %>
+          <%= render "_action_status.html", class: "notification", conn: @conn, action: :announce, action_activity: announce_activity, action_actor: announce_actor, original_activity: original_activity, original_note: original_activity.object.data, original_actor: @current_user.actor %>
 
-        <% {:mention, mention_activity, actor, _} -> %>
+        <% %{type: "mention", activity: mention_activity} -> %>
-          <%= render "_status.html", class: "notification", conn: @conn, author: actor, status: mention_activity, note: mention_activity.data["object"] %>
+          <%= render "_status.html", class: "notification", conn: @conn, author: mention_activity.actor, status: mention_activity, note: mention_activity.object.data %>
 
-          <% {:follow, follow_activity, actor, _} -> %>
+        <% %{type: "follow", activity: follow_activity} -> %>
-            <%= render "_follow_notification.html", activity: follow_activity, actor: actor %>
+          <%= render "_follow_notification.html", activity: follow_activity, actor: follow_activity.actor %>
       <% end %>
     </li>
   <% end %>

priv/repo/migrations/20210826220029_more_belongs_to.exs

@@ -0,0 +1,14 @@
+defmodule Clacks.Repo.Migrations.MoreBelongsTo do
+  use Ecto.Migration
+
+  def change do
+    alter table(:notifications) do
+      add :referenced_activity_id, references(:activities, type: :uuid)
+    end
+
+    alter table(:activities) do
+      remove :actor
+      add :actor_ap_id, references(:actors, column: :ap_id, type: :string)
+    end
+  end
+end