Merge branch 'bugfix/fix-like-notifications' into 'develop'
authorrinpatch <rinpatch@sdf.org>
Fri, 8 May 2020 15:00:43 +0000 (15:00 +0000)
committerrinpatch <rinpatch@sdf.org>
Fri, 8 May 2020 15:00:43 +0000 (15:00 +0000)
Notifications: Simplify recipient calculation for some Activities.

See merge request pleroma/pleroma!2486

1  2 
lib/pleroma/notification.ex
test/notification_test.exs

index c135306caf8cab8ca327a9a951e525a9256e3121,af49fd7132307c7cdf01da4bd6cf974e7d884c7b..8aa9ed2d48f80098909de61aa53a9d41b4dcf7c2
@@@ -5,10 -5,8 +5,10 @@@
  defmodule Pleroma.Notification do
    use Ecto.Schema
  
 +  alias Ecto.Multi
    alias Pleroma.Activity
    alias Pleroma.FollowingRelationship
 +  alias Pleroma.Marker
    alias Pleroma.Notification
    alias Pleroma.Object
    alias Pleroma.Pagination
      timestamps()
    end
  
 +  @spec unread_notifications_count(User.t()) :: integer()
 +  def unread_notifications_count(%User{id: user_id}) do
 +    from(q in __MODULE__,
 +      where: q.user_id == ^user_id and q.seen == false
 +    )
 +    |> Repo.aggregate(:count, :id)
 +  end
 +
    def changeset(%Notification{} = notification, attrs) do
      notification
      |> cast(attrs, [:seen])
    end
  
 +  @spec last_read_query(User.t()) :: Ecto.Queryable.t()
 +  def last_read_query(user) do
 +    from(q in Pleroma.Notification,
 +      where: q.user_id == ^user.id,
 +      where: q.seen == true,
 +      select: type(q.id, :string),
 +      limit: 1,
 +      order_by: [desc: :id]
 +    )
 +  end
 +
    defp for_user_query_ap_id_opts(user, opts) do
      ap_id_relationships =
        [:block] ++
      |> Repo.all()
    end
  
 -  def set_read_up_to(%{id: user_id} = _user, id) do
 +  def set_read_up_to(%{id: user_id} = user, id) do
      query =
        from(
          n in Notification,
          where: n.user_id == ^user_id,
          where: n.id <= ^id,
          where: n.seen == false,
 -        update: [
 -          set: [
 -            seen: true,
 -            updated_at: ^NaiveDateTime.utc_now()
 -          ]
 -        ],
          # Ideally we would preload object and activities here
          # but Ecto does not support preloads in update_all
          select: n.id
        )
  
 -    {_, notification_ids} = Repo.update_all(query, [])
 +    {:ok, %{ids: {_, notification_ids}}} =
 +      Multi.new()
 +      |> Multi.update_all(:ids, query, set: [seen: true, updated_at: NaiveDateTime.utc_now()])
 +      |> Marker.multi_set_last_read_id(user, "notifications")
 +      |> Repo.transaction()
  
      Notification
      |> where([n], n.id in ^notification_ids)
      |> Repo.all()
    end
  
 +  @spec read_one(User.t(), String.t()) ::
 +          {:ok, Notification.t()} | {:error, Ecto.Changeset.t()} | nil
    def read_one(%User{} = user, notification_id) do
      with {:ok, %Notification{} = notification} <- get(user, notification_id) do
 -      notification
 -      |> changeset(%{seen: true})
 -      |> Repo.update()
 +      Multi.new()
 +      |> Multi.update(:update, changeset(notification, %{seen: true}))
 +      |> Marker.multi_set_last_read_id(user, "notifications")
 +      |> Repo.transaction()
 +      |> case do
 +        {:ok, %{update: notification}} -> {:ok, notification}
 +        {:error, :update, changeset, _} -> {:error, changeset}
 +      end
      end
    end
  
    # TODO move to sql, too.
    def create_notification(%Activity{} = activity, %User{} = user, do_send \\ true) do
      unless skip?(activity, user) do
 -      notification = %Notification{user_id: user.id, activity: activity}
 -      {:ok, notification} = Repo.insert(notification)
 +      {:ok, %{notification: notification}} =
 +        Multi.new()
 +        |> Multi.insert(:notification, %Notification{user_id: user.id, activity: activity})
 +        |> Marker.multi_set_last_read_id(user, "notifications")
 +        |> Repo.transaction()
  
        if do_send do
          Streamer.stream(["user", "user:notification"], notification)
  
    def get_notified_from_activity(%Activity{data: %{"type" => type}} = activity, local_only)
        when type in ["Create", "Like", "Announce", "Follow", "Move", "EmojiReact"] do
