From d4a76b0a6ffba70c0f0bc10a4175cfdd99f8d574 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Wed, 25 Sep 2019 15:59:04 +0300 Subject: [PATCH] Don't embed the first page in inboxes/outboxes and refactor the views to follow View/Controller pattern Note that I mentioned the change in 1.1 section because I intend to backport this, if this is not needed I will move it back to Unreleased. --- CHANGELOG.md | 1 + lib/pleroma/web/activity_pub/activity_pub.ex | 2 +- .../activity_pub/activity_pub_controller.ex | 71 +++++++++++++-- .../web/activity_pub/views/user_view.ex | 86 +++---------------- .../activity_pub_controller_test.exs | 6 +- .../web/activity_pub/views/user_view_test.exs | 31 +++++++ 6 files changed, 112 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c5e43123..1fb7184f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - AdminAPI: Add "godmode" while fetching user statuses (i.e. admin can see private statuses) - Improve digest email template – Pagination: (optional) return `total` alongside with `items` when paginating +- ActivityPub: The first page in inboxes/outboxes is no longer embedded. ### Fixed - Following from Osada diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index d23ec66ac..d556b982f 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -864,7 +864,7 @@ defp restrict_muted_reblogs(query, %{"muting_user" => %User{info: info}}) do defp restrict_muted_reblogs(query, _), do: query - defp exclude_poll_votes(query, %{"include_poll_votes" => "true"}), do: query + defp exclude_poll_votes(query, %{"include_poll_votes" => true}), do: query defp exclude_poll_votes(query, _) do if has_named_binding?(query, :object) do diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index 705dbc1c2..8069d64c9 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -197,12 +197,42 @@ def followers(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname}) d end end - def outbox(conn, %{"nickname" => nickname} = params) do + def outbox(conn, %{"nickname" => nickname, "page" => page?} = params) + when page? in [true, "true"] do + with %User{} = user <- User.get_cached_by_nickname(nickname), + {:ok, user} <- User.ensure_keys_present(user), + activities <- + (if params["max_id"] do + ActivityPub.fetch_user_activities(user, nil, %{ + "max_id" => params["max_id"], + # This is a hack because postgres generates inefficient queries when filtering by 'Answer', + # poll votes will be hidden by the visibility filter in this case anyway + "include_poll_votes" => true, + "limit" => 10 + }) + else + ActivityPub.fetch_user_activities(user, nil, %{ + "limit" => 10, + "include_poll_votes" => true + }) + end) do + conn + |> put_resp_content_type("application/activity+json") + |> put_view(UserView) + |> render("activity_collection_page.json", %{ + activities: activities, + iri: "#{user.ap_id}/outbox" + }) + end + end + + def outbox(conn, %{"nickname" => nickname}) do with %User{} = user <- User.get_cached_by_nickname(nickname), {:ok, user} <- User.ensure_keys_present(user) do conn |> put_resp_content_type("application/activity+json") - |> json(UserView.render("outbox.json", %{user: user, max_id: params["max_id"]})) + |> put_view(UserView) + |> render("activity_collection.json", %{iri: "#{user.ap_id}/outbox"}) end end @@ -278,12 +308,37 @@ def whoami(_conn, _params), do: {:error, :not_found} def read_inbox( %{assigns: %{user: %{nickname: nickname} = user}} = conn, - %{"nickname" => nickname} = params - ) do - conn - |> put_resp_content_type("application/activity+json") - |> put_view(UserView) - |> render("inbox.json", user: user, max_id: params["max_id"]) + %{"nickname" => nickname, "page" => page?} = params + ) + when page? in [true, "true"] do + with activities <- + (if params["max_id"] do + ActivityPub.fetch_activities([user.ap_id | user.following], %{ + "max_id" => params["max_id"], + "limit" => 10 + }) + else + ActivityPub.fetch_activities([user.ap_id | user.following], %{"limit" => 10}) + end) do + conn + |> put_resp_content_type("application/activity+json") + |> put_view(UserView) + |> render("activity_collection_page.json", %{ + activities: activities, + iri: "#{user.ap_id}/inbox" + }) + end + end + + def read_inbox(%{assigns: %{user: %{nickname: nickname} = user}} = conn, %{ + "nickname" => nickname + }) do + with {:ok, user} <- User.ensure_keys_present(user) do + conn + |> put_resp_content_type("application/activity+json") + |> put_view(UserView) + |> render("activity_collection.json", %{iri: "#{user.ap_id}/inbox"}) + end end def read_inbox(%{assigns: %{user: nil}} = conn, %{"nickname" => nickname}) do diff --git a/lib/pleroma/web/activity_pub/views/user_view.ex b/lib/pleroma/web/activity_pub/views/user_view.ex index 7be734b26..08e778839 100644 --- a/lib/pleroma/web/activity_pub/views/user_view.ex +++ b/lib/pleroma/web/activity_pub/views/user_view.ex @@ -8,7 +8,6 @@ defmodule Pleroma.Web.ActivityPub.UserView do alias Pleroma.Keys alias Pleroma.Repo alias Pleroma.User - alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.ActivityPub.Utils alias Pleroma.Web.Endpoint @@ -207,25 +206,22 @@ def render("followers.json", %{user: user} = opts) do |> Map.merge(Utils.make_json_ld_header()) end - def render("outbox.json", %{user: user, max_id: max_qid}) do - params = %{ - "limit" => "10" + def render("activity_collection.json", %{iri: iri}) do + %{ + "id" => iri, + "type" => "OrderedCollection", + "first" => "#{iri}?page=true" } + |> Map.merge(Utils.make_json_ld_header()) + end - params = - if max_qid != nil do - Map.put(params, "max_id", max_qid) - else - params - end - - activities = ActivityPub.fetch_user_activities(user, nil, params) - + def render("activity_collection_page.json", %{activities: activities, iri: iri}) do + # this is sorted chronologically, so first activity is the newest (max) {max_id, min_id, collection} = if length(activities) > 0 do { - Enum.at(Enum.reverse(activities), 0).id, Enum.at(activities, 0).id, + Enum.at(Enum.reverse(activities), 0).id, Enum.map(activities, fn act -> {:ok, data} = Transmogrifier.prepare_outgoing(act.data) data @@ -239,71 +235,15 @@ def render("outbox.json", %{user: user, max_id: max_qid}) do } end - iri = "#{user.ap_id}/outbox" - page = %{ - "id" => "#{iri}?max_id=#{max_id}", + "id" => "#{iri}?max_id=#{max_id}&page=true", "type" => "OrderedCollectionPage", "partOf" => iri, "orderedItems" => collection, - "next" => "#{iri}?max_id=#{min_id}" + "next" => "#{iri}?max_id=#{min_id}&page=true" } - if max_qid == nil do - %{ - "id" => iri, - "type" => "OrderedCollection", - "first" => page - } - |> Map.merge(Utils.make_json_ld_header()) - else - page |> Map.merge(Utils.make_json_ld_header()) - end - end - - def render("inbox.json", %{user: user, max_id: max_qid}) do - params = %{ - "limit" => "10" - } - - params = - if max_qid != nil do - Map.put(params, "max_id", max_qid) - else - params - end - - activities = ActivityPub.fetch_activities([user.ap_id | user.following], params) - - min_id = Enum.at(Enum.reverse(activities), 0).id - max_id = Enum.at(activities, 0).id - - collection = - Enum.map(activities, fn act -> - {:ok, data} = Transmogrifier.prepare_outgoing(act.data) - data - end) - - iri = "#{user.ap_id}/inbox" - - page = %{ - "id" => "#{iri}?max_id=#{max_id}", - "type" => "OrderedCollectionPage", - "partOf" => iri, - "orderedItems" => collection, - "next" => "#{iri}?max_id=#{min_id}" - } - - if max_qid == nil do - %{ - "id" => iri, - "type" => "OrderedCollection", - "first" => page - } - |> Map.merge(Utils.make_json_ld_header()) - else - page |> Map.merge(Utils.make_json_ld_header()) - end + page |> Map.merge(Utils.make_json_ld_header()) end def collection(collection, iri, page, show_items \\ true, total \\ nil) do diff --git a/test/web/activity_pub/activity_pub_controller_test.exs b/test/web/activity_pub/activity_pub_controller_test.exs index 9698c7099..6fb7a7dda 100644 --- a/test/web/activity_pub/activity_pub_controller_test.exs +++ b/test/web/activity_pub/activity_pub_controller_test.exs @@ -473,7 +473,7 @@ test "it returns a note activity in a collection", %{conn: conn} do conn |> assign(:user, user) |> put_req_header("accept", "application/activity+json") - |> get("/users/#{user.nickname}/inbox") + |> get("/users/#{user.nickname}/inbox?page=true") assert response(conn, 200) =~ note_object.data["content"] end @@ -559,7 +559,7 @@ test "it returns a note activity in a collection", %{conn: conn} do conn = conn |> put_req_header("accept", "application/activity+json") - |> get("/users/#{user.nickname}/outbox") + |> get("/users/#{user.nickname}/outbox?page=true") assert response(conn, 200) =~ note_object.data["content"] end @@ -571,7 +571,7 @@ test "it returns an announce activity in a collection", %{conn: conn} do conn = conn |> put_req_header("accept", "application/activity+json") - |> get("/users/#{user.nickname}/outbox") + |> get("/users/#{user.nickname}/outbox?page=true") assert response(conn, 200) =~ announce_activity.data["object"] end diff --git a/test/web/activity_pub/views/user_view_test.exs b/test/web/activity_pub/views/user_view_test.exs index fb7fd9e79..b698b3396 100644 --- a/test/web/activity_pub/views/user_view_test.exs +++ b/test/web/activity_pub/views/user_view_test.exs @@ -121,5 +121,36 @@ test "sets totalItems to zero when follows are hidden" do user = Map.put(user, :info, info) assert %{"totalItems" => 0} = UserView.render("following.json", %{user: user}) end + + test "activity collection page aginates correctly" do + user = insert(:user) + + posts = + for i <- 0..25 do + {:ok, activity} = CommonAPI.post(user, %{"status" => "post #{i}"}) + activity + end + + # outbox sorts chronologically, newest first, with ten per page + posts = Enum.reverse(posts) + + %{"next" => next_url} = + UserView.render("activity_collection_page.json", %{ + iri: "#{user.ap_id}/outbox", + activities: Enum.take(posts, 10) + }) + + next_id = Enum.at(posts, 9).id + assert next_url =~ next_id + + %{"next" => next_url} = + UserView.render("activity_collection_page.json", %{ + iri: "#{user.ap_id}/outbox", + activities: Enum.take(Enum.drop(posts, 10), 10) + }) + + next_id = Enum.at(posts, 19).id + assert next_url =~ next_id + end end end