Merge pull request 'Fix account favourites ordering' (#7914) from issue-3379-favourites-order into develop

Reviewed-on: https://git.pleroma.social/pleroma/pleroma/pulls/7914

changelog.d/account-favourites-order.fix

@@ -0,0 +1 @@
+Fixed `/api/v1/pleroma/accounts/:id/favourites` ordering and pagination to use the favourite date.

lib/pleroma/web/activity_pub/activity_pub.ex

@@ -537,9 +537,12 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
     #   and extra sorting on "activities.id DESC NULLS LAST" would worse the query plan
     opts = Map.put(opts, :skip_extra_order, true)
 
-    Pagination.fetch_paginated(query, opts, pagination)
+    Pagination.fetch_paginated(query, opts, pagination, pagination_binding(opts))
   end
 
+  defp pagination_binding(%{favorited_by: _}), do: :favorited_activity
+  defp pagination_binding(_), do: nil
+
   def fetch_activities(recipients, opts \\ %{}, pagination \\ :keyset) do
     list_memberships = Pleroma.List.memberships(opts[:user])
 
@@ -786,6 +789,8 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
 
   defp restrict_since(query, %{since_id: ""}), do: query
 
+  defp restrict_since(query, %{favorited_by: _, since_id: _}), do: query
+
   defp restrict_since(query, %{since_id: since_id}) do
     from(activity in query, where: activity.id > ^since_id)
   end
@@ -1020,9 +1025,31 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
   defp restrict_assigned_account(query, _), do: query
 
   defp restrict_favorited_by(query, %{favorited_by: ap_id}) do
+    newer_favorite =
+      from(newer_favorite in Activity,
+        where: newer_favorite.actor == ^ap_id,
+        where: newer_favorite.id > parent_as(:favorited_activity).id,
+        where: fragment("?->>'type' = ?", newer_favorite.data, "Like"),
+        where:
+          fragment(
+            "associated_object_id(?) = associated_object_id(?)",
+            newer_favorite.data,
+            parent_as(:favorited_activity).data
+          ),
+        select: 1
+      )
+
     from(
-      [_activity, object] in query,
+      [activity, object: object] in query,
-      where: fragment("(?)->'likes' \\? (?)", object.data, ^ap_id)
+      join: favorited_activity in Activity,
+      as: :favorited_activity,
+      on:
+        favorited_activity.actor == ^ap_id and
+          fragment("?->>'type' = ?", favorited_activity.data, "Like") and
+          fragment("associated_object_id(?) = (?)->>'id'", favorited_activity.data, object.data),
+      where: fragment("(?)->'likes' \\? (?)", object.data, ^ap_id),
+      where: not exists(subquery(newer_favorite)),
+      select: %Activity{activity | pagination_id: favorited_activity.id}
     )
   end
 

priv/repo/migrations/20260725000000_add_activity_favorites_lookup_index.exs

@@ -0,0 +1,20 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2022 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Repo.Migrations.AddActivityFavoritesLookupIndex do
+  use Ecto.Migration
+
+  @disable_ddl_transaction true
+  @disable_migration_lock true
+
+  def change do
+    create_if_not_exists(
+      index(:activities, [:actor, "associated_object_id(data)", "id DESC"],
+        name: :activities_favorites_lookup_index,
+        where: "data->>'type' = 'Like'",
+        concurrently: true
+      )
+    )
+  end
+end

test/pleroma/web/pleroma_api/controllers/account_controller_test.exs

@@ -5,9 +5,12 @@
 defmodule Pleroma.Web.PleromaAPI.AccountControllerTest do
   use Pleroma.Web.ConnCase
 
+  alias Pleroma.Activity
   alias Pleroma.Config
+  alias Pleroma.Repo
   alias Pleroma.Tests.ObanHelpers
   alias Pleroma.User
+  alias Pleroma.Web.ActivityPub.Utils
   alias Pleroma.Web.CommonAPI
 
   import Pleroma.Factory
@@ -105,6 +108,71 @@ defmodule Pleroma.Web.PleromaAPI.AccountControllerTest do
       assert length(response) == 1
     end
 
+    test "orders statuses by favorite date", %{
+      conn: conn,
+      user: user
+    } do
+      other_user = insert(:user)
+
+      {:ok, first_post} = CommonAPI.post(other_user, %{status: "first post"})
+      {:ok, second_post} = CommonAPI.post(other_user, %{status: "second post"})
+      {:ok, third_post} = CommonAPI.post(other_user, %{status: "third post"})
+
+      {:ok, _} = CommonAPI.favorite(third_post.id, user)
+      {:ok, _} = CommonAPI.favorite(first_post.id, user)
+      {:ok, _} = CommonAPI.favorite(second_post.id, user)
+
+      response =
+        conn
+        |> get("/api/v1/pleroma/accounts/#{user.id}/favourites")
+        |> json_response_and_validate_schema(:ok)
+
+      assert Enum.map(response, & &1["id"]) == [second_post.id, first_post.id, third_post.id]
+    end
+
+    test "returns a status once when duplicate favorite activities exist", %{
+      conn: conn,
+      user: user
+    } do
+      older_activity = insert(:note_activity)
+      {:ok, _} = CommonAPI.favorite(older_activity.id, user)
+
+      activity = insert(:note_activity)
+      {:ok, favorite} = CommonAPI.favorite(activity.id, user)
+
+      duplicate =
+        Repo.insert!(%Activity{
+          actor: favorite.actor,
+          data: Map.put(favorite.data, "id", Utils.generate_activity_id()),
+          local: favorite.local,
+          recipients: favorite.recipients
+        })
+
+      conn = get(conn, "/api/v1/pleroma/accounts/#{user.id}/favourites?limit=1")
+      response = json_response_and_validate_schema(conn, :ok)
+
+      assert Enum.map(response, & &1["id"]) == [activity.id]
+      assert [link_header] = get_resp_header(conn, "link")
+      assert link_header =~ "max_id=#{duplicate.id}"
+      assert link_header =~ "min_id=#{duplicate.id}"
+
+      response =
+        conn
+        |> get("/api/v1/pleroma/accounts/#{user.id}/favourites?max_id=#{duplicate.id}&limit=1")
+        |> json_response_and_validate_schema(:ok)
+
+      assert Enum.map(response, & &1["id"]) == [older_activity.id]
+
+      {:ok, _} = CommonAPI.unfavorite(activity.id, user)
+
+      response =
+        conn
+        |> get("/api/v1/pleroma/accounts/#{user.id}/favourites")
+        |> json_response_and_validate_schema(:ok)
+
+      assert Enum.map(response, & &1["id"]) == [older_activity.id]
+    end
+
     test "returns favorited DM only when user is logged in and he is one of recipients", %{
       current_user: current_user,
       user: user
@@ -164,23 +232,67 @@ defmodule Pleroma.Web.PleromaAPI.AccountControllerTest do
     } do
       activities = insert_list(10, :note_activity)
 
-      Enum.each(activities, fn activity ->
+      favorite_pairs =
-        CommonAPI.favorite(activity.id, user)
+        Enum.map(activities, fn activity ->
-      end)
+          {:ok, favorite} = CommonAPI.favorite(activity.id, user)
+          {favorite, activity}
+        end)
+        |> Enum.sort_by(fn {favorite, _activity} -> FlakeId.from_string(favorite.id) end)
 
-      third_activity = Enum.at(activities, 2)
+      {third_favorite, _} = Enum.at(favorite_pairs, 2)
-      seventh_activity = Enum.at(activities, 6)
+      {seventh_favorite, _} = Enum.at(favorite_pairs, 6)
+
+      expected_ids =
+        favorite_pairs
+        |> Enum.slice(3, 3)
+        |> Enum.reverse()
+        |> Enum.map(fn {_favorite, activity} -> activity.id end)
 
       response =
         conn
         |> get(
-          "/api/v1/pleroma/accounts/#{user.id}/favourites?since_id=#{third_activity.id}&max_id=#{seventh_activity.id}"
+          "/api/v1/pleroma/accounts/#{user.id}/favourites?since_id=#{third_favorite.id}&max_id=#{seventh_favorite.id}"
         )
         |> json_response_and_validate_schema(:ok)
 
-      assert length(response) == 3
+      assert Enum.map(response, & &1["id"]) == expected_ids
-      refute third_activity in response
+    end
-      refute seventh_activity in response
+
+    test "paginates favorites using min_id and limit", %{
+      conn: conn,
+      user: user
+    } do
+      activities = insert_list(10, :note_activity)
+
+      favorite_pairs =
+        Enum.map(activities, fn activity ->
+          {:ok, favorite} = CommonAPI.favorite(activity.id, user)
+          {favorite, activity}
+        end)
+        |> Enum.sort_by(fn {favorite, _activity} -> FlakeId.from_string(favorite.id) end)
+
+      {third_favorite, _} = Enum.at(favorite_pairs, 2)
+      {fourth_favorite, _} = Enum.at(favorite_pairs, 3)
+      {fifth_favorite, _} = Enum.at(favorite_pairs, 4)
+
+      expected_ids =
+        favorite_pairs
+        |> Enum.slice(3, 2)
+        |> Enum.reverse()
+        |> Enum.map(fn {_favorite, activity} -> activity.id end)
+
+      conn =
+        get(
+          conn,
+          "/api/v1/pleroma/accounts/#{user.id}/favourites?min_id=#{third_favorite.id}&limit=2"
+        )
+
+      response = json_response_and_validate_schema(conn, :ok)
+
+      assert Enum.map(response, & &1["id"]) == expected_ids
+      assert [link_header] = get_resp_header(conn, "link")
+      assert link_header =~ "max_id=#{fourth_favorite.id}"
+      assert link_header =~ "min_id=#{fifth_favorite.id}"
     end
 
     test "limits favorites using limit parameter", %{