From 73d01857e3ff1737a4ea733a3f6c6419379ce8e8 Mon Sep 17 00:00:00 2001 From: Alex S Date: Sun, 14 Apr 2019 19:45:56 +0700 Subject: [PATCH] bookmarks in separate table --- lib/pleroma/bookmark.ex | 53 +++++++++++++++++++ lib/pleroma/user.ex | 19 +------ .../mastodon_api/mastodon_api_controller.ex | 33 +++++++++--- .../web/mastodon_api/views/status_view.ex | 4 +- .../20190413082658_create_bookmarks.exs | 14 +++++ test/bookmark_test.exs | 37 +++++++++++++ test/user_test.exs | 27 ---------- 7 files changed, 135 insertions(+), 52 deletions(-) create mode 100644 lib/pleroma/bookmark.ex create mode 100644 priv/repo/migrations/20190413082658_create_bookmarks.exs create mode 100644 test/bookmark_test.exs diff --git a/lib/pleroma/bookmark.ex b/lib/pleroma/bookmark.ex new file mode 100644 index 000000000..c5c3e078b --- /dev/null +++ b/lib/pleroma/bookmark.ex @@ -0,0 +1,53 @@ +defmodule Pleroma.Bookmark do + use Ecto.Schema + + import Ecto.Changeset + import Ecto.Query + + alias Pleroma.Activity + alias Pleroma.Bookmark + alias Pleroma.FlakeId + alias Pleroma.Repo + alias Pleroma.User + + @type t :: %__MODULE__{} + + schema "bookmarks" do + belongs_to(:user, User, type: FlakeId) + belongs_to(:activity, Activity, type: FlakeId) + + timestamps() + end + + @spec create(FlakeId.t(), FlakeId.t()) :: {:ok, Bookmark.t()} | {:error, Changeset.t()} + def create(user_id, activity_id) do + attrs = %{ + user_id: user_id, + activity_id: activity_id + } + + %Bookmark{} + |> cast(attrs, [:user_id, :activity_id]) + |> validate_required([:user_id, :activity_id]) + |> unique_constraint(:activity_id, name: :bookmarks_user_id_activity_id_index) + |> Repo.insert() + end + + @spec for_user_query(FlakeId.t()) :: Ecto.Query.t() + def for_user_query(user_id) do + Bookmark + |> where(user_id: ^user_id) + |> join(:inner, [b], activity in assoc(b, :activity)) + |> preload([b, a], activity: a) + end + + @spec destroy(FlakeId.t(), FlakeId.t()) :: {:ok, Bookmark.t()} | {:error, Changeset.t()} + def destroy(user_id, activity_id) do + from(b in Bookmark, + where: b.user_id == ^user_id, + where: b.activity_id == ^activity_id + ) + |> Repo.one() + |> Repo.delete() + end +end diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index f1feab279..c5b1ddc5d 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -10,6 +10,7 @@ defmodule Pleroma.User do alias Comeonin.Pbkdf2 alias Pleroma.Activity + alias Pleroma.Bookmark alias Pleroma.Formatter alias Pleroma.Notification alias Pleroma.Object @@ -53,8 +54,8 @@ defmodule Pleroma.User do field(:search_rank, :float, virtual: true) field(:search_type, :integer, virtual: true) field(:tags, {:array, :string}, default: []) - field(:bookmarks, {:array, :string}, default: []) field(:last_refreshed_at, :naive_datetime_usec) + has_many(:bookmarks, Bookmark) has_many(:notifications, Notification) has_many(:registrations, Registration) embeds_one(:info, Pleroma.User.Info) @@ -1379,22 +1380,6 @@ defp update_tags(%User{} = user, new_tags) do updated_user end - def bookmark(%User{} = user, status_id) do - bookmarks = Enum.uniq(user.bookmarks ++ [status_id]) - update_bookmarks(user, bookmarks) - end - - def unbookmark(%User{} = user, status_id) do - bookmarks = Enum.uniq(user.bookmarks -- [status_id]) - update_bookmarks(user, bookmarks) - end - - def update_bookmarks(%User{} = user, bookmarks) do - user - |> change(%{bookmarks: bookmarks}) - |> update_and_set_cache - end - defp normalize_tags(tags) do [tags] |> List.flatten() diff --git a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex index 0ba8d9eea..a93aa6ad5 100644 --- a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex +++ b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex @@ -6,6 +6,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do use Pleroma.Web, :controller alias Ecto.Changeset alias Pleroma.Activity + alias Pleroma.Bookmark alias Pleroma.Config alias Pleroma.Filter alias Pleroma.Notification @@ -279,6 +280,8 @@ def home_timeline(%{assigns: %{user: user}} = conn, params) do |> ActivityPub.contain_timeline(user) |> Enum.reverse() + user = Repo.preload(user, :bookmarks) + conn |> add_link_headers(:home_timeline, activities) |> put_view(StatusView) @@ -297,6 +300,8 @@ def public_timeline(%{assigns: %{user: user}} = conn, params) do |> ActivityPub.fetch_public_activities() |> Enum.reverse() + user = Repo.preload(user, :bookmarks) + conn |> add_link_headers(:public_timeline, activities, false, %{"local" => local_only}) |> put_view(StatusView) @@ -304,7 +309,8 @@ def public_timeline(%{assigns: %{user: user}} = conn, params) do end def user_statuses(%{assigns: %{user: reading_user}} = conn, params) do - with %User{} = user <- User.get_cached_by_id(params["id"]) do + with %User{} = user <- User.get_cached_by_id(params["id"]), + reading_user <- Repo.preload(reading_user, :bookmarks) do activities = ActivityPub.fetch_user_activities(user, reading_user, params) conn @@ -331,6 +337,8 @@ def dm_timeline(%{assigns: %{user: user}} = conn, params) do |> ActivityPub.fetch_activities_query(params) |> Pagination.fetch_paginated(params) + user = Repo.preload(user, :bookmarks) + conn |> add_link_headers(:dm_timeline, activities) |> put_view(StatusView) @@ -548,7 +556,9 @@ def bookmark_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do %Object{} = object <- Object.normalize(activity), %User{} = user <- User.get_cached_by_nickname(user.nickname), true <- Visibility.visible_for_user?(activity, user), - {:ok, user} <- User.bookmark(user, object.data["id"]) do + {:ok, _bookmark} <- Bookmark.create(user.id, activity.id) do + user = Repo.preload(user, :bookmarks) + conn |> put_view(StatusView) |> try_render("status.json", %{activity: activity, for: user, as: :activity}) @@ -560,7 +570,9 @@ def unbookmark_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do %Object{} = object <- Object.normalize(activity), %User{} = user <- User.get_cached_by_nickname(user.nickname), true <- Visibility.visible_for_user?(activity, user), - {:ok, user} <- User.unbookmark(user, object.data["id"]) do + {:ok, _bookmark} <- Bookmark.destroy(user.id, activity.id) do + user = Repo.preload(user, :bookmarks) + conn |> put_view(StatusView) |> try_render("status.json", %{activity: activity, for: user, as: :activity}) @@ -1124,15 +1136,20 @@ def user_favourites(%{assigns: %{user: for_user}} = conn, %{"id" => id} = params end end - def bookmarks(%{assigns: %{user: user}} = conn, _) do + def bookmarks(%{assigns: %{user: user}} = conn, params) do user = User.get_cached_by_id(user.id) + user = Repo.preload(user, :bookmarks) + + bookmarks = + Bookmark.for_user_query(user.id) + |> Pagination.fetch_paginated(params) activities = - user.bookmarks - |> Enum.map(fn id -> Activity.get_create_by_object_ap_id(id) end) - |> Enum.reverse() + bookmarks + |> Enum.map(fn b -> b.activity end) conn + |> add_link_headers(:bookmarks, bookmarks) |> put_view(StatusView) |> render("index.json", %{activities: activities, for: user, as: :activity}) end @@ -1238,6 +1255,8 @@ def list_timeline(%{assigns: %{user: user}} = conn, %{"list_id" => id} = params) |> ActivityPub.fetch_activities_bounded(following, params) |> Enum.reverse() + user = Repo.preload(user, :bookmarks) + conn |> put_view(StatusView) |> render("index.json", %{activities: activities, for: user, as: :activity}) diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index 7dd80d708..b2ed023dc 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -148,7 +148,9 @@ def render("status.json", %{activity: %{data: %{"object" => _object}} = activity favorited = opts[:for] && opts[:for].ap_id in (object.data["likes"] || []) - bookmarked = opts[:for] && object.data["id"] in opts[:for].bookmarks + bookmarked = + opts[:for] && Ecto.assoc_loaded?(opts[:for].bookmarks) && + Enum.any?(opts[:for].bookmarks, fn b -> b.activity_id == activity.id end) attachment_data = object.data["attachment"] || [] attachments = render_many(attachment_data, StatusView, "attachment.json", as: :attachment) diff --git a/priv/repo/migrations/20190413082658_create_bookmarks.exs b/priv/repo/migrations/20190413082658_create_bookmarks.exs new file mode 100644 index 000000000..38b108158 --- /dev/null +++ b/priv/repo/migrations/20190413082658_create_bookmarks.exs @@ -0,0 +1,14 @@ +defmodule Pleroma.Repo.Migrations.CreateBookmarks do + use Ecto.Migration + + def change do + create table(:bookmarks) do + add(:user_id, references(:users, type: :uuid, on_delete: :delete_all)) + add(:activity_id, references(:activities, type: :uuid, on_delete: :delete_all)) + + timestamps() + end + + create(unique_index(:bookmarks, [:user_id, :activity_id])) + end +end diff --git a/test/bookmark_test.exs b/test/bookmark_test.exs new file mode 100644 index 000000000..3be148023 --- /dev/null +++ b/test/bookmark_test.exs @@ -0,0 +1,37 @@ +defmodule Pleroma.BookmarkTest do + use Pleroma.DataCase + import Pleroma.Factory + alias Pleroma.Bookmark + alias Pleroma.Web.CommonAPI + + describe "create/2" do + test "with valid params" do + user = insert(:user) + {:ok, activity} = CommonAPI.post(user, %{"status" => "Some cool information"}) + {:ok, bookmark} = Bookmark.create(user.id, activity.id) + assert bookmark.user_id == user.id + assert bookmark.activity_id == activity.id + end + + test "with invalid params" do + {:error, changeset} = Bookmark.create(nil, "") + refute changeset.valid? + + assert changeset.errors == [ + user_id: {"can't be blank", [validation: :required]}, + activity_id: {"can't be blank", [validation: :required]} + ] + end + end + + describe "destroy/2" do + test "with valid params" do + user = insert(:user) + + {:ok, activity} = CommonAPI.post(user, %{"status" => "Some cool information"}) + {:ok, _bookmark} = Bookmark.create(user.id, activity.id) + + {:ok, _deleted_bookmark} = Bookmark.destroy(user.id, activity.id) + end + end +end diff --git a/test/user_test.exs b/test/user_test.exs index 42d570c50..7be47e5fb 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -1125,33 +1125,6 @@ test "Adds rel=me on linkbacked urls" do end end - test "bookmarks" do - user = insert(:user) - - {:ok, activity1} = - CommonAPI.post(user, %{ - "status" => "heweoo!" - }) - - id1 = Object.normalize(activity1).data["id"] - - {:ok, activity2} = - CommonAPI.post(user, %{ - "status" => "heweoo!" - }) - - id2 = Object.normalize(activity2).data["id"] - - assert {:ok, user_state1} = User.bookmark(user, id1) - assert user_state1.bookmarks == [id1] - - assert {:ok, user_state2} = User.unbookmark(user, id1) - assert user_state2.bookmarks == [] - - assert {:ok, user_state3} = User.bookmark(user, id2) - assert user_state3.bookmarks == [id2] - end - test "follower count is updated when a follower is blocked" do user = insert(:user) follower = insert(:user)