-     potential_receiver_ap_ids =
-       []
-       |> Utils.maybe_notify_to_recipients(activity)
-       |> Utils.maybe_notify_mentioned_recipients(activity)
-       |> Utils.maybe_notify_subscribers(activity)
-       |> Utils.maybe_notify_followers(activity)
-       |> Enum.uniq()
+     potential_receiver_ap_ids = get_potential_receiver_ap_ids(activity)
  
      potential_receivers = User.get_users_from_set(potential_receiver_ap_ids, local_only)
  
  
    def get_notified_from_activity(_, _local_only), do: {[], []}
  
+   # For some activities, only notify the author of the object
+   def get_potential_receiver_ap_ids(%{data: %{"type" => type, "object" => object_id}})
+       when type in ~w{Like Announce EmojiReact} do
+     case Object.get_cached_by_ap_id(object_id) do
+       %Object{data: %{"actor" => actor}} ->
+         [actor]
+       _ ->
+         []
+     end
+   end
+   def get_potential_receiver_ap_ids(activity) do
+     []
+     |> Utils.maybe_notify_to_recipients(activity)
+     |> Utils.maybe_notify_mentioned_recipients(activity)
+     |> Utils.maybe_notify_subscribers(activity)
+     |> Utils.maybe_notify_followers(activity)
+     |> Enum.uniq()
+   end
    @doc "Filters out AP IDs domain-blocking and not following the activity's actor"
    def exclude_domain_blocker_ap_ids(ap_ids, activity, preloaded_users \\ [])
  
index 0783c325d136a79cd600cc9fb017e51085c383e3,509ca0b0b0600ac717319de418b07de1bd3a40e2..24e5f0c73d4d2e80c54200952166a397051404b8
@@@ -12,6 -12,8 +12,8 @@@ defmodule Pleroma.NotificationTest d
    alias Pleroma.Notification
    alias Pleroma.Tests.ObanHelpers
    alias Pleroma.User
+   alias Pleroma.Web.ActivityPub.ActivityPub
+   alias Pleroma.Web.ActivityPub.Builder
    alias Pleroma.Web.ActivityPub.Transmogrifier
    alias Pleroma.Web.CommonAPI
    alias Pleroma.Web.MastodonAPI.NotificationView
@@@ -24,7 -26,7 +26,7 @@@
        other_user = insert(:user)
  
        {:ok, activity} = CommonAPI.post(user, %{"status" => "yeah"})
 -      {:ok, activity, _object} = CommonAPI.react_with_emoji(activity.id, other_user, "☕")
 +      {:ok, activity} = CommonAPI.react_with_emoji(activity.id, other_user, "☕")
  
        {:ok, [notification]} = Notification.create_notifications(activity)
  
@@@ -47,9 -49,6 +49,9 @@@
        assert notified_ids == [other_user.id, third_user.id]
        assert notification.activity_id == activity.id
        assert other_notification.activity_id == activity.id
 +
 +      assert [%Pleroma.Marker{unread_count: 2}] =
 +               Pleroma.Marker.get_markers(other_user, ["notifications"])
      end
  
      test "it creates a notification for subscribed users" do
        assert n1.seen == true
        assert n2.seen == true
        assert n3.seen == false
 +
 +      assert %Pleroma.Marker{} =
 +               m =
 +               Pleroma.Repo.get_by(
 +                 Pleroma.Marker,
 +                 user_id: other_user.id,
 +                 timeline: "notifications"
 +               )
 +
 +      assert m.last_read_id == to_string(n2.id)
      end
    end
  
        assert other_user not in enabled_receivers
      end
  
+     test "it only notifies the post's author in likes" do
+       user = insert(:user)
+       other_user = insert(:user)
+       third_user = insert(:user)
+       {:ok, activity_one} =
+         CommonAPI.post(user, %{
+           "status" => "hey @#{other_user.nickname}!"
+         })
+       {:ok, like_data, _} = Builder.like(third_user, activity_one.object)
+       {:ok, like, _} =
+         like_data
+         |> Map.put("to", [other_user.ap_id | like_data["to"]])
+         |> ActivityPub.persist(local: true)
+       {enabled_receivers, _disabled_receivers} = Notification.get_notified_from_activity(like)
+       assert other_user not in enabled_receivers
+     end
      test "it does not send notification to mentioned users in announces" do
        user = insert(:user)
        other_user = insert(:user)
  
        assert length(Notification.for_user(user)) == 1
  
 -      {:ok, _, _, _} = CommonAPI.unfavorite(activity.id, other_user)
 +      {:ok, _} = CommonAPI.unfavorite(activity.id, other_user)
  
        assert Enum.empty?(Notification.for_user(user))
      end
  
        assert length(Notification.for_user(user)) == 1
  
 -      {:ok, _, _} = CommonAPI.unrepeat(activity.id, other_user)
 +      {:ok, _} = CommonAPI.unrepeat(activity.id, other_user)
  
        assert Enum.empty?(Notification.for_user(user))
      end